OCPBUGS-18641: Set dual-stack IPFamilyPriority for vSphere #279

mkowalski · 2023-09-18T17:22:52Z

We have discovered that in dual-stack setups NodeAddresses field of
the instance metadata contains only IPv4 addresses for VMs that do have
both IPv4 and IPv6 addresses assigned (and detected by the VM agent).

It has been traced back to the function responsible for populating this
metadata field. We found out that for our configuration we always filter
only IPv4 addresses even if running in dual-stack. Reason for that is
that IPFamilyPriority has a value of ipv4 even when running in a
dual-stack setup.

This causes an issue because this instance metadata is cross-checked
with addresses provided by the kubelet as part of the
alpha.kubernetes.io/provided-node-ip annotation. Without correct value
of IPFamilyPriority we are thus removing all the IPv6 addresses.

This PR takes advantage of the Service Networks configured by the user in
install-config and the fact that we only allow 2 networks to be configured
if the setup is dual-stack.

Fixes: OCPBUGS-18641

openshift-ci · 2023-09-18T17:22:57Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2023-09-18T17:22:59Z

@mkowalski: This pull request references Jira Issue OCPBUGS-18641, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.15.0) matches configured target version for branch (4.15.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @sunzhaohua2

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

We have discovered that in dual-stack setups NodeAddresses field of the instance metadata contains only IPv4 addresses for VMs that do have both IPv4 and IPv6 addresses assigned (and detected by the VM agent).

It has been traced back to the function responsible for populating this metadata field. We found out that for our configuration we always filter only IPv4 addresses even if running in dual-stack. Reason for that is that IPFamilyPriority has a value of ipv4 even when running in a dual-stack setup.

This causes an issue because this instance metadata is cross-checked with addresses provided by the kubelet as part of the alpha.kubernetes.io/provided-node-ip annotation. Without correct value of IPFamilyPriority we are thus removing all the IPv6 addresses.

This PR takes advantage of the API VIPs configured by the user in install-config and the fact that we only allow 2 VIPs to be configured if the setup is dual-stack.

Fixes: OCPBUGS-18641

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.

mkowalski · 2023-09-18T17:27:07Z

/test all

openshift-ci-robot · 2023-09-19T12:20:55Z

@mkowalski: This pull request references Jira Issue OCPBUGS-18641, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.15.0) matches configured target version for branch (4.15.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @huali9

In response to this:

We have discovered that in dual-stack setups NodeAddresses field of
the instance metadata contains only IPv4 addresses for VMs that do have
both IPv4 and IPv6 addresses assigned (and detected by the VM agent).

It has been traced back to the function responsible for populating this
metadata field. We found out that for our configuration we always filter
only IPv4 addresses even if running in dual-stack. Reason for that is
that IPFamilyPriority has a value of ipv4 even when running in a
dual-stack setup.

This causes an issue because this instance metadata is cross-checked
with addresses provided by the kubelet as part of the
alpha.kubernetes.io/provided-node-ip annotation. Without correct value
of IPFamilyPriority we are thus removing all the IPv6 addresses.

This PR takes advantage of the Service Networks configured by the user in
install-config and the fact that we only allow 2 networks to be configured
if the setup is dual-stack.

Fixes: OCPBUGS-18641

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.

mkowalski · 2023-09-19T12:20:57Z

/test all

mkowalski · 2023-09-19T15:29:25Z

/test all

mkowalski · 2023-09-20T10:21:47Z

/test e2e-vsphere-ovn

It's sig-storage that fails, let's just retest

JoelSpeed · 2023-09-20T10:26:59Z

pkg/cloud/vsphere/assets/cloud-controller-manager-deployment.yaml

+            - name: ENABLE_ALPHA_DUAL_STACK
+              value: "true"


Is this forcing on an alpha feature gate for all vSphere clusters?

This is not really a feature gate, at least not in the sense how we implemented CloudDualStackNodeIPs in https://kubernetes.io/docs/concepts/services-networking/dual-stack/

This is some VMware internal(?) code that requires an existence of env variable (without even looking at its value), look at https://github.com/openshift/cloud-provider-vsphere/blob/master/pkg/cloudprovider/vsphere/cloud.go#L253-L255 to see how it's handled. Not really the way how every other component handles CloudDualStackNodeIPs (even the name does not match).

From the moment I've seen it I got confused about how we should handle this. The only thing that made me feel safe is that this validator I have linked is the only place in the whole codebase where this is used.

Any suggestion?

Looking at the code, it seems relatively minor, I guess we can leave it as is

JoelSpeed · 2023-09-20T10:27:42Z

pkg/cloud/vsphere/vsphere_config_transformer.go

+		if net.IsIPv4CIDRString(network.Spec.ServiceNetwork[0]) {
+			cfg.Global.IPFamilyPriority = []string{"ipv4", "ipv6"}
+		} else {
+			cfg.Global.IPFamilyPriority = []string{"ipv6", "ipv4"}
+		}


Is there a possibility that both entries in the list are ipv4?

No, having ["ipv4,"ipv4"] would behave exactly the same as ["ipv4"]

My point was more, ServiceNetwork could be two IPv4 addresses right? Why aren't we checking both entries?

No, that's not allowed. If you have 2 service networks, then one has to be v4 and the other one v6

In fact well, let me confirm this with o/installer... It may have changed and the validations are way more complex now

https://github.com/openshift/installer/blob/master/pkg/types/validation/installconfig.go#L310-L313 <-- so if you are installing single-stack v4 cluster, you may have only 1 service network entry

https://github.com/openshift/installer/blob/master/pkg/types/validation/installconfig.go#L252-L254 <-- if you are installing dual-stack cluster, you must have exactly 2 entries

Great, can you just add a comment inline to explain that if there are two service networks, the installer already validated that one is ipv4 and the other is ipv6, so we don't lose this context?

Of course, done

JoelSpeed · 2023-09-20T16:23:49Z

/hold
/approve cancel

CI failures indicate that the CSI driver is probably not ok with this change, @mkowalski can you please take a look

mkowalski · 2023-09-20T16:43:05Z

As discussed, seems like wider failure what makes me think it's not my code what introduced it (luckily this time)

Ref.: https://redhat-internal.slack.com/archives/C01CQA76KMX/p1695228004392599

mkowalski · 2023-09-20T16:52:06Z

/test e2e-vsphere-ovn

openshift-ci · 2023-09-20T18:58:46Z

@mkowalski: 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
ci/prow/e2e-alibaba-ovn	`428d639`	link	true	`/test e2e-alibaba-ovn`
ci/prow/e2e-openstack-ovn	`428d639`	link	false	`/test e2e-openstack-ovn`

Full PR test history. Your PR dashboard.

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.

rbbratta · 2023-09-25T14:41:17Z

pkg/cloud/vsphere/vsphere_config_transformer.go

+			}
+		}
+
+		nodeNetworking.External.ExcludeNetworkSubnetCIDR = append(nodeNetworking.External.ExcludeNetworkSubnetCIDR, "fd69::2/128")


