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

Jelmer Vernooij jelmer at jelmer.uk
Tue Jun 2 17:53:34 MDT 2015


Hi Rowland,

Thanks for working on improving samba-tool. Here are some quick
comments - most are around style but there are also some regressions:

On Sat, May 30, 2015 at 04:34:52PM +0100, Rowland Penny wrote:
> From a12c46a8b64e025c30b3d8a717c67cb3b164a5d0 Mon Sep 17 00:00:00 2001
> From: Rowland Penny <repenny241155 at gmail.com>
> Date: Sat, 30 May 2015 16:23:11 +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 |  282 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 236 insertions(+), 46 deletions(-)
> 
> diff --git a/python/samba/netcmd/fsmo.py b/python/samba/netcmd/fsmo.py
> index 1bc4a96..031b927 100644
> --- a/python/samba/netcmd/fsmo.py
> +++ b/python/samba/netcmd/fsmo.py
> +
> +    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')))
^^ Please keep lines under 80 characters (see PEP8 -
https://www.python.org/dev/peps/pep-0008/)

> +                master_owner = str(ldb.Dn(samdb, res[0]['fSMORoleOwner'][0]))
> +            except:
> +                print "Can't find GUID in naming master on partition DN %s" % res[0]['fSMORoleOwner'][0]
^^ Please write to self.outf rather than stdout *or* raise an
exception (e.g. CommandError)

> +                return
> +    except LdbError, (num, msg):
> +        raise CommandError("DNS partion %s not found : %s" % (role, msg))
> +        return
^^ There is no need to return after "raise"; it doesn't have any effect.

> +
> +    if role == "domaindns":
> +        master_dns_name = '%s._msdcs.%s' % (master_guid, samdb.domain_dns_name())
> +        new_dns_name = '%s._msdcs.%s' % (samdb.get_ntds_GUID(), samdb.domain_dns_name())
> +    elif role == "forestdns":
> +        master_dns_name = '%s._msdcs.%s' % (master_guid, samdb.forest_dns_name())
> +        new_dns_name = '%s._msdcs.%s' % (samdb.get_ntds_GUID(), samdb.forest_dns_name())
> +
> +    new_owner = samdb.get_dsServiceName()
> +
> +    if master_dns_name != new_dns_name:
> +        lp = sambaopts.get_loadparm()
> +        creds = credopts.get_credentials(lp, fallback_machine=True)
> +        samdb = SamDB(url="ldap://%s" % (master_dns_name), session_info=system_session(),
> +                      credentials=creds, lp=lp)
> +
> +        m = ldb.Message()
> +        m.dn = ldb.Dn(samdb, role_object)
> +        m["fSMORoleOwner"]= ldb.MessageElement(
> +            "%s" % master_owner, ldb.FLAG_MOD_DELETE,
^^ master_owner is already a string, there's no need to use a format
string here. It looks like this could also be all on one line.

> +            "fSMORoleOwner")
> +
> +        try:
> +            samdb.modify(m)
> +        except LdbError, (num, msg):
> +            raise CommandError("Failed to delete role '%s': %s" % (role, msg))
> +        else:
^^ If you're raise an exception, there's no need for an else
statement. No having an else statement prevents lots of indentation,
making the code easier to read.

> +             m = ldb.Message()
> +             m.dn = ldb.Dn(samdb, role_object)
> +             m["fSMORoleOwner"]= ldb.MessageElement(
> +                 "%s" % new_owner, ldb.FLAG_MOD_ADD,
^^ IIUC new_owner is already a string

> +                 "fSMORoleOwner")
> +             try:
> +                 samdb.modify(m)
> +             except LdbError, (num, msg):
> +                 raise CommandError("Failed to add role '%s': %s" % (role, msg))
> +             else:
^^ Same comment as above wrt else

> +                  try:
> +                      connection = (samba.drs_utils.drsuapi_connect(samdb.host_dns_name(), lp, creds))
^^ There is no need for parentheses around this function call

> +                  except samba.drs_utils.drsException, estr:
> +                      raise CommandError("Drsuapi Connect failed", estr)
^^ estr will actually be an exception object (not a string) so calling
"estr" is somewhat misleading. Perhaps just "e" ?

> +                  else:
^^ Same comment as above wrt else

> +                       try:
> +                           drsuapi_connection = connection[0]
> +                           drsuapi_handle = connection[1]
> +                           req_options = drsuapi.DRSUAPI_DRS_WRIT_REP
> +                           NC = role_object[18:]
> +                           samba.drs_utils.sendDsReplicaSync(drsuapi_connection, drsuapi_handle, master_guid, NC, req_options)
> +                       except samba.drs_utils.drsException, estr:
> +                           raise CommandError("Replication failed", estr)
> +
> +        outf.write("FSMO transfer of '%s' role successful\n" % role)
> +    else:
> +        print "This DC already has the '%s' FSMO role" % role
^^ Please write to outf and not to standard out.

> +
> +
>  def transfer_role(outf, role, samdb):
^^ Please add a docstring to this function

> +    domain_dn = samdb.domain_dn()
> +    new_owner = samdb.get_dsServiceName()
>      m = ldb.Message()
>      m.dn = ldb.Dn(samdb, "")
>      if role == "rid":
> +        rid_dn = "CN=RID Manager$,CN=System," + domain_dn
> +        res = samdb.search(rid_dn,
> +                           scope=ldb.SCOPE_BASE, attrs=["fSMORoleOwner"])
> +        assert len(res) == 1
^^ It seems that there is fair bit of duplication between all of these
if statements. Perhaps some of the code can be factored out?

> +        master_owner = res[0]["fSMORoleOwner"][0]
>          m["becomeRidMaster"]= ldb.MessageElement(
>              "1", ldb.FLAG_MOD_REPLACE,
>              "becomeRidMaster")
>      elif role == "pdc":
> -        domain_dn = samdb.domain_dn()
> +        res = samdb.search(domain_dn,
> +                           scope=ldb.SCOPE_BASE, attrs=["fSMORoleOwner"])
> +        assert len(res) == 1
> +        master_owner = res[0]["fSMORoleOwner"][0]
>          res = samdb.search(domain_dn,
>                             scope=ldb.SCOPE_BASE, attrs=["objectSid"])
>          assert len(res) == 1
> @@ -47,24 +136,44 @@ def transfer_role(outf, role, samdb):
>              sid, ldb.FLAG_MOD_REPLACE,
>              "becomePdc")
>      elif role == "naming":
> +        naming_dn = "CN=Partitions,%s" % samdb.get_config_basedn()
> +        res = samdb.search(naming_dn,
> +                           scope=ldb.SCOPE_BASE, attrs=["fSMORoleOwner"])
> +        assert len(res) == 1
> +        master_owner = res[0]["fSMORoleOwner"][0]
>          m["becomeDomainMaster"]= ldb.MessageElement(
>              "1", ldb.FLAG_MOD_REPLACE,
>              "becomeDomainMaster")
>      elif role == "infrastructure":
> +        infrastructure_dn = "CN=Infrastructure," + domain_dn
> +        res = samdb.search(infrastructure_dn,
> +                           scope=ldb.SCOPE_BASE, attrs=["fSMORoleOwner"])
> +        assert len(res) == 1
> +        master_owner = res[0]["fSMORoleOwner"][0]
>          m["becomeInfrastructureMaster"]= ldb.MessageElement(
>              "1", ldb.FLAG_MOD_REPLACE,
>              "becomeInfrastructureMaster")
>      elif role == "schema":
> +        schema_dn = str(samdb.get_schema_basedn())
> +        res = samdb.search(schema_dn,
> +                           scope=ldb.SCOPE_BASE, attrs=["fSMORoleOwner"])
> +        assert len(res) == 1
> +        master_owner = res[0]["fSMORoleOwner"][0]
>          m["becomeSchemaMaster"]= ldb.MessageElement(
>              "1", ldb.FLAG_MOD_REPLACE,
>              "becomeSchemaMaster")
>      else:
>          raise CommandError("Invalid FSMO role.")
> -    try:
> -        samdb.modify(m)
> -    except LdbError, (num, msg):
> -        raise CommandError("Failed to initiate transfer of '%s' role: %s" % (role, msg))
> -    outf.write("FSMO transfer of '%s' role successful\n" % role)
> +
> +    if master_owner != new_owner:
> +        try:
> +            samdb.modify(m)
> +        except LdbError, (num, msg):
> +            raise CommandError("Failed to initiate transfer of '%s' role: %s" % (role, msg))
> +        else:
> +            outf.write("FSMO transfer of '%s' role successful\n" % role)
> +    else:
> +        print "This DC already has the '%s' FSMO role" % role
^^ Please use outf.write rather than print.

