extend the EntityOperator, CruiseControl and KafkaExporter to support PDB#11699
extend the EntityOperator, CruiseControl and KafkaExporter to support PDB#11699katheris merged 11 commits intostrimzi:mainfrom
Conversation
|
i tested locally and all of the tests did pass, also i deployed it locally to a kind cluster and saw that a PDB was created for each with |
scholzj
left a comment
There was a problem hiding this comment.
Thanks for the PR. I left some comments, but it looks mostly good. 👍
|
/azp run regression |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@scholzj updated the PR to support CC as well, |
scholzj
left a comment
There was a problem hiding this comment.
Few more nits. But LGTM otherwise. Thanks.
|
/azp run regression |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@katheris Any chance you can have a look at this please? |
katheris
left a comment
There was a problem hiding this comment.
Thanks for the PR @KyriosGN0. I found a couple of JavaDoc nits, but also it looks like the step to create the Pod Disruption Budget is in the wrong place for Cruise Control reconciler.
Finally I wanted to confirm my understanding of the change with regards to Cruise Control. The Cruise Control template already supports Pod Disruption Budget, so is that just being ignored in the main branch today?
| result.templateService = template.getApiService(); | ||
| result.templateServiceAccount = template.getServiceAccount(); | ||
| result.templateContainer = template.getCruiseControlContainer(); | ||
| result.templatePodDisruptionBudget = template.getPodDisruptionBudget(); |
There was a problem hiding this comment.
The getPodDisruptionBudget() method is already present in Cruise Control, so is the current state that Cruise Control had it as part of the template, but we were ignoring it? @kyguy can you confirm whether PodDisruptionBudget is already supported for Cruise Control?
There was a problem hiding this comment.
The getPodDisruptionBudget() method is already present in Cruise Control, so is the current state that Cruise Control had it as part of the template, but we were ignoring it?
Yes, we were ignoring it. It looks like I added the template years ago but never added the spec or reconciliation bits.
can you confirm whether PodDisruptionBudget is already supported for Cruise Control?
I can confirm it is not currently supported for Cruise Control.
| The `strimzi.io/node-pools` and `strimzi.io/kraft` annotations are not required anymore and will be ignored if set. | ||
| * Make properties `broker.session.timeout.ms`, `broker.heartbeat.interval.ms` and `controller.socket.timeout.ms` configurable | ||
| * Add monitoring of custom resources using [kubernetes-state-metrics (KSM)](https://github.com/kubernetes/kube-state-metrics) (see [Strimzi proposal 087](https://github.com/strimzi/proposals/blob/main/087-monitoring-of-custom-resources.md)) | ||
| * Extend the EntityOperator, Cruise Control and KafkaExporter deployment to support PDB. |
There was a problem hiding this comment.
I think this could be a little clearer, e.g. Extend the EntityOperator, Cruise Control and KafkaExporter deployment to support PDB via the template section in the CR spec.
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
|
@katheris Thanks for the review, i addressed your comments, and yes, i think that up until now the PDB of CC was in the CRD but it was ignored, but i never tried it so i cannot confirm 100% |
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
katheris
left a comment
There was a problem hiding this comment.
Thanks @KyriosGN0 I'm happy with these changes. I've asked @kyguy to take a look at the Cruise Control parts so will hold off merging until he's reviewed
There was a problem hiding this comment.
@katheris The Cruise Control parts look good! Nice work @KyriosGN0
| result.templateService = template.getApiService(); | ||
| result.templateServiceAccount = template.getServiceAccount(); | ||
| result.templateContainer = template.getCruiseControlContainer(); | ||
| result.templatePodDisruptionBudget = template.getPodDisruptionBudget(); |
There was a problem hiding this comment.
The getPodDisruptionBudget() method is already present in Cruise Control, so is the current state that Cruise Control had it as part of the template, but we were ignoring it?
Yes, we were ignoring it. It looks like I added the template years ago but never added the spec or reconciliation bits.
can you confirm whether PodDisruptionBudget is already supported for Cruise Control?
I can confirm it is not currently supported for Cruise Control.
|
Thanks for the PR @KyriosGN0 |
… PDB (strimzi#11699) Signed-off-by: AvivGuiser <avivguiser@gmail.com>
Type of change
Select the type of your PR
Description
This PR adds PDB for EntityOperator, CruiseControl and KafkaExporter deployment
fixes #11402
Checklist
Please go through this checklist and make sure all applicable tasks have been done