-
Notifications
You must be signed in to change notification settings - Fork 213
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
INSIGHTS-157 - PDB <> HPA check (#1057)
* fix typo * fix failure message * fix changelog * fix missingPodDisruptionBudget validation * add tests for pdbMinAvailableLessThenHPAMaxReplicas * add simple success test * fix typo * lowercasing warnings * WIP implement pdbMinAvailableLessThanHPAMaxReplicas * change check name * rename testes * fix check message * change check name * minor fixes * improving tests * improve tests * fix check name * Update docs/checks/reliability.md Co-authored-by: Andy Suderman <[email protected]> * fix/add tests * fixes from PR * fix error message --------- Co-authored-by: Andy Suderman <[email protected]>
- Loading branch information
1 parent
875a8ff
commit 952b6ae
Showing
26 changed files
with
708 additions
and
23 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 changes: 7 additions & 0 deletions
7
pkg/config/checks/pdbMinAvailableGreaterThanHPAMinReplicas.yaml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| successMessage: PDB and HPA are correctly configured | ||
| failureMessage: PDB minAvailable is greater than HPA minReplicas | ||
| category: Reliability | ||
| target: Controller | ||
| controllers: | ||
| include: | ||
| - Deployment |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| package validator | ||
|
|
||
| import ( | ||
| "sync" | ||
|
|
||
| "github.com/qri-io/jsonschema" | ||
| ) | ||
|
|
||
| type validatorFunction func(test schemaTestCase) (bool, []jsonschema.ValError, error) | ||
|
|
||
| var validatorMapper = map[string]validatorFunction{} | ||
| var lock = &sync.Mutex{} | ||
|
|
||
| func registerCustomChecks(name string, check validatorFunction) { | ||
| lock.Lock() | ||
| defer lock.Unlock() | ||
|
|
||
| validatorMapper[name] = check | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| package validator | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "strconv" | ||
| "strings" | ||
|
|
||
| "github.com/fairwindsops/polaris/pkg/kube" | ||
| "github.com/qri-io/jsonschema" | ||
| "github.com/sirupsen/logrus" | ||
| appsv1 "k8s.io/api/apps/v1" | ||
| autoscalingv1 "k8s.io/api/autoscaling/v1" | ||
| policyv1 "k8s.io/api/policy/v1" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "k8s.io/apimachinery/pkg/util/intstr" | ||
| ) | ||
|
|
||
| func init() { | ||
| registerCustomChecks("pdbMinAvailableGreaterThanHPAMinReplicas", pdbMinAvailableGreaterThanHPAMinReplicas) | ||
| } | ||
|
|
||
| func pdbMinAvailableGreaterThanHPAMinReplicas(test schemaTestCase) (bool, []jsonschema.ValError, error) { | ||
| if test.ResourceProvider == nil { | ||
| logrus.Debug("ResourceProvider is nil") | ||
| return true, nil, nil | ||
| } | ||
|
|
||
| deployment := &appsv1.Deployment{} | ||
| err := runtime.DefaultUnstructuredConverter.FromUnstructured(test.Resource.Resource.Object, deployment) | ||
| if err != nil { | ||
| logrus.Warnf("error converting unstructured to Deployment: %v", err) | ||
| return true, nil, nil | ||
| } | ||
|
|
||
| attachedPDB, err := hasPDBAttached(*deployment, test.ResourceProvider.Resources["policy/PodDisruptionBudget"]) | ||
| if err != nil { | ||
| logrus.Warnf("error getting PodDisruptionBudget: %v", err) | ||
| return true, nil, nil | ||
| } | ||
|
|
||
| attachedHPA, err := hasHPAAttached(*deployment, test.ResourceProvider.Resources["autoscaling/HorizontalPodAutoscaler"]) | ||
| if err != nil { | ||
| logrus.Warnf("error getting HorizontalPodAutoscaler: %v", err) | ||
| return true, nil, nil | ||
| } | ||
|
|
||
| if attachedPDB != nil && attachedHPA != nil { | ||
| logrus.Debugf("both PDB and HPA are attached to deployment %s", deployment.Name) | ||
|
|
||
| pdbMinAvailable, isPercent, err := getIntOrPercentValueSafely(attachedPDB.Spec.MinAvailable) | ||
| if err != nil { | ||
| logrus.Warnf("error getting getIntOrPercentValueSafely: %v", err) | ||
| return true, nil, nil | ||
| } | ||
|
|
||
| if isPercent { | ||
| // if the value is a percentage, we need to calculate the actual value | ||
| if attachedHPA.Spec.MinReplicas == nil { | ||
| logrus.Debug("attachedHPA.Spec.MinReplicas is nil") | ||
| return true, nil, nil | ||
| } | ||
|
|
||
| pdbMinAvailable, err = intstr.GetScaledValueFromIntOrPercent(attachedPDB.Spec.MinAvailable, int(*attachedHPA.Spec.MinReplicas), true) | ||
| if err != nil { | ||
| logrus.Warnf("error getting minAvailable value from PodDisruptionBudget: %v", err) | ||
| return true, nil, nil | ||
| } | ||
| } | ||
|
|
||
| if attachedHPA.Spec.MinReplicas != nil && pdbMinAvailable >= int(*attachedHPA.Spec.MinReplicas) { | ||
| return false, []jsonschema.ValError{ | ||
| { | ||
| PropertyPath: "spec.minAvailable", | ||
| InvalidValue: pdbMinAvailable, | ||
| Message: fmt.Sprintf("The minAvailable value in the PodDisruptionBudget(%s) is %d, which is greater or equal than the minReplicas value in the HorizontalPodAutoscaler(%s) (%d)", attachedPDB.Name, pdbMinAvailable, attachedHPA.Name, *attachedHPA.Spec.MinReplicas), | ||
| }, | ||
| }, nil | ||
| } | ||
| } | ||
|
|
||
| return true, nil, nil | ||
| } | ||
|
|
||
| func hasPDBAttached(deployment appsv1.Deployment, pdbs []kube.GenericResource) (*policyv1.PodDisruptionBudget, error) { | ||
| for _, generic := range pdbs { | ||
| pdb := &policyv1.PodDisruptionBudget{} | ||
| err := runtime.DefaultUnstructuredConverter.FromUnstructured(generic.Resource.Object, pdb) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error converting unstructured to PodDisruptionBudget: %v", err) | ||
| } | ||
|
|
||
| if pdb.Spec.Selector == nil { | ||
| logrus.Debug("pdb.Spec.Selector is nil") | ||
| continue | ||
| } | ||
|
|
||
| if matchesPDBForDeployment(deployment.Spec.Template.Labels, pdb.Spec.Selector.MatchLabels) { | ||
| return pdb, nil | ||
| } | ||
| } | ||
| return nil, nil | ||
| } | ||
|
|
||
| // matchesPDBForDeployment checks if the labels of the deployment match the labels of the PDB | ||
| func matchesPDBForDeployment(deploymentLabels, pdbLabels map[string]string) bool { | ||
| for key, value := range pdbLabels { | ||
| if deploymentLabels[key] == value { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| func hasHPAAttached(deployment appsv1.Deployment, hpas []kube.GenericResource) (*autoscalingv1.HorizontalPodAutoscaler, error) { | ||
| for _, generic := range hpas { | ||
| hpa := &autoscalingv1.HorizontalPodAutoscaler{} | ||
| err := runtime.DefaultUnstructuredConverter.FromUnstructured(generic.Resource.Object, hpa) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error converting unstructured to HorizontalPodAutoscaler: %v", err) | ||
| } | ||
|
|
||
| if hpa.Spec.ScaleTargetRef.Kind == "Deployment" && hpa.Spec.ScaleTargetRef.Name == deployment.Name { | ||
| return hpa, nil | ||
| } | ||
| } | ||
| return nil, nil | ||
| } | ||
|
|
||
| // getIntOrPercentValueSafely is a safer version of getIntOrPercentValue based on private function intstr.getIntOrPercentValueSafely | ||
| func getIntOrPercentValueSafely(intOrStr *intstr.IntOrString) (int, bool, error) { | ||
| switch intOrStr.Type { | ||
| case intstr.Int: | ||
| return intOrStr.IntValue(), false, nil | ||
| case intstr.String: | ||
| isPercent := false | ||
| s := intOrStr.StrVal | ||
| if strings.HasSuffix(s, "%") { | ||
| isPercent = true | ||
| s = strings.TrimSuffix(intOrStr.StrVal, "%") | ||
| } else { | ||
| return 0, false, fmt.Errorf("invalid type: string is not a percentage") | ||
| } | ||
| v, err := strconv.Atoi(s) | ||
| if err != nil { | ||
| return 0, false, fmt.Errorf("invalid value %q: %v", intOrStr.StrVal, err) | ||
| } | ||
| return int(v), isPercent, nil | ||
| } | ||
| return 0, false, fmt.Errorf("invalid type: neither int nor percentage") | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
44 changes: 44 additions & 0 deletions
44
test/checks/pdbMinAvailableGreaterThanHPAMinReplicas/failure-gt-percent.yaml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| apiVersion: apps/v1 | ||
| kind: Deployment | ||
| metadata: | ||
| name: zookeeper | ||
| spec: | ||
| replicas: 10 | ||
| template: | ||
| metadata: | ||
| labels: | ||
| app.kubernetes.io/name: zookeeper | ||
| foo: bar | ||
| spec: | ||
| containers: | ||
| - name: zookeeper | ||
| image: zookeeper | ||
| --- | ||
| apiVersion: policy/v1 | ||
| kind: PodDisruptionBudget | ||
| metadata: | ||
| name: zookeeper-pdb | ||
| spec: | ||
| minAvailable: 150% # 1.5 * 10 = 15 | ||
| selector: | ||
| matchLabels: | ||
| app.kubernetes.io/name: zookeeper | ||
| --- | ||
| apiVersion: autoscaling/v2 | ||
| kind: HorizontalPodAutoscaler | ||
| metadata: | ||
| name: zookeeper-hpa | ||
| spec: | ||
| scaleTargetRef: | ||
| apiVersion: apps/v1 | ||
| kind: Deployment | ||
| name: zookeeper | ||
| minReplicas: 10 | ||
| maxReplicas: 15 | ||
| metrics: | ||
| - type: Resource | ||
| resource: | ||
| name: cpu | ||
| target: | ||
| type: Utilization | ||
| averageUtilization: 50 |
Oops, something went wrong.