Add PodDisruptionBudget to router components #346
Conversation
ariefrahmansyah
requested review from
deadlycoconuts and
krithika369
and removed request for
deadlycoconuts
There was a problem hiding this comment.
Left some questions / comments. The rest LGTM. Thanks, @ariefrahmansyah !
| - apiGroups: | ||
| - policy | ||
| resources: | ||
| - poddisruptionbudgets |
There was a problem hiding this comment.
Btw, we'd need to make this change in the chart specs in the helm-charts repo too.
| @@ -443,7 +453,7 @@ func (c *OpenapiConfig) GenerateSpecFile() error { | |||
| return err | |||
| } | |||
|
|
|||
| err = os.MkdirAll(filepath.Dir(c.MergedSpecFile), 0755) | |||
| err = os.MkdirAll(filepath.Dir(c.MergedSpecFile), 0o755) | |||
There was a problem hiding this comment.
Not specifically about this change, but it would be useful to leave an inline comment about what the file mode does and why it's being set.
api/turing/config/config.go
Outdated
| type PodDisruptionBudgetConfig struct { | ||
| Enabled bool | ||
| // Can specify only one of maxUnavailable and minAvailable | ||
| MaxUnavailable string |
There was a problem hiding this comment.
Shall we instead name this as MaxUnavailablePercent and use *int ? Or, if we are sticking with the string type, should we check that the value is of the expected format (like "20%")? Would be nice to add some checks around this as well because otherwise, the earlier we can discover a problem with this config is when we deploy a router, which may be too late.
There was a problem hiding this comment.
LGTM. Thanks for the changes, @ariefrahmansyah ! Please feel free to merge once the linter errors are addressed.
api/turing/cluster/controller.go
Outdated
| pdbCfg.WithLabels(pdb.Labels) | ||
| pdbCfg.WithSpec(pdbSpec) | ||
|
|
||
| pdbObj, err := c.k8sPolicyClient.PodDisruptionBudgets(pdb.Namespace).Apply(ctx, pdbCfg, metav1.ApplyOptions{FieldManager: "application/apply-patch"}) |
There was a problem hiding this comment.
For Turing, we wouldn't need to "Update" an existing k8s resource - it's always create. In any case, I suppose it's ok to use Apply as it covers both scenarios.
- Add PDB config - Add component label - Modify updateFunc to use generic
This PR will add PodDisruptionBudget resources to Turing components (router, enricher, and ensembler).
Modifications:
gotestto Makefile and GitHub actionsa. In GitHub actions, add
make setupto installgotest