Implement Limit controller by chenwng · Pull Request #868 · k-orc/openstack-resource-controller

chenwng · 2026-07-28T13:58:40Z

This PR implements the controller for Limit in Keystone.

Close #851

dlaw4608

Hey Winston, great job on the PR!! , I left a few comments after a quick first look, I am looking into it more but just to show you what I found so far WDYT?

chenwng · 2026-07-29T13:50:02Z

Thanks @dlaw4608 for the review. I will fix them. At the same time, I am also trying to figure out the CI failures.

chenwng · 2026-07-30T11:17:42Z

After further debugging, I found two issues in the CI e2e failures.

The adhoc setup/teardown pod takes too long time to complete creating registered limit, and the long backoff of limit controller makes test cases unable to complete in time. Using prebuilt image will solve this issue, but this will bring in an image to build/maintain. So I will wait for the merge of registered limit controller and take advantage of it.
Keystone API list limit returns empty even though limits have been created successfully in OpenStack versions flamingo (stable/2025.2) and epoxy (stable/2025.1), and listing with limit id does return the limit. No such an issue was found in version gazpacho (stable/2026.1). Here is the test result in my local env with version flamingo.

Limits can be created successfully.

NAMESPACE   NAME                                               ID                                 AVAILABLE   RESOURCE             LIMIT   POLICY    MESSAGE
default     limit.openstack.k-orc.cloud/limit-import-error-1   578222e588f74fb4921d5d561c88b560   True        limit-import-error   50      managed   OpenStack resource is up to date
default     limit.openstack.k-orc.cloud/limit-import-error-2   aab77ebf7ce7447d85d12cac91526dcc   True        limit-import-error   60      managed   OpenStack resource is up to date

openstack limit list returns nothing. However, openstack limit show limit-id returns the limit correctly.

$openstack limit list 


$openstack limit show 578222e588f74fb4921d5d561c88b560 
+----------------+-----------------------------------------------------------------+
| Field          | Value                                                           |
+----------------+-----------------------------------------------------------------+
| description    | Limit from "import error" test for project-limit-import-error-1 |
| domain_id      | None                                                            |
| id             | 578222e588f74fb4921d5d561c88b560                                |
| project_id     | ab9e509176894c408707611de07fb88c                                |
| region_id      | None                                                            |
| resource_limit | 50                                                              |
| resource_name  | limit-import-error                                              |
| service_id     | 5924b6f0c7c840faa46450ff73b1a1a2                                |
+----------------+-----------------------------------------------------------------+

$openstack limit show aab77ebf7ce7447d85d12cac91526dcc
+----------------+-----------------------------------------------------------------+
| Field          | Value                                                           |
+----------------+-----------------------------------------------------------------+
| description    | Limit from "import error" test for project-limit-import-error-2 |
| domain_id      | None                                                            |
| id             | aab77ebf7ce7447d85d12cac91526dcc                                |
| project_id     | 562d74e5088941068fd04c4e93da32e5                                |
| region_id      | None                                                            |
| resource_limit | 60                                                              |
| resource_name  | limit-import-error                                              |
| service_id     | 5924b6f0c7c840faa46450ff73b1a1a2                                |
+----------------+-----------------------------------------------------------------+

This makes all limit import related test cases unable to pass in flamingo and epoxy envs.

chenwng · 2026-07-31T09:09:23Z

I added a step in e2e workflow to skip the limit import test cases for openstack versions other than gazpacho.

winiciusallan

Hey @chenwng, I did a first round of review, but I need to take a look at the rest. Thanks for your PR.

winiciusallan · 2026-08-03T19:36:13Z

 | group                       |         |    ✔    |     ✔    |
 | image                       |    ✔    |    ✔    |     ✔    |
 | keypair                     |         |    ◐    |     ◐    |
+| limit                       |         |         |     ◐    |


When we have RegionRef on this controller, I believe we will be pretty much done.

