cloud-init

Merge ~xnox/cloud-init:cidr into cloud-init:master

  1. Git
  2. lp:~xnox/cloud-init
  3. cidr
  4. Merge into master
Proposed by Dimitri John Ledkov on 2017-05-13
Status: Merged
Merged at revision: 16a7302f6acb69adb0aee75eaf12392fa3688853
Proposed branch: ~xnox/cloud-init:cidr
Merge into: cloud-init:master
Diff against target: 81 lines (+12/-10)
4 files modified
cloudinit/net/netplan.py (+8/-6)
cloudinit/net/network_state.py (+2/-2)
tests/unittests/test_distros/test_netconfig.py (+1/-1)
tests/unittests/test_net.py (+1/-1)
Reviewer Review Type Date Requested Status
Scott Moser Approve on 2017-05-24
Server Team CI bot continuous-integration Approve on 2017-05-22
Ryan Harper 2017-05-13 Needs Fixing on 2017-05-15
Review via email: mp+324010@code.launchpad.net

Commit Message

netplan: generate cidr routes and netmasks.

LP: #1689346

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

FAILED: Continuous integration, rev:678db0c7f5f031725ba72625504e50c5c8f55936
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~xnox/cloud-init/+git/cloud-init/+merge/324010/+edit-commit-message

https://jenkins.ubuntu.com/server/job/cloud-init-ci/330/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/330
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/330
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/330
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/330

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

review: Needs Fixing (continuous-integration)
Ryan Harper (raharper) wrote :

In both cases, use of mask2cidr when not a dotted quad will return the original string sent, but that will end up embedded strings instead of integers:

>>> network_state.mask2cidr('24')
'24'
>>> network_state.mask2cidr('255.255.255.0')
24

I think we either need to modify mask2cidr to do something else (convert to integer) so it it's consistent across input invocations, or if we don't change mask2cidr, then the callers need to handle all of the following

address = 192.168.1.2/24 , netmask: None
address = 192.168.1.2/255.255.255.0 , netmask: None
address = 192.168.1.2 , netmask: 24
address = 192.168.1.2 , netmask: 255.255.255.0

https://code.launchpad.net/~raharper/curtin/trunk.vmtest-reuse-output/+merge/318150

review: Needs Fixing
~xnox/cloud-init:cidr updated on 2017-05-16
9e5cdbb... by Dimitri John Ledkov on 2017-05-16

nplan: correctly generate ipv6 and ipv4 netmask addresses

Insure that only cidr used when outputing netmasks in netplan.

LP: #1689346
LP: #1691100

Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Dimitri John Ledkov (xnox) wrote :

Any idea why test_handler_apt_source_v3.py is failing on the i386?

Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Scott Moser (smoser) wrote :

I just re-built this at
 https://jenkins.ubuntu.com/server/job/cloud-init-ci/366/
and the i386 ran fine. obviously it wasn't a problem with Dimitri's code that triggered it.

