You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "HoustonPutman (via GitHub)" <gi...@apache.org> on 2023/04/07 20:29:30 UTC
[GitHub] [solr-operator] HoustonPutman commented on a diff in pull request #540: Make PodDisruptionBudget an optional feature
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org