✨ Extend Failure Domain API to support extra network configuration #1967

rikatz · 2023-06-30T17:12:40Z

What this PR does / why we need it:
This PR is the initial work to extend failure domain API to support extra network configuration.

The idea is that extra network configuration can be established on failure domains, so different configurations (like DHCP, a different set of DNS servers) can be configured based on the selected region/zone

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1964

Special notes for your reviewer:
Some points to be discussed:

Should we copy the networkspec struct or should we implement (as it is now) a different API?
Can we drop/remove the dead code from Network struct (inside deploymentzone_types) and ignore the apidiff?
Is there any e2e test that can be added here? I couldn't find any failure domain tests on integration or e2e

Release note:

Introduce extra network configuration on Failure domain API

k8s-ci-robot · 2023-06-30T17:12:53Z

[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 vincepri 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

rikatz · 2023-06-30T17:55:48Z

On the previous run: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-vsphere/1967/pull-cluster-api-provider-vsphere-apidiff-main/1674828318085484544

Probably as a last commit, we should remove the dead Network struct and override this alert from apidiff.

apis/v1beta1/vspherefailuredomain_webhook.go

pkg/services/vimmachine.go

Co-authored-by: Christian Schlotter <[email protected]>

apis/v1beta1/vspherefailuredomain_types.go

rikatz · 2023-07-03T20:30:42Z

Fyi, tested locally with the changes, it works fine.

Test executed:

Created 3 Failure domains/deployment zones
Each failure domain has a different portgroup, a different set of DNS, some with DHCPv6 disabled other enabled
Could verify that on each node the right configuration was added to netplan
Implemented the tests for controllers to verify that deploymentzone doesn't get ready if the Network Port group doesn't exists

chrischdi · 2023-07-04T10:58:36Z

config/default/crd/bases/infrastructure.cluster.x-k8s.io_vsphereclusters.yaml

@@ -321,7 +321,6 @@ spec:
                    description: 'UID of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids'
                    type: string
                type: object
-                x-kubernetes-map-type: atomic


I think this should not have happened 🤔 may be related to #1928

Yeah, @srm09 pointed me to some issue on controller-gen IIRC, it keeps changing :)

chrischdi · 2023-07-04T11:00:27Z

controllers/vspheredeploymentzone_controller_domain.go

@@ -84,6 +84,16 @@ func (r vsphereDeploymentZoneReconciler) reconcileTopology(ctx *context.VSphereD
 		}
 	}