Yes. After Region controller is merged, we can add the RegionRef field.

We have the Region controller now, we can now add RegionRef. However, don't feel pressured to add support for it in this PR, we can do with a follow-up.

winiciusallan · 2026-08-03T20:04:33Z

+		return nil, progress.WrapError(err)
+	}
+
+	logger.Info("limit created", "createOpts", createOpts)


Let's remove this. Maybe you have put for debug purposes.

I added it during debugging the CI issue and it was because I wasn't able to tell if the limit actually was created. And after debugging, I thought given it is informational and helps understand whether the controller is working normally, so I left it.

I understand there are other general places printing logs which would tell whether a resource has been created successfully after reconciling, such as Reconcile successful, but I would argue some concret log confirming it would help better when checking the log for either information only or troubleshooting.

Let's remove this (it's not consistent with other controller).

Also, if we add logging, we should use the project conventions: https://k-orc.cloud/development/coding-standards/#logging

I will remove it although I would still suggest adding more necessary log prints at critical locations in general given how frustrated I was when troubleshooting issues from other projects without sufficient logs 😄

winiciusallan · 2026-08-03T20:30:00Z

+	// There seems to be a bug that keystone doesn't clear the description field when receiving a PATCH request with an empty description.
+	// This will cause the `Progressing` condition stuck with `Resource status will be refreshed`.
+	// Tested with `openstack limit set --description ""`
+	handleDescriptionUpdate(&updateOpts, resource, osResource)
+	// The same issue exists with resourceLimit. Updating resourceLimit with 0 doesn't work.
+	handleResourceLimitUpdate(&updateOpts, resource, osResource)


Indeed it looks like something on Keystone's side. Keystone doesn't differentiate description unset and empty string. I think this is something that we could address in upstream if you would like to give it a try.

https://github.com/openstack/keystone/blob/30ef2ffa65a3486ef882f00538e20f2253c57d4c/keystone/limit/backends/sql.py#L285-L292

Thanks for pointing to the root cause. I am not quite familiar with the implementation of keystone. I will put it in low priority and probably give it a try in the future when I have time.

gndrmnn · 2026-09-03T12:06:52Z

@chenwng FYI, RegisteredLimit has been merged

chenwng · 2026-09-03T13:22:02Z

@gndrmnn thanks. I will update this PR accordingly.

mandre · 2026-09-09T07:44:18Z

 | group                       |         |    ✔    |     ✔    |
 | image                       |    ✔    |    ✔    |     ✔    |
 | keypair                     |         |    ◐    |     ◐    |
+| limit                       |         |         |     ◐    |


We have the Region controller now, we can now add RegionRef. However, don't feel pressured to add support for it in this PR, we can do with a follow-up.

mandre · 2026-09-09T07:50:39Z


    - name: Run e2e tests
      run: |
+        if [[ "${{ matrix.name }}" != "gazpacho" ]]; then


This seems very fragile. Don't we have a better way to skip tests? Perhaps we need to improve kuttl (in my experience, they're very receptive).

I was considering using labels, but then it seems I will need to update all cases with labels. If kubettl has something like --skip-labels, it will be a good fit for this case.

mandre · 2026-09-09T08:27:41Z

+    spec:
+      containers:
+        - name: setup-teardown
+          image: ghcr.io/chenwng/orc-helper:latest


What is this container image about, why do we need it?

I suspect the cleanup can be executed via CRD operations, like we do in the role tests for example.

Also we can't run an image from a personal registry, especially one tagged with latest for obvious supply chain dependency reasons.

It was used to create RegisteredLimits during case setup and delete RegisteredLimits and disable Domains during teardown before RegisteredLimit was merged. I will update it since RegisteredLimit has been merged.

We can use steps to do cleanup for things like disabling Domains. However, I suspect in case there is any failure before these cleanup steps, they will not be executed and the Domains will get stuck. Ideally, kuttl should have something like pre-crd/resource-delete step/hook to allow some user cleanup actions which can always run no matter if the case fails or not. I didn't find such a feature in kuttl, or maybe I missed it.

