Merge ~mgerdts/cloud-init:lp1765801 into cloud-init:master

Proposed by Mike Gerdts
Status: Superseded
Proposed branch: ~mgerdts/cloud-init:lp1765801
Merge into: cloud-init:master
Prerequisite: ~mgerdts/cloud-init:update_metadata
Diff against target: 86 lines (+15/-6)
3 files modified
cloudinit/sources/DataSourceSmartOS.py (+7/-1)
cloudinit/sources/tests/test_init.py (+0/-4)
tests/unittests/test_datasource/test_smartos.py (+8/-1)
Reviewer Review Type Date Requested Status
Ryan Harper Needs Fixing
Chad Smith Pending
Review via email: mp+373171@code.launchpad.net

This proposal supersedes a proposal from 2018-07-20.

This proposal has been superseded by a proposal from 2019-10-31.

Commit message

DataSourceSmartOS: reconfigure network on each boot

In typical cases, SmartOS does not use DHCP for network configuration.
As such, if the network configuration changes that is reflected in
metadata and will be picked up during the next boot.

LP: #1765801
Joyent: OS-6902 cloud-init should be able to reconfigure network on each boot

To post a comment you must log in.
Revision history for this message
Chad Smith (chad.smith) wrote : Posted in a previous version of this proposal

Thanks for the branch Mike, just a request for unit test or an additional assert on the SmartOs datasource update_events attribute as well as what seems to be a git rebase required on your branch.

Revision history for this message
Mike Gerdts (mgerdts) : Posted in a previous version of this proposal
Revision history for this message
Chad Smith (chad.smith) : Posted in a previous version of this proposal
Revision history for this message
Chad Smith (chad.smith) wrote : Posted in a previous version of this proposal

Hi Mike,

  Marking this 'Work in progress' for the time being so it doesn't sit in my 'needs review' column.

When you get a chance to push your changes and want a review, please just set this branch back to 'Needs review' and we'll take care of it.

thanks again,
Chad

Revision history for this message
Mike Gerdts (mgerdts) wrote :

I've looked high and low for what is setting ds._network_config = sources.UNSET and have come up empty. I gave up when I realized that DataSourceAzure follows a similar pattern.

I'm also not sure about what the proper way to allow an instance to say "hands off on future reboots". Is apply_network_config the right approach?

Revision history for this message
Mike Gerdts (mgerdts) wrote :

Upon further experimentation, I don't think that adding (per-datasource) apply_network_config is needed. Setting it can cause a previously working instance to no longer be reachable over the net.

Better is:

network: {config: disabled}

This allows a working config to persist in /etc/netplan/50-cloud-init.yaml while not making further changes on subsequent boots.

Revision history for this message
Ryan Harper (raharper) wrote :

I see a merge conflict in the diff below, I think a rebase should resolve this.

review: Needs Fixing
Revision history for this message
Mike Gerdts (mgerdts) wrote :

As I asked in IRC on September 25:

[10:27:07] <mgerdts> I rebased an old branch, made some changes, force pushed, then resubmitted a merge request. The diff shows conflicts that don't really exist, seemingly related to the 13 month old version that was previously there. Would it be easier if I just submitted a fresh merge request rather than trying to make this one happy?
[10:27:09] <mgerdts> https://code.launchpad.net/~mgerdts/cloud-init/+git/cloud-init/+merge/373171

I have once again rebased it and force pushed. The diff below still shows the conflict. `git show` in my workspace does not show the conflict. My question from September 25 remains.

Unmerged commits

caff922... by Mike Gerdts

DataSourceSmartOS: reconfigure network on each boot

In typical cases, SmartOS does not use DHCP for network configuration. As such,
if the network configuration changes that is reflected in metadata and will be
picked up during the next boot.

