✨ Add NetworkConfigurations to VSphereFailureDomain #3049

Nutrymaco · 2024-06-10T22:23:20Z

What this PR does / why we need it:
This PR adds additional API to Failure Domain. This allows to have different configurations based on the selected region/zone.

(Edited by chrischdi)
User stories:

As a user I would like to define network configuration for machines, which differs per failure domain.
As a user I would like to define a different value for VSphereMachines .spec.template field per failure domain.

(end of Edit by chrischdi)

Which issue(s) this PR fixes
Partially it repeats work that was done for issue #1964 in #1967. But also allows overriding VM template.

linux-foundation-easycla · 2024-06-10T22:23:25Z

The committers listed above are authorized under a signed CLA.

✅ login: Nutrymaco / name: Efim Smykov (970ef58, 77a32a0, 4b8978c, cc001db)

k8s-ci-robot · 2024-06-10T22:23:29Z

Welcome @Nutrymaco!

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-06-10T22:23:30Z

Hi @Nutrymaco. 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-sigs/prow repository.

chrischdi · 2024-06-13T11:59:16Z

/ok-to-test

Do I get it right that the use cases are:

As a user I would like to define network configuration for machines, which differs per failure domain.
As a user I would like to define a different value for VSphereMachines .spec.template field per failure domain.

pkg/services/vimmachine.go

apis/v1beta1/vspherefailuredomain_types.go

Nutrymaco · 2024-06-13T12:49:22Z

/ok-to-test

Do I get it right that the use cases are:

As a user I would like to define network configuration for machines, which differs per failure domain.

As a user I would like to define a different value for VSphereMachines .spec.template field per failure domain.

Yes

chrischdi · 2024-06-17T08:18:48Z

Question: does it make sense to have the full network definition, or is it enought only have the one originally introduced (and not used) here:

cluster-api-provider-vsphere/apis/v1beta1/vspheredeploymentzone_types.go

Lines 63 to 74 in c77f5fb

    
           type Network struct { 
        
           	// Name is the network name for this machine's VM. 
        
           	Name string `json:"name,omitempty"` 
        
           	// DHCP4 is a flag that indicates whether or not to use DHCP for IPv4 
        
           	// +optional 
        
           	DHCP4 *bool `json:"dhcp4,omitempty"` 
        
           	// DHCP6 indicates whether or not to use DHCP for IPv6 
        
           	// +optional 
        
           	DHCP6 *bool `json:"dhcp6,omitempty"` 
        
           }

chrischdi

Sorry for questioning this now and not earlier. But because this is changing APIs we should be careful to only add/change when we think its a long-term commitment because we have to maintain and support it then for a long time.

Question: what is the reasoning behind wanting to overwrite the template of a machine? Why can't it stay the same?

A substantial part of a MachineDeployment is being able to have the same machine multiple times. Could you please explain the actual use-case where this comes from?

Maybe it helps to split up this discussion from the networking part and maybe also handle them as separate Issues/Pull Requests.

apis/v1beta1/vspherefailuredomain_types.go

Nutrymaco · 2024-06-17T08:50:22Z

Sorry for questioning this now and not earlier. But because this is changing APIs we should be careful to only add/change when we think its a long-term commitment because we have to maintain and support it then for a long time.

Question: what is the reasoning behind wanting to overwrite the template of a machine? Why can't it stay the same?

A substantial part of a MachineDeployment is being able to have the same machine multiple times. Could you please explain the actual use-case where this comes from?

Maybe it helps to split up this discussion from the networking part and maybe also handle them as separate Issues/Pull Requests.

Yes, let's discuss it in different issue. I'll remove template related changes.

Nutrymaco · 2024-06-19T06:26:43Z

Question: does it make sense to have the full network definition, or is it enought only have the one originally introduced (and not used) here:

cluster-api-provider-vsphere/apis/v1beta1/vspheredeploymentzone_types.go

Lines 63 to 74 in c77f5fb

