Merge ~bitfehler/cloud-init:bitfehler/load_seed into cloud-init:master

[Conrad Hoffmann]

Support network-config in nocloud(-net) data source

This proposal adds a new function `load_seed` that can be used as a replacement for `read_seeded` and `read_optional_seed`, but has support for additional files (network-config, vendor-data).

The nocloud(-net) data source is updated to make use of it, adding support for network-config in user-supplied seeds. This addresses the following bugs:

https://bugs.launchpad.net/cloud-init/+bug/1809180
https://bugs.launchpad.net/cloud-init/+bug/1809277

The OVH and CloudStack datasources are also updated to use the new function, but without changing the data source semantics. Finally, as all previous callers have been updated, the old functions `read_seeded` and `read_optional_seed` are removed.

[Conrad Hoffmann (bitfehler)]

Hi,

I previously wrote a message to the mailing list, but without any response so far, so I figured I'd write some more code and try this way. Happy about any suggestions for improvements or guidance for other approaches.

Some notes right away:
 - The "xenial" tox env does not work for me, even on clean master, is this a known issue?
 - I thought it would be nice to make a holistic change (i.e. changing all callers to the new function), but in fact I do not have access to a test setup for the OVH or CloudStack data sources. How is this usually handled? Is it preferable to not touch any data sources you can not test? Or are there means for this to be tested before it lands in a release?

Thanks a bunch,
Conrad

[Ryan Harper (raharper)]

Thanks for sending in the merge proposal.

On Mon, Jul 8, 2019 at 7:55 AM Conrad Hoffmann <email address hidden> wrote:

> Hi,
>
> I previously wrote a message to the mailing list, but without any response
> so far, so I figured I'd write some more code and try this way. Happy about
> any suggestions for improvements or guidance for other approaches.
>
> Some notes right away:
> - The "xenial" tox env does not work for me, even on clean master, is
> this a known issue?
>

That's not know, so if you want to file a bug with your output/failure etc
we can take a look.

> - I thought it would be nice to make a holistic change (i.e. changing all
> callers to the new function), but in fact I do not have access to a test
> setup for the OVH or CloudStack data sources. How is this usually handled?
> Is it preferable to not touch any data sources you can not test? Or are
> there means for this to be tested before it lands in a release?
>

We typically do functional behavior changes only in the development release
and then disable them in older releases to prevent behavioral changes.
Depending on the change, we may or maynot test directly on those platforms;
most platform changes are driven from platform owners. That doesn't
preclude such a change so we'd take it case-by-case.

>
> Thanks a bunch,
> Conrad
> --
>
> https://code.launchpad.net/~bitfehler/cloud-init/+git/cloud-init/+merge/369814
> Your team cloud-init commiters is requested to review the proposed merge
> of ~bitfehler/cloud-init:bitfehler/load_seed into cloud-init:master.
>
> _______________________________________________
> Mailing list: https://launchpad.net/~cloud-init-dev
> Post to : <email address hidden>
> Unsubscribe : https://launchpad.net/~cloud-init-dev
> More help : https://help.launchpad.net/ListHelp
>

[Conrad Hoffmann (bitfehler)]

> Thanks for sending in the merge proposal.
>
>
>
> On Mon, Jul 8, 2019 at 7:55 AM Conrad Hoffmann <email address hidden> wrote:
>
> > Hi,
> >
> > I previously wrote a message to the mailing list, but without any response
> > so far, so I figured I'd write some more code and try this way. Happy about
> > any suggestions for improvements or guidance for other approaches.
> >
> > Some notes right away:
> > - The "xenial" tox env does not work for me, even on clean master, is
> > this a known issue?
> >
>
> That's not know, so if you want to file a bug with your output/failure etc
> we can take a look.

Will do.

