cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

one useful comment/question and some nits.


Diff comments:

> diff --git a/cloudinit/net/sysconfig.py b/cloudinit/net/sysconfig.py
> index 39d89c4..87fb96e 100644
> --- a/cloudinit/net/sysconfig.py
> +++ b/cloudinit/net/sysconfig.py
> @@ -550,6 +550,26 @@ class Renderer(renderer.Renderer):
>              cls._render_subnet_routes(iface_cfg, route_cfg, iface_subnets)
>  
>      @classmethod
> +    def _render_static_routes(cls, base_sysconf_dir, network_state, contents):
> +        """Render global routes into /etc/sysconfig/static-routes file"""
> +        sr_content = []
> +        for route in network_state.iter_routes():
> +            ipr_mapping = {'network': 'net', 'netmask': 'netmask',

move this outside the loop. you're just re-declaring a variable each time.

> +                           'metric': 'metric', 'gateway': 'gw'}
> +            route_fmt = ['any']
> +            # use a specific order to generate a net-tools/route command
> +            for kw in ['network', 'netmask', 'metric', 'gateway']:
> +                if kw in route:
> +                    route_fmt.extend([ipr_mapping[kw], str(route[kw])])
> +            if route_fmt:

i'm pretty sure that 'route_fmt' will always be true, since you've initializied it to 'any' above.

perhaps instead do:
  route_fmt = []
  for kw ...
 
  if route_fmt:
     sr_content.append("any" + " ".join(route_fmt))

> +                sr_content.append(" ".join(route_fmt))
> +
> +        if sr_content:

should we always write this?
Am I right that this the only place where a user could add "global" routes?
if one instance wrote some static routes, and capture and re-launch, there would be old content there.
we'd want very least to clear out our content.  if we blindly deleted it, then non-cloud-init changes would be blown away too.

maybe just tag cloud-init authored lines with '#cloud-init' and then remove ones that are there?

> +            static_routes_fn = os.path.join(base_sysconf_dir, 'static-routes')
> +            sr_content = [_make_header()] + sr_content
> +            contents[static_routes_fn] = "\n".join(sr_content) + "\n"
> +
> +    @classmethod
>      def _render_sysconfig(cls, base_sysconf_dir, network_state):
>          '''Given state, return /etc/sysconfig files + contents'''
>          iface_contents = {}
> diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
> index c12a487..8861353 100644
> --- a/tests/unittests/test_net.py
> +++ b/tests/unittests/test_net.py
> @@ -1610,6 +1613,32 @@ class TestSysConfigRendering(CiTestCase):
>          if missing:
>              raise AssertionError("Missing headers in: %s" % missing)
>  
> +    def _assert_static_routes(self, network_config, found):
> +        static_routes = list(self.network_state.iter_routes())
> +        snet_keys = ['network', 'netmask', 'metric', 'gateway']
> +        if len(static_routes) > 0:
> +            self.assertIn('/etc/sysconfig/static-routes', found)
> +            sroute_fn = os.path.join(self.render_dir,
> +                                     'etc/sysconfig/static-routes')
> +            self.assertTrue(os.path.exists(sroute_fn))
> +            sroute_content = util.load_file(sroute_fn)
> +            for line in [line for line in sroute_content.splitlines()
> +                         if line.startswith('any')]:
> +                m = re.search(r'any\snet\s(?P<network>\S+)\s'
> +                              r'netmask\s(?P<netmask>\S+)\s'
> +                              r'metric\s(?P<metric>\S+)\s'
> +                              r'gw\s(?P<gateway>\S+)', line)

probably fewer line breaks if you moved this outside of the loop to a variable
route_re = (r'any\snet\s(?P<network>\S+)\snetmask\s(?P<netmask>\S+)\s'
            r'metric\s(?P<metric>\S+)\sgw\s(?P<gateway>\S+)')

...

fwiw, route_re.match anchors to the beginning of the line, so you
could drop the 'line.startswith' or just use the '^' in the re 
for re.search.

found_match = route
for line in sroute_content.splitlines():
    m = re.match(route_re)
    if not m:
       continue # or ... i think maybe this is an error, could just raise.

> +                found_route = m.groupdict('')
> +                if 'metric' in found_route:
> +                    found_route['metric'] = int(found_route['metric'])
> +                found_match = []
> +                for sroute in static_routes:
> +                    missing = [key for key in snet_keys
> +                               if found_route[key] != sroute[key]]
> +                    if not missing:
> +                        found_match.append(sroute)
> +                self.assertEqual(1, len(found_match))
> +
>      @mock.patch("cloudinit.net.sys_dev_path")
>      @mock.patch("cloudinit.net.read_sys_net")
>      @mock.patch("cloudinit.net.get_devicelist")


-- 
https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/342102
Your team cloud-init commiters is requested to review the proposed merge of ~raharper/cloud-init:sysconfig-handle-global-static-routes into cloud-init:master.