🌱 Add option for non-mandatory IP/DHCP for network device #2727

p-strusiewiczsurmacki-mobica · 2024-02-08T13:38:56Z

What this PR does / why we need it:
This PR introduces ipConfigurationNotMandatory for for network device configuration (deviceSpec). If set true controller should not wait for IP address allocation for this network device.

Which issue(s) this PR fixes:
Fixes #2721

k8s-ci-robot · 2024-02-08T13:39:05Z

Welcome @p-strusiewiczsurmacki-mobica!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-vsphere 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-vsphere has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-02-08T13:39:06Z

Hi @p-strusiewiczsurmacki-mobica. 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.

schrej · 2024-02-08T13:49:09Z

apis/v1beta1/types.go

+
+	// IPConfigurationNotMandatory allows for the device to not have IP address or DHCP configured.
+	// +optional
+	IPConfigurationNotMandatory bool `json:"ipConfigurationNotMandatory,omitempty"`


maybe we could use something a little more explicit here, e.g. skipWaitForAddress?

I think we should go into more detail at the godoc.

If this new variable gets set to true, this device gets ignored when checking for an existing ip address. And propably that at least one device should get an IP address (dunno if this is true but I can think of provisioning to fail at different places if there is no IP here)?

SkipWaitForAddress sounds better to me but not yet perfect. No strong opinion on the name or a better recommendation from my side though...

What about: SkipIPAllocation or IgnoreIPAllocation

SkipIPAllocation or IgnoreIPAllocation sounds more straightforward to me. And I'm thinking perhaps adding static is more precise, like SkipStaticIPAllocation or IgnoreStaticIPAllocation as we're only skipping the static IP allocation but not the DHCP allocation. WDYT?

Changed the name to SkipIPAllocation.

we're only skipping the static IP allocation but not the DHCP allocation

I think this is something I've got wrong. I've added SkipIPAllocation check in few more places now.
I think that if user sets SkipIPAllocation to true, no check should be performed at all.

MaxRink · 2024-02-12T11:02:52Z

/ok-to-test

codecov · 2024-02-12T11:08:21Z

Codecov Report

Attention: Patch coverage is 76.47059% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 64.05%. Comparing base (2768cc0) to head (b020650).
Report is 41 commits behind head on main.

Files	Patch %	Lines
pkg/services/govmomi/util.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2727      +/-   ##
==========================================
+ Coverage   63.88%   64.05%   +0.17%     
==========================================
  Files         160      160              
  Lines        9345     9360      +15     
==========================================
+ Hits         5970     5996      +26     
+ Misses       2914     2908       -6     
+ Partials      461      456       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chrischdi · 2024-02-16T13:07:18Z

controllers/vspherevm_controller.go