> > - I thought it would be nice to make a holistic change (i.e. changing all
> > callers to the new function), but in fact I do not have access to a test
> > setup for the OVH or CloudStack data sources. How is this usually handled?
> > Is it preferable to not touch any data sources you can not test? Or are
> > there means for this to be tested before it lands in a release?
> >
>
> We typically do functional behavior changes only in the development release
> and then disable them in older releases to prevent behavioral changes.
> Depending on the change, we may or maynot test directly on those platforms;
> most platform changes are driven from platform owners. That doesn't
> preclude such a change so we'd take it case-by-case.

What would that mean for this PR? Should I leave it as is? Or should I change it so that only the nocloud data source uses the new function? I don't mind this waiting for the next release, I do understand it is a slight change in behavior.

Thanks again,
Conrad

> > Thanks a bunch,
> > Conrad
> > --
> >
> > https://code.launchpad.net/~bitfehler/cloud-init/+git/cloud-
> init/+merge/369814
> > Your team cloud-init commiters is requested to review the proposed merge
> > of ~bitfehler/cloud-init:bitfehler/load_seed into cloud-init:master.
> >
> > _______________________________________________
> > Mailing list: https://launchpad.net/~cloud-init-dev
> > Post to : <email address hidden>
> > Unsubscribe : https://launchpad.net/~cloud-init-dev
> > More help : https://help.launchpad.net/ListHelp
> >

[Ryan Harper (raharper)]

> What would that mean for this PR? Should I leave it as is? Or should I
> change it so that only the nocloud data source uses the new function?
> I don't mind this waiting for the next release, I do understand it is
> a slight change in behavior.

In case you missed, I had some comments you can see below in the diff output
about the unittests. Let's address those first.

As for this branch; it appears to me to be a super-set of capabilities
more than a change in existing behavior. That is, I expect the datasource
given the same inputs to produce the same behavior;

We are likely to take the updated behavior into master, and include that in
the Ubuntu E release; it could be disabled by default on previous releases
but allowing users to opt-in to the new behavior.

[Ryan Harper (raharper)]

Thanks for updating the unittests. I've done some additional review and added some inline comments in the diff below. Please take a look.

review: Needs Information

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:90186f8113d73acc437f2d690ec1d43a5d6a9b96
https://jenkins.ubuntu.com/server/job/cloud-init-ci/777/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/777/rebuild

review: Approve (continuous-integration)

[Conrad Hoffmann (bitfehler)]

Hi,

sorry it took so long, I think I addressed all your comments in the latest commit.

Cheers,
Conrad

[Ryan Harper (raharper)]

Thanks for the update. I've one more question in-line; The new unittests look great.

review: Needs Information

8ba83bb... by Conrad Hoffmann <email address hidden> on 2019-08-19

Return empty dict on invalid yaml in load_seed()

Also add a test for this to emphasize that this is the expected
behavior.

[Conrad Hoffmann (bitfehler)]

Good catch, I fixed that and also added a test for this for the future.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:8ba83bb8912a8d1614275551be45a7e1fa91173c
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1107/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1107//rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

I'm not sure that I understand the intent here.

The subject line says:
 Support network-config in nocloud(-net) data source

But nocloud-net cannot ever really provide network-data.
It simply runs too late. No "network" datasources can apply network config as before they have the opportunity to read information the network is already up.

At this point in cluod-init none of the renderers are idempotent. They have to be written before networking is brought up by the OS.

So what we *can* accomplish now (and maybe what this does?) is:

a.) datasourceNoCloud could read networking files from a 'seedfrom'. It already can read network-config from /var/lib/cloud/nocloud/network-config

b.) dataSourceNoCloud could use ephemeral networking to attempt 'seedfrom' of http/https.

But what we can't (I don't think) do:

c.) dataSourceNoCloud-net support reading network-config *at all*.

Last thing... In your commit message, if you believe that you fix those bugs,
please use:
 LP: #XXXXX
 LP: #YYYYY

then all the tools and such will read that and handle closing/fixing bugs.

review: Needs Information

[Ryan Harper (raharper)]

Looking at the first bug:

https://bugs.launchpad.net/cloud-init/+bug/1809180

