Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1649652-curtin-routes-rendered into cloud-init:master
Mike, Yeah, thats true. The fix here does verify that the content is better than it was in that bug report though. We need full round trip rendering and reading to do the whole thing the right way. That said, the network config that was provided in the bug, and I think correctly simplified here, has static routes that are not attached to an interface. There is no specific link between the 'route' type and the subnet that it should be off of. we could arguably work backwards and find the right subnet to hang the static route off of, but at the moment we dont do that. Additionally, there could well be two devices with an 'address' that would contain the route's 'gateway'. Again, I agree that this test is minimalistic. You have thoughts on what the route info should look like? (we should probably move this discussion at least to the bug). -- https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/313475 Your team cloud init development team is requested to review the proposed merge of ~smoser/cloud-init:bug/1649652-curtin-routes-rendered into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1649652-curtin-routes-rendered into cloud-init:master
Diff comments:
> diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
> index 789c78b..81b43a8 100755
> --- a/tests/unittests/test_net.py
> +++ b/tests/unittests/test_net.py
> @@ -769,6 +769,52 @@ class TestEniRoundTrip(TestCase):
> entry['expected_eni'].splitlines(),
> files['/etc/network/interfaces'].splitlines())
>
> +def test_routes_rendered(self):
> +# as reported in bug 1649652
> +conf = [
> +{'name': 'eth0', 'type': 'physical',
> + 'subnets': [{
> +'address': '172.23.31.42/26',
> +'dns_nameservers': [], 'gateway': '172.23.31.2',
> +'type': 'static'}]},
> +{'type': 'route', 'id': 4,
> + 'metric': 0, 'destination': '10.0.0.0/12',
> + 'gateway': '172.23.31.1'},
> +{'type': 'route', 'id': 5,
> + 'metric': 0, 'destination': '192.168.2.0/16',
> + 'gateway': '172.23.31.1'},
> +{'type': 'route', 'id': 6,
> + 'metric': 1, 'destination': '10.0.200.0/16',
> + 'gateway': '172.23.31.1'},
> +]
> +
> +files = self._render_and_read(
> +network_config={'config': conf, 'version': 1})
> +expected = [
> +'auto lo',
> +'iface lo inet loopback',
> +'auto eth0',
> +'iface eth0 inet static',
> +'address 172.23.31.42/26',
> +'gateway 172.23.31.2',
> +('post-up route add -net 10.0.0.0 netmask 255.240.0.0 gw '
> + '172.23.31.1 metric 0 || true'),
> +('pre-down route del -net 10.0.0.0 netmask 255.240.0.0 gw '
> + '172.23.31.1 metric 0 || true'),
> +('post-up route add -net 192.168.2.0 netmask 255.255.0.0 gw '
> + '172.23.31.1 metric 0 || true'),
> +('pre-down route del -net 192.168.2.0 netmask 255.255.0.0 gw '
> + '172.23.31.1 metric 0 || true'),
> +('post-up route add -net 10.0.200.0 netmask 255.255.0.0 gw '
> + '172.23.31.1 metric 1 || true'),
> +('pre-down route del -net 10.0.200.0 netmask 255.255.0.0 gw '
> + '172.23.31.1 metric 1 || true'),
> +]
> +found = files['/etc/network/interfaces'].splitlines()
I'm not sure this check is sufficient to test whether or not the routes were
rendered correctly. I observed a /e/n/i from a customer in which the static
routes were placed under an incorrect interface.
> +
> +self.assertEqual(
> +expected, [line for line in found if line])
> +
>
> def _gzip_data(data):
> with io.BytesIO() as iobuf:
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/313475
Your team cloud init development team is requested to review the proposed merge
of ~smoser/cloud-init:bug/1649652-curtin-routes-rendered into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~smoser/cloud-init:bug/1649652-curtin-routes-rendered into cloud-init:master
The proposal to merge ~smoser/cloud-init:bug/1649652-curtin-routes-rendered into cloud-init:master has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/313475 -- Your team cloud init development team is requested to review the proposed merge of ~smoser/cloud-init:bug/1649652-curtin-routes-rendered into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~ffledgling/cloud-init:doc-fix into cloud-init:master
The proposal to merge ~ffledgling/cloud-init:doc-fix into cloud-init:master has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~ffledgling/cloud-init/+git/cloud-init-1/+merge/312917 -- Your team cloud init development team is requested to review the proposed merge of ~ffledgling/cloud-init:doc-fix into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
[Cloud-init-dev] [Merge] ~harlowja/cloud-init:chop-bitly into cloud-init:master
The proposal to merge ~harlowja/cloud-init:chop-bitly into cloud-init:master has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~harlowja/cloud-init/+git/cloud-init/+merge/312390 -- Your team cloud init development team is requested to review the proposed merge of ~harlowja/cloud-init:chop-bitly into cloud-init:master. ___ Mailing list: https://launchpad.net/~cloud-init-dev Post to : cloud-init-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
Re: [Cloud-init-dev] [Merge] ~rski/cloud-init:puppet_4 into cloud-init:master
Romanos,
Over all it looks really good.
In order to take this, you'll need to sign the contributors agreement
(https://www.ubuntu.com/legal/contributors). See the hacking section on
readthedocs for more info
http://cloudinit.readthedocs.io/en/latest/topics/hacking.html .
Also, please feel free to ping me in Freenode #cloud-init or /query me, or via
email.
I've left a few minor questions inline also, but the 'needs fixing' is really
only wrt the contributors agreement.
Diff comments:
> diff --git a/cloudinit/config/cc_puppet.py b/cloudinit/config/cc_puppet.py
> index bfd630d..d5351f6 100644
> --- a/cloudinit/config/cc_puppet.py
> +++ b/cloudinit/config/cc_puppet.py
> @@ -71,10 +81,23 @@ import socket
> from cloudinit import helpers
> from cloudinit import util
>
> -PUPPET_CONF_PATH = '/etc/puppet/puppet.conf'
> -PUPPET_SSL_CERT_DIR = '/var/lib/puppet/ssl/certs/'
> -PUPPET_SSL_DIR = '/var/lib/puppet/ssl'
> -PUPPET_SSL_CERT_PATH = '/var/lib/puppet/ssl/certs/ca.pem'
> +DEFAULT_PACKAGE_NAME = 'puppet'
> +DEFAULT_SSL_DIR = '/var/lib/puppet/ssl'
> +DEFAULT_CONF_DIR = '/etc/puppet'
any reason why you've chosen to allow the directory, and assume/hard code the
conf to be in that directory at 'puppet.conf' rather than allowing the
configuration of the puppet config file path directly ?
> +
> +
> +class PuppetConstants(object):
> +
> +def __init__(self,
> + puppet_conf_dir,
> + puppet_ssl_dir,
> + log):
> +self.conf_dir = puppet_conf_dir
> +self.conf_path = os.path.join(puppet_conf_dir, "puppet.conf")
> +self.ssl_dir = puppet_ssl_dir
> +self.ssl_cert_dir = os.path.join(puppet_ssl_dir, "certs")
> +self.ssl_cert_path = os.path.join(self.ssl_cert_dir,
> + "ca.pem")
>
>
> def _autostart_puppet(log):
> @@ -101,22 +124,35 @@ def handle(name, cfg, cloud, log, _args):
> return
>
> puppet_cfg = cfg['puppet']
> -
> # Start by installing the puppet package if necessary...
> install = util.get_cfg_option_bool(puppet_cfg, 'install', True)
> version = util.get_cfg_option_str(puppet_cfg, 'version', None)
> +package_name = util.get_cfg_option_str(puppet_cfg,
> + 'package_name',
> + DEFAULT_PACKAGE_NAME)
> +conf_dir = util.get_cfg_option_str(puppet_cfg,
> + 'conf_dir',
> + DEFAULT_CONF_DIR)
> +ssl_dir = util.get_cfg_option_str(puppet_cfg,
> + 'ssl_dir',
> + DEFAULT_SSL_DIR)
> +
> +p_constants = PuppetConstants(conf_dir,
i'd just join these three to save vertical space:
p_constants = PuppetConstants(conf_dir, ssl_dir, log)
> + ssl_dir,
> + log)
> if not install and version:
> log.warn(("Puppet install set false but version supplied,"
>" doing nothing."))
> elif install:
> log.debug(("Attempting to install puppet %s,"),
>version if version else 'latest')
> -cloud.distro.install_packages(('puppet', version))
> +
> +cloud.distro.install_packages((package_name, version))
>
> # ... and then update the puppet configuration
> if 'conf' in puppet_cfg:
> # Add all sections from the conf object to puppet.conf
> -contents = util.load_file(PUPPET_CONF_PATH)
> +contents = util.load_file(p_constants.conf_path)
> # Create object for reading puppet.conf values
> puppet_config = helpers.DefaultingConfigParser()
> # Read puppet.conf values from original file in order to be able to
--
https://code.launchpad.net/~rski/cloud-init/+git/cloud-init/+merge/312284
Your team cloud init development team is requested to review the proposed merge
of ~rski/cloud-init:puppet_4 into cloud-init:master.
___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
