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/03/31 19:24:58 UTC
[GitHub] [solr-operator] HoustonPutman opened a new pull request, #540: Add CRD options, helm options and a changelog entry
HoustonPutman opened a new pull request, #540:
URL: https://github.com/apache/solr-operator/pull/540
resolves https://github.com/apache/solr-operator/issues/538
List to do:
- [ ] Create CRD Options
- [ ] Implement feature in controllers
- [ ] Add unit tests
- [ ] Add Helm values and docs
- [ ] Implement Helm values in SolrCloud creation
- [ ] Add docs
- [ ] Add changelog
--
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
[GitHub] [solr-operator] gerlowskija merged pull request #540: Make PodDisruptionBudget an optional feature
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija merged PR #540:
URL: https://github.com/apache/solr-operator/pull/540
--
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
[GitHub] [solr-operator] HoustonPutman commented on a diff in pull request #540: Make PodDisruptionBudget an optional feature
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
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
[GitHub] [solr-operator] gerlowskija commented on a diff in pull request #540: Make PodDisruptionBudget an optional feature
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #540:
URL: https://github.com/apache/solr-operator/pull/540#discussion_r1161646201
##########
helm/solr/templates/solrcloud.yaml:
##########
@@ -108,6 +108,12 @@ spec:
{{- end }}
{{- end }}
+ {{- if and (.Values.availability) (.Values.availability.podDisruptionBudget) }}
Review Comment:
I actually tried that first - I get a helm parse error.
```
$ helm template .
Error: template: solr/templates/solrcloud.yaml:111:16: executing "solr/templates/solrcloud.yaml" at <.Values.availability.podDisruptionBudget>: nil pointer evaluating interface {}.podDisruptionBudget
```
--
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
[GitHub] [solr-operator] gerlowskija commented on pull request #540: Make PodDisruptionBudget an optional feature
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #540:
URL: https://github.com/apache/solr-operator/pull/540#issuecomment-1499845308
Taking a look at this now as a way to brush up on operator code. Thanks for all the work you put into documenting this Houston!
--
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
[GitHub] [solr-operator] HoustonPutman commented on a diff in pull request #540: Make PodDisruptionBudget an optional feature
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #540:
URL: https://github.com/apache/solr-operator/pull/540#discussion_r1161718687
##########
helm/solr/templates/solrcloud.yaml:
##########
@@ -108,6 +108,12 @@ spec:
{{- end }}
{{- end }}
+ {{- if and (.Values.availability) (.Values.availability.podDisruptionBudget) }}
Review Comment:
Ahh nice, that makes sense!
--
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
[GitHub] [solr-operator] gerlowskija commented on pull request #540: Make PodDisruptionBudget an optional feature
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #540:
URL: https://github.com/apache/solr-operator/pull/540#issuecomment-1500600300
Alright, should be ready for review. Lmk if I missed anything!
--
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