Merge ~smoser/cloud-init:fix/1788915-vlan-sysconfig-rendering into cloud-init:master

Proposed by Scott Moser
Status: Rejected
Rejected by: Scott Moser
Proposed branch: ~smoser/cloud-init:fix/1788915-vlan-sysconfig-rendering
Merge into: cloud-init:master
Diff against target: 201 lines (+115/-4)
3 files modified
cloudinit/net/sysconfig.py (+30/-1)
tests/unittests/test_distros/test_netconfig.py (+85/-0)
tests/unittests/test_net.py (+0/-3)
Reviewer Review Type Date Requested Status
Ryan Harper Needs Information
Server Team CI bot continuous-integration Approve
Review via email: mp+366602@code.launchpad.net

Commit message

network: Fix type and respect name when rendering vlan in sysconfig.

Prior to this change, vlans were rendered in sysconfig with
'TYPE=Ethernet', and incorrectly renders the PHYSDEV based on
the name of the vlan device rather than the 'link' provided
in the network config.

The change here fixes:
 * rendering of TYPE=Ethernet for a vlan
 * adds a warning if the configured device name is not supported
   per the RHEL 7 docs "11.5. Naming Scheme for VLAN Interfaces"

LP: #1788915
LP: #1826608

To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

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

review: Approve (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

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

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Scott Moser (smoser) wrote :

The error shows:

ERROR: pylint: could not install deps [pylint==2.3.1, -r/var/lib/jenkins/slaves/torkoal/workspace/cloud-init-ci@2/test-requirements.txt]; v = InvocationError('/var/lib/jenkins/slaves/torkoal/workspace/cloud-init-ci@2/.tox/pylint/bin/pip install pylint==2.3.1 -r/var/lib/jenkins/slaves/torkoal/workspace/cloud-init-ci@2/test-requirements.txt (see /var/lib/jenkins/slaves/torkoal/workspace/cloud-init-ci@2/.tox/pylint/log/pylint-1.log)', 1)

I don't think that is related to my changes.

Help?

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

Collecting httpretty>=0.7.1 (from -r /var/lib/jenkins/slaves/torkoal/workspace/cloud-init-ci@2/test-requirements.txt (line 2))
  ERROR: Could not find a version that satisfies the requirement httpretty>=0.7.1 (from -r /var/lib/jenkins/slaves/torkoal/workspace/cloud-init-ci@2/test-requirements.txt (line 2)) (from versions: none)
ERROR: No matching distribution found for httpretty>=0.7.1 (from -r /var/lib/jenkins/slaves/torkoal/workspace/cloud-init-ci@2/test-requirements.txt (line 2))

I wonder of upstream changed?

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

What's the deal with "unsupported" vs. not? If it's not supported, how is it working?

review: Needs Information
Revision history for this message
Scott Moser (smoser) wrote :

I don't think it will actually work. But I only tested in a container.

So the options
- leave broken with a warning
- ignore the names and create something that works
- something else

This is better than it was, which would always be broken.

On Fri, Apr 26, 2019, 10:12 PM Ryan Harper <email address hidden>
wrote:

> Review: Needs Information
>
> What's the deal with "unsupported" vs. not? If it's not supported, how is
> it working?
> --
> https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/366602
> You are the owner of
> ~smoser/cloud-init:fix/1788915-vlan-sysconfig-rendering.
>

Revision history for this message
Scott Moser (smoser) wrote :

I'm fine to change it to raise RuntimeError or something else other than WARNING.

Revision history for this message
Dan Watkins (oddbloke) wrote :

Given that other tests in the same run were able to install httpretty, I'm pretty sure the failure is just something like a hiccup communicating with PyPI; I've kicked off a re-run of CI.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

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

review: Approve (continuous-integration)
Revision history for this message
Ryan Harper (raharper) wrote :

Sorry, I'm still a little confused. From your test case, previously we rendered an "infra0" file like so:

% cat test_vlan/etc/sysconfig/network-scripts/ifcfg-infra0
# Created by cloud-init on instance boot automatically, do not edit.
#
BOOTPROTO=none
DEVICE=infra0
IPADDR=10.0.1.2
NETMASK=255.255.0.0
NM_CONTROLLED=no
ONBOOT=yes
PHYSDEV=infra
STARTMODE=auto
TYPE=Ethernet
USERCTL=no
VLAN=yes

And the line "PHYSDEV=infra" is the bug, right? Instead it should say

"PHYSDEV=eth0"; and omit "TYPE=Ethernet".

And this fails with an error in Centos/RHEL system on boot? Yes, I see
as captured in

https://code.launchpad.net/bugs/1788915

OK, now, instead of just changing the PHYSDEV line to use the "link" value
from the config, you're also suggesting that we allow (but warn) when
the DEVICE name is not in required format?

You said that you tested in a container and it didn't work; I'm taking
that to mean the DEVICE=infra0 case; to which I agree that it is going
to fail due to lack of VLAN_ID being present in what's rendered.

The config you're providing as an example which has a name "infra0" but
both the link and the port number means that in sysconfig renderer we
could warn but *fix* since we have what we need to render a supported
configuration.

Shouldn't we take that path?

review: Needs Information
Revision history for this message
Scott Moser (smoser) wrote :

You're right. I didn't even notice that sysconfig was essentially reading the vlan id from the DEVICE and had no separate way to declare it. Yuck.

I realize I inadvertantly confused things by using a '0' on the 'infra0'. I
had not intended that as any portion of the vlan information, but rather just
"the zeroth infrastructure interface".
Given the original config in the bug:
| version: 2
| ethernets:
| eth0:
| addresses: ["192.10.1.2/24"]
| match:
| macaddress: "00:16:3e:60:7c:df"
| vlans:
| infra0:
| id: 1001
| link: eth0
| addresses: ["10.0.1.2/16"]

The entity that provided this network config expected/declared that the system
would/should have two network interfaces. One named 'eth0' and one named
'infra0'.

As I understand it, sysconfig (at least as documented by RHEL) does not allow
us to render this. We could:
a.) configure a system with two interfaces 'eth0' and 'eth0.1001' and leave it
at that, probably warning "sorry not supported, we did the best we could".
b.) configure a system as in 'a' and then arrange for a 'ip link set name' to
occur in some way. (possibly with ifup-post or ifup-local [1])
c.) raise a runtime error and just say "sorry"

