[PATCH] make samba-tool aware of all 7 fsmo roles

Jelmer Vernooij jelmer at jelmer.uk
Thu Jun 4 06:23:50 MDT 2015


On Thu, Jun 04, 2015 at 11:36:29AM +0100, Rowland Penny wrote:
> On 03/06/15 00:53, Jelmer Vernooij wrote:
> OK, hopefully the attached patch addresses all the problems pointed out by
> Jelmer (but I not holding my breath ;-) ), that is apart from the lining up
> of the new lines, I have left these in because, on my terminal, it makes it
> easier to read.
Thanks! Here is another round of comments, just a few this time.

Please do fix the newlines. Consistency in coding style matters; we don't
line up newlines anywhere else in our codebase. See also
https://www.python.org/dev/peps/pep-0008/#pet-peeves

> From ad77410f97f246a685ce52fd0f1730c0d571f760 Mon Sep 17 00:00:00 2001
> From: Rowland Penny <repenny241155 at gmail.com>
> Date: Thu, 4 Jun 2015 11:24:13 +0100
> Subject: [PATCH] samba-tool: make 'samba-tool fsmo *' aware of all 7 fsmo
>  roles
> 
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=10734
> 
> Signed-off-by: Rowland Penny <repenny241155 at gmail.com>
> ---
>  python/samba/netcmd/fsmo.py |  360 ++++++++++++++++++++++++++++++++-----------
>  1 file changed, 274 insertions(+), 86 deletions(-)
> 
> diff --git a/python/samba/netcmd/fsmo.py b/python/samba/netcmd/fsmo.py
> index 1bc4a96..d5f2d09 100644
> --- a/python/samba/netcmd/fsmo.py
> +++ b/python/samba/netcmd/fsmo.py
> +def transfer_dns_role(outf, sambaopts, credopts, role, samdb):
> +    """Transfer dns FSMO role. """
> +
> +    if role == "domaindns":
> +        domain_dn = samdb.domain_dn()
> +        role_object = "CN=Infrastructure,DC=DomainDnsZones," + domain_dn
> +    elif role == "forestdns":
> +        forest_dn = "DC=" + samdb.forest_dns_name().replace(".", ",DC=")
> +        role_object = "CN=Infrastructure,DC=ForestDnsZones," + forest_dn
> +
> +    try:
> +        res = samdb.search(role_object, 
> +                           attrs=["fSMORoleOwner"],
> +                           scope=ldb.SCOPE_BASE, 
> +                           controls=["extended_dn:1:1"])
> +
> +        if 'fSMORoleOwner' in res[0]:
> +            try:
> +                master_guid = str(misc.GUID(ldb.Dn(samdb, 
> +                                  res[0]['fSMORoleOwner'][0])
> +                                  .get_extended_component('GUID')))
> +                master_owner = str(ldb.Dn(samdb, res[0]['fSMORoleOwner'][0]))
> +            except:
> +                outf.write("Can't find GUID in naming master on partition DN %s\n"
> +                           % res[0]['fSMORoleOwner'][0])
> +                return
^^ Please don't use "except:" unless you're going to reraise the exception. It
seems like you want to catch LdbError here instead?

In the calling code you rely on this function raising an exception in case of errors. Perhaps
raise an exception here rather than writing to stdout and returning?

> +    except LdbError, (num, msg):
> +        raise CommandError("DNS partion %s not found : %s" % (role, msg))
^^ This except clause looks like it should be higher up, just below the samdb.search() call?

