Re: [Freeipa-devel] [PATCH] 0059 Create DS user and group during ipa-restore
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. -- Regards, Ana Krivokapic Associate Software Engineer FreeIPA team Red Hat Inc. From 217bc68bbee6ef658e6248fa069252184ca1f171 Mon Sep 17 00:00:00 2001 From: Ana Krivokapic akriv...@redhat.com Date: Mon, 2 Sep 2013 10:56:19 +0200 Subject: [PATCH] Create DS user and group during ipa-restore ipa-restore would fail if DS user did not exist. Check for presence of DS user and group and create them if needed. https://fedorahosted.org/freeipa/ticket/3856 --- install/tools/ipa-replica-install | 22 +++-- install/tools/ipa-server-install | 11 +-- ipaserver/install/dsinstance.py | 66 --- ipaserver/install/ipa_restore.py | 12 +++ 4 files changed, 59 insertions(+), 52 deletions(-) diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install index 947c51f6f287ffce52994408352601388faf56a6..2a88c102175f7c9bf7c173f9f9f3437ccc963f29 100755 --- a/install/tools/ipa-replica-install +++ b/install/tools/ipa-replica-install @@ -22,7 +22,6 @@ import sys import socket import os, pwd, shutil -import grp from optparse import OptionGroup from contextlib import contextmanager @@ -33,13 +32,13 @@ import dns.exception from ipapython import ipautil from ipaserver.install import dsinstance, installutils, krbinstance, service -from ipaserver.install import bindinstance, httpinstance, ntpinstance, certs +from ipaserver.install import bindinstance, httpinstance, ntpinstance from ipaserver.install import memcacheinstance from ipaserver.install import otpdinstance from ipaserver.install.replication import replica_conn_check, ReplicationManager -from ipaserver.install.installutils import (HostnameLocalhost, resolve_host, -ReplicaConfig, expand_replica_info, read_replica_info ,get_host_name, -BadHostError, private_ccache) +from ipaserver.install.installutils import
Re: [Freeipa-devel] [PATCH] 0059 Create DS user and group during ipa-restore
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³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0059 Create DS user and group during ipa-restore
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. 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? [...] 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. +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. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
