Set network interface name with cloud-init set-name #707

MnrGreg · 2019-12-23T02:00:15Z

/kind bug
/assign @akutz

What this PR does / why we need it:
Cloud-Init Network Config Version 2 fails to use Predictable Network Interface Names. Instead it uses the Cloud-Init Device Configuration ID as the interface name. Bug #1855945

As a workaround, this PR adds SetName as an optional parameter to the NetworkDeviceSpec. When SetName is not specified, cloud-init metadata is configured with set-name: "eth*" where * equals the Device ID index. When SetName is specified, cloud-init set-name uses this value.

Summary:

Adds SetName to NetworkDeviceSpec
Adds SetName to vspheremachines and vspheremachinetemplates CRDs
Adds templating for set-name to pkg/util/constants.go
Adds & updates tests in machines_tests.go
Updates troubleshooting.md documentation

Which issue(s) this PR fixes
Fixes #583

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Ability to configure Network Interface names

k8s-ci-robot · 2019-12-23T02:00:23Z

Hi @MnrGreg. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

MnrGreg · 2019-12-23T17:44:13Z

/assign @frapposelli

frapposelli · 2019-12-23T21:44:39Z

/ok-to-test

akutz · 2019-12-23T21:58:17Z

Hi @MnrGreg,

Thank you very much for this PR! Here's my suggestion:

Pull out any references to the v1a3 types
Run make generate before resending the PR
Retarget this PR at release-0.5

Currently e2e is broken on master. If you can get it passing against the release-05 branch (v1a2), then we know this works as expected.

docs/troubleshooting.md

api/v1alpha2/types.go

Signed-off-by: Greg May <[email protected]>

MnrGreg · 2019-12-24T20:26:45Z

/retest

MnrGreg · 2019-12-24T20:39:54Z

/test pull-cluster-api-provider-vsphere-verify-shell

MnrGreg · 2019-12-24T20:42:06Z

@akutz could you review when you have a moment

akutz

Hi @MnrGreg,

This looks great. I had one small nit about the casing of a comment. Finally, I'm sorry for this, but I'm wondering if the property shouldn't be DeviceName? For example:

// DeviceName may be used to explicitly assign a name to the network device
// as it exists in the guest operating system.
// +optional
DeviceName string `json:"deviceName,omitempty"`

Obviously the generated cloud-init metadata would still use set-name (since that is the name of the field from cloud-init's point-of-view), but I'm thinking that SetName makes far less sense in the NetworkDeviceSpec than something like DeviceName. What do you think?

akutz · 2019-12-24T21:27:04Z

api/v1alpha2/types.go

+	// setName can be used to give the device a more specific/desirable/nicer
+	// name than the default from udev’s ifnames
+	// +optional
+	SetName string `json:"setName,omitempty"`


Hi @MnrGreg,

The GoDoc comment should reflect the name/casing of the field, not the JSON property. So the comment should be:

// SetName can...

And if I might recommend?

// SetName may be used to explicitly assign a name to the network device // as it exists in the guest operating system.

akutz · 2019-12-24T21:33:34Z

Hi @MnrGreg,

I'm placing a hold on this until you have a chance to respond to the suggestion re: DeviceName. Even GuestDeviceName is appropriate IMO. I think both provide a nicer UX than SetName inside of the NetworkDeviceSpec.

/hold

Signed-off-by: Greg May <[email protected]>

akutz · 2019-12-24T23:25:39Z

/hold cancel

Looks good to me. Please ping me when it passes the CRD check, and I’ll lgtm it. I’d do so now, but it will just get removed when you rebase.

Also, I will cherry pick this (or you can) over to Master once this merges.

akutz · 2019-12-24T23:28:49Z

/lgtm
/approve

Thank you again! Once this merges I or you can cherry pick this to master.

k8s-ci-robot · 2019-12-24T23:29:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akutz, MnrGreg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [akutz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

The pointers were not working as expected so the API is changing to be more functional and leverage kubernetes' DeepCopy function.

k8s-ci-robot assigned akutz Dec 23, 2019

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 23, 2019

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 23, 2019

k8s-ci-robot requested review from sflxn and sidharthsurana December 23, 2019 02:00

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 23, 2019

k8s-ci-robot assigned frapposelli Dec 23, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 23, 2019

akutz reviewed Dec 23, 2019

View reviewed changes

docs/troubleshooting.md Outdated Show resolved Hide resolved

akutz reviewed Dec 23, 2019

View reviewed changes

docs/troubleshooting.md Outdated Show resolved Hide resolved

akutz reviewed Dec 23, 2019

View reviewed changes

api/v1alpha2/types.go Outdated Show resolved Hide resolved

target release-0.5

72c11dd

Signed-off-by: Greg May <[email protected]>

MnrGreg force-pushed the using_cloudinit_set-name branch from ed55708 to 72c11dd Compare December 24, 2019 20:08

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 24, 2019

MnrGreg changed the base branch from master to release-0.5 December 24, 2019 20:09

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 24, 2019

akutz reviewed Dec 24, 2019

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 24, 2019

MnrGreg added 3 commits December 24, 2019 15:09

rename SetName to DeviceName

f9bb023

Signed-off-by: Greg May <[email protected]>

fix DeviceName descriptions

7f1f45f

Signed-off-by: Greg May <[email protected]>

fix DeviceName again

1ba72c4

Signed-off-by: Greg May <[email protected]>

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 24, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 24, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 24, 2019

k8s-ci-robot merged commit d7f1a21 into kubernetes-sigs:release-0.5 Dec 24, 2019

MnrGreg deleted the using_cloudinit_set-name branch December 30, 2019 22:51

This was referenced Feb 16, 2020

Document stance on static IP addresses kubernetes-sigs/cluster-api#1740

Closed

Fail to assign static ipv4 address to centos #756

Closed

MnrGreg mentioned this pull request Mar 17, 2020

Fail to assign static ipv4 addresses to machines #834

Closed

yastij mentioned this pull request Mar 18, 2020

Set network interface name with cloud-init set-name #838

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set network interface name with cloud-init set-name #707

Set network interface name with cloud-init set-name #707

MnrGreg commented Dec 23, 2019

k8s-ci-robot commented Dec 23, 2019

MnrGreg commented Dec 23, 2019

frapposelli commented Dec 23, 2019

akutz commented Dec 23, 2019 •

edited

Loading

MnrGreg commented Dec 24, 2019

MnrGreg commented Dec 24, 2019

MnrGreg commented Dec 24, 2019

akutz left a comment •

edited

Loading

akutz Dec 24, 2019

akutz commented Dec 24, 2019

akutz commented Dec 24, 2019

akutz commented Dec 24, 2019

k8s-ci-robot commented Dec 24, 2019

Set network interface name with cloud-init set-name #707

Set network interface name with cloud-init set-name #707

Conversation

MnrGreg commented Dec 23, 2019

k8s-ci-robot commented Dec 23, 2019

MnrGreg commented Dec 23, 2019

frapposelli commented Dec 23, 2019

akutz commented Dec 23, 2019 • edited Loading

MnrGreg commented Dec 24, 2019

MnrGreg commented Dec 24, 2019

MnrGreg commented Dec 24, 2019

akutz left a comment • edited Loading

Choose a reason for hiding this comment

akutz Dec 24, 2019

Choose a reason for hiding this comment

akutz commented Dec 24, 2019

akutz commented Dec 24, 2019

akutz commented Dec 24, 2019

k8s-ci-robot commented Dec 24, 2019

akutz commented Dec 23, 2019 •

edited

Loading

akutz left a comment •

edited

Loading