HoustonPutman commented on code in PR #540:
URL: https://github.com/apache/solr-operator/pull/540#discussion_r1160939890
##########
controllers/solrcloud_controller_test.go:
##########
@@ -88,6 +88,12 @@ var _ = FDescribe("SolrCloud controller - General", func() {
InitContainers:
extraContainers2,
},
},
+ Availability:
solrv1beta1.SolrAvailabilityOptions{
Review Comment:
Since we want this enabled by default, I think we should remove this and
test that the default does its job.
Or, keep this and add the `expectPodDisruptionBudget(ctx, solrCloud,
solrCloud.StatefulSetName(), statefulSet.Spec.Selector,
intstr.FromString(util.DefaultMaxPodsUnavailable))` line to another test
(without removing the PDB, just do the existence check). Yeah I like this
option more. Keep what you have and add the PDB check to another test in this
file.
##########
config/crd/bases/solr.apache.org_solrclouds.yaml:
##########
@@ -89,6 +89,27 @@ spec:
items:
type: string
type: array
+ availability:
+ description: Define how Solr nodes should be available.
+ properties:
+ podDisruptionBudget:
+ description: Define PodDisruptionBudget(s) to ensure
availability
+ of Solr
+ properties:
+ enabled:
+ default: true
+ description: What method should be used when creating
PodDisruptionBudget(s)
+ type: boolean
+ method:
+ default: ClusterWide
+ description: What method should be used when creating
PodDisruptionBudget(s)
+ enum:
+ - ClusterWide
+ type: string
+ required:
+ - enabled
Review Comment:
I wonder why it has `enabled` but not `method`.... How strange. Nothing for
you to worry about if the tests are passing.
##########
api/v1beta1/solrcloud_types.go:
##########
@@ -758,6 +762,33 @@ type ManagedUpdateOptions struct {
MaxShardReplicasUnavailable *intstr.IntOrString
`json:"maxShardReplicasUnavailable,omitempty"`
}
+type SolrAvailabilityOptions struct {
+ // Define PodDisruptionBudget(s) to ensure availability of Solr
+ // +optional
+ PodDisruptionBudget SolrPodDisruptionBudgetOptions
`json:"podDisruptionBudget,omitempty"`
+}
+
+type SolrPodDisruptionBudgetOptions struct {
+ // What method should be used when creating PodDisruptionBudget(s)
+ // +kubebuilder:default=true
+ Enabled bool `json:"enabled"`
Review Comment:
Let's go ahead and make this a pointer. I think we should steer-clear of
booleans that default to true but are not pointers (or are a child of a
pointer-object, e.g. if PodDisruptionBudget was a pointer).
##########
docs/solr-cloud/solr-cloud-crd.md:
##########
@@ -99,8 +99,17 @@ Under `SolrCloud.Spec.updateStrategy`:
### Pod Disruption Budgets
_Since v0.7.0_
-The Solr Operator will create a
[`PodDisruptionBudget`](https://kubernetes.io/docs/concepts/workloads/pods/disruptions/#pod-disruption-budgets)
to ensure that Kubernetes does not take down more than acceptable amount of
SolrCloud nodes at a time.
-The PDB's `maxUnavailable` setting is populated from the `maxPodsUnavailable`
setting in `SolrCloud.Spec.updateStrategy.managed`.
+The Solr Operator can optionally create a
[`PodDisruptionBudget`](https://kubernetes.io/docs/concepts/workloads/pods/disruptions/#pod-disruption-budgets)
to ensure that Kubernetes does not take down more than acceptable amount of
SolrCloud nodes at a time.
Review Comment:
```suggestion
The Solr Operator can optionally create a
[`PodDisruptionBudget`](https://kubernetes.io/docs/concepts/workloads/pods/disruptions/#pod-disruption-budgets)
to ensure that Kubernetes does not take down more than an acceptable amount of
SolrCloud nodes at a time.
```
##########
helm/solr/templates/solrcloud.yaml:
##########
@@ -108,6 +108,12 @@ spec:
{{- end }}
{{- end }}
+ {{- if and (.Values.availability) (.Values.availability.podDisruptionBudget)
}}
Review Comment:
I don't think you need this `and`... I
think`.Values.availability.podDisruptionBudget` will return false if
`.Values.availability` doesn't exist.
##########
controllers/solrcloud_controller_test.go:
##########
@@ -176,6 +187,12 @@ var _ = FDescribe("SolrCloud controller - General", func()
{
},
RestartSchedule: "@every 30m",
},
+ Availability:
solrv1beta1.SolrAvailabilityOptions{
+ PodDisruptionBudget:
solrv1beta1.SolrPodDisruptionBudgetOptions{
+ Enabled: true,
Review Comment:
For this one, set `Enabled: false`. That way we can check that it never
actually creates one in the first place, and doesn't try to do a delete.
That way you can delete the two lines below:
```
expectPodDisruptionBudget(...)
expectSolrCloudWithChecks(...)
```
##########
controllers/solrcloud_controller.go:
##########
@@ -464,31 +464,37 @@ func (r *SolrCloudReconciler) Reconcile(ctx
context.Context, req ctrl.Request) (
}
}
- // PodDistruptionBudget(s)
+ // Upsert or delete solrcloud-wide PodDisruptionBudget(s) based on
'Enabled' flag.
pdb := util.GeneratePodDisruptionBudget(instance, pvcLabelSelector)
+ if instance.Spec.Availability.PodDisruptionBudget.Enabled {
+ // Check if the PodDistruptionBudget already exists
+ pdbLogger := logger.WithValues("podDisruptionBudget", pdb.Name)
+ foundPDB := &policyv1.PodDisruptionBudget{}
+ err = r.Get(ctx, types.NamespacedName{Name: pdb.Name,
Namespace: pdb.Namespace}, foundPDB)
+ if err != nil && errors.IsNotFound(err) {
+ pdbLogger.Info("Creating PodDisruptionBudget")
+ if err =
controllerutil.SetControllerReference(instance, pdb, r.Scheme); err == nil {
+ err = r.Create(ctx, pdb)
+ }
+ } else if err == nil {
+ var needsUpdate bool
+ needsUpdate, err = util.OvertakeControllerRef(instance,
foundPDB, r.Scheme)
+ needsUpdate = util.CopyPodDisruptionBudgetFields(pdb,
foundPDB, pdbLogger) || needsUpdate
- // Check if the PodDistruptionBudget already exists
- pdbLogger := logger.WithValues("podDisruptionBudget", pdb.Name)
- foundPDB := &policyv1.PodDisruptionBudget{}
- err = r.Get(ctx, types.NamespacedName{Name: pdb.Name, Namespace:
pdb.Namespace}, foundPDB)
- if err != nil && errors.IsNotFound(err) {
- pdbLogger.Info("Creating PodDisruptionBudget")
- if err = controllerutil.SetControllerReference(instance, pdb,
r.Scheme); err == nil {
- err = r.Create(ctx, pdb)
+ // Update the found PodDistruptionBudget and write the
result back if there are any changes
+ if needsUpdate && err == nil {
+ pdbLogger.Info("Updating PodDisruptionBudget")
+ err = r.Update(ctx, foundPDB)
+ }
}
- } else if err == nil {
- var needsUpdate bool
- needsUpdate, err = util.OvertakeControllerRef(instance,
foundPDB, r.Scheme)
- needsUpdate = util.CopyPodDisruptionBudgetFields(pdb, foundPDB,
pdbLogger) || needsUpdate
-
- // Update the found PodDistruptionBudget and write the result
back if there are any changes
- if needsUpdate && err == nil {
- pdbLogger.Info("Updating PodDisruptionBudget")
- err = r.Update(ctx, foundPDB)
+ if err != nil {
+ return requeueOrNot, err
+ }
+ } else { // PDB is disabled, make sure that we delete any previously
created pdb that might exist.
+ err = r.Client.Delete(ctx, pdb)
Review Comment:
Interesting... So it's basically as good to delete something that might not
exist as it is to check if it exists first before deleting it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
