✨ Support propagate uplink status #1481

lentzi90 · 2023-02-17T10:56:49Z

What this PR does / why we need it:

This adds support for propagate_uplink_status on ports.

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 #1480

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
if necessary:
- includes documentation
- adds unit tests
Add support in gophercloud Enhancement: Support propagate_uplink_status for ports gophercloud/gophercloud#2562
Bump gophercloud version to release that includes support for this. 🌱 Bump gophercloud to v1.3.0 #1520

/hold

k8s-ci-robot · 2023-02-17T10:56:51Z

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

netlify · 2023-02-17T10:56:58Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	`3c2bfe6`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/642581172768f10008a67480
😎 Deploy Preview	https://deploy-preview-1481--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

lentzi90 · 2023-02-17T11:00:16Z

/test pull-cluster-api-provider-openstack-build
/test pull-cluster-api-provider-openstack-test

lentzi90 · 2023-02-17T11:18:11Z

/test pull-cluster-api-provider-openstack-e2e-full-test

lentzi90 · 2023-02-17T11:47:22Z

/test pull-cluster-api-provider-openstack-e2e-full-test

mdbooth · 2023-02-17T12:16:39Z

What does it do? The neutron API doc is here: https://docs.openstack.org/api-ref/network/v2/index.html#show-port-details

I read it and I still don't know what it does.

lentzi90 · 2023-02-17T12:31:48Z

The best I could find about it is this release note:

This feature can be used in SRIOV scenario, in which users enable uplink status propagation of the SRIOV port so that the link status of the VF will follow the PF.

From https://opendev.org/openstack/openstacksdk/commit/d027626d7faf92d44afc9b624bd4ccea35bec96a#diff-88ce11c08b50dff1088a7153dd6f5475a160b410

jichenjc · 2023-02-18T08:55:42Z

I also comments on the issue, just want to know the use case for this ..

lentzi90 · 2023-03-08T11:42:12Z

/test pull-cluster-api-provider-openstack-build
/test pull-cluster-api-provider-openstack-test

jichenjc · 2023-03-09T02:16:16Z

@lentzi90 can you help fix the conflict ?Thanks

lentzi90 · 2023-03-10T06:45:21Z

@lentzi90 can you help fix the conflict ?Thanks

The conflict was already fixed, or am I missing something? I'm waiting for a new gophercloud release though before we can take this in. It requires some changes in there also. 🙂

jichenjc · 2023-03-10T06:46:57Z

/test pull-cluster-api-provider-openstack-e2e-test

yes you are right, I missed the latest update of the flag 'need-rebase' removal :)

lentzi90 · 2023-03-30T12:38:36Z

api/v1alpha6/conversion.go