maybe a named constant for fd69::2/128 so we can crossref with ovn-kubernetes.

I'd totally agree if we had a common/shared library that ovn-k8s would consume. In the state like it is now, I'd say address is an address and it's not a big diff if there is variable or not (especially that I use it only in a single place)

we avoid magic numbers? and it is used twice. but maybe we add a comment in OVN-k8s as well.

https://github.com/openshift/ovn-kubernetes/blob/master/go-controller/pkg/config/config.go#L145

can customer override V6HostMasqueradeIP?

Yep, it's configurable ovn-kubernetes/ovn-kubernetes#3594

openshift/cluster-network-operator#1807

Err, we can pull from the API? openshift/api#1410

I'm looking into this. It's not nice because they didn't add it as Network CRD but only in the Operator Config CRD so I'm not sure if I can access it easily from inside the 3cmo

To clarify - until openshift/cluster-network-operator#1807 merges, this value is not configurable. I am now discussing with OVN-K8s folks the rationale, but at this moment this value is the only one that we will see in the wild.

Having said this, I wonder if we can push this change as it is now and then when OVN-K8s folks are ready with openshift/cluster-network-operator#1807 (hopefully with revisited API that stores the custom subnet in the Network CR) improve it for 4.15

Yep, I agree we should merge this to unblock dual-stack.

mkowalski · 2023-09-26T11:53:55Z

openshift/kubernetes#1714 is already in the merge pool. Hopefully we get a nightly today and by tomorrow this one here is green

rbbratta · 2023-09-26T14:36:50Z

launch 4.14,openshift/cluster-cloud-controller-manager-operator#279 vsphere,dualstack

failed, due to lack of nodes

https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/release-openshift-origin-installer-launch-vsphere-modern/1706620554640887808

message="Error creating: pods \"cluster-version-operator-7d7d9c9b99-\" is forbidden: autoscaling.openshift.io/ManagementCPUsOverride the cluster does not have any nodes"

retrying.

mkowalski · 2023-09-26T14:39:59Z

We had a successful cluster from this PR back in September 19th, https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/release-openshift-origin-installer-launch-vsphere-modern/1704172710533271552

rbbratta · 2023-09-26T14:42:54Z

I had successful cluster yesterday, so maybe quota or something.

mkowalski · 2023-09-27T07:34:18Z

/test e2e-vsphere-ovn

JoelSpeed · 2023-09-27T10:40:20Z

/hold cancel
/approve

openshift-ci · 2023-09-27T10:40:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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

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

openshift-ci-robot · 2023-09-27T13:16:32Z

@mkowalski: Jira Issue OCPBUGS-18641: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

openshift/release#43131 is open

These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-18641 has not been moved to the MODIFIED state.

In response to this:

We have discovered that in dual-stack setups NodeAddresses field of
the instance metadata contains only IPv4 addresses for VMs that do have
both IPv4 and IPv6 addresses assigned (and detected by the VM agent).

