← Back to team overview

cloud-init-dev team mailing list archive

Re: [Merge] ~mgerdts/cloud-init:lp1765801 into cloud-init:master

 

As you mentioned in your earlier comment, we probably want this behavior to be mutually-exclusive of network hotplug support because a vendor/cloud that doesn't want (or can't support) full hotplug support might want the abilty to update across reboot.


I think I lean toward agreeing with you Mike on the small impact of such a change against our yet to be delivered hotplug.

In our intial hotplug feature discussion https://hackmd.io/M1Tae41PQBC7a9qMsurTJw, it looks like we'd be calling cloud-init using this same entry point with each network hotplug event we handle. To me, it looks like we still have to do nearly the same amount of work to tease apart the is_new_instance() checks in Init.apply_network_config() anyway because I'd expect that the instance-id didn't change in metadata across a single nic hotplug event. 


I have a few inline points and method names, docstrings and structure of those methods and moving tests. but not much beyond that. thanks for this.

Diff comments:

> diff --git a/cloudinit/sources/DataSourceSmartOS.py b/cloudinit/sources/DataSourceSmartOS.py
> index c8998b4..25a35da 100644
> --- a/cloudinit/sources/DataSourceSmartOS.py
> +++ b/cloudinit/sources/DataSourceSmartOS.py
> @@ -288,6 +289,9 @@ class DataSourceSmartOS(sources.DataSource):
>                                             'per-boot'),
>              }
>  
> +        md['maintain_network'] = md['maintain_network'] \

could be the following and still be < 80 chars
        md['maintain_network'] = bool(md.get('maintain_network') == 'true')

> +            and md['maintain_network'] == 'true'
> +
>          self.metadata = util.mergemanydict([md, self.metadata])
>          self.userdata_raw = ud
>          self.vendordata_raw = md['vendor-data']
> diff --git a/cloudinit/sources/__init__.py b/cloudinit/sources/__init__.py
> index df0b374..6cb6aab 100644
> --- a/cloudinit/sources/__init__.py
> +++ b/cloudinit/sources/__init__.py
> @@ -398,6 +399,15 @@ class DataSource(object):
>          """
>          return
>  
> +    def check_apply_network_config(self, is_new_instance):

If feels like there is no need to pass is_new_instance into this method as we are just muddying the datasource when we can do the compound conditional up in cloudinit/stages.py.

This method could just be:

def check_apply_network_config(self):
    return self.metadata.get('maintain_network', False)

and in stages.py:

   if (self.datasource is not NULL_DATA_SOURCE and
       not any([self.is_new_instance(), self.datasource.check_apply_network_config()]):
       LOG.debug("not a new instance nor apply. network config is not applied.")


It'd be nice in this method's docstring to mention that returning True will indicate that we want to update network configuration at boot time so other datasources can decide whether or not to implement such a feature. 



bikeshed alert:
   - let's call this datasource method get_maintain_network() as stages:Init.apply_network_config() is ultimately responsible for taking into account multiple of factors before 'checking' and performing a network update (systemconfig settings from the distro image /etc/cloud/cloud.cfg, whether datasource.get_maintain_network is set and whether in instance_id is new). 
   - another reason not to call it check_apply_network_config is because that's a distro class method which really shouldn't be coupled with DataSource configuration checks and I fear the name consistency between the two lead 'future me' to misintepret that.

> +        """check_apply_network_config(is_new_instance)
> +
> +        By default, networking configuration is only applied when
> +        is_new_instance is True.  A datasource may override this default by
> +        setting manage_network to True.
> +        """
> +        return is_new_instance or self.metadata.get('maintain_network', False)
> +
>  
>  def normalize_pubkey_data(pubkey_data):
>      keys = []
> diff --git a/cloudinit/stages.py b/cloudinit/stages.py
> index bc4ebc8..a6b046e 100644
> --- a/cloudinit/stages.py
> +++ b/cloudinit/stages.py
> @@ -643,7 +644,8 @@ class Init(object):
>              LOG.warning("Failed to rename devices: %s", e)
>  
>          if (self.datasource is not NULL_DATA_SOURCE and
> -                not self.is_new_instance()):
> +                not self.datasource.check_apply_network_config(

Ultimately I'd like a unit test to cover this, but testing of stages(Init) is a pain at the moment, so I won't put that burden on you till we get that more testable.

> +                    self.is_new_instance())):
>              LOG.debug("not a new instance. network config is not applied.")
>              return
>  
> diff --git a/tests/unittests/test_datasource/test_smartos.py b/tests/unittests/test_datasource/test_smartos.py
> index 2bea7a1..24bd918 100644
> --- a/tests/unittests/test_datasource/test_smartos.py
> +++ b/tests/unittests/test_datasource/test_smartos.py
> @@ -601,6 +601,36 @@ class TestSmartOSDataSource(FilesystemMockingTestCase):
>          self.assertEqual(dsrc.device_name_to_device('FOO'),
>                           mydscfg['disk_aliases']['FOO'])
>  
> +    def test_sdc_maintain_network_unset(self):
> +        dsrc = self._get_ds(mockdata=MOCK_RETURNS)

These tests should all probably live in cloudinit/sources/tests/test_init.py. Ultimately all datsource tests will move to that directory (haven't gotten around to that yet).

Please also add a one-line docstring to each unit test expressing the test case intent and expected result.

'''When maintain_network attribute is not present in metadata, check_apply_network_config returns False'''
 (or get_maintain_network if you rename)

> +        ret = dsrc.get_data()
> +        self.assertTrue(ret)
> +        self.assertFalse(dsrc.check_apply_network_config(False))
> +
> +    def test_sdc_maintain_network_true(self):
> +        mockdata = MOCK_RETURNS.copy()
> +        mockdata['sdc:maintain_network'] = 'true'
> +        dsrc = self._get_ds(mockdata=mockdata)
> +        ret = dsrc.get_data()
> +        self.assertTrue(ret)
> +        self.assertTrue(dsrc.check_apply_network_config(False))
> +
> +    def test_sdc_maintain_network_false(self):
> +        mockdata = MOCK_RETURNS.copy()
> +        mockdata['sdc:maintain_network'] = 'false'
> +        dsrc = self._get_ds(mockdata=mockdata)
> +        ret = dsrc.get_data()
> +        self.assertTrue(ret)
> +        self.assertFalse(dsrc.check_apply_network_config(False))
> +
> +    def test_sdc_maintain_network_invalid(self):
> +        mockdata = MOCK_RETURNS.copy()
> +        mockdata['sdc:maintain_network'] = 'junk'
> +        dsrc = self._get_ds(mockdata=mockdata)
> +        ret = dsrc.get_data()
> +        self.assertTrue(ret)
> +        self.assertFalse(dsrc.check_apply_network_config(False))
> +
>  
>  class ShortReader(object):
>      """Implements a 'read' interface for bytes provided.


-- 
https://code.launchpad.net/~mgerdts/cloud-init/+git/cloud-init/+merge/343712
Your team cloud-init commiters is requested to review the proposed merge of ~mgerdts/cloud-init:lp1765801 into cloud-init:master.


References