Do you see other options?

--
[1] https://www.cyberciti.biz/faq/centos-7-run-script-when-network-interface-is-up-network-manager/

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

I think (a) is the best bet as any (b) would require new plumbing to rename a device after cloud-init has already completed renaming (it renames physical devices only since virtual devices can be declared).

The requested config is just not supported in sysconfig. Not completely rejecting the config I think is user-friendly enough to not be hostile to busted (but not incomplete) config.

Revision history for this message
Scott Moser (smoser) wrote :

ACK. I'll try to get to that sometime.

Revision history for this message
Scott Moser (smoser) wrote :

Unmerged commits

91576dc... by Scott Moser

network: Fix type and respect name when rendering vlan in sysconfig.

Prior to this change, vlans were rendered in sysconfig with
'TYPE=Ethernet', and incorrectly renders the PHYSDEV based on
the name of the vlan device rather than the 'link' provided
in the network config.

The change here fixes:
 * rendering of TYPE=Ethernet for a vlan
 * adds a warning if the configured device name is not supported
   per the RHEL 7 docs "11.5. Naming Scheme for VLAN Interfaces"

LP: #1788915
LP: #1826608

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/net/sysconfig.py b/cloudinit/net/sysconfig.py
2index a47da0a..6118552 100644
3--- a/cloudinit/net/sysconfig.py
4+++ b/cloudinit/net/sysconfig.py
5@@ -96,6 +96,9 @@ class ConfigMap(object):
6 def __len__(self):
7 return len(self._conf)
8
9+ def str_skipped(self, key, val):
10+ return False
11+
12 def to_string(self):
13 buf = six.StringIO()
14 buf.write(_make_header())
15@@ -103,6 +106,8 @@ class ConfigMap(object):
16 buf.write("\n")
17 for key in sorted(self._conf.keys()):
18 value = self._conf[key]
19+ if self.str_skipped(key, value):
20+ continue
21 if isinstance(value, bool):
22 value = self._bool_map[value]
23 if not isinstance(value, six.string_types):
24@@ -208,6 +213,7 @@ class NetInterface(ConfigMap):
25 'bond': 'Bond',
26 'bridge': 'Bridge',
27 'infiniband': 'InfiniBand',
28+ 'vlan': 'Vlan',
29 }
30
31 def __init__(self, iface_name, base_sysconf_dir, templates,
32@@ -261,6 +267,11 @@ class NetInterface(ConfigMap):
33 c.routes = self.routes.copy()
34 return c
35
36+ def str_skipped(self, key, val):
37+ if key == 'TYPE' and val == 'Vlan':
38+ return True
39+ return False
40+
41
42 class Renderer(renderer.Renderer):
43 """Renders network information in a /etc/sysconfig format."""
44@@ -555,7 +566,16 @@ class Renderer(renderer.Renderer):
45 iface_name = iface['name']
46 iface_cfg = iface_contents[iface_name]
47 iface_cfg['VLAN'] = True
48- iface_cfg['PHYSDEV'] = iface_name[:iface_name.rfind('.')]
49+ iface_cfg.kind = 'vlan'
50+
51+ rdev = iface['vlan-raw-device']
52+ supported = _supported_vlan_names(rdev, iface['vlan_id'])
53+ if iface_name not in supported:
54+ LOG.warning(
55+ "Name '%s' not officially supported for vlan "
56+ "device backed by '%s'. Supported: %s",
57+ iface_name, rdev, ' '.join(supported))
58+ iface_cfg['PHYSDEV'] = rdev
59
60 iface_subnets = iface.get("subnets", [])
61 route_cfg = iface_cfg.routes
62@@ -716,6 +736,15 @@ class Renderer(renderer.Renderer):
63 "\n".join(netcfg) + "\n", file_mode)
64
65
66+def _supported_vlan_names(rdev, vid):
67+ """Return list of supported names for vlan devices per RHEL doc
68+ 11.5. Naming Scheme for VLAN Interfaces."""
69+ return [
70+ v.format(rdev=rdev, vid=int(vid))
71+ for v in ("{rdev}{vid:04}", "{rdev}{vid}",
72+ "{rdev}.{vid:04}", "{rdev}.{vid}")]
73+
74+
75 def available(target=None):
76 sysconfig = available_sysconfig(target=target)
77 nm = available_nm(target=target)
78diff --git a/tests/unittests/test_distros/test_netconfig.py b/tests/unittests/test_distros/test_netconfig.py
79index c3c0c8c..b0ef4aa 100644
80--- a/tests/unittests/test_distros/test_netconfig.py
81+++ b/tests/unittests/test_distros/test_netconfig.py
82@@ -526,6 +526,91 @@ class TestNetCfgDistroRedhat(TestNetCfgDistroBase):
83 V1_NET_CFG_IPV6,
84 expected_cfgs=expected_cfgs.copy())
85
86+ def test_vlan_render_unsupported(self):
87+ """Render officially unsupported vlan names."""
88+ cfg = {
89+ 'version': 2,
90+ 'ethernets': {
91+ 'eth0': {'addresses': ["192.10.1.2/24"],
92+ 'match': {'macaddress': "00:16:3e:60:7c:df"}}},
93+ 'vlans': {
94+ 'infra0': {'addresses': ["10.0.1.2/16"],
95+ 'id': 1001, 'link': 'eth0'}},
96+ }
97+ expected_cfgs = {
98+ self.ifcfg_path('eth0'): dedent("""\
99+ BOOTPROTO=none
100+ DEVICE=eth0
101+ HWADDR=00:16:3e:60:7c:df
102+ IPADDR=192.10.1.2
103+ NETMASK=255.255.255.0
104+ NM_CONTROLLED=no
105+ ONBOOT=yes
106+ STARTMODE=auto
107+ TYPE=Ethernet
108+ USERCTL=no
109+ """),
110+ self.ifcfg_path('infra0'): dedent("""\
111+ BOOTPROTO=none
112+ DEVICE=infra0
113+ IPADDR=10.0.1.2
114+ NETMASK=255.255.0.0
115+ NM_CONTROLLED=no
116+ ONBOOT=yes
117+ PHYSDEV=eth0
118+ STARTMODE=auto
119+ USERCTL=no
120+ VLAN=yes
121+ """),
122+ self.control_path(): dedent("""\
123+ NETWORKING=yes
124+ """),
125+ }
126+ self._apply_and_verify(
127+ self.distro.apply_network_config, cfg,
128+ expected_cfgs=expected_cfgs)
129+
130+ def test_vlan_render(self):
131+ cfg = {
132+ 'version': 2,
133+ 'ethernets': {
134+ 'eth0': {'addresses': ["192.10.1.2/24"]}},
135+ 'vlans': {
136+ 'eth0.1001': {'addresses': ["10.0.1.2/16"],
137+ 'id': 1001, 'link': 'eth0'}},
138+ }
139+ expected_cfgs = {
140+ self.ifcfg_path('eth0'): dedent("""\
141+ BOOTPROTO=none
142+ DEVICE=eth0
143+ IPADDR=192.10.1.2
144+ NETMASK=255.255.255.0
145+ NM_CONTROLLED=no
146+ ONBOOT=yes
147+ STARTMODE=auto
148+ TYPE=Ethernet
149+ USERCTL=no
150+ """),
151+ self.ifcfg_path('eth0.1001'): dedent("""\
152+ BOOTPROTO=none
153+ DEVICE=eth0.1001
154+ IPADDR=10.0.1.2
155+ NETMASK=255.255.0.0
156+ NM_CONTROLLED=no
157+ ONBOOT=yes
158+ PHYSDEV=eth0
159+ STARTMODE=auto
160+ USERCTL=no
161+ VLAN=yes
162+ """),
163+ self.control_path(): dedent("""\
164+ NETWORKING=yes
165+ """),
166+ }
167+ self._apply_and_verify(
168+ self.distro.apply_network_config, cfg,
169+ expected_cfgs=expected_cfgs)
170+
171
172 class TestNetCfgDistroOpensuse(TestNetCfgDistroBase):
173
174diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
175index 9db0156..b5e8e3c 100644
176--- a/tests/unittests/test_net.py
177+++ b/tests/unittests/test_net.py
178@@ -1247,7 +1247,6 @@ pre-down route del -net 10.0.0.0/8 gw 11.0.0.1 metric 3 || true
179 ONBOOT=yes
180 PHYSDEV=bond0
181 STARTMODE=auto
182- TYPE=Ethernet
183 USERCTL=no
184 VLAN=yes"""),
185 'ifcfg-br0': textwrap.dedent("""\
186@@ -1295,7 +1294,6 @@ pre-down route del -net 10.0.0.0/8 gw 11.0.0.1 metric 3 || true
187 ONBOOT=yes
188 PHYSDEV=eth0
189 STARTMODE=auto
190- TYPE=Ethernet
191 USERCTL=no
192 VLAN=yes"""),
193 'ifcfg-eth1': textwrap.dedent("""\
194@@ -1856,7 +1854,6 @@ iface bond0 inet6 static
195 ONBOOT=yes
196 PHYSDEV=en0
197 STARTMODE=auto
198- TYPE=Ethernet
199 USERCTL=no
200 VLAN=yes"""),
201 },

Subscribers

People subscribed via source and target branches