cloud-init

Code review comment for ~raharper/cloud-init:azure_run_local

Revision history for this message

Ryan Harper (raharper) wrote on 2017-06-21:

> There are some comments in line. Others here.
>
> 1. I'd have thought that there would need to be a fix for the 'duplicate mac
> address detected' as we see
> https://bugs.launchpad.net/ubuntu/+source/cloud-init/+bug/1692028

That;s still needed, but currently the rename of the interfaces isn't the first priority (not breaking existing instances which don't have duplicate macs is step one).

>
> I'm not sure how that is being avoided. It seems even if we don't call
> 'get_interfaces_by_mac()' in the proposed path for azure, we are likely to end
> up calling that function at some point in some generic code path and on azure
> where there might be duplicate macs, it will fail.
> A solution would be to change callers i guess to accept that there *might* be
> identical macs.

Yes, still looking for unique attributes.

>
> B. It seems like there is duplicate code in DataSourceAzureNet and
> DataSourceAzure. I'd like to have just one class, i think we should be able to
> do that. I suspect that might be just work-in-progress.

Yes, the other class can be dropped since I now run Net class in both modes.

>
> C. It seems to me that you've added support for Azure being able to provide
> config that includes a device driver (in 'network config v1 format'). But
> generically, I do not believe that is present in v1 or v2/netplan format. If
> that *is* true, then there isn't actually a way that those formats can specify
> this configuration, and we should at least open issues to that affect.

v2 supports driver under the Match key, v1 does not; we should likely update v1 to support it without the param trickery.

We'll likely also want (long term)

1) control in the datasource over whether we generate udev rules or not (this is available but not exposed, i.e. if you set the link_path in the renderer class to None, they don't get generated)

2) v1/v2 config might also need to express additional attributes to include in the udev rule (like driver) but also a rule_name field, in our case, we may refer to a device via a tuple( vf1=(vf1,mac,driver) but want a rule that sets NAME=vf%k so it can be parameterized.

>
> D. network_config will still only be applied once per instance. So If a user
> shuts down an instance and attaches a device and starts it back up, I do not
> think we will generate a different/updated config. Is that right? Its likely
> i'm not understanding something.

That's something else we'll need to address in the next stage, we're trying to make sure instances booted with duplicate macs don't comeup without any networking at all.

>
>
> E. When testing this please make sure you test:
> i.) test upgrade and reboot on same instance. This will test the path where
> there exists a /var/lib/cloud/instance/obj.pkl that references
> DataSourceAzureNet.

Ack.

> ii.) test upgrade and reboot with clean of /var/log/cloud* /var/lib/cloud-
> init* and cloud_config entries in /etc/fstab. (mocking a clean instance)

Ack, I've already tested this, but will look back to (i) now that (ii) is known working.

> There are some comments in line.  Others here.
> 
> 1. I'd have thought that there would need to be a fix for the 'duplicate mac
> address detected' as we see
>  https://bugs.launchpad.net/ubuntu/+source/cloud-init/+bug/1692028

That;s still needed, but currently the rename of the interfaces isn't the first priority (not breaking existing instances which don't have duplicate macs is step one).

> 
> I'm not sure how that is being avoided. It seems even if we don't call
> 'get_interfaces_by_mac()' in the proposed path for azure, we are likely to end
> up calling that function at some point in some generic code path and on azure
> where there might be duplicate macs, it will fail.
> A solution would be to change callers i guess to accept that there *might* be
> identical macs.

Yes, still looking for unique attributes.

> 
> B. It seems like there is duplicate code in DataSourceAzureNet and
> DataSourceAzure. I'd like to have just one class, i think we should be able to
> do that.  I suspect that might be just work-in-progress.

Yes, the other class can be dropped since I now run Net class in both modes.

> 
> C. It seems to me that you've added support for Azure being able to provide
> config that includes a device driver (in 'network config v1 format').  But
> generically, I do not believe that is present in v1 or v2/netplan format.  If
> that *is* true, then there isn't actually a way that those formats can specify
> this configuration, and we should at least open issues to that affect.

v2 supports driver under the Match key, v1 does not; we should likely update v1 to support it without the param trickery.

We'll likely also want (long term)

1) control in the datasource over whether we generate udev rules or not (this is available but not exposed, i.e. if you set the link_path in the renderer class to None, they don't get generated)

> 
> D. network_config will still only be applied once per instance.  So If a user
> shuts down an instance and attaches a device and starts it back up, I do not
> think we will generate a different/updated config.  Is that right?  Its likely
> i'm not understanding something.

That's something else we'll need to address in the next stage, we're trying to make sure instances booted with duplicate macs don't comeup without any networking at all.

> 
> 
> E. When testing this please make sure you test:
>   i.) test upgrade and reboot on same instance. This will test the path where
> there exists a /var/lib/cloud/instance/obj.pkl that references
> DataSourceAzureNet.

Ack.

>   ii.) test upgrade and reboot with clean of /var/log/cloud* /var/lib/cloud-
> init* and cloud_config entries in /etc/fstab. (mocking a clean instance)

Ack, I've already tested this, but will look back to (i) now that (ii) is known working.

« Back to merge proposal