Merge ~rmccabe/cloud-init:bug1705804-5 into cloud-init:master
- Git
- lp:~rmccabe/cloud-init
- bug1705804-5
- Merge into master
Status: | Merged |
---|---|
Merged at revision: | bbe91cdc6917adb503b455e6860c21ea7b3f567f |
Proposed branch: | ~rmccabe/cloud-init:bug1705804-5 |
Merge into: | cloud-init:master |
Diff against target: |
72 lines (+23/-2) 3 files modified
cloudinit/net/network_state.py (+10/-2) cloudinit/net/sysconfig.py (+7/-0) tests/unittests/test_net.py (+6/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Server Team CI bot | continuous-integration | Approve | |
Scott Moser | Pending | ||
Review via email: mp+334006@code.launchpad.net |
This proposal supersedes a proposal from 2017-11-16.
Commit message
sysconfig: Correctly render dns and dns search info.
Currently when dns and dns search info is provided, it is not rendered
when outputting to sysconfig format.
This patch causes the DNS and DOMAIN lines to be written out rendering
sysconfig.
LP: #1705804
Description of the change
Ryan McCabe (rmccabe) wrote : Posted in a previous version of this proposal | # |
Server Team CI bot (server-team-bot) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:cc2c67acb04
https:/
Executed test runs:
SUCCESS: Checkout
FAILED: Unit & Style Tests
Click here to trigger a rebuild:
https:/
Ryan Harper (raharper) : Posted in a previous version of this proposal | # |
Ryan McCabe (rmccabe) wrote : Posted in a previous version of this proposal | # |
Updated per your last comments for merge after your fixes.
Server Team CI bot (server-team-bot) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:fd2550516bd
https:/
Executed test runs:
SUCCESS: Checkout
FAILED: Unit & Style Tests
Click here to trigger a rebuild:
https:/
Ryan McCabe (rmccabe) wrote : Posted in a previous version of this proposal | # |
The unit tests will fail on this one until the patch you suggested is applied, btw, because of the dns search info being passed as a list.
Ryan Harper (raharper) wrote : Posted in a previous version of this proposal | # |
I was suggesting you pick up that patch and include it with this fix. If you'd prefer, I can MP that separately.
Ryan McCabe (rmccabe) wrote : Posted in a previous version of this proposal | # |
Sure, no problem. I can stick it in there and resubmit.
Ryan McCabe (rmccabe) : Posted in a previous version of this proposal | # |
Ryan Harper (raharper) wrote : Posted in a previous version of this proposal | # |
On Thu, Nov 16, 2017 at 12:35 PM, Ryan McCabe <email address hidden> wrote:
>
>
> Diff comments:
>
> > diff --git a/cloudinit/
> > index f572796..6e11247 100644
> > --- a/cloudinit/
> > +++ b/cloudinit/
> > @@ -347,6 +347,16 @@ class Renderer(
> > else:
> > iface_cfg[
> >
> > + if 'dns_search' in subnet:
> > + if isinstance(
>
> Is adding the dns search and nameservers info into the network_state dict
> the right thing to do here? It looks like dns info from all interfaces, not
> just subnets of the current one, is being added, and it produces bad output
> e.g.,:
>
>
> iface lo inet loopback
> dns-nameservers 69.9.160.191 69.9.191.4 10.0.0.1
> dns-search foo.com
dns-nameservers is valid and accepts space separated list
>
>
> when it should be
>
> iface lo inet loopback
> dns-nameservers 10.0.0.1
> dns-search foo.com
>
> Doing only the listify and not adding the info to the network_state dict
> there seems to do the right thing.
>
Yes; that may be the best bet here. That keeps with the more general idea
that dns-settings should be
scoped per-interface.
>
> > + iface_cfg['DOMAIN'] = '
> '.join(
> > + else:
> > + iface_cfg['DOMAIN'] = subnet[
> > +
> > + if 'dns_nameservers' in subnet:
> > + for i, k in enumerate(
> 1):
> > + iface_cfg['DNS' + str(i)] = k
> > +
> > @classmethod
> > def _render_
> > for i, subnet in enumerate(subnets,
> start=len(
>
>
> --
> https:/
> init/+merge/333722
> Your team cloud-init commiters is requested to review the proposed merge
> of ~rmccabe/
>
> _______
> Mailing list: https:/
> Post to : <email address hidden>
> Unsubscribe : https:/
> More help : https:/
>
Scott Moser (smoser) wrote : Posted in a previous version of this proposal | # |
one comment on ryan's patch inline
do not use .split(" "), here is why:
> "a b c".split()
['a', 'b', 'c']
> "a b c".split(" ")
['a', 'b', '', '', 'c']
Ryan McCabe (rmccabe) wrote : Posted in a previous version of this proposal | # |
Sorry, what I meant re: "dns-nameservers 69.9.160.191 69.9.191.4 10.0.0.1" is that the first two IPs were pulled from an ethX interface and don't belong there, given the yaml, not that there was a problem with the syntax.
ACK, re: Scott's comment
I'll resubmit a proposal that does only the listify and cleans up the split
Server Team CI bot (server-team-bot) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:f85901e5bab
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
FAILED: MAAS Compatability Testing
Click here to trigger a rebuild:
https:/
Ryan Harper (raharper) wrote : Posted in a previous version of this proposal | # |
> one comment on ryan's patch inline
>
> do not use .split(" "), here is why:
> > "a b c".split()
> ['a', 'b', 'c']
>
> > "a b c".split(" ")
> ['a', 'b', '', '', 'c']
I had to hit reply to see how those two strings looked different
Even cut and pasting, something is removing the additional spaces you added:
"a b c".split(" ")
Crazy, but point taken,
>>> "a...b.
['a', '', '', 'b', 'c']
Scott Moser (smoser) wrote : Posted in a previous version of this proposal | # |
I'm going to mark this as 'rejected' as i'm 99% sure you have fixed with
https:/
re-open if that is not the case.
Ryan McCabe (rmccabe) wrote : Posted in a previous version of this proposal | # |
Yeah, some combination of misclicks landed this in "needs review" from superseded, and I didn't see a way to put it back to there.
Scott Moser (smoser) wrote : Posted in a previous version of this proposal | # |
Ryan,
One question inline.
Also, rebase on master and push and the bot will re-test which will hopefully be happy (trunk was busted, causing the failure that it reported).
Last, i think the "listify" that you're doing better fits in 'normalize_subnets'
http://
Try that diff on top of yours and see what you think.
take it and re-push and we should be good.
Ryan McCabe (rmccabe) wrote : Posted in a previous version of this proposal | # |
Responses inline. Your diff looks good. re-pushed with that and one change per the inline comments.
Ryan McCabe (rmccabe) : Posted in a previous version of this proposal | # |
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:131161a3031
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: MAAS Compatability Testing
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
Scott Moser (smoser) wrote : | # |
Ryan,
Just fyi, you can 'git push --force' over the top of an existing merge proposal and it will update correctly.
Also, while not terribly obvious, launchpad *does* keep track of older versions of the MP. You can see a list of the hashes in the drop down under 'Preview Diff'
Scott Moser (smoser) wrote : | # |
I shoudl have said above that you can 'push --overwrite' instead of pushing a new branch and marking superseeded.
Scott Moser (smoser) wrote : | # |
Did you not like my debug message about dns_nameservers?
You dropped it from my patch.
intentional?
thanks, Ryan.
Scott Moser (smoser) wrote : | # |
Ah, I see your response.
| I just looked into this a bit more. I was told that 3 was the limit,
| but looking at the syntax in the sysconfig.txt file that gets installed
| as part of the documentation, it only shows DNS1 and DNS2, howerver I'm
| finding plenty of configs if I google that show more than 3. I've
| removed the slice and the warning, because it may be trying to hit a
| moving target. Worst that happens is the additional entries that are
| written may not be used.
OK. That works.
The belief is specifically stated in 'man resolv.conf' on my system:
http://
and apparently in RHEL7 also.
http://
The define seems to have moved on my system from resolv.conf to
/usr/include/
does appear true.
I'll just put it back in.
Preview Diff
1 | diff --git a/cloudinit/net/network_state.py b/cloudinit/net/network_state.py |
2 | index 0e830ee..e65a99c 100644 |
3 | --- a/cloudinit/net/network_state.py |
4 | +++ b/cloudinit/net/network_state.py |
5 | @@ -326,8 +326,8 @@ class NetworkStateInterpreter(object): |
6 | subnets = _normalize_subnets(command.get('subnets')) |
7 | |
8 | # automatically set 'use_ipv6' if any addresses are ipv6 |
9 | - if not self.use_ipv6: |
10 | - for subnet in subnets: |
11 | + for subnet in subnets: |
12 | + if not self.use_ipv6: |
13 | if (subnet.get('type').endswith('6') or |
14 | is_ipv6_addr(subnet.get('address'))): |
15 | self.use_ipv6 = True |
16 | @@ -746,6 +746,14 @@ def _normalize_subnet(subnet): |
17 | _normalize_net_keys(normal_subnet, address_keys=('address',))) |
18 | normal_subnet['routes'] = [_normalize_route(r) |
19 | for r in subnet.get('routes', [])] |
20 | + |
21 | + def listify(snet, name): |
22 | + if name in snet and not isinstance(snet[name], list): |
23 | + snet[name] = snet[name].split() |
24 | + |
25 | + for k in ('dns_search', 'dns_nameservers'): |
26 | + listify(normal_subnet, k) |
27 | + |
28 | return normal_subnet |
29 | |
30 | |
31 | diff --git a/cloudinit/net/sysconfig.py b/cloudinit/net/sysconfig.py |
32 | index f572796..090d919 100644 |
33 | --- a/cloudinit/net/sysconfig.py |
34 | +++ b/cloudinit/net/sysconfig.py |
35 | @@ -347,6 +347,13 @@ class Renderer(renderer.Renderer): |
36 | else: |
37 | iface_cfg['GATEWAY'] = subnet['gateway'] |
38 | |
39 | + if 'dns_search' in subnet: |
40 | + iface_cfg['DOMAIN'] = ' '.join(subnet['dns_search']) |
41 | + |
42 | + if 'dns_nameservers' in subnet: |
43 | + for i, k in enumerate(subnet['dns_nameservers'], 1): |
44 | + iface_cfg['DNS' + str(i)] = k |
45 | + |
46 | @classmethod |
47 | def _render_subnet_routes(cls, iface_cfg, route_cfg, subnets): |
48 | for i, subnet in enumerate(subnets, start=len(iface_cfg.children)): |
49 | diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py |
50 | index bbb63cb..f3fa2a3 100644 |
51 | --- a/tests/unittests/test_net.py |
52 | +++ b/tests/unittests/test_net.py |
53 | @@ -436,6 +436,9 @@ NETWORK_CONFIGS = { |
54 | BOOTPROTO=dhcp |
55 | DEFROUTE=yes |
56 | DEVICE=eth99 |
57 | + DNS1=8.8.8.8 |
58 | + DNS2=8.8.4.4 |
59 | + DOMAIN="barley.maas sach.maas" |
60 | GATEWAY=65.61.151.37 |
61 | HWADDR=c0:d6:9f:2c:e8:80 |
62 | IPADDR=192.168.21.3 |
63 | @@ -836,6 +839,9 @@ pre-down route del -net 10.0.0.0 netmask 255.0.0.0 gw 11.0.0.1 metric 3 || true |
64 | BOOTPROTO=none |
65 | DEFROUTE=yes |
66 | DEVICE=eth0.101 |
67 | + DNS1=192.168.0.10 |
68 | + DNS2=10.23.23.134 |
69 | + DOMAIN="barley.maas sacchromyces.maas brettanomyces.maas" |
70 | GATEWAY=192.168.0.1 |
71 | IPADDR=192.168.0.2 |
72 | IPADDR1=192.168.2.10 |
Updated to handle dns_search when received as either a string or a list
Updated per Ryan Harper's other comments on the previous iteration