Adds notification bus by auniyal61 · Pull Request #958 · openstack-k8s-operators/nova-operator

auniyal61 · 2025-04-16T12:08:29Z

Initial commit - Adds tests for notification bus

how this should work is:

1 - human operator will update rabbitmq in openstackcontrolplane CR
a) under rabbitmq: add new rabbitmq, ex: rabbitmq-broadcaster, something like

           rabbitmq-broadcaster:
               replicas: 1

2 - human operator will update Nova in openstackcontrolplane CR
a) under nova.template.spec: register new rabbit
(as we do for new cell but not inside celltemplate, as this one is top-level)

     nova:
	  template:
               spec:
                  apiContainerImageURL: image_url
                  apiMessageBusInstance: rabbitmq
                  // new var
                  notificationsBusInstance: rabbitmq-broadcaster

3 - now nova-operator will recognize notificationsBusInstance and retrieve transport_url of new rabbit (rabbitmq-broadcaster)

4 - nova-operator will set this transport_url in nova.conf

Jira: OSPRH-15392

softwarefactory-project-zuul · 2025-04-16T15:31:22Z

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/5f2f11473cbd44c4b893856a98532d51

✔️ openstack-meta-content-provider SUCCESS in 3h 21m 46s
✔️ nova-operator-kuttl SUCCESS in 37m 42s
✔️ nova-operator-tempest-multinode SUCCESS in 2h 06m 31s
❌ nova-operator-tempest-multinode-ceph FAILURE in 2h 49m 41s

bogdando · 2025-04-17T09:37:32Z

@auniyal61 Please make sure the tests covering expected behavir should be aligned with some changes done to the design spec #948

softwarefactory-project-zuul · 2025-04-30T04:57:36Z

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

✔️ openstack-meta-content-provider SUCCESS in 24m 31s
❌ nova-operator-kuttl MERGE_CONFLICT in 4s
❌ nova-operator-tempest-multinode MERGE_CONFLICT in 4s
❌ nova-operator-tempest-multinode-ceph MERGE_CONFLICT in 4s

softwarefactory-project-zuul · 2025-05-02T09:37:51Z

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/nova-operator for 958,750038d92b3286f729befffd4513d79293b6a48a

softwarefactory-project-zuul · 2025-05-02T11:14:38Z

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/nova-operator for 958,7ecda9c80739a5f5b3ded18d5e58cfe676da8e73

softwarefactory-project-zuul · 2025-05-06T03:02:09Z

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/nova-operator for 958,79573b56cfb6de714bb3f3919bd3a5c73e875ca0

softwarefactory-project-zuul · 2025-05-06T05:50:21Z

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/nova-operator for 958,00bc5a8cdb0668016b08bc3b0531a858df7e62df

gibizer

OK. This looks good to me. If CI agrees I think we can land this. This means that when the CI is green we should unblock #948 , let it land, and then rebase this on the new main and then land this.

my comments were fixed

SeanMooney

I have not done a super detailed review but over all this looks ok

the main gap i see is we have not enabled testing in either of the tempest jobs.

it would be nice to do that in a followup.

my main concern is with the condition reporting commented inline.

SeanMooney · 2025-05-21T16:39:32Z

+  - message: Notification message bus not requested
+    reason: Ready
+    status: "True"
+    type: NovaNotificationMQReady


is this a bug?

so by default the notificaotn message bus is not passed in.

and i don't think you are enabling it in this test so this condition should not be reported.

it should be skipped and only reported if the notifications are enabled no?

i could be wrong but i belive that is how we handle the conditions for the nova metadata when it runs at the top level vs the cell level.

as long as we are consistent, i don't particularly mind, but i would like to avoid divergence if we can.

@auniyal61 now these are not in the status any more as in these tests the notifications are not enabled and your new change removes the condition as Sean requested in that case.

SeanMooney · 2025-05-21T16:53:38Z