> @@ -119,26 +250,78 @@ all=all of the above"""),
>          else:
>              raise CommandError("Invalid FSMO role.")
>          #first try to transfer to avoid problem if the owner is still active
> -        if force is None:
> -            self.message("Attempting transfer...")
> -            try:
> -                transfer_role(self.outf, role, samdb)
> -                self.outf.write("FSMO seize was not required, as transfer of '%s' role was successful\n" % role)
> -                return
> -            except CommandError:
> -            #transfer failed, use the big axe...
> +        if master_owner != serviceName:
> +            if force is None:
> +                self.message("Attempting transfer...")
> +                try:
> +                    transfer_role(self.outf, role, samdb)
> +                    return
> +                except:
> +                #transfer failed, use the big axe...
^^ Please don't use "except:" unless you're going to re-raise the exception. It looks like the indentation here is off, self.message() has the same indentation as except:?

>                  self.message("Transfer unsuccessful, seizing...")
> +            else:
> +                self.message("Seizing %s FSMO role..." % role)
> +
> +            m["fSMORoleOwner"]= ldb.MessageElement(
> +                serviceName, ldb.FLAG_MOD_REPLACE,
> +                "fSMORoleOwner")
> +            try:
> +                samdb.modify(m)
> +            except LdbError, (num, msg):
> +                raise CommandError("Failed to seize '%s' role: %s" % 
> +                                   (role, msg))
> +
> +            self.outf.write("Successful seize of the '%s' FSMO role\n" % role)
>          else:
> -            self.message("Will not attempt transfer, seizing...")
> +            self.outf.write("This DC already has the '%s' FSMO role\n" % role)
>  
> -        m["fSMORoleOwner"]= ldb.MessageElement(
> -            serviceName, ldb.FLAG_MOD_REPLACE,
> -            "fSMORoleOwner")
> -        try:
> -            samdb.modify(m)
> -        except LdbError, (num, msg):
> -            raise CommandError("Failed to initiate role seize of '%s' role: %s" % (role, msg))
> -        self.outf.write("FSMO seize of '%s' role successful\n" % role)
> +
> +    def seize_dns_role(self, role, samdb, credopts, sambaopts, 
> +                       versionopts, force):
> +        """Seize DNS FSMO role. """
> +
> +        serviceName = samdb.get_dsServiceName()
> +        domain_dn = samdb.domain_dn()
> +        forest_dn = "DC=" + samdb.forest_dns_name().replace(".", ",DC=")
> +        self.domaindns_dn = "CN=Infrastructure,DC=DomainDnsZones," + domain_dn
> +        self.forestdns_dn = "CN=Infrastructure,DC=ForestDnsZones," + forest_dn
> +
> +        m = ldb.Message()
> +        if role == "domaindns":
> +            m.dn = ldb.Dn(samdb, self.domaindns_dn)
> +        elif role == "forestdns":
> +            m.dn = ldb.Dn(samdb, self.forestdns_dn)
> +        else:
> +            raise CommandError("Invalid FSMO role.")
> +        #first try to transfer to avoid problem if the owner is still active
> +        master_owner = get_fsmo_roleowner(samdb, m.dn)
> +        if master_owner != serviceName:
> +            if force is None:
> +                self.message("Attempting transfer...")
> +                try:
> +                    transfer_dns_role(self.outf, sambaopts, credopts, role, 
> +                                      samdb)
> +                    #self.outf.write("Transfer of '%s' role was successful\n" % 
> +                    #                role)
> +                    return
> +                except:
^^ Please catch a specific exception.
> +                    #transfer failed, use the big axe...
> +                    self.message("Transfer unsuccessful, seizing...")
^^ transfer_dns_role() doesn't necessarily raise an exception when it fails.

> +            else:
> +                self.message("Seizing %s FSMO role..." % role)
> +
> +            m["fSMORoleOwner"]= ldb.MessageElement(
> +                serviceName, ldb.FLAG_MOD_REPLACE,
> +                "fSMORoleOwner")
> +            try:
> +                samdb.modify(m)
> +            except LdbError, (num, msg):
> +                raise CommandError("Failed to seize '%s' role: %s" % 
> +                                   (role, msg))
> +            else:
> +                self.outf.write("Successful seize of '%s' FSMO role\n" % role)
> +        else:
> +            self.outf.write("This DC already has the '%s' FSMO role\n" % role)
>  
>      def run(self, force=None, H=None, role=None,
>              credopts=None, sambaopts=None, versionopts=None):
> @@ -155,8 +338,16 @@ all=all of the above"""),
>              self.seize_role("naming", samdb, force)
>              self.seize_role("infrastructure", samdb, force)
>              self.seize_role("schema", samdb, force)
> +            self.seize_dns_role("domaindns", samdb, credopts, sambaopts, 
> +                                versionopts, force)
> +            self.seize_dns_role("forestdns", samdb, credopts, sambaopts, 
> +                                versionopts, force)
>          else:
> -            self.seize_role(role, samdb, force)
> +            if role == "domaindns" or role == "forestdns":
> +                self.seize_dns_role(role, samdb, credopts, sambaopts, 
> +                                    versionopts, force)
> +            else:
> +                self.seize_role(role, samdb, force)
>  
>  
>  class cmd_fsmo_show(Command):
> @@ -171,8 +362,8 @@ class cmd_fsmo_show(Command):
>          }
>  
>      takes_options = [
> -        Option("-H", "--URL", help="LDB URL for database or target server", type=str,
> -               metavar="URL", dest="H"),
> +        Option("-H", "--URL", help="LDB URL for database or target server", 
> +               type=str, metavar="URL", dest="H"),
>          ]
>  
>      takes_args = []
> @@ -185,41 +376,29 @@ class cmd_fsmo_show(Command):
>              credentials=creds, lp=lp)
>  
>          domain_dn = samdb.domain_dn()
> -        self.infrastructure_dn = "CN=Infrastructure," + domain_dn
> -        self.naming_dn = "CN=Partitions,%s" % samdb.get_config_basedn()
> -        self.schema_dn = samdb.get_schema_basedn()
> -        self.rid_dn = "CN=RID Manager$,CN=System," + domain_dn
> -
> -        res = samdb.search(self.infrastructure_dn,
> -                           scope=ldb.SCOPE_BASE, attrs=["fSMORoleOwner"])
> -        assert len(res) == 1
> -        self.infrastructureMaster = res[0]["fSMORoleOwner"][0]
> -
> -        res = samdb.search(domain_dn,
> -                           scope=ldb.SCOPE_BASE, attrs=["fSMORoleOwner"])
> -        assert len(res) == 1
> -        self.pdcEmulator = res[0]["fSMORoleOwner"][0]
> -
> -        res = samdb.search(self.naming_dn,
> -                           scope=ldb.SCOPE_BASE, attrs=["fSMORoleOwner"])
> -        assert len(res) == 1
> -        self.namingMaster = res[0]["fSMORoleOwner"][0]
> -
> -        res = samdb.search(self.schema_dn,
> -                           scope=ldb.SCOPE_BASE, attrs=["fSMORoleOwner"])
> -        assert len(res) == 1
> -        self.schemaMaster = res[0]["fSMORoleOwner"][0]
> -
> -        res = samdb.search(self.rid_dn,
> -                           scope=ldb.SCOPE_BASE, attrs=["fSMORoleOwner"])
> -        assert len(res) == 1
> -        self.ridMaster = res[0]["fSMORoleOwner"][0]
> -
> -        self.message("InfrastructureMasterRole owner: " + self.infrastructureMaster)
> -        self.message("RidAllocationMasterRole owner: " + self.ridMaster)
> -        self.message("PdcEmulationMasterRole owner: " + self.pdcEmulator)
> -        self.message("DomainNamingMasterRole owner: " + self.namingMaster)
> -        self.message("SchemaMasterRole owner: " + self.schemaMaster)
> +        forest_dn = "DC=" + samdb.forest_dns_name().replace(".", ",DC=")
^^ Hmm, do we not have a standard function for determining a dn from a DNS name, or a way to get the forest DN?

