Integrate WatcherAPI creation and update on the main Watcher reconcile #38

amoralej · 2025-01-14T17:18:29Z

This PR integrates the creation/update/delete of WatcherAPI in the main Watcher reconcile loop. It:

Adds the relevant WatcherAPI CRD atributes into the Watcher CRD.
Adds the logic to create/update/delete WatcherAPI from the Watcher CR.
Modifty existing envtest functional tests to validate the WatcherAPI creation from the Watcher objects.
Modifies kuttl test to validate watcherapi as part of watcher CR creation.

Depends-On: openstack-k8s-operators/install_yamls#996
Depends-On: openstack-k8s-operators/ci-framework#2658

softwarefactory-project-zuul · 2025-01-14T17:18:35Z

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/openstack-k8s-operators/watcher-operator for 38,f0526823974049e7a52f8801d6268cbe8c8aa8af

This patch integrates the creation/update/delete of WatcherAPI in the main Watcher reconcile loop. It: - Adds the relevant WatcherAPI CRD atributes into the Watcher CRD. - Adds the logic to create/update/delete WatcherAPI from the Watcher CR. - Modifty existing envtest functional tests to validate the WatcherAPI creation from the Watcher objects.

After integrating watcherapi into watcher we need to validate watcherapi and the nested resources are properly created as part of the Watcher creation. Also, this patch is removing the previous kuttl scenario which created watcherapi directly without watcher top level CR.

We are defining the command in the TestAssert twice and the second one is just overriding the first one which is never executed. This patch is merging both in just one assignment which is what kuttl tests definition seems to expect.

amoralej · 2025-01-15T09:33:48Z

recheck

cescgina · 2025-01-15T14:20:47Z

/approve

openshift-ci · 2025-01-15T14:20:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cescgina

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~~ [cescgina]

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

marios · 2025-01-16T07:34:20Z

controllers/watcher_controller.go

+	return apiDeployment, op, nil
+
+}
+


will there be some removal from the watcherapi_controller.go at some point then (maybe a followup)?

not really. The only change is that we will call watcherapi from watcher as-is, not changes neede in watcherapi_controller.go afaik.

abays · 2025-01-16T10:05:54Z

controllers/watcher_controller.go

+		WatcherCommon: watcherv1beta1.WatcherCommon{
+			ServiceUser:       instance.Spec.ServiceUser,
+			PasswordSelectors: instance.Spec.PasswordSelectors,
+			MemcachedInstance: instance.Spec.MemcachedInstance,
+			NodeSelector:      instance.Spec.APIServiceTemplate.NodeSelector,
+			PreserveJobs:      instance.Spec.PreserveJobs,
+		},
+		WatcherSubCrsCommon: watcherv1beta1.WatcherSubCrsCommon{
+			ContainerImage: instance.Spec.APIContainerImageURL,
+			Replicas:       instance.Spec.APIServiceTemplate.Replicas,
+			Resources:      instance.Spec.APIServiceTemplate.Resources,
+			ServiceAccount: "watcher-" + instance.Name,
+		},


I think maybe this can be simplified/flattened. Since WatcherCommon and WatcherSubCrsCommon are inline structs in the WatcherAPISpec, you don't need to explicitly create them. Instead, you should be able to directly use their fields within WatcherAPISpec itself, i.e.