+	} else {
+		instance.Status.Conditions.MarkTrue(
+			novav1.NovaNotificationMQReadyCondition, novav1.NovaNotificationMQNotRequestedMessage)
+


i chatted to gibi on slack baout this about and i think this is misleading

it imples the notification bus is deployed when its not in use.

I'm not sure this is enoch to block the current pr but it is not consistent with the nova metadata API conditions which are only reported when they are enabled at the cell or nova level.

this should also proably be reporeed on the cell contoelr state too.

not just the top level.

i think initally this is configured cluster wide so i guess that is why this is only reporter dat the top level.

but long term we will need to include this in the cell template.

so perhaps per cell reporting is not required until that is addded.

it imples the notification bus is deployed when its not in use.

I'm fine to make it consistent between metadata and notifications and remove the notification condition form the status if notifications are not enabled

this should also proably be reporeed on the cell contoelr state too.
not just the top level.

Today the TransportURL is created at the top level and there we know when the Rabbit and therefore the TransportURL CR is ready, this is reflected in the condition on the top level. On the cell level we only get a transport_url value in the internal secret so nothing to wait for, what is there just gets included into the cell specific compute config Secret. So from the cell perspective the important conditions are the

InputReady : are the internal secret available and has the expected keys

NovaComputeServiceConfigReady: the cell specific compute config secret is created successfully

When we have a cellTemplate specific override (if ever) then we can discuss where to create the TransportURL for that. But note that the cell specific RPC TransportURL is still created on the top level not on the cell level for architectural reasons.

gibizer · 2025-05-22T11:36:36Z

Thanks for fixing my test nits. Those changes look good. Let me know when you implemented the removal of the condition if notificationsBusInstance is empty.

1 - human operator will update rabbitmq in openstackcontrolplane CR a) under rabbitmq: add new rabbitmq, ex: rabbitmq-broadcaster, something like rabbitmq-broadcaster: replicas: 1 2 - human operator will update Nova in openstackcontrolplane CR a) under nova.template.spec: register new rabbit (as we do for new cell but not inside celltemplate, as this one is top-level) nova: template: spec: apiContainerImageURL: image_url apiMessageBusInstance: rabbitmq // new var notificationsBusInstance: rabbitmq-broadcaster in Nova CR instance.Spec.NotificationsBusInstance value nil : means not mentioned in CR, so disable it. "" : disable notification rabbitmq-broadcaster : notification to a bus rabbitmq-broadcaster 3 - nova-operator will recognize notificationsBusInstance and retrieve transport_url of new rabbit (rabbitmq-broadcaster) 4 - nova-operator will set this transport_url in nova.conf Closes: OSPRH-15392

openshift-ci · 2025-05-22T13:11:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: auniyal61, bogdando, 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~~ [auniyal61,bogdando,mrkisaolamb]

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

SeanMooney

/lgtm

there is one minor testing change that we could do but i think I'm fine to merge this

SeanMooney · 2025-05-22T13:12:38Z

+			mariadb.SimulateMariaDBAccountCompleted(cell0.MariaDBAccountName)
+			infra.SimulateTransportURLReady(cell0.TransportURLName)
+			SimulateReadyOfNovaTopServices()
+		})


we coudl have had an addtional check here to assert that the
novav1.NovaNotificationMQReadyCondition, is not in the list of status conditions.

but that the over all ready condition is true.

done

gibizer · 2025-05-22T13:18:46Z

/unhold
Content looks good. The CRD prereq #948 PR merged yesterday. So this is good to go. I will approve it once CI is green...

bogdando · 2025-06-17T14:13:17Z

/cherry-pick 18.0-fr2

openshift-cherrypick-robot · 2025-06-17T14:13:55Z

@bogdando: #958 failed to apply on top of branch "18.0-fr2":

Applying: Adds notification transporturl
Using index info to reconstruct a base tree...
M	api/v1beta1/conditions.go
M	controllers/common.go
M	controllers/nova_controller.go
M	controllers/novaapi_controller.go
M	controllers/novacell_controller.go
M	controllers/novacompute_controller.go
M	controllers/novaconductor_controller.go
M	controllers/novascheduler_controller.go
M	templates/nova.conf
M	test/functional/base_test.go
M	test/functional/nova_controller_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/functional/nova_controller_test.go
Auto-merging test/functional/base_test.go
Auto-merging templates/nova.conf
Auto-merging controllers/novascheduler_controller.go
Auto-merging controllers/novaconductor_controller.go
Auto-merging controllers/novacompute_controller.go
Auto-merging controllers/novacell_controller.go
Auto-merging controllers/novaapi_controller.go
Auto-merging controllers/nova_controller.go
Auto-merging controllers/common.go
Auto-merging api/v1beta1/conditions.go
CONFLICT (content): Merge conflict in api/v1beta1/conditions.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Adds notification transporturl

Details

In response to this:

/cherry-pick 18.0-fr2

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

bogdando · 2025-06-17T14:16:20Z

ok, we need #985 to land then retry with cherry picking

openshift-ci Bot requested review from dprince and gibizer April 16, 2025 12:08

openshift-ci Bot added the approved label Apr 16, 2025

auniyal61 requested review from SeanMooney and bogdando April 16, 2025 13:13

gibizer reviewed Apr 17, 2025

View reviewed changes

bogdando mentioned this pull request Apr 22, 2025

Common Notifications bus interface for services openstack-k8s-operators/openstack-operator#1402

Closed

auniyal61 force-pushed the notifications branch from 6718736 to f983edb Compare April 30, 2025 04:31

openshift-merge-robot added the needs-rebase label Apr 30, 2025

auniyal61 commented Apr 30, 2025

View reviewed changes

Comment thread controllers/nova_controller.go Outdated

auniyal61 commented Apr 30, 2025

View reviewed changes

Comment thread controllers/nova_controller.go Outdated