type Network struct {

// Name is the network name for this machine's VM.

Name string `json:"name,omitempty"`

// DHCP4 is a flag that indicates whether or not to use DHCP for IPv4

// +optional

DHCP4 *bool `json:"dhcp4,omitempty"`

// DHCP6 indicates whether or not to use DHCP for IPv6

// +optional

DHCP6 *bool `json:"dhcp6,omitempty"`

}

We at least need addressesFromPools, so this struct does not fit

Nutrymaco · 2024-07-11T07:48:49Z

@chrischdi @killianmuldoon
What can i do to make this PR move forward?

chrischdi · 2024-07-17T13:36:18Z

cc @lubronzhan , @neolit123 , this is a change to the govmomi API types :-)

Can you maybe take a look if you think they make sense on a high level first?

neolit123

thanks, for the PR.
added a few comments.

apis/v1beta1/vspherefailuredomain_types.go

neolit123 · 2024-07-17T15:22:52Z

controllers/vspheredeploymentzone_controller_domain.go

+			continue
+		}
+		if _, err := deploymentZoneCtx.AuthSession.Finder.Network(ctx, networkConfig.NetworkName); err != nil {
+			conditions.MarkFalse(deploymentZoneCtx.VSphereDeploymentZone, infrav1.VSphereFailureDomainValidatedCondition, infrav1.NetworkNotFoundReason, clusterv1.ConditionSeverityError, "network %s is misconfigured", networkConfig.NetworkName)


is the network not found rather than misconfigured in such a scenario?

Looks like it's copied from the original network config without failure domain. Could you update both places?

Updated both

neolit123 · 2024-07-17T15:27:55Z

pkg/services/vimmachine.go

+	device.NetworkName = network
+}
+
+func mergeFailureDomainNetSpecToNetworkDeviceSpec(device *infrav1.NetworkDeviceSpec, fd infrav1.NetworkConfiguration) {


Suggested change

func mergeFailureDomainNetSpecToNetworkDeviceSpec(device *infrav1.NetworkDeviceSpec, fd infrav1.NetworkConfiguration) {

func mergeNetworkConfigurationInNetworkDeviceSpec(device *infrav1.NetworkDeviceSpec, fd infrav1.NetworkConfiguration) {

more of a question whether having the types in the function makes more sense?
and on that note should fd be replaced with something like nc?

Fixed names as suggested. If you mean types in function name, i don't see other choice due to lack of overloading in go.

lubronzhan · 2024-07-18T01:05:56Z

Should the Network be marked as deprecated in favor of the new NetworkConfiguration?

In general LGTM

chrischdi · 2024-07-01T11:33:41Z

apis/v1alpha3/topology_conversion.go

+	if len(in.NetworkConfigurations) > 0 {
+		networks := make([]string, len(in.NetworkConfigurations))
+		for i := range in.NetworkConfigurations {
+			networks[i] = in.NetworkConfigurations[i].NetworkName
+		}
+		out.Networks = networks
+	}
+	return nil


I think we should not convert it from a different struct in v1alpha3 to v1beta1 (same for v1alpha4), because it would cause different results when forward and backwards converting.

We should use it as new additional field because we already have the networks element also in v1beta1.

Replaced with the generated conversion

lubronzhan · 2024-10-09T01:18:43Z

Sorry for the late reply, overall lgtm

Nutrymaco · 2024-10-22T06:42:04Z

Hi, @chrischdi !
Any estimate on when PR can be merged?

internal/webhooks/vspherefailuredomain.go

apis/v1alpha3/zz_generated.conversion.go

chrischdi · 2024-10-23T10:40:51Z

That should be the final steps :-)

internal/webhooks/vspherefailuredomain.go

Nutrymaco · 2024-11-02T08:49:42Z

@chrischdi
Fixed your comments. Did you have a chance to look?

apis/v1beta1/vspherefailuredomain_types.go

internal/webhooks/vspherefailuredomain.go

pkg/services/vimmachine.go

pkg/services/vimmachine_test.go

sbueringer · 2024-11-12T13:23:43Z

@Nutrymaco just a few smaller findings from my side

sbueringer · 2024-11-13T09:28:27Z

I'll take another look once tests and linter are back to green

apis/v1beta1/vspherefailuredomain_types.go

pkg/services/vimmachine_test.go

sbueringer · 2024-11-13T10:54:41Z

/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main
/test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-main

sbueringer · 2024-11-13T10:55:27Z

Thank you very much!

/lgtm

Will merge once all e2e tests are completed & green

k8s-ci-robot · 2024-11-13T10:55:34Z

LGTM label has been added.

Git tree hash: 8f254d42426a20bf548cb025fdf428fc517ca8c8

sbueringer · 2024-11-13T12:22:05Z

/approve

k8s-ci-robot · 2024-11-13T12:22:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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

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: no Indicates the PR's author has not signed the CNCF CLA. label Jun 10, 2024

k8s-ci-robot requested review from killianmuldoon and vrabbi June 10, 2024 22:23

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 Jun 10, 2024

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 10, 2024

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 Jun 13, 2024

chrischdi reviewed Jun 13, 2024

View reviewed changes

pkg/services/vimmachine.go Show resolved Hide resolved

chrischdi reviewed Jun 13, 2024

View reviewed changes

apis/v1beta1/vspherefailuredomain_types.go Outdated Show resolved Hide resolved

Nutrymaco force-pushed the fd-new-api branch from 10157cf to 15ba0d6 Compare June 14, 2024 17:48

chrischdi reviewed Jun 17, 2024

View reviewed changes

apis/v1beta1/vspherefailuredomain_types.go Outdated Show resolved Hide resolved

neolit123 reviewed Jul 17, 2024

View reviewed changes

chrischdi reviewed Jul 18, 2024

View reviewed changes

chrischdi reviewed Oct 23, 2024

View reviewed changes

internal/webhooks/vspherefailuredomain.go Outdated Show resolved Hide resolved

internal/webhooks/vspherefailuredomain.go Outdated Show resolved Hide resolved

apis/v1alpha3/zz_generated.conversion.go Show resolved Hide resolved

fix errs, conversions

970ef58

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2024

k8s-ci-robot requested a review from chrischdi October 23, 2024 19:18

chrischdi reviewed Oct 24, 2024

View reviewed changes

internal/webhooks/vspherefailuredomain.go Show resolved Hide resolved

add webhook test

cc001db

sbueringer reviewed Nov 12, 2024

View reviewed changes

sbueringer reviewed Nov 13, 2024

View reviewed changes

apis/v1beta1/vspherefailuredomain_types.go Outdated Show resolved Hide resolved

sbueringer reviewed Nov 13, 2024

View reviewed changes

pkg/services/vimmachine_test.go Outdated Show resolved Hide resolved

Nutrymaco force-pushed the fd-new-api branch from edd72e8 to 50ad18e Compare November 13, 2024 10:14

sbueringer added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 13, 2024

fix

4b8978c

Nutrymaco force-pushed the fd-new-api branch from 50ad18e to 4b8978c Compare November 13, 2024 10:22

k8s-ci-robot assigned sbueringer Nov 13, 2024

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

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

k8s-ci-robot merged commit a80b417 into kubernetes-sigs:main Nov 13, 2024
13 checks passed

k8s-ci-robot added this to the v1.12 milestone Nov 13, 2024

sbueringer changed the title ~~✨ Add NetworkConfigurations, VMTemplate to FailureDomain~~ ✨ Add NetworkConfigurations to VSphereFailureDomain Nov 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add NetworkConfigurations to VSphereFailureDomain #3049

✨ Add NetworkConfigurations to VSphereFailureDomain #3049

Nutrymaco commented Jun 10, 2024 •

edited by chrischdi

Loading

linux-foundation-easycla bot commented Jun 10, 2024 •

edited

Loading

k8s-ci-robot commented Jun 10, 2024

k8s-ci-robot commented Jun 10, 2024

chrischdi commented Jun 13, 2024

Nutrymaco commented Jun 13, 2024

chrischdi commented Jun 17, 2024 •

edited

Loading

chrischdi left a comment •

edited

Loading

Nutrymaco commented Jun 17, 2024 •

edited

Loading

Nutrymaco commented Jun 19, 2024

Nutrymaco commented Jul 11, 2024

chrischdi commented Jul 17, 2024

neolit123 left a comment

neolit123 Jul 17, 2024

lubronzhan Jul 18, 2024

Nutrymaco Jul 18, 2024

neolit123 Jul 17, 2024

Nutrymaco Jul 18, 2024

lubronzhan commented Jul 18, 2024 •

edited

Loading

chrischdi Jul 1, 2024

Nutrymaco Jul 18, 2024

lubronzhan commented Oct 9, 2024

Nutrymaco commented Oct 22, 2024

chrischdi commented Oct 23, 2024

Nutrymaco commented Nov 2, 2024

sbueringer commented Nov 12, 2024

sbueringer commented Nov 13, 2024

sbueringer commented Nov 13, 2024

sbueringer commented Nov 13, 2024 •

edited

Loading

k8s-ci-robot commented Nov 13, 2024

sbueringer commented Nov 13, 2024

k8s-ci-robot commented Nov 13, 2024

	func mergeFailureDomainNetSpecToNetworkDeviceSpec(device *infrav1.NetworkDeviceSpec, fd infrav1.NetworkConfiguration) {
	func mergeNetworkConfigurationInNetworkDeviceSpec(device *infrav1.NetworkDeviceSpec, fd infrav1.NetworkConfiguration) {

✨ Add NetworkConfigurations to VSphereFailureDomain #3049

✨ Add NetworkConfigurations to VSphereFailureDomain #3049

Conversation

Nutrymaco commented Jun 10, 2024 • edited by chrischdi Loading

linux-foundation-easycla bot commented Jun 10, 2024 • edited Loading

k8s-ci-robot commented Jun 10, 2024

k8s-ci-robot commented Jun 10, 2024

chrischdi commented Jun 13, 2024

Nutrymaco commented Jun 13, 2024

chrischdi commented Jun 17, 2024 • edited Loading

chrischdi left a comment • edited Loading

Choose a reason for hiding this comment

Nutrymaco commented Jun 17, 2024 • edited Loading

Nutrymaco commented Jun 19, 2024

Nutrymaco commented Jul 11, 2024

chrischdi commented Jul 17, 2024

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 Jul 17, 2024

Choose a reason for hiding this comment

lubronzhan Jul 18, 2024

Choose a reason for hiding this comment

Nutrymaco Jul 18, 2024

Choose a reason for hiding this comment

neolit123 Jul 17, 2024

Choose a reason for hiding this comment

Nutrymaco Jul 18, 2024

Choose a reason for hiding this comment

lubronzhan commented Jul 18, 2024 • edited Loading

chrischdi Jul 1, 2024

Choose a reason for hiding this comment

Nutrymaco Jul 18, 2024

Choose a reason for hiding this comment

lubronzhan commented Oct 9, 2024

Nutrymaco commented Oct 22, 2024

chrischdi commented Oct 23, 2024

Nutrymaco commented Nov 2, 2024

sbueringer commented Nov 12, 2024

sbueringer commented Nov 13, 2024

sbueringer commented Nov 13, 2024

sbueringer commented Nov 13, 2024 • edited Loading

k8s-ci-robot commented Nov 13, 2024

sbueringer commented Nov 13, 2024

k8s-ci-robot commented Nov 13, 2024

Nutrymaco commented Jun 10, 2024 •

edited by chrischdi

Loading

linux-foundation-easycla bot commented Jun 10, 2024 •

edited

Loading

chrischdi commented Jun 17, 2024 •

edited

Loading

chrischdi left a comment •

edited

Loading

Nutrymaco commented Jun 17, 2024 •

edited

Loading

lubronzhan commented Jul 18, 2024 •

edited

Loading

sbueringer commented Nov 13, 2024 •

edited

Loading