Merge ~daniel-thewatkins/cloud-init/+git/cloud-init:oci-vnic into cloud-init:master

Proposed by Dan Watkins on 2019-08-16
Status: Merged
Approved by: Dan Watkins on 2019-08-19
Approved revision: 811c589c402b4832a290043c28767f48f342bcae
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~daniel-thewatkins/cloud-init/+git/cloud-init:oci-vnic
Merge into: cloud-init:master
Diff against target: 46 lines (+16/-1)
2 files modified
cloudinit/sources/DataSourceOracle.py (+7/-0)
cloudinit/sources/tests/test_oracle.py (+9/-1)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve on 2019-08-19
Scott Moser Approve on 2019-08-19
Ryan Harper 2019-08-16 Approve on 2019-08-16
Review via email: mp+371403@code.launchpad.net

Commit message

DataSourceOracle: prefer DS network config over initramfs

The Oracle platform provides networking configuration from two sources:

* the primary interface configuration comes from the initramfs, because
  Oracle instance all iSCSI boot
* secondary interface configuration comes from an IMDS accessed over
  HTTP

As we need to combine these two sources of network configuration, the
default "prefer initramfs config over data source config" behaviour
isn't appropriate; we would never get the IMDS interfaces via that
route. Instead, the Oracle data source has code to combine these two
sources, so we prefer its network configuration over the initramfs
configuration.

(This is not appropriate default behaviour, because _in general_ data
sources won't know how to merge initramfs-provided configuration into
their provided configuration, so switching this order for all data
sources would result in initramfs configuration being discarded on any
data source that implements network_config.)

To post a comment you must log in.
Ryan Harper (raharper) wrote :

This was nice and easy.

review: Approve
Scott Moser (smoser) wrote :

Well...
Not really so nice and easy.

It doesn't seem like a good idea for the network configuration preference to change on a per-datasource basis. It means that if someone has something working on a system in OpenStack via writing initramfs network config, then the system will behave differently on Oracle.

This behavior should be consistent for cloud-init. "Well, if you're on one Cloud then cloud-init works this way, but on another it works another way" is hard to document and not entirely useful for a user.

It feels to me like your change should be the default behavior, but thought is needed there.

review: Needs Information
Scott Moser (smoser) wrote :

I marked Needs Review just to keep this from being auto-landed. I just think it needs thought and justification.

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

review: Approve (continuous-integration)
Ryan Harper (raharper) wrote :

cloudinit/stages.py decision on where to get networking from only selects one source. We previously[1] split up kernel command line from initramfs, as the former retains users ability to specify a network-config completely via the cmdline, where the latter is auto generated from the initramfs.

For OCI, we'd like the datasource provided network config to be preferred over the initramfs _because_ in the OCI datasource we will incorporate the configuration that was extracted from the initramfs (iscsi root network config) and if present, additional secondary nic configuration[2].

If stages selected initramfs config over the datasource config, then we'd never be able to include secondary nic information.

1. https://git.launchpad.net/cloud-init/commit/?id=1dbede64dc645b090b4047a105143b5d5090d214
2. https://git.launchpad.net/cloud-init/commit/?id=0e79a1b89287358a77fe31fb82c4bcd83ff48894

Autolanding: FAILED
Approved revid is not set in launchpad. This is most likely a launchpad issue and re-approve should fix it. There is also a chance (although a very small one) this is a permission problem of the ps-jenkins bot
https://jenkins.ubuntu.com/server/job/cloud-init-autoland-test/286/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

review: Needs Fixing (continuous-integration)
Dan Watkins (daniel-thewatkins) wrote :

I'm working on updating the commit message here to include more of the background.

Scott Moser (smoser) wrote :

@Ryan,
You made an argument for why datasource configuration should override initramfs configuration, but not why network configuration sources should differ on a per-datasource basis. That was what I had issue with.

Why should this change not be made to cloudinit/sources/__init__.py:DataSource.network_config_sources

Dan Watkins (daniel-thewatkins) wrote :

If network_config_sources had its order changed to match this, any platform with a data source that provides network_config (other than Oracle) would stop honouring initramfs configuration entirely. It's only safe to change the order for the Oracle data source because we _know_ that the Oracle data source includes initramfs configuration in its own network_config.

Dan Watkins (daniel-thewatkins) wrote :

(Commit message updated.)

Scott Moser (smoser) wrote :

I think i'd prefer consistency in behavior.

I agree that your change is specifically safer than changing the default behavior. Looking at the datasources, though, it seems like the only one that might get '/run/network/*' would be MAAS, and then only in the ephemeral boot case. And MAAS does not provide network configuration (curtin puts it into system config IIRC).

It seems unlikely to me that we'd find ourselves in this situation where initramfs configuration was desired to trump datasource configuration. This is because:
 a.) initramfs configuration is not documented supported by cloud-init
 b.) its a very poorly documented interface only known to be written by klibc's ipconfig (and then adopted for legacy by mkinitramfs-tools on ubuntu).

In the event that someone *did* want initramfs to provide network configuration via this arbitrary format, then I think I'd suggest they move to kernel command line providing it, or some other solution.

I'd just rather be consistent, and basically do away with the initramfs configuration.

Dan Watkins (daniel-thewatkins) wrote :

> Looking at the datasources

Looking at the data sources can't tell you who is using initramfs network configuration, because initramfs network configuration trumps data source configuration today. Systems that come up using initramfs network configuration (i.e. iSCSI root) will _never_ use data source configuration.

> It seems unlikely to me that we'd find ourselves in this situation where initramfs configuration was desired to trump datasource configuration.

I _believe_ that anyone who iSCSI boots today desires initramfs configuration to trump datasource configuration, so that cloud-init doesn't reconfigure their iSCSI NIC(s) in a way that might cause them to lose their root disk.

