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

Proposed by Dan Watkins
Status: Rejected
Rejected by: Dan Watkins
Proposed branch: ~oddbloke/cloud-init/+git/cloud-init:networking
Merge into: cloud-init:master
Diff against target: 63 lines (+25/-13)
1 file modified
cloudinit/net/__init__.py (+25/-13)
Reviewer Review Type Date Requested Status
cloud-init Commiters Pending
Review via email: mp+372289@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Ryan Harper (raharper) wrote :

If the interface already has an IP, it may not have anything else that we'd apply after parsing the lease output (routes, etc).

This sort of feels OK in that we skip tearing down when we don't do anything else, but it also seems wrong in that the whole context manager should exit if we don't bring up DHCP.

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

On Wed, Sep 04, 2019 at 06:30:07PM -0000, Ryan Harper wrote:
> If the interface already has an IP, it may not have anything else that
> we'd apply after parsing the lease output (routes, etc).

If the interface already has an IP before we DHCP, it's not 100% clear
to me that we should consider routes etc. from the DHCP response
applicable.

> This sort of feels OK in that we skip tearing down when we don't do
> anything else, but it also seems wrong in that the whole context
> manager should exit if we don't bring up DHCP.

What do you mean by "should exit" here? The way we use the context
manager (in DataSourceOpenStack, at least), is essentially "ensure I
have network while executing the body", so not needing to configure
networking isn't really an error condition in that usecase.

Revision history for this message
Chad Smith (chad.smith) :

Unmerged commits

c37ec07... by Dan Watkins

[WIP] net: only apply routes if we bring an ephemeral DHCP interface up

TODO:
* Tests

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
2index ea707c0..f308d03 100644
3--- a/cloudinit/net/__init__.py
4+++ b/cloudinit/net/__init__.py
5@@ -756,7 +756,11 @@ class EphemeralIPv4Network(object):
6 ' to %s', self.connectivity_url)
7 return
8
9- self._bringup_device()
10+ device_brought_up = self._bringup_device()
11+ if not device_brought_up:
12+ # If we didn't bring the device up, no need to do anything else
13+ # with it
14+ return
15
16 # rfc3442 requires us to ignore the router config *if* classless static
17 # routes are provided.
18@@ -787,7 +791,13 @@ class EphemeralIPv4Network(object):
19 capture=True)
20
21 def _bringup_device(self):
22- """Perform the ip comands to fully setup the device."""
23+ """
24+ Perform the ip comands to fully setup the device.
25+
26+ :return:
27+ True if the device was brought up, False if it already had an
28+ address.
29+ """
30 cidr = '{0}/{1}'.format(self.ip, self.prefix)
31 LOG.debug(
32 'Attempting setup of ephemeral network on %s with %s brd %s',
33@@ -803,17 +813,19 @@ class EphemeralIPv4Network(object):
34 LOG.debug(
35 'Skip ephemeral network setup, %s already has address %s',
36 self.interface, self.ip)
37- else:
38- # Address creation success, bring up device and queue cleanup
39- util.subp(
40- ['ip', '-family', 'inet', 'link', 'set', 'dev', self.interface,
41- 'up'], capture=True)
42- self.cleanup_cmds.append(
43- ['ip', '-family', 'inet', 'link', 'set', 'dev', self.interface,
44- 'down'])
45- self.cleanup_cmds.append(
46- ['ip', '-family', 'inet', 'addr', 'del', cidr, 'dev',
47- self.interface])
48+ return False
49+
50+ # Address creation success, bring up device and queue cleanup
51+ util.subp(
52+ ['ip', '-family', 'inet', 'link', 'set', 'dev', self.interface,
53+ 'up'], capture=True)
54+ self.cleanup_cmds.append(
55+ ['ip', '-family', 'inet', 'link', 'set', 'dev', self.interface,
56+ 'down'])
57+ self.cleanup_cmds.append(
58+ ['ip', '-family', 'inet', 'addr', 'del', cidr, 'dev',
59+ self.interface])
60+ return True
61
62 def _bringup_static_routes(self):
63 # static_routes = [("169.254.169.254/32", "130.56.248.255"),

Subscribers

People subscribed via source and target branches