It has been traced back to the function responsible for populating this
metadata field. We found out that for our configuration we always filter
only IPv4 addresses even if running in dual-stack. Reason for that is
that IPFamilyPriority has a value of ipv4 even when running in a
dual-stack setup.

This causes an issue because this instance metadata is cross-checked
with addresses provided by the kubelet as part of the
alpha.kubernetes.io/provided-node-ip annotation. Without correct value
of IPFamilyPriority we are thus removing all the IPv6 addresses.

This PR takes advantage of the Service Networks configured by the user in
install-config and the fact that we only allow 2 networks to be configured
if the setup is dual-stack.

Fixes: OCPBUGS-18641

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.

mkowalski · 2023-09-27T13:30:42Z

/jira refresh

openshift-ci-robot · 2023-09-27T13:30:45Z

@mkowalski: Jira Issue OCPBUGS-18641: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

openshift/release#43131 is open

These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-18641 has not been moved to the MODIFIED state.

In response to this:

/jira refresh

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.

mkowalski · 2023-09-27T13:31:13Z

/jira refresh

openshift-ci-robot · 2023-09-27T13:31:15Z

@mkowalski: Jira Issue OCPBUGS-18641: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

openshift/release#43131 is open

These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-18641 has not been moved to the MODIFIED state.

In response to this:

/jira refresh

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.

mkowalski · 2023-09-27T13:31:56Z

/jira refresh

openshift-ci-robot · 2023-09-27T13:32:00Z

@mkowalski: Jira Issue OCPBUGS-18641: All pull requests linked via external trackers have merged:

openshift/cluster-cloud-controller-manager-operator#279

Jira Issue OCPBUGS-18641 has been moved to the MODIFIED state.

In response to this:

/jira refresh

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.

mkowalski · 2023-09-27T13:33:36Z

/cherry-pick release-4.14

openshift-cherrypick-robot · 2023-09-27T13:34:19Z

@mkowalski: new pull request created: #283

In response to this:

/cherry-pick release-4.14

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.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 18, 2023

openshift-ci bot requested a review from sunzhaohua2 September 18, 2023 17:23

This was referenced Sep 18, 2023

OCPBUGS-18641: Fix dual-stack node addresses in instance metadata openshift/cloud-provider-vsphere#49

Closed

OCPBUGS-18641: Debug what is wrong openshift/cloud-provider-vsphere#47

Closed

mkowalski force-pushed the OCPBUGS-18641 branch 8 times, most recently from 72ffc4b to 013459b Compare September 19, 2023 12:20

openshift-ci bot requested a review from huali9 September 19, 2023 12:21

mkowalski force-pushed the OCPBUGS-18641 branch from 013459b to ec74c8c Compare September 19, 2023 15:28

mkowalski force-pushed the OCPBUGS-18641 branch from ec74c8c to fd8ee8c Compare September 19, 2023 16:36

mkowalski marked this pull request as ready for review September 20, 2023 07:16

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 20, 2023

openshift-ci bot requested review from JoelSpeed and elmiko September 20, 2023 07:17

JoelSpeed reviewed Sep 20, 2023

View reviewed changes

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 20, 2023

rbbratta reviewed Sep 25, 2023

View reviewed changes

mkowalski mentioned this pull request Sep 27, 2023

OPNET-320: Full dual-stack implementation in vSphere openshift/release#41096

Closed

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2023

openshift-merge-robot merged commit 140583c into openshift:master Sep 27, 2023

mkowalski deleted the OCPBUGS-18641 branch September 27, 2023 13:30

openshift-cherrypick-robot mentioned this pull request Sep 27, 2023

[release-4.14] OCPBUGS-19849: Set dual-stack IPFamilyPriority for vSphere #283

Merged

rbbratta mentioned this pull request Sep 27, 2023

[WIP] NO-ISSUE: test ccm vsphere internal cidr openshift/release#43131

Closed

OCPBUGS-18641: Set dual-stack IPFamilyPriority for vSphere #279

OCPBUGS-18641: Set dual-stack IPFamilyPriority for vSphere #279

Uh oh!

Conversation

mkowalski commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Sep 18, 2023

Uh oh!

openshift-ci-robot commented Sep 18, 2023

Uh oh!

mkowalski commented Sep 18, 2023

Uh oh!

openshift-ci-robot commented Sep 19, 2023

Uh oh!

mkowalski commented Sep 19, 2023

Uh oh!

mkowalski commented Sep 19, 2023

Uh oh!

mkowalski commented Sep 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkowalski Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Sep 20, 2023

Uh oh!

mkowalski commented Sep 20, 2023

Uh oh!

mkowalski commented Sep 20, 2023

Uh oh!

openshift-ci bot commented Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkowalski commented Sep 26, 2023

Uh oh!

rbbratta commented Sep 26, 2023

Uh oh!

mkowalski commented Sep 26, 2023

Uh oh!

rbbratta commented Sep 26, 2023

Uh oh!

mkowalski commented Sep 18, 2023 •

edited

Loading

mkowalski Sep 20, 2023 •

edited

Loading

openshift-ci bot commented Sep 20, 2023 •

edited

Loading