Skip to content

Commit 552f37d

Browse files
lmiccinilmiccini
committed
Move queueType default selection to infra-operator
This commit implements the following: - switches the default queueType value to nil (instead of Mirrored) - adds logic to the webhook so that when queueType=nil this is instead set to "Quorum" This would allow us to: - set a sane default for new clusters, unless the user decides otherwise - preserve existing clusters where queueType=Mirrored is configured - orchestrate migration from Mirrored to Quorum by adding logic that would react to the change of value Jira: https://issues.redhat.com/browse/OSPRH-21039
1 parent 7086a5e commit 552f37d

File tree

7 files changed

+111
-49
lines changed

7 files changed

+111
-49
lines changed

‎apis/bases/rabbitmq.openstack.org_rabbitmqs.yaml‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1401,13 +1401,9 @@ spec:
14011401
type: string
14021402
type: object
14031403
queueType:
1404-
default: Mirrored
1405-
description: QueueType to eventually apply the ha-all policy or configure
1406-
default queue type for the cluster
1407-
enum:
1408-
- None
1409-
- Mirrored
1410-
- Quorum
1404+
description: |-
1405+
QueueType to eventually apply the ha-all policy or configure default queue type for the cluster.
1406+
Allowed values are: None, Mirrored, Quorum. Defaults to Quorum if not specified.
14111407
type: string
14121408
rabbitmq:
14131409
description: Configuration options for RabbitMQ Pods created in the

‎apis/rabbitmq/v1beta1/rabbitmq_types.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,10 +66,10 @@ type RabbitMqSpecCore struct {
6666
// by name
6767
TopologyRef *topologyv1.TopoRef `json:"topologyRef,omitempty"`
6868
// +kubebuilder:validation:Optional
69-
// +kubebuilder:validation:Enum=None;Mirrored;Quorum
70-
// +kubebuilder:default=Mirrored
71-
// QueueType to eventually apply the ha-all policy or configure default queue type for the cluster
72-
QueueType string `json:"queueType"`
69+
// +operator-sdk:csv:customresourcedefinitions:type=spec
70+
// QueueType to eventually apply the ha-all policy or configure default queue type for the cluster.
71+
// Allowed values are: None, Mirrored, Quorum. Defaults to Quorum if not specified.
72+
QueueType *string `json:"queueType,omitempty"`
7373
}
7474

7575
// Method to convert RabbitMqSpec to RabbitmqClusterSpec

‎apis/rabbitmq/v1beta1/rabbitmq_webhook.go‎

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,11 @@ func (spec *RabbitMqSpec) Default() {
7272

7373
// Default - common validations go here (for the OpenStackControlplane which uses this one)
7474
func (spec *RabbitMqSpecCore) Default() {
75-
//nothing to validate yet
75+
// Set a sensible default for QueueType only when not explicitly provided
76+
if spec.QueueType == nil || *spec.QueueType == "" {
77+
queueType := "Quorum"
78+
spec.QueueType = &queueType
79+
}
7680
}
7781

7882
// TODO(user): change verbs to "verbs=create;update;delete" if you want to enable deletion validation.
@@ -101,6 +105,9 @@ func (r *RabbitMq) ValidateCreate() (admission.Warnings, error) {
101105
CrMaxLengthCorrection,
102106
)...) // omit issue with statefulset pod label "controller-revision-hash": "<statefulset_name>-<hash>"
103107

