Repository: Azure/WALinuxAgent
Pull request #271 (2016-06-27 22:03:55)

Dhcp caching
When we generalize and capture an image, dhcp options exist on disk. When that image is used to create a vm, communication with the cached endpoint will fail, so we need to allow for both cache invalidation and multiple valid but distinct entries in the leases file. 

- fix bug in cache lookup logic - take the the last valid entry, not the first
- allow skipping the cache in failure mode

msftclas: Hi __@hglkrijger__, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
    <p>
        It looks like you're a Microsoft contributor (Hans Krijger). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.
    </p>

TTYL, MSBOT;

msftclas: Hi __@hglkrijger__, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
    <p>
        It looks like you're a Microsoft contributor (Hans Krijger). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.
    </p>

TTYL, MSBOT;


Patch:
diff --git a/azurelinuxagent/common/dhcp.py b/azurelinuxagent/common/dhcp.py
index 8c81f248..d5c90cb0 100644
--- a/azurelinuxagent/common/dhcp.py
+++ b/azurelinuxagent/common/dhcp.py
@@ -49,6 +49,7 @@ class DhcpHandler(object):
         self.gateway = None
         self.routes = None
         self._request_broadcast = False
+        self.skip_cache = False
 
     def run(self):
         """
@@ -56,9 +57,11 @@ class DhcpHandler(object):
         Configure default gateway and routes
         Save wire server endpoint if found
         """
-        if not self.wireserver_route_exists and not self.dhcp_lease_exists:
-            self.send_dhcp_req()
-            self.conf_routes()
+        if self.wireserver_route_exists or self.dhcp_cache_exists:
+            return
+
+        self.send_dhcp_req()
+        self.conf_routes()
 
     def wait_for_network(self):
         """
@@ -104,17 +107,23 @@ class DhcpHandler(object):
         return route_exists
 
     @property
-    def dhcp_lease_exists(self):
+    def dhcp_cache_exists(self):
         """
         Check whether the dhcp options cache exists and contains the