> +        infrastructure_dn = "CN=Infrastructure," + domain_dn
> +        naming_dn = "CN=Partitions,%s" % samdb.get_config_basedn()
> +        schema_dn = samdb.get_schema_basedn()
> +        rid_dn = "CN=RID Manager$,CN=System," + domain_dn
> +        domaindns_dn = "CN=Infrastructure,DC=DomainDnsZones," + domain_dn
> +        forestdns_dn = "CN=Infrastructure,DC=ForestDnsZones," + forest_dn
> +
> +        infrastructureMaster = get_fsmo_roleowner(samdb, infrastructure_dn)
> +        pdcEmulator = get_fsmo_roleowner(samdb, domain_dn)
> +        namingMaster = get_fsmo_roleowner(samdb, naming_dn)
> +        schemaMaster = get_fsmo_roleowner(samdb, schema_dn)
> +        ridMaster = get_fsmo_roleowner(samdb, rid_dn)
> +        domaindnszonesMaster = get_fsmo_roleowner(samdb, domaindns_dn)
> +        forestdnszonesMaster = get_fsmo_roleowner(samdb, forestdns_dn)
> +
> +        self.message("SchemaMasterRole owner: " + schemaMaster)
> +        self.message("InfrastructureMasterRole owner: " + infrastructureMaster)
> +        self.message("RidAllocationMasterRole owner: " + ridMaster)
> +        self.message("PdcEmulationMasterRole owner: " + pdcEmulator)
> +        self.message("DomainNamingMasterRole owner: " + namingMaster)
> +        self.message("DomainDnsZonesMasterRole owner: " + domaindnszonesMaster)
> +        self.message("ForestDnsZonesMasterRole owner: " + forestdnszonesMaster)
>  
>  
>  class cmd_fsmo_transfer(Command):
> @@ -234,16 +413,20 @@ class cmd_fsmo_transfer(Command):
>          }
>  
>      takes_options = [
> -        Option("-H", "--URL", help="LDB URL for database or target server", type=str,
> -               metavar="URL", dest="H"),
> -        Option("--role", type="choice", choices=["rid", "pdc", "infrastructure","schema","naming","all"],
> +        Option("-H", "--URL", help="LDB URL for database or target server", 
> +               type=str, metavar="URL", dest="H"),
> +        Option("--role", type="choice", choices=["rid", "pdc", "infrastructure",
> +               "schema", "naming", "domaindns", "forestdns", "all"],
>                 help="""The FSMO role to seize or transfer.\n
> -rid=RidAllocationMasterRole\n
> -schema=SchemaMasterRole\n
> -pdc=PdcEmulationMasterRole\n
> -naming=DomainNamingMasterRole\n
> -infrastructure=InfrastructureMasterRole\n
> -all=all of the above"""),
> +rid=RidAllocationMasterRole                         \n
> +schema=SchemaMasterRole                             \n
> +pdc=PdcEmulationMasterRole                          \n
> +naming=DomainNamingMasterRole                       \n
> +infrastructure=InfrastructureMasterRole             \n
> +domaindns=DomainDnsZonesMasterRole                  \n
> +forestdns=ForestDnsZonesMasterRole                  \n
> +all=all of the above                                \n
> +You must provide an Admin user and password."""),
>          ]
>  
>      takes_args = []
> @@ -263,8 +446,13 @@ all=all of the above"""),
>              transfer_role(self.outf, "naming", samdb)
>              transfer_role(self.outf, "infrastructure", samdb)
>              transfer_role(self.outf, "schema", samdb)
> +            transfer_dns_role(self.outf, sambaopts, credopts, "domaindns", samdb)
> +            transfer_dns_role(self.outf, sambaopts, credopts, "forestdns", samdb)
>          else:
> -            transfer_role(self.outf, role, samdb)
> +            if role == "domaindns" or role == "forestdns":
> +                transfer_dns_role(self.outf, sambaopts, credopts, role, samdb)
> +            else:
> +                transfer_role(self.outf, role, samdb)
>  
>  
>  class cmd_fsmo(SuperCommand):
> -- 
> 1.7.10.4
> 



More information about the samba-technical mailing list