Merge ~daniel-thewatkins/cloud-init/+git/cloud-init:networking into cloud-init:master

Proposed by Dan Watkins on 2019-07-05
Status: Merged
Approved by: Dan Watkins on 2019-07-23
Approved revision: 49cb977b5daf14ba966d8296830af8423af1a560
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~daniel-thewatkins/cloud-init/+git/cloud-init:networking
Merge into: cloud-init:master
Diff against target: 270 lines (+116/-19)
4 files modified
cloudinit/sources/__init__.py (+16/-0)
cloudinit/stages.py (+28/-9)
cloudinit/tests/test_stages.py (+61/-10)
tests/unittests/test_datasource/test_common.py (+11/-0)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve on 2019-07-23
Ryan Harper 2019-07-05 Approve on 2019-07-23
Review via email: mp+369783@code.launchpad.net

Commit message

stages: allow data sources to override network config source order

Currently, if a platform provides any network configuration via the
"cmdline" method (i.e. network-data=... on the kernel command line,
ip=... on the kernel command line, or iBFT config via /run/net-*.conf),
the value of the data source's network_config property is completely
ignored.

This means that on platforms that use iSCSI boot (such as Oracle Compute
Infrastructure), there is no way for the data source to configure any
network interfaces other than those that have already been configured by
the initramfs.

This change allows data sources to specify the order in which network
configuration sources are considered. Data sources that opt to use this
mechanism will be expected to consume the command line network data and
integrate it themselves.

(The generic merging of network configuration sources was considered,
but we concluded that the single use case we have presently (a) didn't
warrant the increased complexity, and (b) didn't give us a broad enough
view to be sure that our generic implementation would be sufficiently
generic. This change in no way precludes a merging strategy in future.)

To post a comment you must log in.

PASSED: Continuous integration, rev:26a5013712a2a7470b6d8d28b06b1ff568e3ae2d
https://jenkins.ubuntu.com/server/job/cloud-init-ci/755/
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/755/rebuild

review: Approve (continuous-integration)
Ryan Harper (raharper) :
review: Needs Information
Dan Watkins (daniel-thewatkins) wrote :

Thanks for the review Ryan! I've responded inline.

Ryan Harper (raharper) :

PASSED: Continuous integration, rev:6ef4950bebac1906a002ec300acaa6ebca12aabd
https://jenkins.ubuntu.com/server/job/cloud-init-ci/794/
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/794/rebuild

review: Approve (continuous-integration)
Ryan Harper (raharper) wrote :

Couple of questions inline.

Ryan Harper (raharper) wrote :
Download full text (6.6 KiB)

On Mon, Jul 22, 2019 at 1:16 PM Dan Watkins <email address hidden>
wrote:

>
>
> Diff comments:
>
> > diff --git a/cloudinit/sources/__init__.py
> b/cloudinit/sources/__init__.py
> > index e6966b3..794b7f4 100644
> > --- a/cloudinit/sources/__init__.py
> > +++ b/cloudinit/sources/__init__.py
> > @@ -153,6 +159,13 @@ class DataSource(object):
> > # Track the discovered fallback nic for use in configuration
> generation.
> > _fallback_interface = None
> >
> > + # The network configuration sources that should be considered for
> this data
> > + # source. (The first source in this list that provides network
> > + # configuration will be used without considering any that follow.)
> > + network_config_sources = (NetworkConfigSource.cmdline,
> > + NetworkConfigSource.system_cfg,
> > + NetworkConfigSource.ds)
>
> In my mind, cloudinit.sources.NetworkConfigSource is the canonical source
> of the network config sources that cloud-init knows about.
> DataSource.network_config_sources is the (default) list of which of those
> available network config sources should be considered when we're
> determining network configuration for an instance that is using a
> particular DataSource. (None of this disagrees with what you've said, I'm
> just trying to level set on my intent to ensure that we aren't in
> disagreement at _that_ level. Please do let me know if you think some more
> commenting/documentation would make this distinction clearer!)
>
> So I _think_ this boils down to the semantics of "fallback". The code as
> written assumes that, regardless of data source configuration, we should
> _always_ fall back to the fallback network config source. If we appended
> NetworkConfigSource.fallback to DataSource.network_config_sources then, to
> me, that expresses that we think data sources should potentially be able to
> opt-out of fallback network config generation (while still getting network
> configuration from _some_ source; we already have a way to disable network
> configuration entirely). In my view, that isn't the correct behaviour; if
> we are generating network config at all, we should _always_ fall back to