-        wireserver endpoint.
+        wireserver endpoint, unless skip_cache is True.
         :return: True if the cached endpoint was found in the dhcp lease
         """
+        if self.skip_cache:
+            return False
+
         exists = False
+
+        logger.info("checking for dhcp lease cache")
         cached_endpoint = self.osutil.get_dhcp_lease_endpoint()
         if cached_endpoint is not None:
             self.endpoint = cached_endpoint
             exists = True
+        logger.info("cache exists [{0}]".format(exists))
         return exists
 
     def conf_routes(self):
diff --git a/azurelinuxagent/common/osutil/default.py b/azurelinuxagent/common/osutil/default.py
index 2cf34ce4..76d48ca9 100644
--- a/azurelinuxagent/common/osutil/default.py
+++ b/azurelinuxagent/common/osutil/default.py
@@ -73,7 +73,7 @@ class DefaultOSUtil(object):
         userentry = self.get_userentry(username)
         uidmin = None
         try:
-            uidmin_def = fileutil.get_line_startingwith("UID_MIN", 
+            uidmin_def = fileutil.get_line_startingwith("UID_MIN",
                                                         "/etc/login.defs")
             if uidmin_def is not None:
                 uidmin = int(uidmin_def.split()[1])
@@ -215,7 +215,7 @@ class DefaultOSUtil(object):
             pub_path = os.path.join(lib_dir, thumbprint + '.pub')
             pub = crytputil.get_pubkey_from_crt(crt_path)
             fileutil.write_file(pub_path, pub)
-            self.set_selinux_context(pub_path, 
+            self.set_selinux_context(pub_path,
                                      'unconfined_u:object_r:ssh_home_t:s0')
             self.openssl_to_openssh(pub_path, path)
             fileutil.chmod(pub_path, 0o600)
@@ -320,7 +320,7 @@ class DefaultOSUtil(object):
         retcode = self.umount(mount_point, chk_err=chk_err)
         if chk_err and retcode != 0:
             raise OSUtilError("Failed to umount dvd.")
-    
+
     def eject_dvd(self, chk_err=True):
         dvd = self.get_dvd_device()
         retcode = shellutil.run("eject {0}".format(dvd))
@@ -547,6 +547,7 @@ class DefaultOSUtil(object):
         FOOTER_LEASE = "}"
         FORMAT_DATETIME = "%Y/%m/%d %H:%M:%S"
 
+        logger.info("looking for leases in path [{0}]".format(pathglob))
         for lease_file in glob.glob(pathglob):
             leases = open(lease_file).read()
             if HEADER_OPTION in leases:
@@ -557,6 +558,7 @@ class DefaultOSUtil(object):
                     if line.startswith(HEADER_LEASE):
                         cached_endpoint = None
                         has_option_245 = False
+                        expired = True
                     elif HEADER_DNS in line:
                         cached_endpoint = line.replace(HEADER_DNS, '').strip(" ;")
                     elif HEADER_OPTION in line:
@@ -573,10 +575,17 @@ class DefaultOSUtil(object):
                             except:
                                 logger.error("could not parse expiry token '{0}'".format(line))
                     elif FOOTER_LEASE in line:
+                        logger.info("dhcp entry:{0}, 245:{1}, expired:{2}".format(
+                            cached_endpoint, has_option_245, expired))
                         if not expired and cached_endpoint is not None and has_option_245:
                             endpoint = cached_endpoint
-                            # return the first valid entry
-                            break
+                            logger.info("found endpoint [{0}]".format(endpoint))
+                            # we want to return the last valid entry, so
+                            # keep searching
+        if endpoint is not None:
+            logger.info("cached endpoint found [{0}]".format(endpoint))
+        else:
+            logger.info("cached endpoint not found")
         return endpoint
 
     def is_missing_default_route(self):
@@ -761,7 +770,7 @@ class DefaultOSUtil(object):
             return int(ret[1])
         else:
             raise OSUtilError("Failed to get processor cores")
-    
+
     def set_admin_access_to_ip(self, dest_ip):
         #This allows root to access dest_ip
         rm_old= "iptables -D OUTPUT -d {0} -j ACCEPT -m owner --uid-owner 0"
diff --git a/azurelinuxagent/common/protocol/util.py b/azurelinuxagent/common/protocol/util.py
index 98adeeda..7e7a74f1 100644
--- a/azurelinuxagent/common/protocol/util.py
+++ b/azurelinuxagent/common/protocol/util.py
@@ -139,6 +139,7 @@ class ProtocolUtil(object):
         except ProtocolError as e:
             logger.info("WireServer is not responding. Reset endpoint")
             self.dhcp_handler.endpoint = None
+            self.dhcp_handler.skip_cache = True
             raise e
 
     def _detect_metadata_protocol(self):


Test patch:
diff --git a/tests/common/dhcp/test_dhcp.py b/tests/common/dhcp/test_dhcp.py
index 5e4d6085..c27ffe88 100644
--- a/tests/common/dhcp/test_dhcp.py
+++ b/tests/common/dhcp/test_dhcp.py
@@ -65,12 +65,43 @@ class TestDHCP(AgentTestCase):
         self.assertTrue(dhcp_handler.routes is None)
         self.assertTrue(dhcp_handler.gateway is None)
 
-    def test_dhcp_lease_exists(self):
+    def test_dhcp_cache_exists(self):
         dhcp_handler = dhcp.get_dhcp_handler()
         dhcp_handler.osutil = osutil.DefaultOSUtil()
-        with patch.object(osutil.DefaultOSUtil, 'get_dhcp_lease_endpoint', return_value=None):
-            self.assertFalse(dhcp_handler.dhcp_lease_exists)
+        with patch.object(osutil.DefaultOSUtil, 'get_dhcp_lease_endpoint',
+                          return_value=None):
+            self.assertFalse(dhcp_handler.dhcp_cache_exists)
             self.assertEqual(dhcp_handler.endpoint, None)
-        with patch.object(osutil.DefaultOSUtil, 'get_dhcp_lease_endpoint', return_value="foo"):
-            self.assertTrue(dhcp_handler.dhcp_lease_exists)
+        with patch.object(osutil.DefaultOSUtil, 'get_dhcp_lease_endpoint',
+                          return_value="foo"):
+            self.assertTrue(dhcp_handler.dhcp_cache_exists)
             self.assertEqual(dhcp_handler.endpoint, "foo")
+
+    def test_dhcp_skip_cache(self):
+        handler = dhcp.get_dhcp_handler()
+        handler.osutil = osutil.DefaultOSUtil()
+        with patch('os.path.exists', return_value=False):
+            with patch.object(osutil.DefaultOSUtil, 'get_dhcp_lease_endpoint')\
+                    as patch_dhcp_cache:
+                with patch.object(dhcp.DhcpHandler, 'send_dhcp_req') \
+                        as patch_dhcp_send:
+
+                    endpoint = 'foo'
+                    patch_dhcp_cache.return_value = endpoint
+
+                    # endpoint comes from cache
+                    self.assertFalse(handler.skip_cache)
+                    handler.run()
+                    self.assertTrue(patch_dhcp_cache.call_count == 1)
+                    self.assertTrue(patch_dhcp_send.call_count == 0)
+                    self.assertTrue(handler.endpoint == endpoint)
+
+                    # reset
+                    handler.skip_cache = True
+                    handler.endpoint = None
+
+                    # endpoint comes from dhcp request
+                    self.assertTrue(handler.skip_cache)
+                    handler.run()
+                    self.assertTrue(patch_dhcp_cache.call_count == 1)
+                    self.assertTrue(patch_dhcp_send.call_count == 1)
diff --git a/tests/common/osutil/test_default.py b/tests/common/osutil/test_default.py
index 23c5f57e..22813484 100644
--- a/tests/common/osutil/test_default.py
+++ b/tests/common/osutil/test_default.py
@@ -124,5 +124,12 @@ class TestOSUtil(AgentTestCase):
                 self.assertTrue(endpoint is not None)
                 self.assertEqual(endpoint, "168.63.129.16")
 
+    def test_dhcp_lease_multi(self):
+        with patch.object(glob, "glob", return_value=['/var/lib/dhcp/dhclient.eth0.leases']):
+            with patch(open_patch(), mock.mock_open(read_data=load_data("dhcp.leases.multi"))):
+                endpoint = get_osutil(distro_name='ubuntu', distro_version='12.04').get_dhcp_lease_endpoint()
+                self.assertTrue(endpoint is not None)
+                self.assertEqual(endpoint, "second")
+
 if __name__ == '__main__':
     unittest.main()
diff --git a/tests/data/dhcp.leases b/tests/data/dhcp.leases
index 875b13be..cb4f3961 100644
--- a/tests/data/dhcp.leases
+++ b/tests/data/dhcp.leases
@@ -7,14 +7,15 @@ lease {
   option routers 10.0.1.1;
   option dhcp-message-type 5;
   option dhcp-server-identifier 168.63.129.16;
-  option domain-name-servers expired;
+  option domain-name-servers invalid;
   option dhcp-renewal-time 4294967295;
+  option rfc3442-classless-static-routes 0,10,0,1,1,32,168,63,129,16,10,0,1,1;
   option unknown-245 a8:3f:81:10;
   option dhcp-rebinding-time 4294967295;
   option domain-name "qylsde3bnlhu5dstzf3bav5inc.fx.internal.cloudapp.net";
-  renew 4 2015/06/16 16:58:54;
-  rebind 4 2015/06/16 16:58:54;
-  expire 4 2015/06/16 16:58:54;
+  renew 0 2152/07/23 23:27:10;
+  rebind 0 2152/07/23 23:27:10;
+  expire 0 never;
 }
 lease {
   interface "eth0";
@@ -25,15 +26,14 @@ lease {
   option routers 10.0.1.1;
   option dhcp-message-type 5;
   option dhcp-server-identifier 168.63.129.16;
-  option domain-name-servers 168.63.129.16;
+  option domain-name-servers expired;
   option dhcp-renewal-time 4294967295;
-  option rfc3442-classless-static-routes 0,10,0,1,1,32,168,63,129,16,10,0,1,1;
   option unknown-245 a8:3f:81:10;
   option dhcp-rebinding-time 4294967295;
   option domain-name "qylsde3bnlhu5dstzf3bav5inc.fx.internal.cloudapp.net";
-  renew 0 2152/07/23 23:27:10;
-  rebind 0 2152/07/23 23:27:10;
-  expire 0 2152/07/23 23:27:10;
+  renew 4 2015/06/16 16:58:54;
+  rebind 4 2015/06/16 16:58:54;
+  expire 4 2015/06/16 16:58:54;
 }
 lease {
   interface "eth0";
@@ -44,7 +44,7 @@ lease {
   option routers 10.0.1.1;
   option dhcp-message-type 5;
   option dhcp-server-identifier 168.63.129.16;
-  option domain-name-servers invalid;
+  option domain-name-servers 168.63.129.16;
   option dhcp-renewal-time 4294967295;
   option rfc3442-classless-static-routes 0,10,0,1,1,32,168,63,129,16,10,0,1,1;
   option unknown-245 a8:3f:81:10;
@@ -52,5 +52,5 @@ lease {
   option domain-name "qylsde3bnlhu5dstzf3bav5inc.fx.internal.cloudapp.net";
   renew 0 2152/07/23 23:27:10;
   rebind 0 2152/07/23 23:27:10;
-  expire 0 never;
+  expire 0 2152/07/23 23:27:10;
 }
diff --git a/tests/data/dhcp.leases.multi b/tests/data/dhcp.leases.multi
new file mode 100644
index 00000000..cfe9c67d
--- /dev/null
+++ b/tests/data/dhcp.leases.multi
@@ -0,0 +1,57 @@
+lease {
+  interface "eth0";
+  fixed-address 10.0.1.4;
+  server-name "RDE41D2D9BB18C";
+  option subnet-mask 255.255.255.0;
+  option dhcp-lease-time 4294967295;
+  option routers 10.0.1.1;
+  option dhcp-message-type 5;
+  option dhcp-server-identifier 168.63.129.16;
+  option domain-name-servers first;
+  option dhcp-renewal-time 4294967295;
+  option rfc3442-classless-static-routes 0,10,0,1,1,32,168,63,129,16,10,0,1,1;
+  option unknown-245 a8:3f:81:10;
+  option dhcp-rebinding-time 4294967295;
+  option domain-name "qylsde3bnlhu5dstzf3bav5inc.fx.internal.cloudapp.net";
+  renew 0 2152/07/23 23:27:10;
+  rebind 0 2152/07/23 23:27:10;
+  expire 0 2152/07/23 23:27:10;
+}
+lease {
+  interface "eth0";
+  fixed-address 10.0.1.4;
+  server-name "RDE41D2D9BB18C";
+  option subnet-mask 255.255.255.0;
+  option dhcp-lease-time 4294967295;
+  option routers 10.0.1.1;
+  option dhcp-message-type 5;
+  option dhcp-server-identifier 168.63.129.16;
+  option domain-name-servers second;
+  option dhcp-renewal-time 4294967295;
+  option rfc3442-classless-static-routes 0,10,0,1,1,32,168,63,129,16,10,0,1,1;
+  option unknown-245 a8:3f:81:10;
+  option dhcp-rebinding-time 4294967295;
+  option domain-name "qylsde3bnlhu5dstzf3bav5inc.fx.internal.cloudapp.net";
+  renew 0 2152/07/23 23:27:10;
+  rebind 0 2152/07/23 23:27:10;
+  expire 0 2152/07/23 23:27:10;
+}
+lease {
+  interface "eth0";
+  fixed-address 10.0.1.4;
+  server-name "RDE41D2D9BB18C";
+  option subnet-mask 255.255.255.0;
+  option dhcp-lease-time 4294967295;
+  option routers 10.0.1.1;
+  option dhcp-message-type 5;
+  option dhcp-server-identifier 168.63.129.16;
+  option domain-name-servers expired;
+  option dhcp-renewal-time 4294967295;
+  option rfc3442-classless-static-routes 0,10,0,1,1,32,168,63,129,16,10,0,1,1;
+  option unknown-245 a8:3f:81:10;
+  option dhcp-rebinding-time 4294967295;
+  option domain-name "qylsde3bnlhu5dstzf3bav5inc.fx.internal.cloudapp.net";
+  renew 0 2152/07/23 23:27:10;
+  rebind 0 2152/07/23 23:27:10;
+  expire 0 2012/07/23 23:27:10;
+}