Skip to content

DLPX-69865 IP configuration is lost if a MAC address changes#23

Merged
prakashsurya merged 1 commit into
delphix:masterfrom
prakashsurya:remove-mac-matching
Sep 3, 2020
Merged

DLPX-69865 IP configuration is lost if a MAC address changes#23
prakashsurya merged 1 commit into
delphix:masterfrom
prakashsurya:remove-mac-matching

Conversation

@prakashsurya
Copy link
Copy Markdown
Contributor

The prior commit 271b349 appears to only work on ESX based DE due to the
fact that we don't use any cloud datasource when running on that
platform. This change attempts to extend that same change, but to work
properly on the Azure, Ec2, and Oracle cloud platforms.

The prior commit 271b349 appears to only work on ESX based DE due to the
fact that we don't use any cloud datasource when running on that
platform. This change attempts to extend that same change, but to work
properly on the Azure, Ec2, and Oracle cloud platforms.
@prakashsurya
Copy link
Copy Markdown
Contributor Author

git-ab-pre-push is here

@prakashsurya
Copy link
Copy Markdown
Contributor Author

I tested this on a dlpx-trunk based VM in AWS and verified the network config was correct:

$ cat /etc/netplan/50-cloud-init.yaml
# This file is generated from information provided by the datasource.  Changes
# to it will not persist across an instance reboot.  To disable cloud-init's
# network configuration capabilities, write a file
# /etc/cloud/cloud.cfg.d/99-disable-network-config.cfg with the following:
# network: {config: disabled}
network:
    ethernets:
        ens5:
            dhcp4: true
            dhcp6: false
            match:
                name: ens5
            set-name: ens5
    version: 2

Copy link
Copy Markdown
Contributor

@pzakha pzakha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we also do a similar change for gcp?

@prakashsurya
Copy link
Copy Markdown
Contributor Author

Shouldn't we also do a similar change for gcp?

Can you point to the code that you're concerned about? I didn't see where we match on MAC for GCP, but I might have overlooked something.

@pzakha
Copy link
Copy Markdown
Contributor

pzakha commented Sep 2, 2020

I'm not sure GCP is an issue. I had just noticed that GCP wasn't mentioned in this review, however it does have its own datasource, so I was wondering if it was being handled by the logic in the previous review. Might be worth to verify it if you haven't already.

I'm also wondering if we'd want some tests on the appliance-level to make sure we are using the proper type of matching for the interface.

@prakashsurya
Copy link
Copy Markdown
Contributor Author

Here's what I see on a GCP based VM from DCOA:

delphix@ps-69865-gcp:~$ cat /etc/netplan/50-cloud-init.yaml
# This file is generated from information provided by the datasource.  Changes
# to it will not persist across an instance reboot.  To disable cloud-init's
# network configuration capabilities, write a file
# /etc/cloud/cloud.cfg.d/99-disable-network-config.cfg with the following:
# network: {config: disabled}
network:
    ethernets:
        ens4:
            dhcp4: true
            match:
                name: ens4
            set-name: ens4
    version: 2

so it looks like we're good as-is.

I'm also wondering if we'd want some tests on the appliance-level to make sure we are using the proper type of matching for the interface.

I don't understand.. what do you mean?

@pzakha
Copy link
Copy Markdown
Contributor

pzakha commented Sep 2, 2020

so it looks like we're good as-is.

Great!

I'm also wondering if we'd want some tests on the appliance-level to make sure we are using the proper type of matching for the interface.

I don't understand.. what do you mean?

Like how do we make sure we don't regress back to the default mac-matching behaviour on some platforms. I believe we should be fine in the future for Azure, ec2 and Oracle since we have explicit unit tests in cloud-init, but from my understanding Upstream could modify other DataSources, such as GCP, and have them implement MAC matching without us even noticing.

Also, if we were to add a new DataSource, this is likely something we'd overlook unless we have an explicit test (such as a dxos-test) that would run on that platform and catch it, or perhaps some notes for gotchas to look out for when implementing support for a new DataSource.

We may want to consult with Platform QA to see what are their thoughts on this.

@prakashsurya
Copy link
Copy Markdown
Contributor Author

We may want to consult with Platform QA to see what are their thoughts on this.

Yea, I see what you mean now. IMO, in an ideal situation, we'd have QA verify the cases where this has been an issue for customers, by performing the same VM operations that previously got us into the state where networking was broken. I'm open to discussing this at one of our team meetings.

@prakashsurya
Copy link
Copy Markdown
Contributor Author

I've also verified this change on an Azure based VM:

delphix@ps-69865-azure:~$ cat /etc/netplan/50-cloud-init.yaml
# This file is generated from information provided by the datasource.  Changes
# to it will not persist across an instance reboot.  To disable cloud-init's
# network configuration capabilities, write a file
# /etc/cloud/cloud.cfg.d/99-disable-network-config.cfg with the following:
# network: {config: disabled}
network:
    ethernets:
        eth0:
            dhcp4: true
            dhcp4-overrides:
                route-metric: 100
            dhcp6: false
            match:
                name: eth0
            set-name: eth0
    version: 2

@grwilson
Copy link
Copy Markdown

grwilson commented Sep 3, 2020

There are some test which add NICs to these environments and we should check to make sure that we don't see a regression in those cases. The code LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants