Unify common code by amartyasinha · Pull Request #1130 · openstack-k8s-operators/nova-operator

amartyasinha · 2026-06-18T08:33:09Z

Introduce internal/common for shared nova/placement
controller code: reconciler base, ensure helpers,
config generation, watch mappers, Kolla constants,
and static errors. Nova and placement controllers
call these helpers; nova-only logic stays in
internal/controller/nova/common.go.

This is a move-first refactor: logic, comments, and
naming are preserved where possible. Only
unavoidable mechanical changes are included (e.g.
exported names for cross-package use, import
updates, r.Client.Update because Client is a named
field on ReconcilerBase).

Co-authored-by: Cursor cursoragent@cursor.com

openshift-ci · 2026-06-18T08:33:13Z

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

SeanMooney · 2026-06-22T11:50:24Z

-}
-
 // ReconcilerBase provides a common set of clients scheme and loggers for all reconcilers.
 type ReconcilerBase struct {


so ReconcilerBase is intened to be the thing that hold the common code

so you shoudl be moving this as well if your going to do this.

the orginal inte was for this to become the common bas for the nova placment and evnetully cyborg contolers.

Thanks @SeanMooney! Sure, I'll be moving ReconcilerBase soon to common code. I had it in the initial draft, but removed it because it was getting overwhelming for me to self-review. Once I believe the current code I'm moving are well-placed, I'll move ReconcilerBase as well.

I've tried moving ReconcilerBase and other related func to internal/common. PTAL.

Hey @SeanMooney! Sorry for the ping, but I'll appreciate if you could review the PR.

mrkisaolamb

Something else we can try: unifying FindObjectsForSrcByField and findObjectsWithAppSelectorLabelInNamespace

mrkisaolamb

looks good

openshift-ci · 2026-07-01T06:26:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amartyasinha, mrkisaolamb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [amartyasinha,mrkisaolamb]

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

amoralej

PR looks mostly good to me. Some coments in line.

One of the intermediate commits does not build. Althoug the PR is fine, having unbuildable commits is undesired IMO.
The impelementation of the new common watcher may be simplified. I'm proposing an approach. Current implementation works.
The rest of comments are mostly nits.

Thanks for splitting the PR in commits, it'd be much harder to review otherwise!

amoralej · 2026-06-29T10:29:43Z

 				Secret: &corev1.SecretVolumeSource{
 					DefaultMode: &configMode,
-					SecretName:  name + "-config-data",
+					SecretName:  internalcommon.GetServiceConfigSecretName(name),


This patch left hardcoded secret name in

nova-operator/internal/controller/placement/api_controller.go

Line 1525 in 5acd3e5

Name: fmt.Sprintf("%s-config-data", instance.Name),

but it's cleaned later, when commonizing GenerateConfigs so no problem.

Ack! Will leave as it is since it got cleaned up later.

amoralej · 2026-07-02T08:31:26Z


 	if !ok {
-		return fmt.Errorf("%w but got %T", errExpectedNovaAPIObject, obj)
+		return fmt.Errorf("expected a NovaAPI object but got %T: %w", obj, internalcommon.ErrUnexpectedObjectType)


This change is changing Error types from specific to generic ErrUnexpectedObjectType and slightly changing error text by adding unnexpected object type at the end. I haven't found any code checking error types and error message change seems fine..

Yes, the reason was to keep things simple. So, we changed Error types from specific to generic, since we were not checking those specific Error type later in our code.

amoralej · 2026-07-02T08:58:27Z

this and another couple of errors were left in commit e655c73 . This commit fails to build:

go vet ./... # github.com/openstack-k8s-operators/nova-operator/internal/controller/placement internal/controller/placement/api_controller.go:1019:12: r.List undefined (type *PlacementAPIReconciler has no field or method List) internal/controller/placement/api_controller.go:1051:11: r.List undefined (type *PlacementAPIReconciler has no field or method List) # github.com/openstack-k8s-operators/nova-operator/internal/controller/nova internal/controller/nova/common.go:34:2: "k8s.io/apimachinery/pkg/runtime" imported and not used make: *** [Makefile:131: vet] Error 1

While this has been fixed on the following commit in the PR, it's not a good practice, all commits should be buildable,

Thanks @amoralej ++ for catching this. Yes, I agree that all inidividual commits should be buildable. I was taking care of it, but seems like during the last regrouping of commits, it got messed up.

Anyways, it's been fixed now.

[AI-Generated] Verified end to end. Results: Final tree unchanged ┌────────────────────┬──────────────────────────────────────────┐ │ Reference │ Tree hash │ ├────────────────────┼──────────────────────────────────────────┤ │ Old tip (096833f2) │ a780cab3467aefac63c0dbfd81d93dbeedb7c96d │ ├────────────────────┼──────────────────────────────────────────┤ │ New tip (2aaf127d) │ a780cab3467aefac63c0dbfd81d93dbeedb7c96d │ └────────────────────┴──────────────────────────────────────────┘ git diff 096833f2^{tree} HEAD^{tree} is empty — file content at the tip is identical to before the regroup. Only commit hashes changed. Every commit builds on its own Checked each of the 8 commits from 2085b0cf..HEAD individually: • go vet ./... • go vet ./api/... • go build -o /dev/null ./... • make vet (same as CI) All passed on every commit:

amoralej · 2026-07-02T08:59:47Z

 	appsv1 "k8s.io/api/apps/v1"
-	corev1 "k8s.io/api/core/v1"
 	k8s_errors "k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/runtime"


Should be part of the previous commit.

amoralej · 2026-07-02T09:00:31Z

 			Namespace:     src.GetNamespace(),
 		}
-		err := r.List(ctx, crList, listOps)
+		err := r.Client.List(ctx, crList, listOps)


Should be part of the previous patch.

Done. Thanks

amoralej · 2026-07-02T10:50:52Z


+.PHONY: gotest-unit
+gotest-unit: ## Run unit tests under test/unit/.
+	go test ./test/unit/...


adding -v would be nice to make sure all tests are being executed.

go test -v ./test/unit/...

Thanks! done.

amoralej · 2026-07-02T10:51:36Z

 # TODO: Currently runs all tests (Nova + Placement). In future, optimize CI to run only tests
 # for the operator code that changed (e.g., skip Placement tests if only Nova code changed).
-test: manifests generate fmt vet envtest ginkgo ## Run tests.
+test: manifests generate fmt vet envtest gotest-unit ginkgo ## Run tests.


We rely mostly in envtest functional test in service operators, but I think using unit tests for this common libraries is a good addition, thanks!

I think this is running unit tests twice as the ginkgo command below on ./test/ already runs these tests.

Thanks for the catch! I've removed dedicated gotest-unit from test target, as ginkgo will already run them.

amoralej · 2026-07-02T11:21:21Z

+// placement uses EnsureSecret for the OpenStack password secret referenced in
+// PlacementAPI.Spec.Secret with PasswordSelectors.Service defaulting to
+// PlacementPassword.
+func TestEnsureSecret_placementMissingSecret(t *testing.T) {


Nit: It only validates the NotFound case which has specific logic. For other errors in eader.Get(ctx, secretName, secret) it's does not have coverage.

Added more coverage. PTAL

amoralej · 2026-07-02T11:23:04Z

+	ctrl "sigs.k8s.io/controller-runtime"
+)
+
+func TestEnsureNetworkAttachments_noAttachments(t *testing.T) {


Nit: similar to ensureSecret, missing coverage for other errors than NotFound.

Added more coverage. PTAL

amoralej · 2026-07-02T12:04:12Z

+	newList func() L,
+	getItems func(L) S,


There may be a simpler way to simplify this by passing a single parameter NewList. AI proposes to use meta.ExtractList(crList) so that only the first parameter is needed. i.e.

// FindObjectsForSrcByField returns reconcile requests for CRs in src's namespace // that reference src via one of the indexed watchFields. -func FindObjectsForSrcByField[L client.ObjectList, E any, S ~[]E]( +func FindObjectsForSrcByField( ctx context.Context, log logr.Logger, reader client.Reader, src client.Object, watchFields []string, - newList func() L, - getItems func(L) S, + newList func() client.ObjectList, ) []reconcile.Request { - requests := []reconcile.Request{} + var requests []reconcile.Request for _, field := range watchFields { crList := newList() @@ -54,23 +55,25 @@ func FindObjectsForSrcByField[L client.ObjectList, E any, S ~[]E]( return requests } - requests = appendRequestsForObjects(log, src, requests, itemsAsObjects(getItems(crList))) + items, err := meta.ExtractList(crList) + if err != nil { + log.Error(err, fmt.Sprintf("extracting items from %s", crList.GetObjectKind().GroupVersionKind().Kind)) + return requests + } + requests = appendRequestsForObjects(log, src, requests, items) } return requests }

Additional changes are needed in appendRequestsForObjects.

func appendRequestsForObjects( log logr.Logger, src client.Object, requests []reconcile.Request, - items []client.Object, + items []runtime.Object, ) []reconcile.Request { for _, item := range items { - log.Info(fmt.Sprintf("input source %s changed, reconcile: %s - %s", src.GetName(), item.GetName(), item.GetNamespace())) + accessor, err := meta.Accessor(item) + if err != nil { + log.Error(err, "extracting object metadata") + continue + } + log.Info(fmt.Sprintf("input source %s changed, reconcile: %s - %s", src.GetName(), accessor.GetName(), accessor.GetNamespace())) requests = append(requests, reconcile.Request{ NamespacedName: types.NamespacedName{ - Name: item.GetName(), - Namespace: item.GetNamespace(), + Name: accessor.GetName(), + Namespace: accessor.GetNamespace(), }, }, )

Also, the rest of functions can follow the same approach. so that from services call is only:

+++ b/internal/controller/placement/api_controller.go @@ -879,8 +879,7 @@ func (r *PlacementAPIReconciler) findObjectsForSrc(ctx context.Context, src clie r.Client, src, allWatchFields, - func() *placementv1.PlacementAPIList { return &placementv1.PlacementAPIList{} }, - func(l *placementv1.PlacementAPIList) []placementv1.PlacementAPI { return l.Items }, + func() client.ObjectList { return &placementv1.PlacementAPIList{} }, ) }

I'm attaching the entire change that AI proposed. At least this is passing the tests.

watchers.patch

Updated. Thanks

…et names Move shared Kolla volume and secret name helpers out of internal/nova so nova and placement deployments can reuse them.

Move shared static errors into internal/common/errors.go and update webhook and controller call sites to reference the shared definitions. For placement, replace local ApplicationCredential error vars with the shared ones and wait on a missing ApplicationCredential secret like nova does.

Extract ReconcilerBase, Manageable, Reconciler, NewReconcilerBase, SetRequeueTimeout, Reconcilers, Setup, and OverrideRequeueTimeout into internal/common. Wire nova, placement, and main to use the shared base.

Move EnsureSecret, EnsureNetworkAttachments, EnsureTopology, and ConditionUpdater into internal/common. Switch nova and placement controllers to use the shared helpers.

Remove the duplicate allSubConditionIsTrue helper from nova common; logic matches condition.Conditions.AllSubConditionIsTrue() from lib-common.

Add unit tests for EnsureSecret, EnsureNetworkAttachments, and EnsureTopology. Extend placement functional coverage for missing and incomplete input secrets.

Replace duplicated controller watch map functions with generic helpers in internal/common/watch.go. Co-authored-by: Cursor <cursoragent@cursor.com>

Share config secret generation between nova and placement controllers. Nova passes service-specific templates via novaAdditionalTemplates() in nova/common.go; placement uses GenerateConfigsWithScripts in generateServiceConfigMaps. Co-authored-by: Cursor <cursoragent@cursor.com>

centosinfra-prod-github-app · 2026-07-03T07:59:04Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://gateway-cloud-softwarefactory.apps.ocp.cloud.ci.centos.org/zuul/t/rdoproject.org/buildset/22857439f9a5439d84d7ee32a45e0383

✔️ openstack-meta-content-provider SUCCESS in 2h 28m 20s
✔️ nova-operator-kuttl SUCCESS in 52m 05s
✔️ nova-operator-tempest-multinode SUCCESS in 2h 12m 02s
❌ nova-operator-tempest-multinode-ceph FAILURE in 32m 28s

amartyasinha · 2026-07-03T07:59:55Z

recheck

openshift-ci Bot added do-not-merge/work-in-progress approved labels Jun 18, 2026

amartyasinha mentioned this pull request Jun 19, 2026

[WIP] Unify code common 2 #1132

Closed

amartyasinha force-pushed the unify_code branch 10 times, most recently from 6aab9a1 to 1ef05f9 Compare June 22, 2026 10:20

SeanMooney reviewed Jun 22, 2026

View reviewed changes

amartyasinha force-pushed the unify_code branch from 1ef05f9 to c95289d Compare June 23, 2026 06:48

mrkisaolamb requested changes Jun 23, 2026

View reviewed changes

Comment thread internal/controller/placement/api_controller.go Outdated

Comment thread internal/controller/placement/api_controller.go

Comment thread internal/common/network.go Outdated

Comment thread internal/common/errors.go

openshift-ci Bot assigned mrkisaolamb Jun 23, 2026

amartyasinha force-pushed the unify_code branch 2 times, most recently from fc8d155 to 7d82edc Compare June 23, 2026 11:34

amartyasinha requested a review from mrkisaolamb June 23, 2026 11:38

mrkisaolamb requested changes Jun 24, 2026

View reviewed changes

Comment thread internal/controller/placement/api_controller.go Outdated

Comment thread internal/controller/placement/api_controller.go Outdated

Comment thread internal/common/errors.go

Comment thread internal/controller/placement/api_controller.go

amartyasinha force-pushed the unify_code branch 5 times, most recently from 2444cca to 3a8b6c6 Compare June 24, 2026 15:22

amartyasinha requested review from SeanMooney and mrkisaolamb June 24, 2026 15:34

amartyasinha marked this pull request as ready for review June 29, 2026 03:38

openshift-ci Bot requested a review from amoralej June 29, 2026 03:38

amartyasinha changed the title ~~[WIP] Unify common code~~ Unify common code Jun 29, 2026

openshift-ci Bot removed the do-not-merge/work-in-progress label Jun 29, 2026

amartyasinha force-pushed the unify_code branch from 3a8b6c6 to 9000f33 Compare June 29, 2026 04:13

mrkisaolamb requested changes Jun 29, 2026

View reviewed changes

Comment thread internal/common/errors.go Outdated

Comment thread internal/controller/nova/common.go Outdated

Comment thread internal/common/network.go

Comment thread internal/common/topology.go

amartyasinha force-pushed the unify_code branch 4 times, most recently from 527e504 to 096833f Compare June 29, 2026 09:57

amartyasinha requested a review from mrkisaolamb June 29, 2026 10:08

mrkisaolamb approved these changes Jul 1, 2026

View reviewed changes

openshift-ci Bot added the lgtm label Jul 1, 2026

mrkisaolamb removed the lgtm label Jul 1, 2026

amoralej reviewed Jul 2, 2026

View reviewed changes

amartyasinha force-pushed the unify_code branch 2 times, most recently from 2aaf127 to e832d30 Compare July 3, 2026 04:27

amartyasinha added 6 commits July 3, 2026 10:24

Add internal/common kolla helpers for shared Kolla constants and secr…

87e7cde

…et names Move shared Kolla volume and secret name helpers out of internal/nova so nova and placement deployments can reuse them.

Move shared reconciler infrastructure to internal/common

1f8737c

Extract ReconcilerBase, Manageable, Reconciler, NewReconcilerBase, SetRequeueTimeout, Reconcilers, Setup, and OverrideRequeueTimeout into internal/common. Wire nova, placement, and main to use the shared base.

Extract shared ensure helpers into internal/common

7aaf4a2

Move EnsureSecret, EnsureNetworkAttachments, EnsureTopology, and ConditionUpdater into internal/common. Switch nova and placement controllers to use the shared helpers.

Use lib-common AllSubConditionIsTrue in nova controllers

3f7c090

Remove the duplicate allSubConditionIsTrue helper from nova common; logic matches condition.Conditions.AllSubConditionIsTrue() from lib-common.

Add unit tests for internal/common helpers

cfcdcea

Add unit tests for EnsureSecret, EnsureNetworkAttachments, and EnsureTopology. Extend placement functional coverage for missing and incomplete input secrets.

amartyasinha force-pushed the unify_code branch from e832d30 to 65af763 Compare July 3, 2026 04:59

amartyasinha and others added 2 commits July 3, 2026 10:55

Extract shared watch map helpers into internal/common

4844382

Replace duplicated controller watch map functions with generic helpers in internal/common/watch.go. Co-authored-by: Cursor <cursoragent@cursor.com>

amartyasinha force-pushed the unify_code branch from 65af763 to 2d4ed82 Compare July 3, 2026 05:29

Uh oh!

Conversation

amartyasinha commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Jun 18, 2026

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!

Uh oh!

Uh oh!

mrkisaolamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrkisaolamb left a comment • 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!

openshift-ci Bot commented Jul 1, 2026

Uh oh!

amoralej left a comment • 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!

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!

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!

amartyasinha commented Jun 18, 2026 •

edited

Loading

mrkisaolamb left a comment •

edited

Loading

amoralej left a comment •

edited

Loading