"fallback".
>

I'd really like to see it listed in as one of the possible sources; but
also indicate that it's non-optional;
that, as you say, a datasource won't be able to opt-out by excluding it
from it's list of sources priority.

OTOH, it does seem strange to have a datasource list
NetworkConfigSources.fallback sooner than others
though it could very well just implement 'network_config' property as a
call to net.generate_fallback_config()
and achieve the same thing.

I don't think there's a clear answer here; so I think I'm fine without
listing it in network_config_sources
as long as we log which source we used (which I think we already do).

> (To maintain _exactly_ the current behaviour, this would also add some
> complexity to the implementation: currently we determine the network
> configuration for each network config source _except_ fallback before we
> consider which ones we might apply. If we wanted to continue to generate
> the fallback ne...

Read more...

Dan Watkins (daniel-thewatkins) wrote :
Download full text (3.2 KiB)

On Mon, Jul 22, 2019 at 07:54:20PM -0000, Ryan Harper wrote:
> > So I _think_ this boils down to the semantics of "fallback". The
> > code as written assumes that, regardless of data source
> > configuration, we should _always_ fall back to the fallback network
> > config source. If we appended NetworkConfigSource.fallback to
> > DataSource.network_config_sources then, to me, that expresses that
> > we think data sources should potentially be able to opt-out of
> > fallback network config generation (while still getting network
> > configuration from _some_ source; we already have a way to disable
> > network configuration entirely). In my view, that isn't the correct
> > behaviour; if we are generating network config at all, we should
> > _always_ fall back to "fallback".
>
> I'd really like to see it listed in as one of the possible sources;
> but also indicate that it's non-optional; that, as you say, a
> datasource won't be able to opt-out by excluding it from it's list of
> sources priority.
>
> OTOH, it does seem strange to have a datasource list
> NetworkConfigSources.fallback sooner than others though it could very
> well just implement 'network_config' property as a call to
> net.generate_fallback_config() and achieve the same thing.
>
> I don't think there's a clear answer here; so I think I'm fine without
> listing it in network_config_sources as long as we log which source
> we used (which I think we already do).

OK, thanks! I'll confirm the logging is present, but I believe you're
correct.

> > (To maintain _exactly_ the current behaviour, this would also add
> > some complexity to the implementation: currently we determine the
> > network configuration for each network config source _except_
> > fallback before we consider which ones we might apply. If we wanted
> > to continue to generate the fallback network configuration iff it is
> > needed, we would need some special-casing. This is only really an
> > issue if we're concerned about (a) the cost of determining
> > "fallback" configuration, or (b) the side effects of the code that
> > determines "fallback" configuration. I suspect we don't, but wanted
> > to include this note for completeness.)
>
> I was wondering if we should check for network_config property _first_
> as in the near future, the datasource network_config property may
> already have extracted other NetworkConfigSource values and
> merged/included them in the property implementation.
>
> So generically, I think it would be best to lazily fetch a config
> source per the network-config source order to avoid duplicate effort
> in fetching a particular config.

Yes, I absolutely agree that only determining the network config for a
source once we're considering actually using that source would be a
definite improvement. I've held off on doing that for now, because I
need to confirm that, for example, there aren't any datasource
network_config methods that have (important) side effects. With the
current behaviour, those side effects happen unconditionally (even if we
don't end up using the "ds" network config source), whereas with lazy
evaluation they would only happen if we were definitely considering
us...

Read more...

Dan Watkins (daniel-thewatkins) wrote :

I believe I've addressed the review comments, and have confirmed that the caller of _find_networking_config has:

        LOG.info("Applying network configuration from %s bringup=%s: %s",
                 src, bring_up, netcfg)

Ryan Harper (raharper) wrote :

Thanks. This looks good. One inline request for unittest.

FAILED: Continuous integration, rev:aa337e1225ac80330d3760f970a052c95b687ffb
https://jenkins.ubuntu.com/server/job/cloud-init-ci/798/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

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

review: Needs Fixing (continuous-integration)
Dan Watkins (daniel-thewatkins) wrote :

Thanks, will add that test!

Ryan Harper (raharper) wrote :

LGTM.

review: Approve

PASSED: Continuous integration, rev:b92229997c0c459c2c12600c2731b00ab1a2e112
https://jenkins.ubuntu.com/server/job/cloud-init-ci/799/
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/799/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/sources/__init__.py b/cloudinit/sources/__init__.py
2index e6966b3..9d24936 100644
3--- a/cloudinit/sources/__init__.py
4+++ b/cloudinit/sources/__init__.py
5@@ -66,6 +66,13 @@ CLOUD_ID_REGION_PREFIX_MAP = {
6 'china': ('azure-china', lambda c: c == 'azure'), # only change azure
7 }
8
9+# NetworkConfigSource represents the canonical list of network config sources
10+# that cloud-init knows about. (Python 2.7 lacks PEP 435, so use a singleton
11+# namedtuple as an enum; see https://stackoverflow.com/a/6971002)
12+_NETCFG_SOURCE_NAMES = ('cmdline', 'ds', 'system_cfg', 'fallback')
13+NetworkConfigSource = namedtuple('NetworkConfigSource',
14+ _NETCFG_SOURCE_NAMES)(*_NETCFG_SOURCE_NAMES)
15+
16
17 class DataSourceNotFoundException(Exception):
18 pass
19@@ -153,6 +160,15 @@ class DataSource(object):
20 # Track the discovered fallback nic for use in configuration generation.
21 _fallback_interface = None
22
23+ # The network configuration sources that should be considered for this data
24+ # source. (The first source in this list that provides network
25+ # configuration will be used without considering any that follow.) This
26+ # should always be a subset of the members of NetworkConfigSource with no
27+ # duplicate entries.
28+ network_config_sources = (NetworkConfigSource.cmdline,
29+ NetworkConfigSource.system_cfg,
30+ NetworkConfigSource.ds)
31+
32 # read_url_params
33 url_max_wait = -1 # max_wait < 0 means do not wait
34 url_timeout = 10 # timeout for each metadata url read attempt
35diff --git a/cloudinit/stages.py b/cloudinit/stages.py
36index 5f9d47b..6bcda2d 100644
37--- a/cloudinit/stages.py
38+++ b/cloudinit/stages.py
39@@ -24,6 +24,7 @@ from cloudinit.handlers.shell_script import ShellScriptPartHandler
40 from cloudinit.handlers.upstart_job import UpstartJobPartHandler
41
42 from cloudinit.event import EventType
43+from cloudinit.sources import NetworkConfigSource
44
45 from cloudinit import cloud
46 from cloudinit import config
47@@ -630,19 +631,37 @@ class Init(object):
48 if os.path.exists(disable_file):
49 return (None, disable_file)
50
51- cmdline_cfg = ('cmdline', cmdline.read_kernel_cmdline_config())
52- dscfg = ('ds', None)
53+ available_cfgs = {
54+ NetworkConfigSource.cmdline: cmdline.read_kernel_cmdline_config(),
55+ NetworkConfigSource.ds: None,
56+ NetworkConfigSource.system_cfg: self.cfg.get('network'),
57+ }
58+
59 if self.datasource and hasattr(self.datasource, 'network_config'):
60- dscfg = ('ds', self.datasource.network_config)
61- sys_cfg = ('system_cfg', self.cfg.get('network'))
62+ available_cfgs[NetworkConfigSource.ds] = (
63+ self.datasource.network_config)
64
65- for loc, ncfg in (cmdline_cfg, sys_cfg, dscfg):
66+ if self.datasource:
67+ order = self.datasource.network_config_sources
68+ else:
69+ order = sources.DataSource.network_config_sources
70+ for cfg_source in order:
71+ if not hasattr(NetworkConfigSource, cfg_source):
72+ LOG.warning('data source specifies an invalid network'
73+ ' cfg_source: %s', cfg_source)
74+ continue
75+ if cfg_source not in available_cfgs:
76+ LOG.warning('data source specifies an unavailable network'
77+ ' cfg_source: %s', cfg_source)
78+ continue
79+ ncfg = available_cfgs[cfg_source]
80 if net.is_disabled_cfg(ncfg):
81- LOG.debug("network config disabled by %s", loc)
82- return (None, loc)
83+ LOG.debug("network config disabled by %s", cfg_source)
84+ return (None, cfg_source)
85 if ncfg:
86- return (ncfg, loc)
87- return (self.distro.generate_fallback_config(), "fallback")
88+ return (ncfg, cfg_source)
89+ return (self.distro.generate_fallback_config(),
90+ NetworkConfigSource.fallback)
91
92 def _apply_netcfg_names(self, netcfg):
93 try:
94diff --git a/cloudinit/tests/test_stages.py b/cloudinit/tests/test_stages.py
95index 9b48312..7e13e29 100644
96--- a/cloudinit/tests/test_stages.py
97+++ b/cloudinit/tests/test_stages.py
98@@ -6,6 +6,7 @@ import os
99
100 from cloudinit import stages
101 from cloudinit import sources
102+from cloudinit.sources import NetworkConfigSource
103
104 from cloudinit.event import EventType
105 from cloudinit.util import write_file
106@@ -63,7 +64,7 @@ class TestInit(CiTestCase):
107 """find_networking_config returns when disabled by kernel cmdline."""
108 m_cmdline.return_value = {'config': 'disabled'}
109 self.assertEqual(
110- (None, 'cmdline'),
111+ (None, NetworkConfigSource.cmdline),
112 self.init._find_networking_config())
113 self.assertEqual('DEBUG: network config disabled by cmdline\n',
114 self.logs.getvalue())
115@@ -78,7 +79,7 @@ class TestInit(CiTestCase):
116 self.init.datasource = FakeDataSource(
117 network_config={'config': 'disabled'})
118 self.assertEqual(
119- (None, 'ds'),
120+ (None, NetworkConfigSource.ds),
121 self.init._find_networking_config())
122 self.assertEqual('DEBUG: network config disabled by ds\n',
123 self.logs.getvalue())
124@@ -90,12 +91,62 @@ class TestInit(CiTestCase):
125 self.init._cfg = {'system_info': {'paths': {'cloud_dir': self.tmpdir}},
126 'network': {'config': 'disabled'}}
127 self.assertEqual(
128- (None, 'system_cfg'),
129+ (None, NetworkConfigSource.system_cfg),
130 self.init._find_networking_config())
131 self.assertEqual('DEBUG: network config disabled by system_cfg\n',
132 self.logs.getvalue())
133
134 @mock.patch('cloudinit.stages.cmdline.read_kernel_cmdline_config')
135+ def test__find_networking_config_uses_datasrc_order(self, m_cmdline):
136+ """find_networking_config should check sources in DS defined order"""
137+ # cmdline, which would normally be preferred over other sources,
138+ # disables networking; in this case, though, the DS moves cmdline later
139+ # so its own config is preferred
140+ m_cmdline.return_value = {'config': 'disabled'}
141+
142+ ds_net_cfg = {'config': {'needle': True}}
143+ self.init.datasource = FakeDataSource(network_config=ds_net_cfg)
144+ self.init.datasource.network_config_sources = [
145+ NetworkConfigSource.ds, NetworkConfigSource.system_cfg,
146+ NetworkConfigSource.cmdline]
147+
148+ self.assertEqual(
149+ (ds_net_cfg, NetworkConfigSource.ds),
150+ self.init._find_networking_config())
151+
152+ @mock.patch('cloudinit.stages.cmdline.read_kernel_cmdline_config')
153+ def test__find_networking_config_warns_if_datasrc_uses_invalid_src(
154+ self, m_cmdline):
155+ """find_networking_config should check sources in DS defined order"""
156+ ds_net_cfg = {'config': {'needle': True}}
157+ self.init.datasource = FakeDataSource(network_config=ds_net_cfg)
158+ self.init.datasource.network_config_sources = [
159+ 'invalid_src', NetworkConfigSource.ds]
160+
161+ self.assertEqual(
162+ (ds_net_cfg, NetworkConfigSource.ds),
163+ self.init._find_networking_config())
164+ self.assertIn('WARNING: data source specifies an invalid network'
165+ ' cfg_source: invalid_src',
166+ self.logs.getvalue())
167+
168+ @mock.patch('cloudinit.stages.cmdline.read_kernel_cmdline_config')
169+ def test__find_networking_config_warns_if_datasrc_uses_unavailable_src(
170+ self, m_cmdline):
171+ """find_networking_config should check sources in DS defined order"""
172+ ds_net_cfg = {'config': {'needle': True}}
173+ self.init.datasource = FakeDataSource(network_config=ds_net_cfg)
174+ self.init.datasource.network_config_sources = [
175+ NetworkConfigSource.fallback, NetworkConfigSource.ds]
176+
177+ self.assertEqual(
178+ (ds_net_cfg, NetworkConfigSource.ds),
179+ self.init._find_networking_config())
180+ self.assertIn('WARNING: data source specifies an unavailable network'
181+ ' cfg_source: fallback',
182+ self.logs.getvalue())
183+
184+ @mock.patch('cloudinit.stages.cmdline.read_kernel_cmdline_config')
185 def test_wb__find_networking_config_returns_kernel(self, m_cmdline):
186 """find_networking_config returns kernel cmdline config if present."""
187 expected_cfg = {'config': ['fakekernel']}
188@@ -105,7 +156,7 @@ class TestInit(CiTestCase):
189 self.init.datasource = FakeDataSource(
190 network_config={'config': ['fakedatasource']})
191 self.assertEqual(
192- (expected_cfg, 'cmdline'),
193+ (expected_cfg, NetworkConfigSource.cmdline),
194 self.init._find_networking_config())
195
196 @mock.patch('cloudinit.stages.cmdline.read_kernel_cmdline_config')
197@@ -118,7 +169,7 @@ class TestInit(CiTestCase):
198 self.init.datasource = FakeDataSource(
199 network_config={'config': ['fakedatasource']})
200 self.assertEqual(
201- (expected_cfg, 'system_cfg'),
202+ (expected_cfg, NetworkConfigSource.system_cfg),
203 self.init._find_networking_config())
204
205 @mock.patch('cloudinit.stages.cmdline.read_kernel_cmdline_config')
206@@ -129,7 +180,7 @@ class TestInit(CiTestCase):
207 expected_cfg = {'config': ['fakedatasource']}
208 self.init.datasource = FakeDataSource(network_config=expected_cfg)
209 self.assertEqual(
210- (expected_cfg, 'ds'),
211+ (expected_cfg, NetworkConfigSource.ds),
212 self.init._find_networking_config())
213
214 @mock.patch('cloudinit.stages.cmdline.read_kernel_cmdline_config')
215@@ -148,7 +199,7 @@ class TestInit(CiTestCase):
216 distro = self.init.distro
217 distro.generate_fallback_config = fake_generate_fallback
218 self.assertEqual(
219- (fake_cfg, 'fallback'),
220+ (fake_cfg, NetworkConfigSource.fallback),
221 self.init._find_networking_config())
222 self.assertNotIn('network config disabled', self.logs.getvalue())
223
224@@ -177,7 +228,7 @@ class TestInit(CiTestCase):
225 'name': 'eth9', 'mac_address': '42:42:42:42:42:42'}]}
226
227 def fake_network_config():
228- return net_cfg, 'fallback'
229+ return net_cfg, NetworkConfigSource.fallback
230
231 m_macs.return_value = {'42:42:42:42:42:42': 'eth9'}
232
233@@ -199,7 +250,7 @@ class TestInit(CiTestCase):
234 'name': 'eth9', 'mac_address': '42:42:42:42:42:42'}]}
235
236 def fake_network_config():
237- return net_cfg, 'fallback'
238+ return net_cfg, NetworkConfigSource.fallback
239
240 self.init._find_networking_config = fake_network_config
241 self.init.apply_network_config(True)
242@@ -223,7 +274,7 @@ class TestInit(CiTestCase):
243 'name': 'eth9', 'mac_address': '42:42:42:42:42:42'}]}
244
245 def fake_network_config():
246- return net_cfg, 'fallback'
247+ return net_cfg, NetworkConfigSource.fallback
248
249 m_macs.return_value = {'42:42:42:42:42:42': 'eth9'}
250
251diff --git a/tests/unittests/test_datasource/test_common.py b/tests/unittests/test_datasource/test_common.py
252index 6b01a4e..2a9cfb2 100644
253--- a/tests/unittests/test_datasource/test_common.py
254+++ b/tests/unittests/test_datasource/test_common.py
255@@ -83,4 +83,15 @@ class ExpectedDataSources(test_helpers.TestCase):
256 self.assertEqual(set([AliYun.DataSourceAliYun]), set(found))
257
258
259+class TestDataSourceInvariants(test_helpers.TestCase):
260+
261+ def test_data_sources_have_valid_network_config_sources(self):
262+ for ds in DEFAULT_LOCAL + DEFAULT_NETWORK:
263+ for cfg_src in ds.network_config_sources:
264+ fail_msg = ('{} has an invalid network_config_sources entry:'
265+ ' {}'.format(str(ds), cfg_src))
266+ self.assertTrue(hasattr(sources.NetworkConfigSource, cfg_src),
267+ fail_msg)
268+
269+
270 # vi: ts=4 expandtab

Subscribers

People subscribed via source and target branches