Fix unneeded stops and restarts in AD monitoring #1340

anodos325 · 2018-06-09T15:44:41Z

Ticket #33453

There are a several distinct situations where our AD monitoring will either turn off the Active Directory service or restart the Active Directory service when it is not required.
The improvements are as follows:

Use SRV records to enumerate the LDAP servers in the environment and then check to see if the LDAP port is listening on any of them. If we pass this check, then we do not turn off AD service.
Simplify checks to see if the AD service is properly running, by reducing it to a single test "wbinfo -t". This reduces the liklihood of a false-positive that we are no longer joined to the domain.
Add a low-impact recovery attempt if we fail the getStarted() check. Winbind in its current form establishes a connection to only a single DC. It has been observed in testing that if this DC goes down, then we will fail the getStarted() check. Restarting winbind was sufficient in these situations to put the domain join back into a healthy state. Try this first before performing a more disruptive recovery attempt.
The default AD DNS timeout and lifetime values make it so that if Nameserver 1 in the FreeNAS configuration is down, then get_SRV_records() in freenasldap.py will always fail (we never try to get SRV records from Nameserver 2). If we reduce the timeout to 1/3 of the overall lifetime, then we will query other nameservers and AD will stay up.

b-a-t · 2018-06-12T19:03:21Z

rebuild

b-a-t · 2018-06-12T19:34:33Z

src/middlewared/middlewared/plugins/service_monitor.py

-                s.connect((host, port))
-                connected = True
+            for i in range(0, max_tries):
+                # Make three attempts to get SRV records from DNS


"Make max_tries attempts...." - current value of 3 is definable

b-a-t · 2018-06-13T01:30:42Z

gui/common/freenasldap.py

        r.lifetime = _fs().directoryservice.activedirectory.dns.lifetime
+        r.timeout = r.lifetime / 3



I understand the reasons you made it this way and that you don't want to mess with sysctl() updates, but I'd keep ability to modify timeout through the sysctl(). How about:

r.timeout = _fs().directoryservice.activedirectory.dns.timeout / 2
Or
r.lifetime = _fs().directoryservice.activedirectory.dns.lifetime * 2

b-a-t · 2018-06-13T01:42:54Z

src/middlewared/middlewared/plugins/service_monitor.py

+                if host_list:
+                    break
+                else:
+                    self.logger.debug("[ServiceMonitorThread] Attempt %d to query SRV records failed " % (i))


Please, try to avoid legacy "%x" % (v) syntax. Use either f'{v}' or '{}'.format(v) instead.

b-a-t · 2018-06-13T01:53:54Z

src/middlewared/middlewared/plugins/service_monitor.py


-            if connected:
-                break
+        for h in host_list:


It's a bit hard to see it here, but looks like host_list you are referencing here is declared, if any, in another scope. I actually commented about this to you before.
It's declared on an inner scope regarding this one. Also, if if self.name != 'activedirectory': - it's not declared at all. Move host_list = [] outside of the if.

b-a-t · 2018-06-13T01:55:50Z

src/middlewared/middlewared/plugins/service_monitor.py

+        elif service == 'ldap':
+            fnldap = FreeNAS_LDAP(flags=FLAGS_DBINIT)
+        else:
+            fnldap = None


Minor thing, but you can just declare above fnldap = None and avoid having else: clause.

b-a-t · 2018-06-13T01:57:02Z

src/middlewared/middlewared/plugins/service_monitor.py

@@ -82,31 +85,36 @@ def isEnabled(self, service):
        return enabled

    @private
-    def tryConnect(self, host, port):
+    def tryConnect(self, host, port, fnldap):


Better to declare it as: def tryConnect(self, host, port, fnldap = None):

Actually, as we only call get_ldap_servers we don't need all this code around fnldap, see below.

b-a-t · 2018-06-13T02:01:13Z

@miwi-fbsd says that Jenkins fails cause your branch is outdated and requires rebase. Can you try it with next iteration?

b-a-t · 2018-06-13T02:11:49Z

src/middlewared/middlewared/plugins/service_monitor.py

