✨ Add NetworkConfigurations, VMTemplate to FailureDomain #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 (77a32a0)

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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign fabriziopandini for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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 · 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

Nutrymaco · 2024-07-29T14:06:52Z

@chrischdi @lubronzhan @neolit123
I have updated the PR to address your comments, can you take a look?

Nutrymaco · 2024-08-28T06:16:16Z

Hi everyone

Just checking in on the status. Is there an estimated timeline for when work on it might continue? Let me know if there’s anything I can do to help move it forward.

Thanks!

chrischdi

Sorry for the long round-trip, busy times...

One addition for the webhook and one more or less question on the API type.

As of that good for me.

apis/v1beta1/vspherefailuredomain_types.go

internal/webhooks/vspherefailuredomain.go

chrischdi · 2024-08-28T09:46:52Z

Also: needs a proper rebase to not have merge commits.

chrischdi · 2024-08-29T08:18:53Z

/test help

k8s-ci-robot · 2024-08-29T08:18:56Z

@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-vcsim-govmomi-main
/test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-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-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-sigs/prow repository.

chrischdi · 2024-08-29T08:19:02Z

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

chrischdi · 2024-09-04T15:04:25Z

/lgtm

cc @lubronzhan @neolit123 @rikatz

k8s-ci-robot · 2024-09-04T15:04:31Z

LGTM label has been added.

Git tree hash: f6886e0604252eff59e2e41883e2baad7defa907

chrischdi · 2024-09-04T15:04:43Z

pull-cluster-api-provider-vsphere-e2e-govmomi-main at team-cluster-api#19 are green

Nutrymaco · 2024-09-26T05:59:10Z

Hi @chrischdi! When is this pr planned to be merged? Maybe there is something i need to do?

chrischdi · 2024-09-26T06:15:24Z

@lubronzhan @neolit123 @rikatz : would be awesome to get another pair of eyes on this :-)

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

Nutrymaco force-pushed the fd-new-api branch 2 times, most recently from b6e9a99 to 7fa4378 Compare July 18, 2024 13:26

chrischdi reviewed Aug 28, 2024

View reviewed changes

apis/v1beta1/vspherefailuredomain_types.go Show resolved Hide resolved

internal/webhooks/vspherefailuredomain.go Show resolved Hide resolved

add Topology.NetworkConfigurations

77a32a0

Nutrymaco force-pushed the fd-new-api branch from f03eb35 to 77a32a0 Compare August 28, 2024 11:05

chrischdi mentioned this pull request Aug 29, 2024

add Topology.NetworkConfigurations team-cluster-api/cluster-api-provider-vsphere#19

Open

k8s-ci-robot assigned chrischdi Sep 4, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add NetworkConfigurations, VMTemplate to FailureDomain #3049

✨ Add NetworkConfigurations, VMTemplate to FailureDomain #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

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

Nutrymaco commented Jul 29, 2024

Nutrymaco commented Aug 28, 2024

chrischdi left a comment

chrischdi commented Aug 28, 2024

chrischdi commented Aug 29, 2024

k8s-ci-robot commented Aug 29, 2024

chrischdi commented Aug 29, 2024

chrischdi commented Sep 4, 2024

k8s-ci-robot commented Sep 4, 2024

chrischdi commented Sep 4, 2024

Nutrymaco commented Sep 26, 2024

chrischdi commented Sep 26, 2024

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

✨ Add NetworkConfigurations, VMTemplate to FailureDomain #3049

Are you sure you want to change the base?

✨ Add NetworkConfigurations, VMTemplate to FailureDomain #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

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

Nutrymaco commented Jul 29, 2024

Nutrymaco commented Aug 28, 2024

chrischdi left a comment

Choose a reason for hiding this comment

chrischdi commented Aug 28, 2024

chrischdi commented Aug 29, 2024

k8s-ci-robot commented Aug 29, 2024

chrischdi commented Aug 29, 2024

chrischdi commented Sep 4, 2024

k8s-ci-robot commented Sep 4, 2024

chrischdi commented Sep 4, 2024

Nutrymaco commented Sep 26, 2024

chrischdi commented Sep 26, 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