Scott Moser (smoser) wrote :

Give an example? They boot iSCSI on a cloud that provides network
configuration? But that network configuration does not contain
information about the device configured via iscsi?

On Fri, Aug 16, 2019 at 12:58 PM Dan Watkins
<email address hidden> wrote:
>
> > Looking at the datasources
>
> Looking at the data sources can't tell you who is using initramfs network configuration, because initramfs network configuration trumps data source configuration today. Systems that come up using initramfs network configuration (i.e. iSCSI root) will _never_ use data source configuration.
>
> > It seems unlikely to me that we'd find ourselves in this situation where initramfs configuration was desired to trump datasource configuration.
>
> I _believe_ that anyone who iSCSI boots today desires initramfs configuration to trump datasource configuration, so that cloud-init doesn't reconfigure their iSCSI NIC(s) in a way that might cause them to lose their root disk.
> --
> https://code.launchpad.net/~daniel-thewatkins/cloud-init/+git/cloud-init/+merge/371403
> You are reviewing the proposed merge of ~daniel-thewatkins/cloud-init/+git/cloud-init:oci-vnic into cloud-init:master.

Dan Watkins (daniel-thewatkins) wrote :

On Fri, Aug 16, 2019 at 05:05:15PM -0000, Scott Moser wrote:
> Give an example? They boot iSCSI on a cloud that provides network
> configuration? But that network configuration does not contain
> information about the device configured via iscsi?

Well, Oracle. :)

I don't have another concrete use case. But, for example, AFAICT
OpenStack's network_data.json doesn't have a way to indicate that an
interface shouldn't be automatically brought up and down with all the
other interfaces (i.e. control=manual), and cloud-init's
network_data.json handling certainly doesn't handle that case. So even
if the OpenStack network_data.json is completely accurate regarding
every other detail of configuration, users who currently can safely
restart networking.service in their iSCSI root instances would lose
their instance instead if the OpenStack DS network configuration was
preferred.

>
> On Fri, Aug 16, 2019 at 12:58 PM Dan Watkins
> <email address hidden> wrote:
> >
> > > Looking at the datasources
> >
> > Looking at the data sources can't tell you who is using initramfs network configuration, because initramfs network configuration trumps data source configuration today. Systems that come up using initramfs network configuration (i.e. iSCSI root) will _never_ use data source configuration.
> >
> > > It seems unlikely to me that we'd find ourselves in this situation where initramfs configuration was desired to trump datasource configuration.
> >
> > I _believe_ that anyone who iSCSI boots today desires initramfs configuration to trump datasource configuration, so that cloud-init doesn't reconfigure their iSCSI NIC(s) in a way that might cause them to lose their root disk.
> > --
> > https://code.launchpad.net/~daniel-thewatkins/cloud-init/+git/cloud-init/+merge/371403
> > You are reviewing the proposed merge of ~daniel-thewatkins/cloud-init/+git/cloud-init:oci-vnic into cloud-init:master.
>
> --
> https://code.launchpad.net/~daniel-thewatkins/cloud-init/+git/cloud-init/+merge/371403
> You are the owner of ~daniel-thewatkins/cloud-init/+git/cloud-init:oci-vnic.

Scott Moser (smoser) wrote :

Changing my comment.
We had a voice chat, and I think that the end goal we came up with (i hope someone can document that *somewhere*) seems sane and this is sane as a un-block merge.

review: Approve
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/sources/DataSourceOracle.py b/cloudinit/sources/DataSourceOracle.py
2index 086af79..6e73f56 100644
3--- a/cloudinit/sources/DataSourceOracle.py
4+++ b/cloudinit/sources/DataSourceOracle.py
5@@ -109,6 +109,13 @@ class DataSourceOracle(sources.DataSource):
6 dsname = 'Oracle'
7 system_uuid = None
8 vendordata_pure = None
9+ network_config_sources = (
10+ sources.NetworkConfigSource.cmdline,
11+ sources.NetworkConfigSource.ds,
12+ sources.NetworkConfigSource.initramfs,
13+ sources.NetworkConfigSource.system_cfg,
14+ )
15+
16 _network_config = sources.UNSET
17
18 def __init__(self, sys_cfg, *args, **kwargs):
19diff --git a/cloudinit/sources/tests/test_oracle.py b/cloudinit/sources/tests/test_oracle.py
20index 3e14677..3ddf7df 100644
21--- a/cloudinit/sources/tests/test_oracle.py
22+++ b/cloudinit/sources/tests/test_oracle.py
23@@ -1,7 +1,7 @@
24 # This file is part of cloud-init. See LICENSE file for license information.
25
26 from cloudinit.sources import DataSourceOracle as oracle
27-from cloudinit.sources import BrokenMetadata
28+from cloudinit.sources import BrokenMetadata, NetworkConfigSource
29 from cloudinit import helpers
30
31 from cloudinit.tests import helpers as test_helpers
32@@ -303,6 +303,14 @@ class TestDataSourceOracle(test_helpers.CiTestCase):
33 self.assertIn('Failed to fetch secondary network configuration',
34 self.logs.getvalue())
35
36+ def test_ds_network_cfg_preferred_over_initramfs(self):
37+ """Ensure that DS net config is preferred over initramfs config"""
38+ network_config_sources = oracle.DataSourceOracle.network_config_sources
39+ self.assertLess(
40+ network_config_sources.index(NetworkConfigSource.ds),
41+ network_config_sources.index(NetworkConfigSource.initramfs)
42+ )
43+
44
45 @mock.patch(DS_PATH + "._read_system_uuid", return_value=str(uuid.uuid4()))
46 class TestReadMetaData(test_helpers.HttprettyTestCase):

Subscribers

People subscribed via source and target branches