+            for i in range(0, max_tries):
+                # Make max_tries attempts to get SRV records from DNS
+                host_list = fnldap.get_ldap_servers(host)
+                if host_list:


So, who is the host here? Originally that was one of the DCs, but get_ldap_servers() expects domain instead:

def get_ldap_servers(domain, site=None): dcs = [] if not domain: return dcs host = "_ldap._tcp.%s" % domain if site: host = "_ldap._tcp.%s._sites.%s" % (site, domain) dcs = FreeNAS_ActiveDirectory_Base.get_SRV_records(host) return dcs

Host is ultimately from freenas-v1.db here:
sqlite> SELECT sm_host FROM services_servicemonitor ;
sm_host = THEGIBSON.LAN

thread = ServiceMonitorThread( id=s['id'], frequency=s['sm_frequency'], retry=s['sm_retry'], host=s['sm_host'], port=s['sm_port'], name=thread_name, logger=self.logger, middleware=self.middleware )

It is the domain.

b-a-t · 2018-06-13T02:16:35Z

src/middlewared/middlewared/plugins/service_monitor.py

-        for i in range(0, max_tries):
-            # XXX What about UDP?
-            s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
-            s.settimeout(timeout)


def port_is_listening(host, port, errors=[]): looks very similar to the given code, except it doesn't set any timeout for the socket connection. It may be reasonable to add:

def port_is_listening(host, port, errors=[], timeout=0): .... if timeout: s.settimeout(timeout)

in the freenasldap.py.

I think we talked about this Andrew, timur is correct

Okay. Made changes.

b-a-t · 2018-06-13T02:39:19Z

src/middlewared/middlewared/plugins/service_monitor.py

-                connected = True
+            for i in range(0, max_tries):
+                # Make max_tries attempts to get SRV records from DNS
+                host_list = fnldap.get_ldap_servers(host)


We can simply call:

host_list = FreeNAS_ActiveDirectory_Base.get_ldap_servers(host)

as it's a static method of the class. So no need for creating fnldap object at all, neither for passing it around.

b-a-t · 2018-06-13T02:48:46Z

Found in the dns.resolver.__init__ reference to the attribute self.rotate, which shuffles list of the nameservers, allowing to pick the random one for the start. Not sure, does that make any benefit in this situation.

Incorporate suggestions form Timur to clean up usage of freenasldap.py

jhixson74

I like my bike sheds painted hot pink

jhixson74 · 2018-06-14T03:24:35Z

src/middlewared/middlewared/plugins/service.py

@@ -639,6 +635,14 @@ def _started_notify(self, verb, what):
            res = True
        return res

+    async def _reload_activedirectory(self, **kwargs):


How about naming this "_restart_activedirectory" since that is what it is actually doing ;-) If you are going to make a reload method, then have to reload samba_server ;-)

We already have a _restart_active directory method that calls "/etc/directoryservice/ActiveDirectory/ctl restart". I don't see any reason to change the existing restart method because we want to start from a clean slate to ensure proper AD join.

What I would ideally prefer here is regenerating smb4.conf (if needed) and killing / restarting winbind process as an "AD reload". I.e. _reload_cifs, then restart winbind. This will force new DC connection, but not kill existing SMB sessions.

It should be something less than a full "AD restart", but more than just reloading a config file.

jhixson74 · 2018-06-14T03:25:01Z

src/middlewared/middlewared/plugins/service_monitor.py

@@ -17,7 +16,10 @@
 if not apps.ready:
    django.setup()