+	}
+}
+
+func restorev1alpha7ClusterStatus(previous *infrav1.OpenStackClusterStatus, dst *infrav1.OpenStackClusterStatus) {


I'm not too sure about this. Should we really restore the cluster status or is it better to let it re-build when reconciled?

We might need this, but please check my thinking:

Lets ignore for a moment whether this information should be in the cluster status at all (I think it should not), because that's a separate issue. Lets assume that the information is required.

We are running v0.8. OpenStackCluster is v1alpha6. Code in v0.8 expects that if it sets a value in Status then when it reads it, it will still be set in Status.

Code does the thing.

Code sets v1alpha7.Status.Foo to record that it did the thing

Webhook down-converts to v1alpha6

v1alpha6 is persisted in API

Next reconcile v1alpha6 is read from API

Webhook up-converts to v1alpha7

v1alpha7.Status.Foo is no longer set, so we might have to do the thing again.

Making my own counter-argument, this case might be different though because the value is immutable so it can only be set in the storage version, and if the storage version is v1alpha6 it can't be set at all 🤔

If that was the case (v1alpha6 was storage version for v0.8), then I guess that would be true. But the storage version is set to v1alpha7, which makes much more sense IMO. So I'm thinking there would not be any conversions required "internally" and thus no need to restore it really (other than for happy fuzzing tests).

I was also thinking about what happens if the user asks for v1alpha6, makes changes to it and applies. Then it would be up-converted to v1alpha7, including the status. But the user should not normally make changes to the status (or even be able to) so maybe this scenario is irrelevant.

As discussed in the office hours, let's keep this just in case. It should not do any harm
@mdbooth

mdbooth · 2023-04-17T12:26:37Z

/lgtm

mdbooth · 2023-04-17T12:28:51Z

Actually, @dulek are you able to look at this? I think it's good from an API pov but I don't fully understand the neutron side. I think it's good, though. Please bless with lgtm if appropriate :)

/lgtm cancel

api/v1alpha6/conversion.go

dulek · 2023-04-19T09:05:42Z

Actually, @dulek are you able to look at this? I think it's good from an API pov but I don't fully understand the neutron side. I think it's good, though. Please bless with lgtm if appropriate :)

/lgtm cancel

It's just an option used for SR-IOV ports, not a big deal from K8s point of view, I bet it's to be used on secondary ports in the workers. Looks good from my side, leaving lgtm to @seanschneeweiss once he's satisfied with remarks he made.

mdbooth · 2023-04-20T13:33:19Z

/lgtm

jichenjc · 2023-04-20T23:31:12Z

/approve

leave someone else want to take a look then do the hold cancel to merge

k8s-ci-robot · 2023-04-20T23:31:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc, lentzi90, mdbooth

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~~ [jichenjc,mdbooth]

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

mdbooth · 2023-04-21T12:29:51Z

@lentzi90 Down to you to remove the hold.

seanschneeweiss

/lgtm

mdbooth · 2023-04-24T08:43:21Z

/hold cancel

mdbooth · 2023-04-24T12:47:38Z

/test pull-cluster-api-provider-openstack-e2e-test

k8s-ci-robot requested review from jichenjc and mdbooth February 17, 2023 10:57

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 17, 2023

lentzi90 mentioned this pull request Feb 27, 2023

[DO NOT MERGE] Port patch Nordix/cluster-api-provider-openstack#7

Closed

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

lentzi90 force-pushed the lentzi90/propagate_uplink_status branch from d270f31 to d0cefe2 Compare March 8, 2023 09:42

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

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

lentzi90 mentioned this pull request Mar 30, 2023

🌱 Bump gophercloud to v1.3.0 #1520

Merged

3 tasks

Support propagate uplink status for ports

3c2bfe6

lentzi90 force-pushed the lentzi90/propagate_uplink_status branch from d0cefe2 to 3c2bfe6 Compare March 30, 2023 12:31

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

lentzi90 marked this pull request as ready for review March 30, 2023 12:35

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 Mar 30, 2023

k8s-ci-robot requested a review from seanschneeweiss March 30, 2023 12:36

k8s-ci-robot requested a review from tobiasgiese March 30, 2023 12:36

lentzi90 commented Mar 30, 2023

View reviewed changes

mdbooth approved these changes Apr 17, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2023

k8s-ci-robot assigned mdbooth Apr 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2023

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2023

seanschneeweiss reviewed Apr 18, 2023

View reviewed changes

api/v1alpha6/conversion.go Show resolved Hide resolved

api/v1alpha6/conversion.go Show resolved Hide resolved

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 20, 2023

seanschneeweiss reviewed Apr 22, 2023

View reviewed changes

k8s-ci-robot assigned seanschneeweiss Apr 22, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 24, 2023

k8s-ci-robot merged commit 71a9a2e into kubernetes-sigs:main Apr 24, 2023

lentzi90 deleted the lentzi90/propagate_uplink_status branch April 25, 2023 05:07

✨ Support propagate uplink status #1481

✨ Support propagate uplink status #1481

Uh oh!

Conversation

lentzi90 commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 17, 2023

Uh oh!

netlify bot commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Uh oh!

lentzi90 commented Feb 17, 2023

Uh oh!

lentzi90 commented Feb 17, 2023

Uh oh!

lentzi90 commented Feb 17, 2023

Uh oh!

mdbooth commented Feb 17, 2023

Uh oh!

lentzi90 commented Feb 17, 2023

Uh oh!

jichenjc commented Feb 18, 2023

Uh oh!

lentzi90 commented Mar 8, 2023

Uh oh!

jichenjc commented Mar 9, 2023

Uh oh!

lentzi90 commented Mar 10, 2023

Uh oh!

jichenjc commented Mar 10, 2023

Uh oh!

lentzi90 Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

mdbooth Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

lentzi90 Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

lentzi90 Apr 14, 2023

Choose a reason for hiding this comment

Uh oh!

mdbooth commented Apr 17, 2023

Uh oh!

mdbooth commented Apr 17, 2023

Uh oh!

Uh oh!

Uh oh!

dulek commented Apr 19, 2023

Uh oh!

mdbooth commented Apr 20, 2023

Uh oh!

jichenjc commented Apr 20, 2023

Uh oh!

k8s-ci-robot commented Apr 20, 2023

Uh oh!

mdbooth commented Apr 21, 2023

Uh oh!

seanschneeweiss left a comment

Choose a reason for hiding this comment

Uh oh!

mdbooth commented Apr 24, 2023

Uh oh!

mdbooth commented Apr 24, 2023

Uh oh!

Uh oh!

lentzi90 commented Feb 17, 2023 •

edited

Loading

netlify bot commented Feb 17, 2023 •

edited

Loading