I understand it to:
  1) boot NoCloud (fallback networking is utilized)
  2) at cloud-init init stage, NoCloud-Net is loaded, which now supports reading URLs
     including network-config files from URL.
  3) during init stage the .network_config property on NoCloudNet is provided via loading the seed and reading the URLs
  4) cloud-init init stage will render an updated network-config to the system.

This leaves a system with a fallback network config up (including dhclient) but the on-disk network-config is from the URL. On subsequent boots, it will utilize the network-config from the seed URL.

This appears to resolve the request; though it's not completely clear without more feedback from the submitter w.r.t the use-case "boot an image in a new environment without running cloud-init a second time".

A more complete fix would be to modify NoCloud to use EphemeralDHCP to bring up networking if a seed prefix included URLs (as Scott suggests in (b)).

The second bug suggests exactly (b).

I don't think it would be too much more effort to pull in the EphemeralDHCP bits.

[Conrad Hoffmann (bitfehler)]

Thank you for the comments. I indeed somewhat misjudged the relation of the nocloud and nocloud-net data sources.

To clarify, my main objective was a) as mentioned by Scott and also the hope to maybe unify some of the approaches to reading the seed (like getting rid of read_optional_seed and potentially also pathprefix2dict in a follow-up).

However, I also think b) would be valuable. I am happy to look at adding ephemeral DHCP, but maybe in a different MR?

I now understand that this certainly doesn't fix https://bugs.launchpad.net/cloud-init/+bug/1809277. I am unsure about https://bugs.launchpad.net/cloud-init/+bug/1809180, but I read it like if the nocloud-net does render the "new" network config even though the network is already up then that's fine. I will try to get clarification from the submitter.

Would you be ok with merging this if I update the description and commit messages to reflect the narrower scope?

[Scott Moser (smoser)]

"if the nocloud-net does render the "new" network config even though the network is already up then that's fine."

Its *not* fine. network datasources should not render network information at this point. Doing so will only confuse things and create a system that "maybe works". We cannot really ensure that it will work, and we're not going to support if it didn't.

So just make it simpler, and only read network config in the local.

[Scott Moser (smoser)]

(and just so i dont sound rude.... thank you for attempts to clean things up. for sure, there have been many iterative ways of doing things over time and you're seeing that here).

[Conrad Hoffmann (bitfehler)]

No worries, I understand your point. I guess I am just surprised that that bug report is in state triaged then. It should probably be closed as won't fix with a link to this discussion maybe?

I'll update this MR accordingly.

[Scott Moser (smoser)]

In the end, it doesn't really matter if its nocloud-net or nocloud.
If nocloud supports reading 'seedfrom' via the ephemeral network, then you get the exact same thing you would have gotten if nocloud-net could actually work like that.

[Scott Moser (smoser)]

So lets do this...
stage 1) support seedfrom of local paths to get the network config
stage 2) use ephemeral networking to nocloud-net if seedfrom is http or https.

Sound reasonable?

64ce489... by Conrad Hoffmann <email address hidden> on 2019-10-14

Support seeded network-config in local mode only

[Conrad Hoffmann (bitfehler)]

I added another commit which I think should achieve what you described as stage 1), while not allowing a seed network-config in nocloud-net. Let me know what you think.

Unmerged commits

64ce489... by Conrad Hoffmann <email address hidden> on 2019-10-14

Support seeded network-config in local mode only

8ba83bb... by Conrad Hoffmann <email address hidden> on 2019-08-19

Return empty dict on invalid yaml in load_seed()

Also add a test for this to emphasize that this is the expected
behavior.

efb12b3... by Conrad Hoffmann on 2019-07-22

More kwargs and better exception handling

90186f8... by Conrad Hoffmann on 2019-07-15

Improve tests for load_seed

Make test name more descriptive (again) and provide test for URL
handling capabilities.

5a07ceb... by Conrad Hoffmann on 2019-07-08

Remove read_seeded and read_optional_seed

They are no longer used, mostly replaced by calls to `load_seed`.

