Webhook for default images #25

raukadah · 2024-12-11T11:07:08Z

Jira: https://issues.redhat.com/browse/OSPRH-11933

openshift-ci · 2024-12-11T11:07: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

abays · 2024-12-11T11:50:54Z

api/bases/watcher.openstack.org_watchers.yaml

+              containerImage:
+                description: The service specific Container Image URL (will be set
+                  to environmental default if empty)
+                type: string


Does the outer/umbrella Watcher CRD have an image itself? Isn't it only the sub-CR that actually have images?

Nope, Watcher CRD does not have image. Nice catch, let me remove that.

amoralej · 2024-12-12T17:52:46Z

api/v1beta1/common_types.go

+
+	// +kubebuilder:validation:Optional
+	// The service specific Container Image URL (will be set to environmental default if empty)
+	ContainerImage string `json:"containerImage"`


WatcherCommon is used in all CRDs, included Watcher, but Watcher should not have ContainerImage, but WatcherImages. ContainerImage should go out of WatcherCommon, we may create another struct for the params that go to the 2nd level CRs but not to Watcher, or add it to them directly.

Done! Added WatcherContainerImages struct to achieve the same.

api/v1beta1/common_types.go

amoralej · 2024-12-12T18:01:48Z

api/v1beta1/common_types.go

+	DecisionEngineContainerImageURL string `json:"decisionengineContainerImageURL"`
+
+	// +kubebuilder:validation:Required
+	// DecisionEngineContainerImageURL


This should be ApplierContainerImageURL ?

api/v1beta1/watcher_webhook.go

amoralej · 2024-12-12T18:17:02Z

config/default/manager_default_images.yaml

+      - name: manager
+        env:
+        - name: WATCHER_API_IMAGE_URL_DEFAULT
+          value: quay.io/tripleozedcentos9/openstack-watcher-api:current-tripleo


api/v1beta1/common_types.go

amoralej · 2024-12-12T18:40:57Z

api/v1beta1/common_types.go

+	watcherDefaults := WatcherDefaults{
+		APIContainerImageURL:            GetEnvDefault("WATCHER_API_IMAGE_URL_DEFAULT", WatcherAPIContainerImage),
+		ApplierContainerImageURL:        GetEnvDefault("WATCHER_APPLIER_IMAGE_URL_DEFAULT", WatcherApplierContainerImage),
+		DecisionEngineContainerImageURL: GetEnvDefault("WATCHER_DECISION_ENGINE_IMAGE_URI_DEFAULT", WatcherDecisionEngineContainerImage),


Should be WATCHER_DECISION_ENGINE_IMAGE_URL_DEFAULT? note URI vs URL

softwarefactory-project-zuul · 2024-12-16T13:02:12Z

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/365c81644381478b8dca6aec12116be4

✔️ noop SUCCESS in 0s
✔️ openstack-meta-content-provider SUCCESS in 1h 24m 34s
✔️ watcher-operator-validation SUCCESS in 1h 14m 17s
❌ watcher-operator-kuttl FAILURE in 35m 01s

softwarefactory-project-zuul · 2024-12-17T08:45:42Z

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/6bfe287af7c046fdad51d643d5c4eef0

✔️ noop SUCCESS in 0s
✔️ openstack-meta-content-provider SUCCESS in 1h 36m 48s
✔️ watcher-operator-validation SUCCESS in 1h 25m 47s
❌ watcher-operator-kuttl FAILURE in 41m 26s

amoralej

It works well in my local test but I'd suggest to improve the test coverage.

Also fix the default values set in env variables (see my comments).

amoralej · 2024-12-17T17:20:23Z

config/default/manager_default_images.yaml

+      - name: manager
+        env:
+        - name: WATCHER_API_IMAGE_URL_DEFAULT
+          value: quay.io/tripleoantelopecentos9/openstack-watcher-api:current-tripleo


Should be pulled from quay.io/podified-antelope-centos9 with current-podified tag:

quay.io/podified-antelope-centos9/openstack-watcher-api:current-podified

amoralej · 2024-12-17T17:20:35Z

config/default/manager_default_images.yaml

+        - name: WATCHER_API_IMAGE_URL_DEFAULT
+          value: quay.io/tripleoantelopecentos9/openstack-watcher-api:current-tripleo
+        - name: WATCHER_DECISION_ENGINE_IMAGE_URL_DEFAULT
+          value: quay.io/tripleoantelopecentos9/openstack-watcher-decision-engine:current-tripleo


amoralej · 2024-12-17T17:20:43Z

config/default/manager_default_images.yaml

+        - name: WATCHER_DECISION_ENGINE_IMAGE_URL_DEFAULT
+          value: quay.io/tripleoantelopecentos9/openstack-watcher-decision-engine:current-tripleo
+        - name: WATCHER_APPLIER_IMAGE_URL_DEFAULT
+          value: quay.io/tripleoantelopecentos9/openstack-watcher-applier:current-tripleo


amoralej · 2024-12-17T17:22:44Z

api/v1beta1/common_types.go

@@ -64,3 +73,54 @@ type PasswordSelector struct {
 	// Service - Selector to get the watcher service user password from the Secret
 	Service string `json:"service"`
 }
+
+// WatcherContainerImages
+type WatcherContainerImages struct {


I have the doubt if we should generalize this more as it may be good to use it for other common parameters for all the 2nd level CRs but not in the initial one. It could be something like WatcherSubCrsCommon ?

I'm not sure if having a reusable struct for only one parameter makes sense.

We will see it in future if we can add more params there.

amoralej · 2024-12-17T17:25:50Z

tests/functional/watcher_controller_test.go

@@ -371,4 +372,31 @@ var _ = Describe("Watcher controller", func() {
 		})
 	})