watcherAPISpec := watcherv1beta1.WatcherAPISpec{ Secret: instance.Name, // WatcherCommon fields below ServiceUser: instance.Spec.ServiceUser, PasswordSelectors: instance.Spec.PasswordSelectors, MemcachedInstance: instance.Spec.MemcachedInstance, NodeSelector: instance.Spec.APIServiceTemplate.NodeSelector, PreserveJobs: instance.Spec.PreserveJobs, // WatcherSubCrsCommon fields below ContainerImage: instance.Spec.APIContainerImageURL, Replicas: instance.Spec.APIServiceTemplate.Replicas, Resources: instance.Spec.APIServiceTemplate.Resources, ServiceAccount: "watcher-" + instance.Name, }

Maybe you need to be explicit though. I just thought inline worked this way.

Nevermind. I think I am just confusing myself. 😆

Yeah, I was thinking of composition, not in-lining: https://stackoverflow.com/questions/70094078/what-is-the-proper-way-using-composition-in-golang. And you're doing the latter, not the former, here.

Honestly, I followed the same approach as in nova operator, i.e. in https://github.com/openstack-k8s-operators/nova-operator/blob/70784bbb8ddd11fff955a2f8b5151a3b3cbbb7f3/controllers/nova_controller.go#L1267-L1274 . Do you think we should follow a better way of doing it?

No, this is fine. Again, sorry for the confusion.

raukadah · 2025-01-16T10:11:42Z

api/bases/watcher.openstack.org_watchers.yaml

                type: string
            required:
            - apiContainerImageURL
+            - apiServiceTemplate


Do we want to add a webhook validation for apiServiceTemplate? Or Can I include it in this this pr #33 ?

I'd bet by adding more validation later, once we have a final definition. I'd propose to keep a basic skeleton of the validation in #33 and complete it when we have the operator in a functional status.

cescgina · 2025-01-16T14:58:03Z

recheck

softwarefactory-project-zuul · 2025-01-16T16:56:09Z

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://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/c6741871d50d4b8b98b87b9756701995

✔️ noop SUCCESS in 0s
✔️ openstack-meta-content-provider SUCCESS in 1h 56m 04s
✔️ watcher-operator-validation SUCCESS in 1h 25m 25s
❌ watcher-operator-kuttl RETRY_LIMIT in 1h 02m 09s

cescgina · 2025-01-16T17:02:22Z

recheck

softwarefactory-project-zuul · 2025-01-16T18:59:13Z

This change depends on a change that failed to merge.

Change openstack-k8s-operators/ci-framework#2658 is needed.

cescgina · 2025-01-16T18:59:51Z

recheck

raukadah · 2025-01-17T12:03:30Z

/lgtm

Overall looks good!

openshift-ci bot requested review from frenzyfriday and rlandy January 14, 2025 17:18

openshift-merge-robot added the needs-rebase label Jan 14, 2025

amoralej force-pushed the createapi branch from f052682 to ebd4e52 Compare January 14, 2025 17:22

openshift-merge-robot removed the needs-rebase label Jan 14, 2025

amoralej added 2 commits January 14, 2025 18:25

amoralej force-pushed the createapi branch from ebd4e52 to 0b3cb9c Compare January 14, 2025 17:26

fix command execution in kuttl assert for case 01

a5612dd

We are defining the command in the TestAssert twice and the second one is just overriding the first one which is never executed. This patch is merging both in just one assignment which is what kuttl tests definition seems to expect.

openshift-ci bot added the approved label Jan 15, 2025

marios reviewed Jan 16, 2025

View reviewed changes

abays reviewed Jan 16, 2025

View reviewed changes

raukadah reviewed Jan 16, 2025

View reviewed changes

openshift-ci bot assigned raukadah Jan 17, 2025

openshift-ci bot added the lgtm label Jan 17, 2025

openshift-merge-bot bot merged commit bdd0e99 into openstack-k8s-operators:main Jan 17, 2025
6 checks passed

Integrate WatcherAPI creation and update on the main Watcher reconcile #38

Integrate WatcherAPI creation and update on the main Watcher reconcile #38

Uh oh!

Conversation

amoralej commented Jan 14, 2025 • edited by cescgina Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

softwarefactory-project-zuul bot commented Jan 14, 2025

Uh oh!

amoralej commented Jan 15, 2025

Uh oh!

cescgina commented Jan 15, 2025

Uh oh!

openshift-ci bot commented Jan 15, 2025

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!

cescgina commented Jan 16, 2025

Uh oh!

softwarefactory-project-zuul bot commented Jan 16, 2025

Uh oh!

cescgina commented Jan 16, 2025

Uh oh!

softwarefactory-project-zuul bot commented Jan 16, 2025

Uh oh!

cescgina commented Jan 16, 2025

Uh oh!

raukadah commented Jan 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

amoralej commented Jan 14, 2025 •

edited by cescgina

Loading