Agreed that it would be nice if kuttl had something like a cleanup step that runs unconditionally of previous steps success. Another possible enhancement to the project 😉

Removed the setup/cleanup deployments. I will find some time for the kuttl enhancements for teardown step and --skip-labels.

fwiw, kuttl maintainers are very receptive and I've been having a good experience. I've been working on this pull request which adds some conditional deletion of resources when a case fail or succeed. You could re-use some of the structures in your enhacements, it'd be nice to have it 😄.

mandre

LGTM overall. Almost ready to merge.

mandre · 2026-09-14T07:28:07Z

+// +kubebuilder:validation:XValidation:rule="!(has(self.projectRef) && has(self.domainRef))",message="projectRef and domainRef are mutually exclusive"
+type LimitResourceSpec struct {
+	// description is a human-readable description for the resource.
+	// Note, currently, there is an issue with keystone that it fails to clear the description field


Is the keystone bug proper, or the gophercloud implementation of it (i.e. missing field pointer)? Do we have a bug report?

It is a bug from keystone. It can be reproduced with openstack cli, so it is not an issue of gophercloud. @winiciusallan has helped locate the issue https://github.com/k-orc/openstack-resource-controller/pull/868/changes#r3707450414 . I haven't reported it yet.

mandre · 2026-09-14T08:56:30Z

+		return nil, progress.WrapError(err)
+	}
+
+	logger.Info("limit created", "createOpts", createOpts)


Let's remove this (it's not consistent with other controller).

Also, if we add logging, we should use the project conventions: https://k-orc.cloud/development/coding-standards/#logging

winiciusallan · 2026-09-14T15:56:55Z

+      openstack registered limit list --resource-name limit-create-minimal-1 &&
+      openstack registered limit list --resource-name limit-create-minimal-2


Would you mind explaining why you are listing the registered limits?

This was from the previous to confirm they have been created successfully before RegisteredLimit controller was merged. Now, we don't need it anymore. I will remove it.

Oh, so in that case you might want to remove the others as well

e.g. https://github.com/k-orc/openstack-resource-controller/pull/868/changes#diff-71007d2d6a6218983bde41c00763080746936426ffe229990ddbe993d5c5b89eR113-R114

edit:

> grep -rn "registered limit list" internal/ internal/controllers/limit/tests/limit-create-full/00-assert.yaml:87: openstack registered limit list --resource-name limit-create-full-1 && internal/controllers/limit/tests/limit-create-full/00-assert.yaml:88: openstack registered limit list --resource-name limit-create-full-2 internal/controllers/limit/tests/limit-create-minimal/00-assert.yaml:87: openstack registered limit list --resource-name limit-create-minimal-1 && internal/controllers/limit/tests/limit-create-minimal/00-assert.yaml:88: openstack registered limit list --resource-name limit-create-minimal-2 internal/controllers/limit/tests/limit-dependency/01-assert.yaml:100: openstack registered limit list --resource-name limit-dependency-1 && internal/controllers/limit/tests/limit-dependency/01-assert.yaml:101: openstack registered limit list --resource-name limit-dependency-2 && internal/controllers/limit/tests/limit-dependency/01-assert.yaml:102: openstack registered limit list --resource-name limit-dependency-3 internal/controllers/limit/tests/limit-import-dependency/01-assert.yaml:73: openstack registered limit list --resource-name limit-import-dependency internal/controllers/limit/tests/limit-import-error/00-assert.yaml:98: openstack registered limit list --resource-name limit-import-error internal/controllers/limit/tests/limit-import/00-assert.yaml:70: openstack registered limit list --resource-name limit-import && internal/controllers/limit/tests/limit-import/00-assert.yaml:71: openstack registered limit list --resource-name limit-import-nonexistent-resource internal/controllers/limit/tests/limit-update/00-assert.yaml:72: openstack registered limit list --resource-name limit-update