LP: #1765801
Joyent: OS-6902 cloud-init should be able to reconfigure network on each boot

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/sources/DataSourceSmartOS.py b/cloudinit/sources/DataSourceSmartOS.py
2index 32b57cd..cf67650 100644
3--- a/cloudinit/sources/DataSourceSmartOS.py
4+++ b/cloudinit/sources/DataSourceSmartOS.py
5@@ -1,5 +1,5 @@
6 # Copyright (C) 2013 Canonical Ltd.
7-# Copyright (c) 2018, Joyent, Inc.
8+# Copyright 2019 Joyent, Inc.
9 #
10 # Author: Ben Howard <ben.howard@canonical.com>
11 #
12@@ -34,6 +34,7 @@ from cloudinit import log as logging
13 from cloudinit import serial
14 from cloudinit import sources
15 from cloudinit import util
16+from cloudinit.event import EventType
17
18 LOG = logging.getLogger(__name__)
19
20@@ -178,6 +179,7 @@ class DataSourceSmartOS(sources.DataSource):
21 self.metadata = {}
22 self.network_data = None
23 self._network_config = None
24+ self.update_events['network'].add(EventType.BOOT)
25
26 self.script_base_d = os.path.join(self.paths.get_cpath("scripts"))
27
28@@ -319,6 +321,10 @@ class DataSourceSmartOS(sources.DataSource):
29
30 @property
31 def network_config(self):
32+ # sources.clear_cached_data() may set _network_config to '_unset'.
33+ if self._network_config == sources.UNSET:
34+ self._network_config = None
35+
36 if self._network_config is None:
37 if self.network_data is not None:
38 self._network_config = (
39diff --git a/cloudinit/sources/tests/test_init.py b/cloudinit/sources/tests/test_init.py
40index 4154a5f..9698261 100644
41--- a/cloudinit/sources/tests/test_init.py
42+++ b/cloudinit/sources/tests/test_init.py
43@@ -543,11 +543,7 @@ class TestDataSource(CiTestCase):
44 """update_metadata won't get_data on unsupported update events."""
45 self.datasource.update_events['network'].discard(EventType.BOOT)
46 self.assertEqual(
47-<<<<<<< cloudinit/sources/tests/test_init.py
48 {'network': set([EventType.BOOT_NEW_INSTANCE])},
49-=======
50- {'network': {EventType.BOOT_NEW_INSTANCE}},
51->>>>>>> cloudinit/sources/tests/test_init.py
52 self.datasource.update_events)
53
54 def fake_get_data():
55diff --git a/tests/unittests/test_datasource/test_smartos.py b/tests/unittests/test_datasource/test_smartos.py
56index 42ac697..d5b1c29 100644
57--- a/tests/unittests/test_datasource/test_smartos.py
58+++ b/tests/unittests/test_datasource/test_smartos.py
59@@ -1,5 +1,5 @@
60 # Copyright (C) 2013 Canonical Ltd.
61-# Copyright (c) 2018, Joyent, Inc.
62+# Copyright 2019 Joyent, Inc.
63 #
64 # Author: Ben Howard <ben.howard@canonical.com>
65 #
66@@ -31,6 +31,7 @@ from cloudinit.sources.DataSourceSmartOS import (
67 convert_smartos_network_data as convert_net,
68 SMARTOS_ENV_KVM, SERIAL_DEVICE, get_smartos_environ,
69 identify_file)
70+from cloudinit.event import EventType
71
72 import six
73
74@@ -653,6 +654,12 @@ class TestSmartOSDataSource(FilesystemMockingTestCase):
75 self.assertEqual(dsrc.device_name_to_device('FOO'),
76 mydscfg['disk_aliases']['FOO'])
77
78+ def test_reconfig_network_on_boot(self):
79+ # Test to ensure that network is configured from metadata on each boot
80+ dsrc = self._get_ds(mockdata=MOCK_RETURNS)
81+ self.assertSetEqual(set([EventType.BOOT_NEW_INSTANCE, EventType.BOOT]),
82+ dsrc.update_events['network'])
83+
84
85 class TestIdentifyFile(CiTestCase):
86 """Test the 'identify_file' utility."""

Subscribers

People subscribed via source and target branches