Add NotificationsBusInstance API field #1591
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fmount The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Note that I found #1424 that can be considered a follow up of this patch, where a dedicated rabbitmq instance is introduced. |
| // Bus Service instance used by all services that produce or consume notifications. | ||
| // Avoid colocating it with RabbitMQ services used for PRC. | ||
| // That instance will be pushed down for services, unless overriden in templates. | ||
| NotificationsBusInstance *string `json:"notificationsBusInstance,omitempty"` |
There was a problem hiding this comment.
OK so the usual considerations:
- if this is nil then it means nothing to propagate to the service level values. If the service level value is also nil then it means notifications are disabled. And this is our default.
- if this is set to the name of a rabbitmqcluster CR and the service level value is nil, then the rabbitmqcluster name is propagated to that service. This should be our normal way of enabling notififcations for ceilometer across the whole cluster.
- if this is set to the name of a rabbitmqcluster CR and the service level value is also set to a different rabbitmqcluster CR name then the sevice level value is kept for that service. This is the complicated case where ceilometer is not needed or only partially needed, and for some reason this specific service needs to use an independent rabbitmqcluster.
There was a problem hiding this comment.
We need to check if we need both "" and nil value here to mean different things. If not the we can either opt to use "" only, or define that "" and means the same for this field.
There was a problem hiding this comment.
I believe the linked top-scope impl patch follows the above (except we had reached no agreement for empty values meaning)
There was a problem hiding this comment.
OK so the usual considerations:
* if this is nil then it means nothing to propagate to the service level values. If the service level value is also nil then it means notifications are disabled. And this is our default.
Correct, this is consistent with the current implementation for storage operators.
* if this is set to the name of a rabbitmqcluster CR and the service level value is nil, then the rabbitmqcluster name is propagated to that service. This should be our normal way of enabling notififcations for ceilometer across the whole cluster.
Correct, this is an assumption consistent with the top-down propagation model.
* if this is set to the name of a rabbitmqcluster CR and the service level value is also set to a different rabbitmqcluster CR name then the sevice level value is kept for that service. This is the complicated case where ceilometer is not needed or only partially needed, and for some reason this specific service needs to use an independent rabbitmqcluster.
Correct, from an API perspective this is how we use to handle overrides at service level. I think the "complication" is something that might be handled by service operators, unless you envision some logic at the openstack-operator level. I think we might want to keep things easy here, and defer more logic (based on the input we receive) at service operator level.
There was a problem hiding this comment.
We need to check if we need both "" and nil value here to mean different things. If not the we can either opt to use "" only, or define that "" and means the same for this field.
I think checking if that what the user sets is not an empty string is something we can catch from webhooks, so we can cover updates of the same field (while kubebuilder would help us only at CR creation time) and we can decide to default it to nil. However, I think that raising an error (usual webhook flow) would be better to have a human operator fix the input or remove the parameter entirely.
| // When no NotificationsBusInstance is referenced in the subCR (override) | ||
| // try to inject the top-level one if defined | ||
| if instance.Spec.Cinder.Template.NotificationsBusInstance == nil { | ||
| instance.Spec.Cinder.Template.NotificationsBusInstance = instance.Spec.NotificationsBusInstance |
There was a problem hiding this comment.
this inherits the "" value to the service level. We need to see if that is what we want
There was a problem hiding this comment.
yes, how to handle empty values remained a gray area in the design. See in the #1402 descr:
Note about a special handling expected for an empty value by the services
that will be supporting this interface. It should provide backwards
compatibility during oscp and services CRDs upgrades.
There is no an empty value handling top scope (cannot disable notifications top-scope as a cluster-wide), however. It may only take a default value of a 'rabbitmq'. Use the service templates to override it for an empty value, if needed.
There was a problem hiding this comment.
If you look at cinder/glance/manila implementation, for example [1], the parameter is the same, and at openstack-operator it realizes only a parameter inheritance, if required, so I don't see any problem w/ set/update/delete for storage CRs (basically it's the same thing we did for topologies).
There was a problem hiding this comment.
I think this situation is no longer possible as empty strings became prohibited in webhook
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/95c8ffa28bb84df6841fd4a84713376c ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 25m 15s |
fmount
force-pushed
the
notif
branch
3 times, most recently
from
a593d78 to
71ce410
Compare
Of course, done. Added nova propagation as well. |
|
/retest |
|
Watcher section also supports NotificationsBusInstance. If merging this, It should be also covered for consistency. Also, neutron supports it, although I'm not sure if there is any reason to skip it in this patch. |
Interesting, thanks @amoralej, I wasn't aware about the current status. I started with storage but I was going to extend this patch as needed. Let me check the current status and add both neutron and watcher! |
|
how existing flow works for nova and neutron is then consumer can listen/read on new rabbit. can you please explain how this will be used from user perspective. So I think this will works as single top-level switch to enable/disable external notifications from respected service operator. is above correct ? |
Correct. It perfectly fine to create a dedicated rabbitmq instance and make this parameter point to the new rabbitmq instance name. By doing this all the underlying services will point to it and emit notifications. From a service perspective this mechanism is not different from the regular rabbimq/transportURL request that is already implemented as a pattern across the board. This part is clearly handled in each service (with a dedicated
Any processing logic already exists in the service operators to properly reconcile and configure the services. |
fmount
force-pushed
the
notif
branch
2 times, most recently
from
2565159 to
bb05a21
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/885e5285a3054f3a83223613d66fb5c2 ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 01m 03s |
|
/test openstack-operator-build-deploy-kuttl-4-18 |
|
recheck |
|
FYI, there is an issue with the adoption test, which make it to fail. |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/0551c0c5f8b54a11aa8cfb52af05c29f ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 07m 40s |
|
recheck |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/4840aa8f77ec4372b20e30509a9ccaf5 ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 48m 57s |
|
rebased and good to go. |
|
/test openstack-operator-build-deploy-kuttl |
1 similar comment
|
/test openstack-operator-build-deploy-kuttl |
|
/retest |
|
/test openstack-operator-build-deploy-kuttl |
1 similar comment
|
/test openstack-operator-build-deploy-kuttl |
|
still can't get a cluster claim to run kuttl tests in Prow, retrying in a bit |
This patch introduces the notificationsBusInstance parameters at API level. When an override is not defined, it is propagated to the underlying storage components where this field is implemented. Signed-off-by: Francesco Pantano <[email protected]>
Because we can't control user input, this patch introduces a webhook function to validate what we expect users to set as notificationsBusInstance top-level parameter. It also adds the webhook associated envTest to validate the use cases covered by the new function. Signed-off-by: Francesco Pantano <[email protected]>
Add comprehensive envTests covering the "notificationsBusInstance" parameter lifecycle and its propagation to implementing services. The tests validate three key scenarios: - Base use case: parameter is properly referenced and used - Override use case: service overrides with a custom/dedicated rabbitmq instance - Removal use case: services disable notifications when parameter is removed This demonstrates the flexibility of referencing non-default rabbit instances and ensures consistent interface implementation across different services. Signed-off-by: Francesco Pantano <[email protected]>
|
lgtm |
This patch introduces the
notificationsBusInstance(optional) parameter atAPIlevel.When an override is not defined, it is propagated to the underlying storage components where this field is implemented.
Based on: #1402
Jira: https://issues.redhat.com/browse/OSPRH-15389