475b563... by Conrad Hoffmann on 2019-07-08

Port several data sources to use load_seed

Use `load_seed` instead of either `read_seeded` or `read_optional_seed`
for the following data sources: CloudStack, NoCloud, OVF. This removes
all usage of both of these functions.

For the nocloud(-net) data source, this enables support for reading a
network-config from a user-supplied seed. This addresses the following
bugs:

https://bugs.launchpad.net/cloud-init/+bug/1809180
https://bugs.launchpad.net/cloud-init/+bug/1809277

For the other data sources, semantics were not changed, though this
change should facilitate e.g. supporting a seed network-config.

48fd9c8... by Conrad Hoffmann on 2019-07-08

Add load_seed function to facilitate seed loading

It can handle both local files and remote URLS, and otherwise has an
interface similar to pathprefix2dict, supporting required and optional
files (e.g. to support network-config without requiring it). It parses
meta-data and network-config to YAML, others must be handled by the
caller.

Intended to be a replacement for `read_seeded` and `read_optional_seed`.
Includes some basic tests.

diff --git a/cloudinit/sources/DataSourceCloudStack.py b/cloudinit/sources/DataSourceCloudStack.py
index f185dc7..4bc40ae 100644
--- a/cloudinit/sources/DataSourceCloudStack.py
+++ b/cloudinit/sources/DataSourceCloudStack.py
@@ -110,12 +110,17 @@ class DataSourceCloudStack(sources.DataSource):
         return self.cfg
 
     def _get_data(self):
-        seed_ret = {}
-        if util.read_optional_seed(seed_ret, base=(self.seed_dir + "/")):
-            self.userdata_raw = seed_ret['user-data']
-            self.metadata = seed_ret['meta-data']
+        try:
+            # Using `required` wrapped in try because we want both or none
+            seed = util.load_seed(self.seed_dir,
+                                  required=['meta-data', 'user-data'])
+            self.userdata_raw = seed['user-data']
+            self.metadata = seed['meta-data']
             LOG.debug("Using seeded cloudstack data from: %s", self.seed_dir)
             return True
+        except Exception:
+            util.logexc(LOG, 'Failed to read seeded data from %s',
+                        self.seed_dir)
         try:
             if not self.wait_for_metadata_service():
                 return False
diff --git a/cloudinit/sources/DataSourceNoCloud.py b/cloudinit/sources/DataSourceNoCloud.py
index 474773d..2b158fe 100644
--- a/cloudinit/sources/DataSourceNoCloud.py
+++ b/cloudinit/sources/DataSourceNoCloud.py
@@ -163,15 +163,23 @@ class DataSourceNoCloud(sources.DataSource):
                 LOG.debug("Seed from %s not supported by %s", seedfrom, self)
                 return False
 
-            # This could throw errors, but the user told us to do it
-            # so if errors are raised, let them raise
-            (md_seed, ud) = util.read_seeded(seedfrom, timeout=None)
+            # This could fail, but the user told us to do it,
+            # so if it does, abort
+            try:
+                kwargs = {'required': ['user-data', 'meta-data'],
+                          'optional': []}
+                if self.dsmode == sources.DSMODE_LOCAL:
+                    kwargs['optional'] = ['network-config']
+                seed = util.load_seed(seedfrom, timeout=None, **kwargs)
+            except Exception:
+                util.logexc(LOG, 'Failed to read seeded data from %s',
+                            seedfrom)
+                return False
+
             LOG.debug("Using seeded cache data from %s", seedfrom)
 
             # Values in the command line override those from the seed
-            mydata['meta-data'] = util.mergemanydict([mydata['meta-data'],
-                                                      md_seed])
-            mydata['user-data'] = ud
+            mydata = _merge_new_seed(mydata, seed)
             found.append(seedfrom)
 
         # Now that we have exhausted any other places merge in the defaults
@@ -357,8 +365,10 @@ def _merge_new_seed(cur, seeded):
     ret['meta-data'] = util.mergemanydict([cur['meta-data'], newmd])
 
     if seeded.get('network-config'):