>  
>  
>  class cmd_fsmo_seize(Command):
> @@ -82,23 +191,23 @@ class cmd_fsmo_seize(Command):
>          Option("-H", "--URL", help="LDB URL for database or target server", type=str,
>                 metavar="URL", dest="H"),
>          Option("--force", help="Force seizing of the role without attempting to transfer first.", action="store_true"),
> -        Option("--role", type="choice", choices=["rid", "pdc", "infrastructure","schema","naming","all"],
> +        Option("--role", type="choice", choices=["rid", "pdc", "infrastructure","schema","naming","domaindns","forestdns","all"],
^^ Please add a space after comma's (see PEP8) and keep lines under 80
characters. This line actually already seems to be violating that, but
we should fix it if this line is being changed anyway.

>                 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
^^ Why line up the newlines?

> +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."""),
>          ]
> @@ -119,26 +228,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...")
> +        res = samdb.search(m.dn,
> +                           scope=ldb.SCOPE_BASE, attrs=["fSMORoleOwner"])
> +        assert len(res) == 1
> +        master_owner = res[0]["fSMORoleOwner"][0]
> +        if master_owner != serviceName:
> +            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...
> +                    self.message("Transfer unsuccessful, seizing...")
^^ Why does this catch CommandError? samba-tool will display it
properly if you just let it propagate to the caller.

Also, self.message() doesn't raise an exception, so your program flow
continues here despite the error message saying it is seizing.

> +            else:
> +                self.message("Will not attempt transfer, seizing...")
^^ This should print why it will not transfer. Like above, this
doesn't actually seize the flow.

> +
> +            m["fSMORoleOwner"]= ldb.MessageElement(
> +                serviceName, ldb.FLAG_MOD_REPLACE,
> +                "fSMORoleOwner")
>              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...
> -                self.message("Transfer unsuccessful, seizing...")
> +                samdb.modify(m)
> +            except LdbError, (num, msg):
> +                raise CommandError("Failed to initiate role seize of '%s' role: %s" % (role, msg))
> +            else:
> +                self.outf.write("FSMO seize of '%s' role successful\n" % role)
>          else:
> -            self.message("Will not attempt transfer, seizing...")
> +            print "This DC already has the '%s' FSMO role" % role
^ Please use self.outf or self.message.
>  
> -        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):
> +        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
> +        res = samdb.search(m.dn,
> +                           scope=ldb.SCOPE_BASE, attrs=["fSMORoleOwner"])
> +        assert len(res) == 1
> +        master_owner = res[0]["fSMORoleOwner"][0]
> +        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("FSMO seize was not required, as transfer of '%s' role was successful\n" % role)
^ This line looks like it is too long
> +                    return
> +                except CommandError:
> +                #transfer failed, use the big axe...
> +                    self.message("Transfer unsuccessful, seizing...")
> +            else:
> +                self.message("Will not attempt transfer, seizing...")
> +
> +            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))
> +            else:
> +                self.outf.write("FSMO seize of '%s' role successful\n" % role)
> +        else:
> +            print "This DC already has the '%s' FSMO role" % role
>  
>      def run(self, force=None, H=None, role=None,
>              credopts=None, sambaopts=None, versionopts=None):

Cheers,

Jelmer


More information about the samba-technical mailing list