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.

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.

winiciusallan · 2026-08-03T20:37:27Z

+func validateUpdate(resource *resourceSpecT, osResource *osResourceT) error {
+	if resource.DomainRef != nil && osResource.ProjectID != "" {
+		return errInvalidDomainRefUpdate
+	}
+
+	if resource.ProjectRef != nil && osResource.DomainID != "" {
+		return errInvalidProjectRefUpdate
+	}
+
+	return nil
+}


What is your idea with this function? Are not these fields already immutable and checked during creation?

If user sets ProjectRef first, later he sets DomainRef and removes ProjectRef in the modifed yaml, this will not trigger the API validation of ProjectRef because ProjectRef is nil and not modified. Adding this validtion would block user to do such a change given we don't have a webhook avaiable.

Here is an example without this validation. After creating this Limit, I modifed the yaml with domianRef = default without projectRef. You can see below the spec and the status.resource don't match anymore.

apiVersion: openstack.k-orc.cloud/v1alpha1 kind: Limit metadata: annotations: ... finalizers: - openstack.k-orc.cloud/limit generation: 2 name: limit-managed-domain namespace: default resourceVersion: "557586" uid: 409c3ab1-91fa-4f29-b9c3-bba4e1353399 spec: cloudCredentialsRef: cloudName: openstack-admin secretName: openstack-clouds managementPolicy: managed resource: description: Managed limit domainRef: default resourceLimit: 50 resourceName: servers serviceRef: nova status: conditions: - lastTransitionTime: "2026-08-04T09:14:05Z" message: OpenStack resource is available observedGeneration: 2 reason: Success status: "True" type: Available - lastTransitionTime: "2026-08-04T09:14:05Z" message: OpenStack resource is up to date observedGeneration: 2 reason: Success status: "False" type: Progressing id: 56f2b0102ca1461dbf45e0c7558d90aa lastSyncTime: "2026-08-04T09:14:28Z" resource: description: Managed limit projectID: 6fd7df4cf061469ab27d39d6c91b25a0 resourceLimit: 50 resourceName: servers serviceID: b8e6f8f3a9264fcaa5dc99e34aea1e31

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.

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

…mit struct

Update generated resources to reflect the addition of description in Limit spec Update the OLM bundle

Fix parameters in dependencies in Limit actuator Fix default name in test utils

…to Keystone API issue Use prebuilt image to avoid slow creation of registered limits

Add `resource.ServiceRef` check in the filter for serviceDependency creation

* Set ResourceLimit in imitResourceStatus to pointer type * Update description for ResourceLimit and Description fields * Update example file config/samples/openstack_v1alpha1_limit.yaml

* Remove temporary setup/cleanup deployments * Use `RegisteredLimit` to create registered limits * Add steps to disable domains before deletion

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 added 15 commits September 11, 2026 16:44

Generate resource for Limit controller

8bf24a3

Implement scaffolded TODO for Limit controller

9c14aab

Update fields for Limit type and fix openstack apis for Limit os client

534f6e3

Implement limit actuator and status writer

103efec

Add api validation and acutator test cases and fix field rules for Li…

bbddd7b

…mit struct

Add e2e test cases for Limit controller

04d6eaf

Add description field in LimitFilter to align with RegisteredLimit

4adaa98

Add limit controller in README

a62e863

Fix warnings from golint and kubeapilinter

e9df3eb

Update generated resources to reflect the addition of description in Limit spec Update the OLM bundle

Address PR review comments

01ee012

Fix parameters in dependencies in Limit actuator Fix default name in test utils

Skip limit import tests on older OpenStack versions in e2e tests due …

0b811bc

…to Keystone API issue Use prebuilt image to avoid slow creation of registered limits

Address review comment for Limit controller

2cf6189

Add `resource.ServiceRef` check in the filter for serviceDependency creation

Address review comment for Limit controller

7219b79

* Set ResourceLimit in imitResourceStatus to pointer type * Update description for ResourceLimit and Description fields * Update example file config/samples/openstack_v1alpha1_limit.yaml

Address review comments for Limit controller

c089c09

* Remove temporary setup/cleanup deployments * Use `RegisteredLimit` to create registered limits * Add steps to disable domains before deletion

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

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenwng Aug 4, 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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chenwng commented Jul 28, 2026 •

edited

Loading

chenwng Aug 4, 2026 •

edited

Loading

chenwng commented Sep 3, 2026 •

edited

Loading

chenwng Sep 9, 2026 •

edited

Loading

chenwng Sep 9, 2026 •

edited

Loading