108+
// Validate QueueType if specified
109+
allErrs = append(allErrs, r.Spec.ValidateQueueType(basePath)...)
110+
104111
if len(allErrs) != 0 {
105112
return allWarn, apierrors.NewInvalid(
106113
schema.GroupKind{Group: "rabbitmq.openstack.org", Kind: "RabbitMq"},
@@ -128,6 +135,9 @@ func (r *RabbitMq) ValidateUpdate(_ runtime.Object) (admission.Warnings, error)
128135
allWarn = append(allWarn, warn...)
129136
allErrs = append(allErrs, errs...)
130137

138+
// Validate QueueType if specified
139+
allErrs = append(allErrs, r.Spec.ValidateQueueType(basePath)...)
140+
131141
if len(allErrs) != 0 {
132142
return allWarn, apierrors.NewInvalid(
133143
schema.GroupKind{Group: "rabbitmq.openstack.org", Kind: "RabbitMq"},
@@ -173,3 +183,28 @@ func (r *RabbitMq) ValidateDelete() (admission.Warnings, error) {
173183
// TODO(user): fill in your validation logic upon object deletion.
174184
return nil, nil
175185
}
186+
187+
// ValidateQueueType validates that QueueType is one of the allowed values
188+
func (spec *RabbitMqSpec) ValidateQueueType(basePath *field.Path) field.ErrorList {
189+
var allErrs field.ErrorList
190+
191+
if spec.QueueType != nil {
192+
allowedValues := []string{"None", "Mirrored", "Quorum"}
193+
isValid := false
194+
for _, allowed := range allowedValues {
195+
if *spec.QueueType == allowed {
196+
isValid = true
197+
break
198+
}
199+
}
200+
if !isValid {
201+
allErrs = append(allErrs, field.NotSupported(
202+
basePath.Child("queueType"),
203+
*spec.QueueType,
204+
allowedValues,
205+
))
206+
}
207+
}
208+
209+
return allErrs
210+
}

‎apis/rabbitmq/v1beta1/zz_generated.deepcopy.go‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎config/crd/bases/rabbitmq.openstack.org_rabbitmqs.yaml‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1401,13 +1401,9 @@ spec:
14011401
type: string
14021402
type: object
14031403
queueType:
1404-
default: Mirrored
1405-
description: QueueType to eventually apply the ha-all policy or configure
1406-
default queue type for the cluster
1407-
enum:
1408-
- None
1409-
- Mirrored
1410-
- Quorum
1404+
description: |-
1405+
QueueType to eventually apply the ha-all policy or configure default queue type for the cluster.
1406+
Allowed values are: None, Mirrored, Quorum. Defaults to Quorum if not specified.
14111407
type: string
14121408
rabbitmq:
14131409
description: Configuration options for RabbitMQ Pods created in the

‎controllers/rabbitmq/rabbitmq_controller.go‎

Lines changed: 33 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -425,40 +425,43 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ct
425425
instance.Status.Conditions.MarkTrue(condition.PDBReadyCondition, condition.PDBReadyMessage)
426426

427427
// Let's wait DeploymentReadyCondition=True to apply the policy
428-
if instance.Spec.QueueType == "Mirrored" && *instance.Spec.Replicas > 1 && instance.Status.QueueType != "Mirrored" {
429-
Log.Info("ha-all policy not present. Applying.")
430-
err := updateMirroredPolicy(ctx, helper, instance, r.config, true)
431-
if err != nil {
432-
Log.Error(err, "Could not apply ha-all policy")
433-
instance.Status.Conditions.Set(condition.FalseCondition(
434-
condition.DeploymentReadyCondition,
435-
condition.ErrorReason,
436-
condition.SeverityWarning,
437-
condition.DeploymentReadyErrorMessage, err.Error()))
438-
return ctrl.Result{}, err
428+
// QueueType should never be nil due to webhook defaulting, but add safety check
429+
if instance.Spec.QueueType != nil {
430+
if *instance.Spec.QueueType == "Mirrored" && *instance.Spec.Replicas > 1 && instance.Status.QueueType != "Mirrored" {
431+
Log.Info("ha-all policy not present. Applying.")
432+
err := updateMirroredPolicy(ctx, helper, instance, r.config, true)
433+
if err != nil {
434+
Log.Error(err, "Could not apply ha-all policy")
435+
instance.Status.Conditions.Set(condition.FalseCondition(
436+
condition.DeploymentReadyCondition,
437+
condition.ErrorReason,
438+
condition.SeverityWarning,
439+
condition.DeploymentReadyErrorMessage, err.Error()))
440+
return ctrl.Result{}, err
441+
}
442+
} else if *instance.Spec.QueueType != "Mirrored" && instance.Status.QueueType == "Mirrored" {
443+
Log.Info("Removing ha-all policy")
444+
err := updateMirroredPolicy(ctx, helper, instance, r.config, false)
445+
if err != nil {
446+
Log.Error(err, "Could not remove ha-all policy")
447+
instance.Status.Conditions.Set(condition.FalseCondition(
448+
condition.DeploymentReadyCondition,
449+
condition.ErrorReason,
450+
condition.SeverityWarning,
451+
condition.DeploymentReadyErrorMessage, err.Error()))
452+
return ctrl.Result{}, err
453+
}
439454
}
440-
} else if instance.Spec.QueueType != "Mirrored" && instance.Status.QueueType == "Mirrored" {
441-
Log.Info("Removing ha-all policy")
442-
err := updateMirroredPolicy(ctx, helper, instance, r.config, false)
443-
if err != nil {
444-
Log.Error(err, "Could not remove ha-all policy")
445-
instance.Status.Conditions.Set(condition.FalseCondition(
446-
condition.DeploymentReadyCondition,
447-
condition.ErrorReason,
448-
condition.SeverityWarning,
449-
condition.DeploymentReadyErrorMessage, err.Error()))
450-
return ctrl.Result{}, err
455+
456+
// Update status for Quorum queue type
457+
if *instance.Spec.QueueType == "Quorum" && instance.Status.QueueType != "Quorum" {
458+
Log.Info("Setting queue type status to quorum")
459+
} else if *instance.Spec.QueueType != "Quorum" && instance.Status.QueueType == "Quorum" {
460+
Log.Info("Removing quorum queue type status")
451461
}
452-
}
453462

454-
// Update status for Quorum queue type
455-
if instance.Spec.QueueType == "Quorum" && instance.Status.QueueType != "Quorum" {
456-
Log.Info("Setting queue type status to quorum")
457-
} else if instance.Spec.QueueType != "Quorum" && instance.Status.QueueType == "Quorum" {
458-
Log.Info("Removing quorum queue type status")
463+
instance.Status.QueueType = *instance.Spec.QueueType
459464
}
460-
461-
instance.Status.QueueType = instance.Spec.QueueType
462465
}
463466

464467
if instance.Status.Conditions.AllSubConditionIsTrue() {

‎tests/functional/rabbitmq_controller_test.go‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,33 @@ var _ = Describe("RabbitMQ Controller", func() {
5555
DeferCleanup(th.DeleteConfigMap, clusterCm)
5656
})
5757

58+
When("QueueType defaulting and explicit values", func() {
59+
It("defaults QueueType to Quorum when unspecified", func() {
60+
spec := GetDefaultRabbitMQSpec()
61+
rabbitmq := CreateRabbitMQ(rabbitmqName, spec)
62+
DeferCleanup(th.DeleteInstance, rabbitmq)
63+
64+
Eventually(func(g Gomega) {
65+
instance := GetRabbitMQ(rabbitmqName)
66+
g.Expect(instance.Spec.QueueType).ToNot(BeNil())
67+
g.Expect(*instance.Spec.QueueType).To(Equal("Quorum"))
68+
}, timeout, interval).Should(Succeed())
69+
})
70+
71+
It("preserves explicitly set QueueType", func() {
72+
spec := GetDefaultRabbitMQSpec()
73+
spec["queueType"] = "Mirrored"
74+
rabbitmq := CreateRabbitMQ(rabbitmqName, spec)
75+
DeferCleanup(th.DeleteInstance, rabbitmq)
76+
77+
Eventually(func(g Gomega) {
78+
instance := GetRabbitMQ(rabbitmqName)
79+
g.Expect(instance.Spec.QueueType).ToNot(BeNil())
80+
g.Expect(*instance.Spec.QueueType).To(Equal("Mirrored"))
81+
}, timeout, interval).Should(Succeed())
82+
})
83+
})
84+
5885
When("a default RabbitMQ gets created", func() {
5986
BeforeEach(func() {
6087
rabbitmq := CreateRabbitMQ(rabbitmqName, GetDefaultRabbitMQSpec())

0 commit comments

Comments
 (0)