@chenwng Just making sure you've seen Winicius' comment above ⬆️

Thanks. Just removed all of them.

winiciusallan

I'm happy with this. Thanks for your amazing contribution @chenwng!

Before merging, and if this won't bother you, could you squash your commits? There are a few addressing-review commits that might fall into cracks in our history. We usually have:

One commit with the controller scaffolding, having the run command in its description.
One or more commits with the implementation itself. Here I believe you can use your best judgement

go run ./cmd/scaffold-controller -interactive=false \ -kind=Limit \ -gophercloud-client=NewIdentityV3 \ -gophercloud-module=github.com/gophercloud/gophercloud/v2/openstack/identity/v3/limits \ -gophercloud-type=Limit \ -openstack-json-object=limit \ -available-polling-period=0 \ -deleting-polling-period=0 \ -required-create-dependency=Service \ -optional-create-dependency=Project \ -optional-create-dependency=Domain \ -import-dependency=Service \ -import-dependency=Project \ -import-dependency=Domain

github-actions Bot added the semver:major Breaking change label Jul 28, 2026

chenwng force-pushed the limit-controller branch 2 times, most recently from e556212 to 0b8d70e Compare July 29, 2026 11:06

dlaw4608 reviewed Jul 29, 2026

View reviewed changes

Comment thread internal/controllers/limit/actuator.go Outdated

dlaw4608 reviewed Jul 29, 2026

View reviewed changes

Comment thread internal/controllers/limit/actuator.go Outdated

dlaw4608 reviewed Jul 29, 2026

View reviewed changes

Comment thread internal/controllers/limit/actuator.go Outdated

Comment thread internal/controllers/limit/actuator.go Outdated

Comment thread internal/controllers/limit/tests/utils/main.go Outdated

chenwng force-pushed the limit-controller branch from 0b8d70e to b41c2e4 Compare July 29, 2026 13:53

chenwng force-pushed the limit-controller branch from 3540f8b to 60bb5b3 Compare July 30, 2026 19:42

winiciusallan reviewed Aug 3, 2026

View reviewed changes

mandre reviewed Sep 9, 2026

View reviewed changes

chenwng force-pushed the limit-controller branch from 2da7215 to c089c09 Compare September 11, 2026 15:46

mandre reviewed Sep 14, 2026

View reviewed changes

winiciusallan reviewed Sep 14, 2026

View reviewed changes

winiciusallan reviewed Sep 15, 2026

View reviewed changes

chenwng added 2 commits September 15, 2026 14:27

Implement Limit controller

4b96d4e

chenwng force-pushed the limit-controller branch from b114351 to 4b96d4e Compare September 15, 2026 13:39

winiciusallan approved these changes Sep 15, 2026

View reviewed changes

winiciusallan added this pull request to the merge queue Sep 15, 2026

Merged via the queue into k-orc:main with commit 15fe873 Sep 15, 2026
12 checks passed

		openstack registered limit list --resource-name limit-create-minimal-1 &&
		openstack registered limit list --resource-name limit-create-minimal-2

Conversation

chenwng commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dlaw4608 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chenwng commented Jul 29, 2026

Uh oh!

chenwng commented Jul 30, 2026

Uh oh!

chenwng commented Jul 31, 2026

Uh oh!

winiciusallan left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Uh oh!

gndrmnn commented Sep 3, 2026

Uh oh!

chenwng commented Sep 3, 2026 • 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!

chenwng Sep 9, 2026 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenwng Sep 9, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mandre left a comment

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!

chenwng commented Jul 28, 2026 •

edited

Loading

chenwng commented Sep 3, 2026 •

edited

Loading

chenwng Sep 9, 2026 •

edited

Loading

chenwng Sep 9, 2026 •

edited

Loading

winiciusallan Sep 14, 2026 •

edited

Loading