-from freenasUI.common.freenassysctl import freenas_sysctl as _fs
+from freenasUI.common.freenasldap import (
+    FreeNAS_ActiveDirectory_Base,


You can just import FreeNAS_ActiveDirectory here, there is no need to use the base class

jhixson74 · 2018-06-14T03:25:28Z

src/middlewared/middlewared/plugins/service_monitor.py

-        for i in range(0, max_tries):
-            # XXX What about UDP?
-            s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
-            s.settimeout(timeout)


I think we talked about this Andrew, timur is correct

jhixson74 · 2018-06-14T03:25:52Z

src/middlewared/middlewared/plugins/service_monitor.py

-                connected = True
+            for i in range(0, max_tries):
+                # Make max_tries attempts to get SRV records from DNS
+                host_list = FreeNAS_ActiveDirectory_Base.get_ldap_servers(host)


s/FreeNAS_ActiveDirectory_Base/FreeNAS_ActiveDirectory/

jhixson74 · 2018-06-14T03:26:07Z

src/middlewared/middlewared/plugins/service_monitor.py

-                s.settimeout(None)
-                s.close()
+            for h in host_list:
+                port_is_listening = FreeNAS_ActiveDirectory_Base.port_is_listening(str(h.target), h.port, errors=[])


s/FreeNAS_ActiveDirectory_Base/FreeNAS_ActiveDirectory/

midclt call servicemonitor.stop resulted in self.finished.set() being called, which effectively removed the frequency control for service monitoring. As long as the service remained "connected", "started", and "enabled", then service monitoring would never stop.

jhixson74

Good to go

Fix unneeded stops and restarts in AD monitoring

15419a1

ghost assigned anodos325 Jun 9, 2018

ghost added the review label Jun 9, 2018

flake8

369f0ee

anodos325 requested review from b-a-t and jhixson74 June 9, 2018 16:00

b-a-t reviewed Jun 12, 2018

View reviewed changes

fix comment

d915970

b-a-t reviewed Jun 13, 2018

View reviewed changes

anodos325 added 3 commits June 13, 2018 20:56

rotate through nameservers when getting SRV records

43e8aa1

Code cleanups in service_monitor.py

8b2114a

Incorporate suggestions form Timur to clean up usage of freenasldap.py

More cleanups in service_monitor.py

7a7500e

jhixson74 reviewed Jun 14, 2018

View reviewed changes

anodos325 added 4 commits June 16, 2018 07:16

Add socket timeout to port_is_listening function

a1a3303

Correct freenasldap.py class and set socket timeout

45a5324

More flake8

cf3b4fc

jhixson74 approved these changes Jul 11, 2018

View reviewed changes

jhixson74 merged commit 172ac4d into master Jul 11, 2018

ghost removed the review label Jul 11, 2018

jhixson74 deleted the FIX-33453-MASTER branch July 11, 2018 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unneeded stops and restarts in AD monitoring #1340

Fix unneeded stops and restarts in AD monitoring #1340

anodos325 commented Jun 9, 2018

b-a-t commented Jun 12, 2018

b-a-t Jun 12, 2018

b-a-t Jun 13, 2018

b-a-t Jun 13, 2018

b-a-t Jun 13, 2018

b-a-t Jun 13, 2018

b-a-t Jun 13, 2018

b-a-t Jun 13, 2018

b-a-t commented Jun 13, 2018

b-a-t Jun 13, 2018

anodos325 Jun 13, 2018 •

edited

b-a-t Jun 13, 2018 •

edited

jhixson74 Jun 14, 2018

anodos325 Jun 17, 2018

b-a-t Jun 13, 2018

b-a-t commented Jun 13, 2018

jhixson74 left a comment

jhixson74 Jun 14, 2018

anodos325 Jun 16, 2018 •

edited

jhixson74 Jun 14, 2018

jhixson74 Jun 14, 2018

jhixson74 Jun 14, 2018

jhixson74 Jun 14, 2018

jhixson74 left a comment

		r.lifetime = _fs().directoryservice.activedirectory.dns.lifetime
		r.timeout = r.lifetime / 3

Fix unneeded stops and restarts in AD monitoring #1340

Fix unneeded stops and restarts in AD monitoring #1340

Conversation

anodos325 commented Jun 9, 2018

b-a-t commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-a-t commented Jun 13, 2018

Choose a reason for hiding this comment

anodos325 Jun 13, 2018 • edited

Choose a reason for hiding this comment

b-a-t Jun 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-a-t commented Jun 13, 2018

jhixson74 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anodos325 Jun 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhixson74 left a comment

Choose a reason for hiding this comment

anodos325 Jun 13, 2018 •

edited

b-a-t Jun 13, 2018 •

edited

anodos325 Jun 16, 2018 •

edited