-        ret['network-config'] = _maybe_remove_top_network(
-            util.load_yaml(seeded.get('network-config')))
+        newnc = seeded['network-config']
+        if not isinstance(newnc, dict):
+            newnc = util.load_yaml(seeded['network-config'])
+        ret['network-config'] = _maybe_remove_top_network(newnc)
 
     if 'user-data' in seeded:
         ret['user-data'] = seeded['user-data']
diff --git a/cloudinit/sources/DataSourceOVF.py b/cloudinit/sources/DataSourceOVF.py
index e7794aa..7ddd063 100644
--- a/cloudinit/sources/DataSourceOVF.py
+++ b/cloudinit/sources/DataSourceOVF.py
@@ -286,10 +286,11 @@ class DataSourceOVF(sources.DataSource):
                           seedfrom, self)
                 return False
 
-            (md_seed, ud) = util.read_seeded(seedfrom, timeout=None)
+            seed = util.load_seed(seedfrom, required=['meta-data'],
+                                  timeout=None)
             LOG.debug("Using seeded cache data from %s", seedfrom)
 
-            md = util.mergemanydict([md, md_seed])
+            md = util.mergemanydict([md, seed.get('meta-data', {})])
             found.append(seedfrom)
 
         # Now that we have exhausted any other places merge in the defaults
diff --git a/cloudinit/util.py b/cloudinit/util.py
index 0d338ca..6f11dec 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -893,22 +893,6 @@ def runparts(dirp, skip_no_exist=True, exe_prefix=None):
                            % (len(failed), len(attempted)))
 
 
-# read_optional_seed
-# returns boolean indicating success or failure (presense of files)
-# if files are present, populates 'fill' dictionary with 'user-data' and
-# 'meta-data' entries
-def read_optional_seed(fill, base="", ext="", timeout=5):
-    try:
-        (md, ud) = read_seeded(base, ext, timeout)
-        fill['user-data'] = ud
-        fill['meta-data'] = md
-        return True
-    except url_helper.UrlError as e:
-        if e.code == url_helper.NOT_FOUND:
-            return False
-        raise
-
-
 def fetch_ssl_details(paths=None):
     ssl_details = {}
     # Lookup in these locations for ssl key/cert files
@@ -975,34 +959,33 @@ def load_yaml(blob, default=None, allowed=(dict,)):
     return loaded
 
 
-def read_seeded(base="", ext="", timeout=5, retries=10, file_retries=0):
+def load_seed(base='', required=None, optional=None, timeout=5):
+    result = {}
+    if required is None:
+        required = []
+    if optional is None:
+        optional = []
     if base.startswith("/"):
         base = "file://%s" % base
 
-    # default retries for file is 0. for network is 10
-    if base.startswith("file://"):
-        retries = file_retries
+    for item in required + optional:
+        try:
+            url = url_helper.combine_url(base, item)
+            resp = url_helper.read_file_or_url(url, timeout)
+            if resp.ok():
+                if item in ['meta-data', 'network-config']:
+                    result[item] = load_yaml(decode_binary(resp.contents),
+                                             default={})
+                else:
+                    result[item] = resp.contents
+        except url_helper.UrlError as e:
+            LOG.debug('Failed to read %s: %s', url, e)
+            if item in optional and e.code == url_helper.NOT_FOUND:
+                pass
+            else:
+                raise e
 
-    if base.find("%s") >= 0:
-        ud_url = base % ("user-data" + ext)
-        md_url = base % ("meta-data" + ext)
-    else:
-        ud_url = "%s%s%s" % (base, "user-data", ext)
-        md_url = "%s%s%s" % (base, "meta-data", ext)
-
-    md_resp = url_helper.read_file_or_url(md_url, timeout, retries,
-                                          file_retries)
-    md = None
-    if md_resp.ok():
-        md = load_yaml(decode_binary(md_resp.contents), default={})
-
-    ud_resp = url_helper.read_file_or_url(ud_url, timeout, retries,
-                                          file_retries)
-    ud = None
-    if ud_resp.ok():
-        ud = ud_resp.contents
-
-    return (md, ud)
+    return result
 
 
 def read_conf_d(confd):
diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
index 0e71db8..f5d2c63 100644
--- a/tests/unittests/test_util.py
+++ b/tests/unittests/test_util.py
@@ -9,6 +9,7 @@ import shutil
 import stat
 import tempfile
 
+import httpretty
 import json
 import six
 import sys
@@ -724,9 +725,9 @@ class TestMessageFromString(helpers.TestCase):
         self.assertNotIn('\x00', roundtripped)
 
 
-class TestReadSeeded(helpers.TestCase):
+class TestLoadSeed(helpers.HttprettyTestCase):
     def setUp(self):
-        super(TestReadSeeded, self).setUp()
+        super(TestLoadSeed, self).setUp()
         self.tmp = tempfile.mkdtemp()
         self.addCleanup(shutil.rmtree, self.tmp)
 
@@ -734,12 +735,69 @@ class TestReadSeeded(helpers.TestCase):
         ud = b"userdatablob"
         helpers.populate_dir(
             self.tmp, {'meta-data': "key1: val1", 'user-data': ud})
-        sdir = self.tmp + os.path.sep
-        (found_md, found_ud) = util.read_seeded(sdir)
+        seed = util.load_seed(self.tmp, required=['meta-data', 'user-data'])
+        found_md = seed.get('meta-data', {})
+        found_ud = seed.get('user-data', '')
 
         self.assertEqual(found_md, {'key1': 'val1'})
         self.assertEqual(found_ud, ud)
 
+    def test_non_yaml_returns_empty_dict(self):
+        blob = b"randomblob"
+        helpers.populate_dir(self.tmp, {'meta-data': blob})
+        seed = util.load_seed(self.tmp, required=['meta-data'])
+        found_md = seed.get('meta-data')
+
+        self.assertEqual(found_md, {})
+
+    def test_handles_urls(self):
+        ud = b"userdatablob"
+        base = 'http://foo.bar/datasource'
+        httpretty.register_uri(
+            httpretty.GET,
+            base + '/meta-data',
+            body='foo: bar')
+        httpretty.register_uri(
+            httpretty.GET,
+            base + '/user-data',
+            body=ud)
+        seed = util.load_seed(base, required=['meta-data', 'user-data'])
+        found_md = seed.get('meta-data', {})
+        found_ud = seed.get('user-data', '')
+
+        self.assertEqual(found_md, {'foo': 'bar'})
+        self.assertEqual(found_ud, ud)
+
+    def test_ignores_optional(self):
+        ud = b"userdatablob"
+        helpers.populate_dir(
+            self.tmp, {'meta-data': 'key1: val1', 'user-data': ud})
+        seed = util.load_seed(
+            self.tmp, ['meta-data', 'user-data'], ['network-config'])
+        found_md = seed.get('meta-data', {})
+        found_ud = seed.get('user-data', '')
+
+        self.assertEqual(found_md, {'key1': 'val1'})
+        self.assertEqual(found_ud, ud)
+
+    def test_loads_optional(self):
+        ud = b"userdatablob"
+        helpers.populate_dir(
+            self.tmp, {
+                'meta-data': 'key1: val1',
+                'user-data': ud,
+                'network-config': 'key: val'
+            })
+        seed = util.load_seed(
+            self.tmp, ['meta-data', 'user-data'], ['network-config'])
+        found_md = seed.get('meta-data', {})
+        found_ud = seed.get('user-data', '')
+        found_nc = seed.get('network-config', {})
+
+        self.assertEqual(found_md, {'key1': 'val1'})
+        self.assertEqual(found_ud, ud)
+        self.assertEqual(found_nc, {'key': 'val'})
+
 
 class TestSubp(helpers.CiTestCase):
     with_logs = True