It feels like a bug in cheetah or python. :(.

I've put the following diff in place:
diff --git a/cloudinit/templater.py b/cloudinit/templater.py
index b3ea64e..c24265a 100644
--- a/cloudinit/templater.py
+++ b/cloudinit/templater.py
@@ -76,6 +76,8 @@ def basic_render(content, params):
 def detect_template(text):

     def cheetah_render(content, params):
+ print("Calling CTemplate(%r, searchList=[%r]).respond()" %
+ (content, params))
         return CTemplate(content, searchList=[params]).respond()

     def jinja_render(content, params):

And then run the failing test case like:
 tox-venv py27 nosetests --nocapture -v tests/unittests/test_handler/test_handl
er_apt_source_v3.py:TestAptSourceConfig.test_apt_v3_disable_suites

It shows:
test_disable_suites - disable_suites with many configurations ... Calling CTemplate('$RELEASE', searchList=[{'RELEASE': 'xenial'}]).respond()
Calling CTemplate('$RELEASE-updates', searchList=[{'RELEASE': 'xenial'}]).respond()
Calling CTemplate('$RELEASE-updates', searchList=[{'RELEASE': 'xenial'}]).respond()
Calling CTemplate('$RELEASE-security', searchList=[{'RELEASE': 'xenial'}]).respond()
Calling CTemplate('$RELEASE-updates', searchList=[{'RELEASE': 'xenial'}]).respond()
Calling CTemplate('$RELEASE-security', searchList=[{'RELEASE': 'xenial'}]).respond()
Calling CTemplate('$RELEASE-updates', searchList=[{'RELEASE': 'xenial'}]).respond()
Calling CTemplate('$RELEASE-security', searchList=[{'RELEASE': 'xenial'}]).respond()
Calling CTemplate('foobar', searchList=[{'RELEASE': 'xenial'}]).respond()
Calling CTemplate('foobar', searchList=[{'RELEASE': 'xenial'}]).respond()
Calling CTemplate('$RELEASE-updates', searchList=[{'RELEASE': 'xenial'}]).respond()
Calling CTemplate('$RELEASE-updates', searchList=[{'RELEASE': 'xenial'}]).respond()
Calling CTemplate('$RELEASE-security', searchList=[{'RELEASE': 'xenial'}]).respond()

This is pretty straight forward usage of cheetah, the CTemplate is just
 from Cheetah.Template import Template as CTemplate

I've also tried calling CTemplate with None in either or both positions and you end up with a different stack trace than above. All of this on both trusty and xenial i386.

I was running in lxc where as c-i runs in a vm.

Scott Moser (smoser) wrote :

I really want to clean up the 'network state' object.
It really should store only address and network_prefix and then renderers would render that to either a cidr or address and netmask.

note, i think that <address>/<netmask> is an abomination.
Ie:
 good: 192.168.1.2/24
 good: address=192.168.1.2 netmask: 255.255.255.0
 bad: 192.168.1.2/255.255.255.0

Wherever possible i want to warn or raise error on the bad above.

Rant aside, I'll grab this.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/net/netplan.py b/cloudinit/net/netplan.py
2index 56b41be..1c0cec9 100644
3--- a/cloudinit/net/netplan.py
4+++ b/cloudinit/net/netplan.py
5@@ -4,7 +4,7 @@ import copy
6 import os
7
8 from . import renderer
9-from .network_state import subnet_is_ipv6
10+from .network_state import mask2cidr, subnet_is_ipv6
11
12 from cloudinit import log as logging
13 from cloudinit import util
14@@ -118,9 +118,10 @@ def _extract_addresses(config, entry):
15 sn_type += '4'
16 entry.update({sn_type: True})
17 elif sn_type in ['static']:
18- addr = "%s" % subnet.get('address')
19- if 'netmask' in subnet:
20- addr += "/%s" % subnet.get('netmask')
21+ addr = '%s' % subnet.get('address')
22+ netmask = subnet.get('netmask')
23+ if netmask and '/' not in addr:
24+ addr += '/%s' % mask2cidr(netmask)
25 if 'gateway' in subnet and subnet.get('gateway'):
26 gateway = subnet.get('gateway')
27 if ":" in gateway:
28@@ -137,8 +138,9 @@ def _extract_addresses(config, entry):
29 mtukey += '6'
30 entry.update({mtukey: subnet.get('mtu')})
31 for route in subnet.get('routes', []):
32- to_net = "%s/%s" % (route.get('network'),
33- route.get('netmask'))
34+ network = route.get('network')
35+ netmask = route.get('netmask')
36+ to_net = '%s/%s' % (network, mask2cidr(netmask))
37 route = {
38 'via': route.get('gateway'),
39 'to': to_net,
40diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py
41index db3c357..9e9c05a 100644
42--- a/cloudinit/net/network_state.py
43+++ b/cloudinit/net/network_state.py
44@@ -734,9 +734,9 @@ def ipv6mask2cidr(mask):
45
46
47 def mask2cidr(mask):
48- if ':' in mask:
49+ if ':' in str(mask):
50 return ipv6mask2cidr(mask)
51- elif '.' in mask:
52+ elif '.' in str(mask):
53 return ipv4mask2cidr(mask)
54 else:
55 return mask
56diff --git a/tests/unittests/test_distros/test_netconfig.py b/tests/unittests/test_distros/test_netconfig.py
57index 1e10a33..4fe16a9 100644
58--- a/tests/unittests/test_distros/test_netconfig.py
59+++ b/tests/unittests/test_distros/test_netconfig.py
60@@ -127,7 +127,7 @@ network:
61 ethernets:
62 eth0:
63 addresses:
64- - 192.168.1.5/255.255.255.0
65+ - 192.168.1.5/24
66 gateway4: 192.168.1.254
67 eth1:
68 dhcp4: true
69diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
70index d36d0e7..951a071 100644
71--- a/tests/unittests/test_net.py
72+++ b/tests/unittests/test_net.py
73@@ -441,7 +441,7 @@ NETWORK_CONFIGS = {
74 - sach.maas
75 - wark.maas
76 routes:
77- - to: 0.0.0.0/0.0.0.0
78+ - to: 0.0.0.0/0
79 via: 65.61.151.37
80 set-name: eth99
81 """).rstrip(' '),

Subscribers

People subscribed via source and target branches