✨ Enable setting QoSPolicy on ports #2800

nikParasyr · 2025-10-26T15:38:57Z

What this PR does / why we need it:
Enable users to set QoSPolicyID on ports. This can be done by either defining the ID or a filter to query the QoS Policy.

Which issue(s) this PR fixes:
Fixes #2672

Special notes for your reviewer:

Chose to add both ID and Filter to keep UX consistent with other fields
~~I have not added e2e tests, did a quick look and qos extension doesnt seem enabled. Let me know if this is required~~

TODOs:

squashed commits
if necessary:
- includes documentation
- adds unit tests

/hold

netlify · 2025-10-26T15:39:02Z

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

Name	Link
🔨 Latest commit	`976b0a3`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-cluster-api-openstack/deploys/6915f39c52701a0008b01bbc
😎 Deploy Preview	https://deploy-preview-2800--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 project configuration.

k8s-ci-robot · 2025-10-26T15:39:04Z

[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 cecilerobertmichon for approval. For more information see the 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 · 2025-10-26T15:39:06Z

Hi @nikParasyr. Thanks for your PR.

I'm waiting for a github.com 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.

lentzi90 · 2025-10-29T11:21:06Z

/ok-to-test

docs/book/src/clusteropenstack/configuration.md

api/v1beta1/types.go

bnallapeta · 2025-11-10T13:27:39Z

pkg/cloud/services/networking/port.go

 		builder = portSecurityOpts
 	}

+	if portSpec.QoSPolicyID != nil {


this only works in the Create scenario. There is no handling for Update cases.

There is already an existing ensurePortTagsAndTrunk function that handles the update scenario. But that takes care of just the port tags and trunk. I think we should modify this to handle the update cases for QoS as well.
Rename it to something like ensurePortProperties and add QoS as well.

Updated to also handle the Update scenario. Also added a unit test for that use case.
Question: Do I also need to update webhooks to allow mutations for qosPolicy? and if so are there some pointers for that?

pkg/cloud/services/networking/port.go

bnallapeta · 2025-11-10T13:36:25Z

api/v1beta1/types.go


+	// QoSPolicyID is the ID of the qos policy the port will use.
+	// +optional
+	QoSPolicyID *string `json:"qosPolicyID"`


omitempty missing?

indeed. fixed

nikParasyr

@bnallapeta thanks for the review.
Your suggestions were implemented, have a minor question on the update comment.
Moreover, I updated ci and e2e to cover this case ( decided to add it on the existing multi-network scenario since it allows to test various ways of setting qos policy and didnt want to make another scenario just for this )

pkg/cloud/services/networking/port.go

nikParasyr · 2025-11-11T15:01:58Z

pkg/cloud/services/networking/port.go

 		builder = portSecurityOpts
 	}