@@ -473,7 +473,7 @@ func (r vmReconciler) reconcileNormal(ctx context.Context, vmCtx *capvcontext.VM
 func (r vmReconciler) isWaitingForStaticIPAllocation(vmCtx *capvcontext.VMContext) bool {
 	devices := vmCtx.VSphereVM.Spec.Network.Devices
 	for _, dev := range devices {
-		if !dev.DHCP4 && !dev.DHCP6 && len(dev.IPAddrs) == 0 && len(dev.AddressesFromPools) == 0 {
+		if !dev.DHCP4 && !dev.DHCP6 && len(dev.IPAddrs) == 0 && len(dev.AddressesFromPools) == 0 && !dev.IPConfigurationNotMandatory {


Can we have a comment here explaining this long if? Its hard to read and remember again.

We should also adjust the godoc of the function.

Maybe there's a way to write this function in a way so its more readable: (please check if the logic is correct)

// TODO explain if dev.IgnoreIPAllocation { continue } // TODO explain if len(dev.IPAddrs) > 0 || len(dev.AddressesFromPools) > 0 { continue } // TODO explain if dev.DHCP4 || dev.DHCP6 { continue } // TODO explain return true

I've split this if statement into 3 separate statements as by tour suggestion.

chrischdi · 2024-02-16T13:29:52Z

apis/v1beta1/types.go

+
+	// IPConfigurationNotMandatory allows for the device to not have IP address or DHCP configured.
+	// +optional
+	IPConfigurationNotMandatory bool `json:"ipConfigurationNotMandatory,omitempty"`


I think we should go into more detail at the godoc.

If this new variable gets set to true, this device gets ignored when checking for an existing ip address. And propably that at least one device should get an IP address (dunno if this is true but I can think of provisioning to fail at different places if there is no IP here)?

SkipWaitForAddress sounds better to me but not yet perfect. No strong opinion on the name or a better recommendation from my side though...

What about: SkipIPAllocation or IgnoreIPAllocation

p-strusiewiczsurmacki-mobica · 2024-02-19T15:35:36Z

@chrischdi I've tried to add some more details into this godoc comment. Hope it will be OK now.
I can add a check to ensure that at least 1 device has IP configured/DHCP enabled, but I am not sure if that's necessary. If user has to explicitly set this flag to true I would assume they have at least a minimal knowledge of what and why they are doing anyway.
But maybe I am too optimistic about this, so I'll just let you decide on that. :)

chrischdi · 2024-02-19T15:39:32Z

Looks like some generated code needs to get regenerated.

Thanks for tackling this. I'll try to come back to this tomorrow.

p-strusiewiczsurmacki-mobica · 2024-02-19T15:55:00Z

Looks like some generated code needs to get regenerated.

That's weird, because I've run make verify-gen several times already and there are no changes I could commit.

EDIT: I've force-pushed those once again and it seems to be ok. Maybe CI did not get the last commit.

Signed-off-by: Patryk Strusiewicz-Surmacki <patryk-pawel.strusiewicz-surmacki@external.telekom.de>

apis/v1beta1/types.go

controllers/vspherevm_controller.go

chrischdi · 2024-02-21T14:34:37Z

pkg/services/govmomi/util.go

@@ -457,6 +457,11 @@ func waitForIPAddresses(
 				// Look at each IP and determine whether a reconcile has
 				// been triggered for the IP.
 				for _, discoveredIPInfo := range nic.IpConfig.IpAddress {
+					// Ignore device if SkipIPAllocation is set.
+					if deviceSpec.SkipIPAllocation {


Not sure if we want to run continue in this first case. Wouldn't this also skip discovering if IP Addresses get set on this interface?

If so, we should add it to the description that IPs for this device don't get bubbled up.

I think you're right. Even if there is IP address discovered but the device has no static IP or DHCP configured, the discovered address will be skipped anyway.
Deleted this if statement.

chrischdi · 2024-02-21T14:37:33Z

pkg/util/machines.go

@@ -109,10 +109,21 @@ func GetMachineMetadata(hostname string, vsphereVM infrav1.VSphereVM, ipamState
 			devices[i].Gateway6 = state.Gateway6
 		}

+		// Ignore device if SkipIPAllocation is set


Can we add why we want to skip this device here? What impact has that?

After some consideration I think it's not really necessary. If the device has no static IP address or DHCP configured, then waitForIPv4 and waitForIPv6 should be false anyway.

chrischdi · 2024-02-21T14:37:50Z

pkg/util/machines.go

 		if waitForIPv4 && waitForIPv6 {
 			// break early as we already wait for ipv4 and ipv6
 			continue
 		}
+
+		if vsphereVM.Spec.Network.Devices[i].SkipIPAllocation {


duplicate to above?

Deleted that.

Co-authored-by: Christian Schlotter <chrischdi@users.noreply.github.com>

Signed-off-by: Patryk Strusiewicz-Surmacki <patryk-pawel.strusiewicz-surmacki@external.telekom.de>

chrischdi · 2024-02-22T12:39:33Z

apis/v1beta1/types.go

+	// If true, both static IP address allocation and DHCP allocation will be skipped,
+	// and provider will not verify IP address allocation.


Sorry for nitpicking here, but this may not be entirely true?

CAPV will only not verify IP address allocation. The vm itself may still doe static IP configuration or DHCP allocation (depending on the other configuration)

Changed that to

// If true, CAPV will not verify IP address allocation.

Signed-off-by: Patryk Strusiewicz-Surmacki <patryk-pawel.strusiewicz-surmacki@external.telekom.de>

chrischdi · 2024-02-27T13:20:28Z

/test help

k8s-ci-robot · 2024-02-27T13:20:30Z

@chrischdi: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-vsphere-e2e-blocking-main
/test pull-cluster-api-provider-vsphere-e2e-conformance-ci-latest-main
/test pull-cluster-api-provider-vsphere-e2e-conformance-main
/test pull-cluster-api-provider-vsphere-e2e-main
/test pull-cluster-api-provider-vsphere-e2e-upgrade-1-29-1-30-main
/test pull-cluster-api-provider-vsphere-test-integration-main
/test pull-cluster-api-provider-vsphere-test-main
/test pull-cluster-api-provider-vsphere-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-vsphere-apidiff-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-vsphere-apidiff-main
pull-cluster-api-provider-vsphere-e2e-blocking-main
pull-cluster-api-provider-vsphere-test-integration-main
pull-cluster-api-provider-vsphere-test-main
pull-cluster-api-provider-vsphere-verify-main

In response to this:

/test help

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.

chrischdi · 2024-02-27T13:20:50Z

/test pull-cluster-api-provider-vsphere-e2e-conformance-ci-latest-main

chrischdi · 2024-02-27T13:20:52Z

/test pull-cluster-api-provider-vsphere-e2e-conformance-main

chrischdi · 2024-02-27T13:20:55Z

/test pull-cluster-api-provider-vsphere-e2e-main

chrischdi · 2024-02-27T13:20:56Z

/test pull-cluster-api-provider-vsphere-e2e-upgrade-1-29-1-30-main

chrischdi

/lgtm

@schrej could you maybe give this another look in its final state :-)

k8s-ci-robot · 2024-02-27T17:28:42Z

LGTM label has been added.

Git tree hash: 1fcc6b6fed9e860a93895d30b9198608f06e1e40

schrej · 2024-03-04T14:21:19Z

lgtm!

Thanks @p-strusiewiczsurmacki-mobica 🎉

chrischdi · 2024-03-04T14:48:31Z

/approve

k8s-ci-robot · 2024-03-04T14:48:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi

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~~ [chrischdi]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 8, 2024

k8s-ci-robot requested review from srm09 and vrabbi February 8, 2024 13:39

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 8, 2024

p-strusiewiczsurmacki-mobica force-pushed the optional-interface-config branch from ac11b8c to d5fbef4 Compare February 8, 2024 13:42

p-strusiewiczsurmacki-mobica changed the title ~~:seedling Add option for non-mandatory IP/DHCP for network device~~ 🌱 Add option for non-mandatory IP/DHCP for network device Feb 8, 2024

p-strusiewiczsurmacki-mobica mentioned this pull request Feb 8, 2024

feat: make ip addresses on secondary interfaces optional #2721

Closed

schrej reviewed Feb 8, 2024

View reviewed changes

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 Feb 12, 2024

chrischdi reviewed Feb 16, 2024

View reviewed changes

k8s-ci-robot added 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 Feb 19, 2024

p-strusiewiczsurmacki-mobica force-pushed the optional-interface-config branch from f384759 to 2f8fb02 Compare February 19, 2024 15:56

p-strusiewiczsurmacki-mobica added 3 commits February 19, 2024 19:35

Added non-mandatory network device config switch

60810ef

Signed-off-by: Patryk Strusiewicz-Surmacki <patryk-pawel.strusiewicz-surmacki@external.telekom.de>

Changed variable name, added allocation skipping in govmomi and utils

e415622

Signed-off-by: Patryk Strusiewicz-Surmacki <patryk-pawel.strusiewicz-surmacki@external.telekom.de>

Regenerated v1alpha3 and v1alpha4 apis

1b9a82f

Signed-off-by: Patryk Strusiewicz-Surmacki <patryk-pawel.strusiewicz-surmacki@external.telekom.de>

p-strusiewiczsurmacki-mobica force-pushed the optional-interface-config branch from 2f8fb02 to 1b9a82f Compare February 19, 2024 18:37

Added manual conversion of SkipIPAllocation for v1alpha3 and v1alpha4

146ab3b

Signed-off-by: Patryk Strusiewicz-Surmacki <patryk-pawel.strusiewicz-surmacki@external.telekom.de>

chrischdi reviewed Feb 21, 2024

View reviewed changes

p-strusiewiczsurmacki-mobica and others added 2 commits February 21, 2024 16:14

Apply suggestions from code review

5730d0a

Co-authored-by: Christian Schlotter <chrischdi@users.noreply.github.com>

Fixed issues pointed out in CR

2f50975

Signed-off-by: Patryk Strusiewicz-Surmacki <patryk-pawel.strusiewicz-surmacki@external.telekom.de>

p-strusiewiczsurmacki-mobica force-pushed the optional-interface-config branch from 4582c46 to 2f50975 Compare February 21, 2024 16:39

chrischdi reviewed Feb 22, 2024

View reviewed changes

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. labels Feb 27, 2024

Updated godoc

b020650

Signed-off-by: Patryk Strusiewicz-Surmacki <patryk-pawel.strusiewicz-surmacki@external.telekom.de>

p-strusiewiczsurmacki-mobica force-pushed the optional-interface-config branch from 52a634f to b020650 Compare February 27, 2024 13:15

chrischdi reviewed Feb 27, 2024

View reviewed changes

k8s-ci-robot assigned chrischdi Feb 27, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2024

k8s-ci-robot merged commit 185d036 into kubernetes-sigs:main Mar 4, 2024
22 checks passed

k8s-ci-robot added this to the v1.10 milestone Mar 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Add option for non-mandatory IP/DHCP for network device #2727

🌱 Add option for non-mandatory IP/DHCP for network device #2727

p-strusiewiczsurmacki-mobica commented Feb 8, 2024

k8s-ci-robot commented Feb 8, 2024

k8s-ci-robot commented Feb 8, 2024

schrej Feb 8, 2024

chrischdi Feb 16, 2024

zhanggbj Feb 19, 2024 •

edited

Loading

p-strusiewiczsurmacki-mobica Feb 19, 2024

MaxRink commented Feb 12, 2024

codecov bot commented Feb 12, 2024 •

edited

Loading

chrischdi Feb 16, 2024

p-strusiewiczsurmacki-mobica Feb 19, 2024

chrischdi Feb 16, 2024

p-strusiewiczsurmacki-mobica commented Feb 19, 2024 •

edited

Loading

chrischdi commented Feb 19, 2024

p-strusiewiczsurmacki-mobica commented Feb 19, 2024 •

edited

Loading

chrischdi Feb 21, 2024

p-strusiewiczsurmacki-mobica Feb 21, 2024

chrischdi Feb 21, 2024

p-strusiewiczsurmacki-mobica Feb 21, 2024

chrischdi Feb 21, 2024

p-strusiewiczsurmacki-mobica Feb 21, 2024

chrischdi Feb 22, 2024

p-strusiewiczsurmacki-mobica Feb 27, 2024

chrischdi commented Feb 27, 2024

k8s-ci-robot commented Feb 27, 2024

chrischdi commented Feb 27, 2024

chrischdi commented Feb 27, 2024

chrischdi commented Feb 27, 2024

chrischdi commented Feb 27, 2024

chrischdi left a comment

k8s-ci-robot commented Feb 27, 2024

schrej commented Mar 4, 2024

chrischdi commented Mar 4, 2024

k8s-ci-robot commented Mar 4, 2024

		// If true, both static IP address allocation and DHCP allocation will be skipped,
		// and provider will not verify IP address allocation.

🌱 Add option for non-mandatory IP/DHCP for network device #2727

🌱 Add option for non-mandatory IP/DHCP for network device #2727

Conversation

p-strusiewiczsurmacki-mobica commented Feb 8, 2024

k8s-ci-robot commented Feb 8, 2024

k8s-ci-robot commented Feb 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhanggbj Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxRink commented Feb 12, 2024

codecov bot commented Feb 12, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-strusiewiczsurmacki-mobica commented Feb 19, 2024 • edited Loading

chrischdi commented Feb 19, 2024

p-strusiewiczsurmacki-mobica commented Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrischdi commented Feb 27, 2024

k8s-ci-robot commented Feb 27, 2024

chrischdi commented Feb 27, 2024

chrischdi commented Feb 27, 2024

chrischdi commented Feb 27, 2024

chrischdi commented Feb 27, 2024

chrischdi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 27, 2024

schrej commented Mar 4, 2024

chrischdi commented Mar 4, 2024

k8s-ci-robot commented Mar 4, 2024

zhanggbj Feb 19, 2024 •

edited

Loading

codecov bot commented Feb 12, 2024 •

edited

Loading

p-strusiewiczsurmacki-mobica commented Feb 19, 2024 •

edited

Loading

p-strusiewiczsurmacki-mobica commented Feb 19, 2024 •

edited

Loading