+	When("Watcher CR is created without container images defined", func() {


We may use any of the existing cases for this tests, i.e. When("A Watcher instance is created from minimal spec").

amoralej · 2024-12-17T17:30:21Z

tests/functional/watcher_controller_test.go

+			Expect(Watcher.Spec.APIContainerImageURL).To(Equal(util.GetEnvVar("RELATED_IMAGE_WATCHER_API_IMAGE_URL_DEFAULT", watcherv1beta1.WatcherAPIContainerImage)))
+			Expect(Watcher.Spec.DecisionEngineContainerImageURL).To(Equal(util.GetEnvVar("RELATED_IMAGE_WATCHER_DECISION_ENGINE_IMAGE_URL_DEFAULT", watcherv1beta1.WatcherDecisionEngineContainerImage)))
+			Expect(Watcher.Spec.ApplierContainerImageURL).To(Equal(util.GetEnvVar("RELATED_IMAGE_WATCHER_APPLIER_IMAGE_URL_DEFAULT", watcherv1beta1.WatcherApplierContainerImage)))


IMO we should run three different cases to fully cover the defaulting functionality:

No container images defined and no env variable defined. Spec values should == watcherv1beta1.WatcherApplierContainerImage

No container images defined ane env variable defined to some fake values (this will require a different When case). Spec values should == util.GetEnvVar("RELATED_IMAGE_WATCHER_APPLIER_IMAGE_URL_DEFAULT")

When Watcher is created with container images values in the spec, Spec values should == to the defined ones (This will also required a different When case to set a different spec).

@amoralej I need help on 3rd option, I am not sure i have done it correctly. https://github.com/raukadah/watcher-operator/blob/webhook_default_images/tests/functional/watcher_controller_test.go#L70 Please have a look.

I'd say that's incorrect as you are modifying the spec after the reconcile loop is finised. I think you need to:

Add a new spec in addition to MinimalWatcherSpec with your fake values for the images. Something like:

https://github.com/raukadah/watcher-operator/blob/webhook_default_images/tests/functional/watcher_controller_test.go#L20-L24

Define an entirely new test watcher deployment using something similar to:

https://github.com/raukadah/watcher-operator/blob/webhook_default_images/tests/functional/watcher_controller_test.go#L27-L31

but instead of using MinimalWatcherSpec you need to use yours.

Check the values as in:

https://github.com/raukadah/watcher-operator/blob/webhook_default_images/tests/functional/watcher_controller_test.go#L32-L40

Note all this has to go into a new When block after the existing one, i.e. after

https://github.com/raukadah/watcher-operator/blob/webhook_default_images/tests/functional/watcher_controller_test.go#L399

Actually, Case 2 (getting defaults from env values) also need some rework from https://github.com/raukadah/watcher-operator/blob/webhook_default_images/tests/functional/watcher_controller_test.go#L63-L68 , but I'm not sure how to inject env values for a specific test in envtest. Unless we find a way we may need to skip that case from functional test coverage.

Thank you @amoralej for the detailed writeup. Here is what I have implemented for testcase 3: https://github.com/raukadah/watcher-operator/blob/webhook_default_images/tests/functional/watcher_controller_test.go#L413

For second case, I tried to go via this https://github.com/raukadah/watcher-operator/blob/webhook_default_images/tests/functional/watcher_controller_test.go#L390 Not sure how much it is correct.

I think you are going in the right path, but i'm adding some suggestions.

tests/kuttl/test-suites/default/watcher/01-assert.yaml

marios

spent some time looking though this and trying to follow along
can you bring this to one of the team calls and give some overview of the changes it would be helpful at least for me (eg the last comments/discussion with amoralej about the defaulting)

softwarefactory-project-zuul · 2024-12-18T16:45: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/ceae0842f22540619faa29e080127b71

✔️ noop SUCCESS in 0s
✔️ openstack-meta-content-provider SUCCESS in 1h 56m 39s
✔️ watcher-operator-validation SUCCESS in 1h 28m 29s
❌ watcher-operator-kuttl FAILURE in 43m 03s

amoralej · 2024-12-19T08:58:36Z

tests/functional/watcher_controller_test.go

+
+		It("should have the Spec fields with above environment container images", func() {
+			Watcher := GetWatcher(watcherTest.Instance)
+			Expect(Watcher.Spec.APIContainerImageURL).To(Equal(util.GetEnvVar("RELATED_IMAGE_WATCHER_API_IMAGE_URL_DEFAULT", watcherv1beta1.WatcherAPIContainerImage)))


In this case we should ensure it has the value of the env variable, while GetEnvVar can take default value. I'd simplify by doing:

Expect(Watcher.Spec.APIContainerImageURL).To(Equal("watcher-api-custom-image"))

And similar for the other two images.

If we want to make sure the values come from env variables, as mentioned, i would use some different value as "watcher-api-custom-image-env" and check it.

tests/functional/watcher_controller_test.go

amoralej · 2024-12-19T09:45:17Z

tests/functional/watcher_controller_test.go

+		BeforeEach(func() {
+			DeferCleanup(th.DeleteInstance, CreateWatcher(watcherTest.Instance, MinimalWatcherContainerSpec))
+			// Set environment variables
+			os.Setenv("RELATED_IMAGE_WATCHER_API_IMAGE_URL_DEFAULT", "watcher-api-custom-image")


You are setting the same values in env variables that the ones on MinimalWatcherContainerSpec so in this case to guess if the real parameter is coming from env variable or from the spec. I'd set different values for env (mabye adding -env or something.)

amoralej · 2024-12-19T09:53:22Z

tests/functional/watcher_controller_test.go

@@ -371,4 +387,32 @@ var _ = Describe("Watcher controller", func() {
 		})
 	})

+	When("Watcher is created  with no container images defined and env variables contains fake value", func() {


It says "no container images defined and env variables contains fake value" but actually, you are using MinimalWatcherContainerSpec which defines images in the Spec parameters. So we have two options:

If we want to check that the resulting Watcher get the values from env variables when it's CRs do not includes images you need to CreateWatcher using MinimalWatcherSpec instead of MinimalWatcherContainerSpec.

If we want to check that the resulting Watcher get the values from CR even if env variables are set (which is what current test would be doing, then i think this should be "Watcher is created with container images defined and env variables contains fake value". In this case the check should be:

It("should have the Spec fields with container images as defined in the CR")

As spec values wins to env variables.

My proposal would be to include these two checks separatedly.

I failed to achieve the second case so skipping it for now.

amoralej · 2024-12-19T09:54:59Z

tests/functional/watcher_controller_test.go

+			Expect(Watcher.Spec.APIContainerImageURL).To(Equal(util.GetEnvVar("RELATED_IMAGE_WATCHER_API_IMAGE_URL_DEFAULT", watcherv1beta1.WatcherAPIContainerImage)))
+			Expect(Watcher.Spec.DecisionEngineContainerImageURL).To(Equal(util.GetEnvVar("RELATED_IMAGE_WATCHER_DECISION_ENGINE_IMAGE_URL_DEFAULT", watcherv1beta1.WatcherDecisionEngineContainerImage)))
+			Expect(Watcher.Spec.ApplierContainerImageURL).To(Equal(util.GetEnvVar("RELATED_IMAGE_WATCHER_APPLIER_IMAGE_URL_DEFAULT", watcherv1beta1.WatcherApplierContainerImage)))


I think you are going in the right path, but i'm adding some suggestions.

tests/kuttl/test-suites/default/watcher/01-assert.yaml

cescgina · 2024-12-19T11:06:34Z

tests/functional/watcher_controller_test.go

@@ -52,6 +61,13 @@ var _ = Describe("Watcher controller with minimal spec values", func() {
 			}, timeout, interval).Should(ContainElement("openstack.org/watcher"))
 		})

+		It("It has the expected container image defaults", func() {
+			Watcher := GetWatcher(watcherTest.Instance)
+			Expect(Watcher.Spec.APIContainerImageURL).To(Equal(util.GetEnvVar("RELATED_IMAGE_WATCHER_API_IMAGE_URL_DEFAULT", watcherv1beta1.WatcherAPIContainerImage)))


are those *_URL_DEFAULT vars defined in these tests? I think we should simply use the watcherv1beta1 variables here

softwarefactory-project-zuul · 2024-12-19T13:08:26Z

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 25,771002fe67f23238fe499e3c5bdd9b4c1d661e71

softwarefactory-project-zuul · 2024-12-19T17:20:31Z

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/c2ae24b40cfa4baeb6aceb7644fb5d69

✔️ noop SUCCESS in 0s
✔️ openstack-meta-content-provider SUCCESS in 2h 10m 32s
✔️ watcher-operator-validation SUCCESS in 1h 27m 41s
❌ watcher-operator-kuttl FAILURE in 41m 07s

Jira: https://issues.redhat.com/browse/OSPRH-11933 Signed-off-by: Chandan Kumar <[email protected]>

cescgina

thanks for the changes Chandan, lgtm

amoralej · 2024-12-20T08:38:22Z

/aprove

amoralej · 2024-12-20T08:49:29Z

/approve

openshift-ci · 2024-12-20T08:49:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amoralej

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [amoralej]

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

openshift-ci bot added the do-not-merge/work-in-progress label Dec 11, 2024

raukadah force-pushed the webhook_default_images branch from 89a5223 to 2281d74 Compare December 11, 2024 11:24

abays reviewed Dec 11, 2024

View reviewed changes

raukadah force-pushed the webhook_default_images branch 2 times, most recently from f99f490 to a6bef3b Compare December 12, 2024 09:41

amoralej reviewed Dec 12, 2024

View reviewed changes

amoralej requested changes Dec 12, 2024

View reviewed changes

openshift-ci bot assigned amoralej Dec 12, 2024

amoralej reviewed Dec 12, 2024

View reviewed changes

raukadah force-pushed the webhook_default_images branch 4 times, most recently from 1ade9c2 to 4c34279 Compare December 16, 2024 11:36

raukadah force-pushed the webhook_default_images branch from 4c34279 to 3d0e2fc Compare December 17, 2024 07:07

raukadah force-pushed the webhook_default_images branch from 3d0e2fc to 0f47ba0 Compare December 17, 2024 12:40

raukadah changed the title ~~[WIP] Webhook for default images~~ Webhook for default images Dec 17, 2024

amoralej requested changes Dec 17, 2024

View reviewed changes

marios reviewed Dec 18, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase label Dec 18, 2024

raukadah force-pushed the webhook_default_images branch from 0f47ba0 to 7e064bd Compare December 18, 2024 14:02

openshift-merge-robot removed the needs-rebase label Dec 18, 2024

amoralej mentioned this pull request Dec 18, 2024

Create the Watcher database schema #29

Merged

raukadah force-pushed the webhook_default_images branch 2 times, most recently from 9a1bb64 to 80f2871 Compare December 19, 2024 07:05

raukadah marked this pull request as ready for review December 19, 2024 07:05

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 19, 2024

openshift-ci bot requested review from cescgina and viroel December 19, 2024 07:05

amoralej reviewed Dec 19, 2024

View reviewed changes

cescgina reviewed Dec 19, 2024

View reviewed changes

tests/kuttl/test-suites/default/watcher/01-assert.yaml Show resolved Hide resolved

cescgina reviewed Dec 19, 2024

View reviewed changes

raukadah force-pushed the webhook_default_images branch from 80f2871 to 771002f Compare December 19, 2024 13:08

openshift-merge-robot added the needs-rebase label Dec 19, 2024

raukadah force-pushed the webhook_default_images branch from 771002f to 4dfe962 Compare December 19, 2024 13:10

openshift-merge-robot removed the needs-rebase label Dec 19, 2024

raukadah force-pushed the webhook_default_images branch 3 times, most recently from 69b0a06 to 0110797 Compare December 19, 2024 14:03

Added webhook support for Default Images

945d8bf

Jira: https://issues.redhat.com/browse/OSPRH-11933 Signed-off-by: Chandan Kumar <[email protected]>

raukadah force-pushed the webhook_default_images branch from 0110797 to 945d8bf Compare December 20, 2024 06:10

cescgina approved these changes Dec 20, 2024

View reviewed changes

openshift-ci bot assigned cescgina Dec 20, 2024

openshift-ci bot added the lgtm label Dec 20, 2024

openshift-ci bot added the approved label Dec 20, 2024

openshift-merge-bot bot merged commit 0930858 into openstack-k8s-operators:main Dec 20, 2024
6 checks passed

raukadah deleted the webhook_default_images branch December 20, 2024 08:58

Webhook for default images #25

Webhook for default images #25

Conversation

raukadah commented Dec 11, 2024 • edited Loading

openshift-ci bot commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Dec 16, 2024

softwarefactory-project-zuul bot commented Dec 17, 2024

amoralej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marios left a comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Dec 19, 2024

softwarefactory-project-zuul bot commented Dec 19, 2024

cescgina left a comment

Choose a reason for hiding this comment

amoralej commented Dec 20, 2024

amoralej commented Dec 20, 2024

openshift-ci bot commented Dec 20, 2024

raukadah commented Dec 11, 2024 •

edited

Loading