+	if portSpec.QoSPolicyID != nil {


Updated to also handle the Update scenario. Also added a unit test for that use case.
Question: Do I also need to update webhooks to allow mutations for qosPolicy? and if so are there some pointers for that?

bnallapeta · 2025-11-12T06:04:10Z

@bnallapeta thanks for the review. Your suggestions were implemented, have a minor question on the update comment. Moreover, I updated ci and e2e to cover this case ( decided to add it on the existing multi-network scenario since it allows to test various ways of setting qos policy and didnt want to make another scenario just for this )

Awesome! thanks for this. I have replied on that comment. Looks good overall.

bnallapeta · 2025-11-12T06:04:14Z

/lgtm

bnallapeta · 2025-11-12T06:05:14Z

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

k8s-ci-robot · 2025-11-12T08:19:10Z

New changes are detected. LGTM label has been removed.

nikParasyr · 2025-11-12T08:19:54Z

the e2e tests fails on the multi-network one. looking into it but it might take a while

nikParasyr · 2025-11-12T11:05:34Z

@bnallapeta / @lentzi90 can i get some help on where to look/troubleshoot?

The 1st control-plane node doesnt come up.

What i have checked this far:

qos on devstack seems to be configured correct:

i can see it in the cloud-init logs, and various ml2 extensions are added correctly automatically
qos policy gets created ( grep controller devstack logs for qos-policy => shows the creation + couple of gets )
the checks on extension on the code dont fail

qosPolicyID gets resolved properly:

no matter if i pass it by id/filter.name/filter.shared the OpenStackServer.status.resolved of the control-plane machine have the correct qosPolicyID

Only 1 port gets created, i can see that both on logs and ports.json:

i can see that it contains qos_policy in the request body ( and no err returned )
not sure why only 1 port gets created and not 3

The OpenStackServer for the control-plane doesnt get created:

the status is .status.Ready: false
it is not dumped on servers.json
i cannot find any logs on the controller or worker devstack => Given that I have the port ID and server name I would expect that i can at least find the request but i cant
k-orc logs are completely empty

It sort of feels to me that this might be k-orc for some reason not creating the server, but not sure if im correct in this and why it wouldn't. (do we need downstream support for QoS on k-orc maybe? while working on this it didnt feel like that but maybe im wrong)

Any insights/help would be much appreciated

bnallapeta · 2025-11-13T01:45:40Z

@nikParasyr curious.. why did you comment out the qos here? I think the first time the e2e ran on this PR, these were enabled yes? And still the same failure?

Also, instead of using shared, how about we give the same policy for all ports and see if that works. I'm thinking that if there are multiple policies and for some reason, the policy resolution becomes an issue, the port won't come up and that causes the whole thing to fail. Perhaps you can test this locally.

I will try to find some time tomorrow to look into this deeply.

Enable users to set QoSPolicyID on ports. This can be done by either defining the ID or a filter to query the QoS Policy

Ensure that the qos extension is enabled on the openstack deployment before creating ports with QoS policy set

Allow the QoSPolicyID of a port to be updated when different from spec.

lentzi90 · 2025-11-13T11:08:05Z

The missing logs from ORC (and also very minimal from CAPO) is strange and disturbing. Maybe it is because the test gets interrupted and fails to cleanup properly?

Regarding ORC, we are actually not using it for the OpenStackServers yet 🙁
See #2814.
This is also an issue because we are now introducing changes in CAPO that diverges from ORC. You will probably need to check and think about how to implement this in ORC as well, or instead of CAPO.

For progressing here though, I think we need to figure out why we are not getting proper logs from CAPO and fix that. Then it will be clear why the server is not created. Maybe try focusing on only the multi-network test? It will be easier (and faster) to see what is going on.

hack/ci/cloud-init/controller.yaml.tpl

pkg/cloud/services/networking/qospolicy.go

pkg/cloud/services/networking/qospolicy_test.go

lentzi90 · 2025-11-13T11:20:50Z

pkg/cloud/services/networking/qospolicy_test.go

+
+	"github.com/go-logr/logr/testr"
+	"github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/qos/policies"
+	. "github.com/onsi/gomega" //nolint:revive


Is the nolint really needed? We should be covered by the config here:

cluster-api-provider-openstack/.golangci.yml

Lines 163 to 166 in ee4be02

- linters:

- revive

- staticcheck

path: (test)/.*.go

or if we are not covered by that we should update it to cover

nikParasyr · 2025-11-13T15:01:17Z

@bnallapeta

why did you comment out the qos here?

This is on one of my later runs to see if i still get the same problems with 1 port with qosPolicy instead of all 3. I have made ci runs with either defining the same qosPolicy.id on all 3 ports or with the uncommented lines => they all fail the same way. History of runs is here: https://prow.k8s.io/pr-history/?org=kubernetes-sigs&repo=cluster-api-provider-openstack&pr=2800

I think the first time the e2e ran on this PR, these were enabled yes? And still the same failure?

Initially e2e tests were not added, i added them after tackling your input ( validating qos extension is enabled etc ). And since then they consistently fail.

Also, instead of using shared, how about we give the same policy for all ports and see if that works. I'm thinking that if there are multiple policies and for some reason, the policy resolution becomes an issue, the port won't come up and that causes the whole thing to fail.

I am passing the same policy on all 3 ports, just with different ways ( to test that the filter works as expected ). On the openstackServer.Status.Resolved i can see that all 3 are resolved properly to the same qosPolicyID.

@lentzi90

The missing logs from ORC (and also very minimal from CAPO) is strange and disturbing. Maybe it is because the test gets interrupted and fails to cleanup properly?

I have noticed in general that orc doesn't create many logs ( we had servers fail due to quotas multiple times and orc logs are silent -- probably because as you mention below k-orc is still not used by capo for servers... ).

Regarding ORC, we are actually not using it for the OpenStackServers yet 🙁
See #2814.
This is also an issue because we are now introducing changes in CAPO that diverges from ORC. You will probably need to check and think about how to implement this in ORC as well, or instead of CAPO.

Ok this is good to know. If this is to happen, its probably better to wait for it and implement it once in k-orc ( and apis etc here ), instead of doing it now on capo, then on k-orc and patching capo again. from my side this doesn't have the highest priority atm and can wait for a bit for the migration to occur. I'll check a bit more in-depth the related ticket and k-orc development (maybe i can start implementing qos there and we meet in the middle :P )

For progressing here though, I think we need to figure out why we are not getting proper logs from CAPO and fix that. Then it will be clear why the server is not created. Maybe try focusing on only the multi-network test? It will be easier (and faster) to see what is going on.

So this PR on the last commit has multiple runs (because i was amending ) https://prow.k8s.io/pr-history/?org=kubernetes-sigs&repo=cluster-api-provider-openstack&pr=2800. All of them fail on the multi-network (which is the only e2e that i "touched"). CAPO logs there are consistently empty (other than the first 50 lines of starting controllers etc). Unless im checking on the wrong place

Update the e2e tests to add coverage for port with QoS policy set. For this: - enable q-qos neutron plugin on ci devstack - update multi-network scenario to add qos policies using ID and filter

k8s-ci-robot · 2025-11-13T15:25:14Z

@nikParasyr: 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-openstack-test	`976b0a3`	link	true	`/test pull-cluster-api-provider-openstack-test`
pull-cluster-api-provider-openstack-e2e-test	`976b0a3`	link	true	`/test pull-cluster-api-provider-openstack-e2e-test`

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

github-project-automation bot added this to CAPO Roadmap Oct 26, 2025

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2025

github-project-automation bot moved this to Inbox in CAPO Roadmap Oct 26, 2025

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 26, 2025

k8s-ci-robot requested review from EmilienM and lentzi90 October 26, 2025 15:39

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 26, 2025

nikParasyr force-pushed the qos_policy branch 2 times, most recently from 4ca468e to c2eba8d Compare October 29, 2025 08:52

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 Oct 29, 2025

nikParasyr force-pushed the qos_policy branch from c2eba8d to 1e0eee1 Compare October 30, 2025 09:18

bnallapeta reviewed Nov 10, 2025

View reviewed changes

nikParasyr commented Nov 11, 2025

View reviewed changes

nikParasyr force-pushed the qos_policy branch from 6681eff to c89012d Compare November 11, 2025 16:16

k8s-ci-robot assigned bnallapeta Nov 12, 2025

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

nikParasyr force-pushed the qos_policy branch from c89012d to 05a76a1 Compare November 12, 2025 08:19

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2025

nikParasyr force-pushed the qos_policy branch from 05a76a1 to b2f2c39 Compare November 12, 2025 10:00

nikParasyr added 3 commits November 13, 2025 09:57

Enable setting QoSPolicy on ports

a9a2503

Enable users to set QoSPolicyID on ports. This can be done by either defining the ID or a filter to query the QoS Policy

Ensure qos extension is enabled

8aa3709

Ensure that the qos extension is enabled on the openstack deployment before creating ports with QoS policy set

Allow updates on port.QoSPolicyID

b3aea05

Allow the QoSPolicyID of a port to be updated when different from spec.

nikParasyr force-pushed the qos_policy branch from b2f2c39 to b792dff Compare November 13, 2025 08:57

lentzi90 reviewed Nov 13, 2025

View reviewed changes

nikParasyr force-pushed the qos_policy branch from b792dff to 4a17c00 Compare November 13, 2025 15:03

Update e2e tests with QoS policy

976b0a3

Update the e2e tests to add coverage for port with QoS policy set. For this: - enable q-qos neutron plugin on ci devstack - update multi-network scenario to add qos policies using ID and filter

nikParasyr force-pushed the qos_policy branch from 4a17c00 to 976b0a3 Compare November 13, 2025 15:04

✨ Enable setting QoSPolicy on ports #2800

Are you sure you want to change the base?

✨ Enable setting QoSPolicy on ports #2800

Conversation

nikParasyr commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Oct 26, 2025 • 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!

k8s-ci-robot commented Oct 26, 2025

Uh oh!

k8s-ci-robot commented Oct 26, 2025

Uh oh!

lentzi90 commented Oct 29, 2025

Uh oh!

Uh oh!

Uh oh!

bnallapeta Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

nikParasyr Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bnallapeta Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

nikParasyr Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

nikParasyr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nikParasyr Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

bnallapeta commented Nov 12, 2025

Uh oh!

bnallapeta commented Nov 12, 2025

Uh oh!

bnallapeta commented Nov 12, 2025

Uh oh!

k8s-ci-robot commented Nov 12, 2025

Uh oh!

nikParasyr commented Nov 12, 2025

Uh oh!

nikParasyr commented Nov 12, 2025

Uh oh!

bnallapeta commented Nov 13, 2025

Uh oh!

lentzi90 commented Nov 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lentzi90 Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

nikParasyr Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

nikParasyr commented Nov 13, 2025

Uh oh!

k8s-ci-robot commented Nov 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nikParasyr commented Oct 26, 2025 •

edited

Loading

netlify bot commented Oct 26, 2025 •

edited

Loading