[Freeipa-devel] [PATCH] 0059 Create DS user and group during ipa-restore

[Petr Viktorin]

On 09/02/2013 11:36 AM, Ana Krivokapic wrote:
> On 08/30/2013 04:37 PM, Petr Viktorin wrote:
>> On 08/29/2013 02:16 PM, Ana Krivokapic wrote:
>>> Hello,
>>>
>>> This patch addresses tickethttps://fedorahosted.org/freeipa/ticket/3856.
>>>
>>
>> Thanks for the patch!
>> I haven't tested it yet, but I've read it and I have some comments below.
>>
>>
>>> diff --git a/install/share/copy-schema-to-ca.py
>>> b/install/share/copy-schema-to-ca.py
>>> index
>>> 1888f12513aa3edf22149e9330afea99f62bf41d..fe99a9256f1298bae1c746ea0c4d41339a4fbebb
>>> 100755
>>> --- a/install/share/copy-schema-to-ca.py
>>> +++ b/install/share/copy-schema-to-ca.py
>>> @@ -15,10 +15,11 @@
>>>    import pwd
>>>    import shutil
>>>
>>> -from ipapython import services, ipautil, dogtag
>>> +from ipapython import services, ipautil
>>>    from ipapython.ipa_log_manager import root_logger, standard_logging_setup
>>> -from ipaserver.install.dsinstance import DS_USER, schema_dirname
>>> +from ipaserver.install.dsinstance import schema_dirname
>>>    from ipaserver.install.cainstance import PKI_USER
>>> +from ipaserver.install.installutils import DS_USER
>>>    from ipalib import api
>>
>> copy-schema-to-ca.py is intended to be copied to older systems, to prepare the
>> CA DSs of old IPA masters for replication with new masters with unified DSs.
>> This means that we can't remove anything we import here; the constants need to
>> be available at the old locations.
>
> Thanks for clearing that up, I didn't know about this requirement.
>
>>
>> That's the hard requirement, but anyway I think the constants, and the
>> create_ds_user & create_ds_group functions, are specific to the DS and so
>> belong in dsinstance.
>> Did I overlook a reason for moving them to installutils?
>
> You are right. I refactored these functions so that they can be more easily used
> from other modules, but there is no special reason to put them in installutils.
> I moved them back to dsinstance.
>
>>
>> [...]
>>> diff --git a/ipaserver/install/installutils.py
>>> b/ipaserver/install/installutils.py
>>> index
>>> 268279dc9d22b9f983406303cbfc80c00a2b8fa0..84846221d2800443ba6e291ec9c28b37a482d735
>>> 100644
>> [...]
>>> +def create_ds_user():
>>> +    """
>>> +    Create DS user if it doesn't exist yet
>>> +    """
>>> +    try:
>>> +        pwd.getpwnam(DS_USER)
>>> +        root_logger.debug("DS user %s exists" % DS_USER)
>>
>> Use comma for debug() arguments, i.e. debug("DS group %s exists", DS_GROUP).
>> Same in other places.
>
> Fixed.
>
>>
>>> +    except KeyError:
>>> +        root_logger.debug("Adding DS user %s" % DS_USER)
>>> +        args = ["/usr/sbin/useradd", "-g", DS_GROUP,
>>> +                                     "-c", "DS System User",
>>> +                                     "-d", "/var/lib/dirsrv",
>>> +                                     "-s", "/sbin/nologin",
>>> +                                     "-M", "-r", DS_USER]
>>> +        try:
>>> +            ipautil.run(args)
>>> +            root_logger.debug("Done adding DS user")
>>> +        except ipautil.CalledProcessError, e:
>>> +            root_logger.critical("Failed to add DS user %s" % e)
>>> +
>>> +
>>> +def create_ds_group():
>>> +    """
>>> +    Create DS group if it doesn't exist yet
>>> +    """
>>
>> Please document the return value in the docstring, it's not obvious that this
>> returns True iff it didn't do anything.
>>
>
> Done.
>
> Updated patch attached.
>

I've run into other backup/restore problems that I still need to 
investigate, but the patch fixes this bug.

ACK with a small change in create_ds_group docstring: s/DS/the group/ in 
"Returns True if DS already exists."

I've made the change and pushed to
master: de7b1f86dc5bc120e570a99e722a06865cad3fdd[[BR]]
ipa-3-3: bc559c0b386cf6e55df6e60d6dcfbc39cf68b85e

-- 
Petr³