auniyal61 requested a review from gibizer April 30, 2025 04:34

dprince reviewed Apr 30, 2025

View reviewed changes

Comment thread api/bases/nova.openstack.org_nova.yaml Outdated

auniyal61 force-pushed the notifications branch from f983edb to 750038d Compare May 2, 2025 09:37

auniyal61 commented May 2, 2025

View reviewed changes

Comment thread templates/nova.conf

SeanMooney reviewed May 2, 2025

View reviewed changes

Comment thread templates/nova.conf Outdated

Comment thread controllers/novaapi_controller.go

auniyal61 force-pushed the notifications branch from 750038d to 7ecda9c Compare May 2, 2025 11:14

gibizer reviewed May 5, 2025

View reviewed changes

Comment thread controllers/novaapi_controller.go

Comment thread controllers/nova_controller.go

Comment thread controllers/nova_controller.go Outdated

auniyal61 force-pushed the notifications branch from 7ecda9c to 79573b5 Compare May 6, 2025 03:02

auniyal61 commented May 6, 2025

View reviewed changes

Comment thread controllers/novascheduler_controller.go

auniyal61 commented May 6, 2025

View reviewed changes

Comment thread controllers/novaconductor_controller.go Outdated

auniyal61 force-pushed the notifications branch from 79573b5 to 00bc5a8 Compare May 6, 2025 05:50

auniyal61 force-pushed the notifications branch from 00bc5a8 to dcbe555 Compare May 6, 2025 05:56

auniyal61 force-pushed the notifications branch from 018ebbc to f2df7ec Compare May 21, 2025 12:31

auniyal61 requested a review from gibizer May 21, 2025 12:32

gibizer reviewed May 21, 2025

View reviewed changes

Comment thread test/functional/base_test.go Outdated

Comment thread test/functional/nova_controller_test.go

Comment thread test/functional/nova_controller_test.go

SeanMooney reviewed May 21, 2025

View reviewed changes

auniyal61 force-pushed the notifications branch 3 times, most recently from bb8f595 to 73c5768 Compare May 22, 2025 09:31

auniyal61 requested review from SeanMooney, gibizer and mrkisaolamb May 22, 2025 10:22

auniyal61 force-pushed the notifications branch from 73c5768 to 73956ef Compare May 22, 2025 12:46

auniyal61 added 2 commits May 22, 2025 09:05

Adds tests for notification bus

3a6534a

auniyal61 force-pushed the notifications branch from 73956ef to 3a6534a Compare May 22, 2025 13:07

mrkisaolamb approved these changes May 22, 2025

View reviewed changes

openshift-ci Bot assigned mrkisaolamb May 22, 2025

openshift-ci Bot added the lgtm label May 22, 2025

mrkisaolamb removed the lgtm label May 22, 2025

SeanMooney reviewed May 22, 2025

View reviewed changes

openshift-ci Bot added the lgtm label May 22, 2025

openshift-ci Bot removed the do-not-merge/hold label May 22, 2025

openshift-merge-bot Bot merged commit c71791e into openstack-k8s-operators:main May 22, 2025
7 checks passed

Conversation

auniyal61 commented Apr 16, 2025 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

softwarefactory-project-zuul Bot commented Apr 16, 2025

Uh oh!

bogdando commented Apr 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

softwarefactory-project-zuul Bot commented Apr 30, 2025

Uh oh!

Uh oh!

softwarefactory-project-zuul Bot commented May 2, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

softwarefactory-project-zuul Bot commented May 2, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

softwarefactory-project-zuul Bot commented May 6, 2025

Uh oh!

Uh oh!

Uh oh!

softwarefactory-project-zuul Bot commented May 6, 2025

Uh oh!

gibizer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SeanMooney left a comment

Choose a reason for hiding this comment

Uh oh!

SeanMooney May 21, 2025

Choose a reason for hiding this comment

Uh oh!

gibizer May 22, 2025

Choose a reason for hiding this comment

Uh oh!

SeanMooney May 21, 2025

Choose a reason for hiding this comment

Uh oh!

SeanMooney May 21, 2025

Choose a reason for hiding this comment

Uh oh!

gibizer May 21, 2025

Choose a reason for hiding this comment

Uh oh!

gibizer commented May 22, 2025

Uh oh!

openshift-ci Bot commented May 22, 2025

Uh oh!

SeanMooney left a comment

Choose a reason for hiding this comment

Uh oh!

SeanMooney May 22, 2025

Choose a reason for hiding this comment

Uh oh!

gibizer commented May 22, 2025

Uh oh!

Uh oh!

bogdando commented Jun 17, 2025

Uh oh!

openshift-cherrypick-robot commented Jun 17, 2025

Uh oh!

bogdando commented Jun 17, 2025

Uh oh!

auniyal61 commented Apr 16, 2025 •

edited by openshift-ci Bot

Loading