Skip to content

Add NotificationsBusInstance API field #1591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 2, 2025
+408 −61
Merged

Add NotificationsBusInstance API field #1591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aa5c12c
Add NotificationsBusInstance API field
fmount Sep 5, 2025
e0e6b42
Add notificationBusInstance webhook validation
fmount Sep 5, 2025
9b9ffa9
Add envTests for notificationsBusInstance interface lifecycle
fmount Sep 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions apis/bases/core.openstack.org_openstackcontrolplanes.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9855,6 +9855,8 @@ spec:
additionalProperties:
type: string
type: object
notificationsBusInstance:
type: string
nova:
properties:
apiOverride:
Expand Down
7 changes: 7 additions & 0 deletions apis/core/v1beta1/openstackcontrolplane_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,13 @@ type OpenStackControlPlaneSpec struct {
// Rabbitmq - Parameters related to the Rabbitmq service
Rabbitmq RabbitmqSection `json:"rabbitmq,omitempty"`

// +kubebuilder:validation:Optional
// NotificationsBusInstance - the name of RabbitMQ Cluster CR to select a Messaging
// 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"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@bogdando bogdando Sep 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the linked top-scope impl patch follows the above (except we had reached no agreement for empty values meaning)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// +kubebuilder:validation:Optional
// +operator-sdk:csv:customresourcedefinitions:type=spec
// Memcached - Parameters related to the Memcached service
Expand Down
33 changes: 33 additions & 0 deletions apis/core/v1beta1/openstackcontrolplane_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,10 @@ func (r *OpenStackControlPlane) ValidateCreate() (admission.Warnings, error) {
allErrs = append(allErrs, err)
}

if err := r.ValidateNotificationsBusInstance(basePath); err != nil {
allErrs = append(allErrs, err)
}

if len(allErrs) != 0 {
return allWarn, apierrors.NewInvalid(
schema.GroupKind{Group: "core.openstack.org", Kind: "OpenStackControlPlane"},
Expand Down Expand Up @@ -161,6 +165,10 @@ func (r *OpenStackControlPlane) ValidateUpdate(old runtime.Object) (admission.Wa
allErrs = append(allErrs, err)
}

if err := r.ValidateNotificationsBusInstance(basePath); err != nil {
allErrs = append(allErrs, err)
}

if len(allErrs) != 0 {
return nil, apierrors.NewInvalid(
schema.GroupKind{Group: "core.openstack.org", Kind: "OpenStackControlPlane"},
Expand Down Expand Up @@ -1138,3 +1146,28 @@ func (r *OpenStackControlPlane) ValidateTopology(basePath *field.Path) *field.Er
}
return nil
}

// ValidateNotificationsBusInstance - returns an error if the notificationsBusInstance
// parameter is not valid.
// - nil or empty string must be raised as an error
// - when notificationsBusInstance does not point to an existing RabbitMQ instance
func (r *OpenStackControlPlane) ValidateNotificationsBusInstance(basePath *field.Path) *field.Error {
notificationsField := basePath.Child("notificationsBusInstance")
// no notificationsBusInstance field set, nothing to validate here
if r.Spec.NotificationsBusInstance == nil {
return nil
}
// When NotificationsBusInstance is set, fail if it is an empty string
if *r.Spec.NotificationsBusInstance == "" {
return field.Invalid(notificationsField, *r.Spec.NotificationsBusInstance, "notificationsBusInstance is not a valid string")
}
// NotificationsBusInstance is set and must be equal to an existing
// deployed rabbitmq instance, otherwise we should fail because it
// does not represent a valid string
for k := range(*r.Spec.Rabbitmq.Templates) {
if *r.Spec.NotificationsBusInstance == k {
return nil
}
}
return field.Invalid(notificationsField, *r.Spec.NotificationsBusInstance, "notificationsBusInstance must match an existing RabbitMQ instance name")
}
5 changes: 5 additions & 0 deletions apis/core/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions bindata/crds/crds.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10021,6 +10021,8 @@ spec:
additionalProperties:
type: string
type: object
notificationsBusInstance:
type: string
nova:
properties:
apiOverride:
Expand Down
2 changes: 2 additions & 0 deletions config/crd/bases/core.openstack.org_openstackcontrolplanes.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9855,6 +9855,8 @@ spec:
additionalProperties:
type: string
type: object
notificationsBusInstance:
type: string
nova:
properties:
apiOverride:
Expand Down
6 changes: 6 additions & 0 deletions pkg/openstack/cinder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,12 @@ func ReconcileCinder(ctx context.Context, instance *corev1beta1.OpenStackControl
instance.Spec.Cinder.Template.TopologyRef = instance.Spec.TopologyRef
}

// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this inherits the "" value to the service level. We need to see if that is what we want

Copy link
Contributor

@bogdando bogdando Sep 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

[1] https://github.com/openstack-k8s-operators/manila-operator/blob/main/api/v1beta1/manila_types.go#L135

Copy link
Contributor

@bogdando bogdando Sep 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this situation is no longer possible as empty strings became prohibited in webhook

}

Log.Info("Reconciling Cinder", "Cinder.Namespace", instance.Namespace, "Cinder.Name", cinderName)
op, err := controllerutil.CreateOrPatch(ctx, helper.GetClient(), cinder, func() error {
instance.Spec.Cinder.Template.CinderSpecBase.DeepCopyInto(&cinder.Spec.CinderSpecBase)
Expand Down
6 changes: 6 additions & 0 deletions pkg/openstack/glance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,12 @@ func ReconcileGlance(ctx context.Context, instance *corev1beta1.OpenStackControl
instance.Spec.Glance.Template.TopologyRef = instance.Spec.TopologyRef
}

// When no NotificationsBusInstance is referenced in the subCR (override)
// try to inject the top-level one if defined
if instance.Spec.Glance.Template.NotificationBusInstance == nil {
instance.Spec.Glance.Template.NotificationBusInstance = instance.Spec.NotificationsBusInstance
}

// When component services got created check if there is the need to create a route
if err := helper.GetClient().Get(ctx, types.NamespacedName{Name: glanceName, Namespace: instance.Namespace}, glance); err != nil {
if !k8s_errors.IsNotFound(err) {
Expand Down
6 changes: 6 additions & 0 deletions pkg/openstack/manila.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,12 @@ func ReconcileManila(ctx context.Context, instance *corev1beta1.OpenStackControl
instance.Spec.Manila.Template.TopologyRef = instance.Spec.TopologyRef
}

// When no NotificationsBusInstance is referenced in the subCR (override)
// try to inject the top-level one if defined
if instance.Spec.Manila.Template.NotificationsBusInstance == nil {
instance.Spec.Manila.Template.NotificationsBusInstance = instance.Spec.NotificationsBusInstance
}

Log.Info("Reconciling Manila", "Manila.Namespace", instance.Namespace, "Manila.Name", "manila")
op, err := controllerutil.CreateOrPatch(ctx, helper.GetClient(), manila, func() error {
instance.Spec.Manila.Template.ManilaSpecBase.DeepCopyInto(&manila.Spec.ManilaSpecBase)
Expand Down
6 changes: 6 additions & 0 deletions pkg/openstack/neutron.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,12 @@ func ReconcileNeutron(ctx context.Context, instance *corev1beta1.OpenStackContro
instance.Spec.Neutron.Template.TopologyRef = instance.Spec.TopologyRef
}

// When no NotificationsBusInstance is referenced in the subCR (override)
// try to inject the top-level one if defined
if instance.Spec.Neutron.Template.NotificationsBusInstance == nil {
instance.Spec.Neutron.Template.NotificationsBusInstance = instance.Spec.NotificationsBusInstance
}

Log.Info("Reconciling NeutronAPI", "NeutronAPI.Namespace", instance.Namespace, "NeutronAPI.Name", "neutron")
op, err := controllerutil.CreateOrPatch(ctx, helper.GetClient(), neutronAPI, func() error {
instance.Spec.Neutron.Template.DeepCopyInto(&neutronAPI.Spec.NeutronAPISpecCore)
Expand Down
6 changes: 6 additions & 0 deletions pkg/openstack/nova.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ func ReconcileNova(ctx context.Context, instance *corev1beta1.OpenStackControlPl
instance.Spec.Nova.Template.NodeSelector = &instance.Spec.NodeSelector
}

// When no NotificationsBusInstance is referenced in the subCR (override)
// try to inject the top-level one if defined
if instance.Spec.Nova.Template.NotificationsBusInstance == nil {
instance.Spec.Nova.Template.NotificationsBusInstance = instance.Spec.NotificationsBusInstance
}

// When there's no Topology referenced in the Service Template, inject the
// top-level one
// NOTE: This does not check the Service subCRs: by default the generated
Expand Down
6 changes: 6 additions & 0 deletions pkg/openstack/watcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,12 @@ func ReconcileWatcher(ctx context.Context, instance *corev1beta1.OpenStackContro
instance.Spec.Watcher.Template.TopologyRef = instance.Spec.TopologyRef
}

// When no NotificationsBusInstance is referenced in the subCR (override)
// try to inject the top-level one if defined
if instance.Spec.Watcher.Template.NotificationsBusInstance == nil {
instance.Spec.Watcher.Template.NotificationsBusInstance = instance.Spec.NotificationsBusInstance
}

helper.GetLogger().Info("Reconciling Watcher", "Watcher.Namespace", instance.Namespace, "Watcher.Name", "watcher")
op, err := controllerutil.CreateOrPatch(ctx, helper.GetClient(), watcher, func() error {
instance.Spec.Watcher.Template.DeepCopyInto(&watcher.Spec.WatcherSpecCore)
Expand Down
23 changes: 23 additions & 0 deletions tests/functional/ctlplane/base_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
manilav1 "github.com/openstack-k8s-operators/manila-operator/api/v1beta1"
mariadbv1 "github.com/openstack-k8s-operators/mariadb-operator/api/v1beta1"
neutronv1 "github.com/openstack-k8s-operators/neutron-operator/api/v1beta1"
novav1 "github.com/openstack-k8s-operators/nova-operator/api/v1beta1"
openstackclientv1 "github.com/openstack-k8s-operators/openstack-operator/apis/client/v1beta1"
corev1 "github.com/openstack-k8s-operators/openstack-operator/apis/core/v1beta1"
dataplanev1 "github.com/openstack-k8s-operators/openstack-operator/apis/dataplane/v1beta1"
Expand Down Expand Up @@ -69,6 +70,8 @@ type Names struct {
RabbitMQCertName types.NamespacedName
RabbitMQCell1Name types.NamespacedName
RabbitMQCell1CertName types.NamespacedName
RabbitMQNotificationsName types.NamespacedName
RabbitMQNotificationsCertName types.NamespacedName
NoVNCProxyCell1CertPublicRouteName types.NamespacedName
NoVNCProxyCell1CertPublicSvcName types.NamespacedName
NoVNCProxyCell1CertVencryptName types.NamespacedName
Expand Down Expand Up @@ -223,6 +226,14 @@ func CreateNames(openstackControlplaneName types.NamespacedName) Names {
Namespace: openstackControlplaneName.Namespace,
Name: "cert-rabbitmq-cell1-svc",
},
RabbitMQNotificationsName: types.NamespacedName{
Namespace: openstackControlplaneName.Namespace,
Name: "rabbitmq-notifications",
},
RabbitMQNotificationsCertName: types.NamespacedName{
Namespace: openstackControlplaneName.Namespace,
Name: "cert-rabbitmq-notifications-svc",
},
NoVNCProxyCell1CertPublicRouteName: types.NamespacedName{
Name: "cert-nova-novncproxy-cell1-public-route",
Namespace: openstackControlplaneName.Namespace,
Expand Down Expand Up @@ -535,6 +546,9 @@ func GetDefaultOpenStackControlPlaneSpec() map[string]interface{} {
names.RabbitMQCell1Name.Name: map[string]interface{}{
"replicas": 1,
},
names.RabbitMQNotificationsName.Name: map[string]interface{}{
"replicas": 1,
},
}
galeraTemplate := map[string]interface{}{
names.DBName.Name: map[string]interface{}{
Expand Down Expand Up @@ -876,6 +890,15 @@ func GetNeutron(name types.NamespacedName) *neutronv1.NeutronAPI {
return instance
}

// GetNova
func GetNova(name types.NamespacedName) *novav1.Nova {
instance := &novav1.Nova{}
Eventually(func(g Gomega) {
g.Expect(k8sClient.Get(ctx, name, instance)).Should(Succeed())
}, timeout, interval).Should(Succeed())
return instance
}

// GetManila
func GetManila(name types.NamespacedName) *manilav1.Manila {
instance := &manilav1.Manila{}
Expand Down
Loading