+	for _, networkConfig := range topology.NetworkConfigurations {
+		if networkConfig.NetworkName == "" {


Should we validate for NetworkName being set instead of silently ignoring the array entry?

My question actually is: should we make NetworkName required? Per https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/pull/1967/files#diff-2f616e5f5f40526609f3723e82cbcf01b16b9f240ea00fcdf3fc06b6aa6d164cR548 we just merge if it is not empty. This would allow a case where a failure domain has different nameservers, but same Portgroup name.

OTOH, there's no point IMO in validate a failure domain network config if the portgroup is not configured, so probably marking Network Name as required would be better IMHO.

WDYT?

If the other code which overwrites stuff from NetworkConfig to the vmNetworkDeviceSpec and is ok with not replacing the NetworkName, we should drop the if here and keep it non-mandatory.

If that is not a use case, (and I assume it isn't according your above comment), then we should mark the NetworkName as required so we ensure on api side already that it cannot be misconfigured (by not setting the NetworkName).

Happy to also hear thoughts from @yastij or others :-)

I think is better to make the networkName as a key inside the network array. Because if there is Multiple devices
configured vsphereMachineTemplate, if the use forgot to specify the network name, it might accidentally configure Devices config on the vsphereMachineTemplate.

apis/v1beta1/vspherefailuredomain_webhook.go

Co-authored-by: Christian Schlotter <[email protected]>

k8s-ci-robot · 2023-07-11T18:35:28Z

PR needs rebase.

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.

randomvariable · 2023-07-28T17:26:13Z

/assign @lubronzhan
for review on networking side.

@rikatz , can you rebase this onto main? this will also fix the CRD generation.

lubronzhan · 2023-07-28T19:07:05Z

I think better to call out in the doc that the network devices specified in FailureDomain will conduct a Merge operation instead of Overwrite operation to the devices specified inside VSphereMachineTemplate

k8s-ci-robot · 2023-09-07T08:25:12Z

@rikatz: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-vsphere-verify-crds	`4c1451a`	link	false	`/test pull-cluster-api-provider-vsphere-verify-crds`
pull-cluster-api-provider-vsphere-verify-gen	`4c1451a`	link	false	`/test pull-cluster-api-provider-vsphere-verify-gen`
pull-cluster-api-provider-vsphere-test-main	`4c1451a`	link	true	`/test pull-cluster-api-provider-vsphere-test-main`
pull-cluster-api-provider-vsphere-test-integration-main	`4c1451a`	link	true	`/test pull-cluster-api-provider-vsphere-test-integration-main`
pull-cluster-api-provider-vsphere-e2e-main	`4c1451a`	link	true	`/test pull-cluster-api-provider-vsphere-e2e-main`
pull-cluster-api-provider-vsphere-e2e-main-oldpreset	`4c1451a`	link	true	`/test pull-cluster-api-provider-vsphere-e2e-main-oldpreset`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

k8s-triage-robot · 2024-01-21T11:20:53Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

sbueringer · 2024-01-25T18:15:53Z

/close

for now

@rikatz feel free to just reopen if there is continued interest in making this happen

k8s-ci-robot · 2024-01-25T18:15:58Z

@sbueringer: Closed this PR.

In response to this:

/close

for now

@rikatz feel free to just reopen if there is continued interest in making this happen

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.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 30, 2023

k8s-ci-robot requested review from vrabbi and yastij June 30, 2023 17:12

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 30, 2023

Implement failure domain network configurations

09cd72f

rikatz force-pushed the fd-network-api branch from ecc3fa8 to d6a9706 Compare June 30, 2023 17:49

rikatz added 2 commits June 30, 2023 15:57

Add tests for failure domain network config

25c3e14

Add generated files

08aecde

rikatz force-pushed the fd-network-api branch from d6a9706 to 08aecde Compare June 30, 2023 18:58

rikatz changed the title ~~WIP: ✨ Extend Failure Domain API to support extra network configuration~~ ✨ Extend Failure Domain API to support extra network configuration Jun 30, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 30, 2023

chrischdi reviewed Jul 3, 2023

View reviewed changes

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

chrischdi reviewed Jul 3, 2023

View reviewed changes

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

rikatz and others added 2 commits July 3, 2023 10:15

Do not change the original fd object

405077c

Co-authored-by: Christian Schlotter <[email protected]>

Validate topology network creation

10a9ab4

rikatz force-pushed the fd-network-api branch from ef5e732 to 10a9ab4 Compare July 3, 2023 13:48

chrischdi reviewed Jul 3, 2023

View reviewed changes

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

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

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

rikatz added 2 commits July 3, 2023 16:03

Rename network config API and regenerate

c09ec63

Verify also networkConfig portgroup name

93d304e

rikatz added 2 commits July 3, 2023 18:27

Add tests for failure domains on controllers

6ede105

Fix linter errors

3b311f5

chrischdi reviewed Jul 4, 2023

View reviewed changes

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

Improve webhook error message

4c1451a

Co-authored-by: Christian Schlotter <[email protected]>

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2023

k8s-ci-robot assigned lubronzhan Jul 28, 2023

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 21, 2024

k8s-ci-robot closed this Jan 25, 2024

Nutrymaco mentioned this pull request Jun 10, 2024

✨ Add NetworkConfigurations, VMTemplate to FailureDomain #3049

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Extend Failure Domain API to support extra network configuration #1967

✨ Extend Failure Domain API to support extra network configuration #1967

rikatz commented Jun 30, 2023 •

edited

Loading

k8s-ci-robot commented Jun 30, 2023

rikatz commented Jun 30, 2023

rikatz commented Jul 3, 2023 •

edited

Loading

chrischdi Jul 4, 2023

rikatz Jul 4, 2023

chrischdi Jul 4, 2023

rikatz Jul 4, 2023

chrischdi Jul 5, 2023

lubronzhan Jul 28, 2023

k8s-ci-robot commented Jul 11, 2023

randomvariable commented Jul 28, 2023

lubronzhan commented Jul 28, 2023

k8s-ci-robot commented Sep 7, 2023

k8s-triage-robot commented Jan 21, 2024

sbueringer commented Jan 25, 2024

k8s-ci-robot commented Jan 25, 2024

✨ Extend Failure Domain API to support extra network configuration #1967

✨ Extend Failure Domain API to support extra network configuration #1967

Conversation

rikatz commented Jun 30, 2023 • edited Loading

k8s-ci-robot commented Jun 30, 2023

rikatz commented Jun 30, 2023

rikatz commented Jul 3, 2023 • edited Loading

chrischdi Jul 4, 2023

Choose a reason for hiding this comment

rikatz Jul 4, 2023

Choose a reason for hiding this comment

chrischdi Jul 4, 2023

Choose a reason for hiding this comment

rikatz Jul 4, 2023

Choose a reason for hiding this comment

chrischdi Jul 5, 2023

Choose a reason for hiding this comment

lubronzhan Jul 28, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 11, 2023

randomvariable commented Jul 28, 2023

lubronzhan commented Jul 28, 2023

k8s-ci-robot commented Sep 7, 2023

k8s-triage-robot commented Jan 21, 2024

sbueringer commented Jan 25, 2024

k8s-ci-robot commented Jan 25, 2024

rikatz commented Jun 30, 2023 •

edited

Loading

rikatz commented Jul 3, 2023 •

edited

Loading