diff --git a/api/cluster/pdb.go b/api/cluster/pdb.go index 758c5b09c..26e5013ee 100644 --- a/api/cluster/pdb.go +++ b/api/cluster/pdb.go @@ -20,7 +20,6 @@ type PodDisruptionBudget struct { Labels map[string]string MaxUnavailablePercentage *int MinAvailablePercentage *int - Selector *metav1.LabelSelector } func NewPodDisruptionBudget(modelService *models.Service, componentType string, pdbConfig config.PodDisruptionBudgetConfig) *PodDisruptionBudget { @@ -47,10 +46,10 @@ func (cfg PodDisruptionBudget) BuildPDBSpec() (*policyv1cfg.PodDisruptionBudgetS // Since we can specify only one of maxUnavailable and minAvailable, minAvailable takes precedence // https://kubernetes.io/docs/tasks/run-application/configure-pdb/#specifying-a-poddisruptionbudget if cfg.MinAvailablePercentage != nil { - minAvailable := intstr.FromInt(*cfg.MinAvailablePercentage) + minAvailable := intstr.FromString(fmt.Sprintf("%d%%", *cfg.MinAvailablePercentage)) pdbSpec.MinAvailable = &minAvailable } else if cfg.MaxUnavailablePercentage != nil { - maxUnavailable := intstr.FromInt(*cfg.MaxUnavailablePercentage) + maxUnavailable := intstr.FromString(fmt.Sprintf("%d%%", *cfg.MaxUnavailablePercentage)) pdbSpec.MaxUnavailable = &maxUnavailable } @@ -90,7 +89,7 @@ func (c *controller) deployPodDisruptionBudget(ctx context.Context, pdb *PodDisr pdbCfg.WithLabels(pdb.Labels) pdbCfg.WithSpec(pdbSpec) - _, err = c.policyClient.PodDisruptionBudgets(pdb.Namespace).Apply(ctx, pdbCfg, metav1.ApplyOptions{}) + _, err = c.policyClient.PodDisruptionBudgets(pdb.Namespace).Apply(ctx, pdbCfg, metav1.ApplyOptions{FieldManager: "application/apply-patch"}) if err != nil { return err } diff --git a/api/cluster/pdb_test.go b/api/cluster/pdb_test.go new file mode 100644 index 000000000..f250bfa94 --- /dev/null +++ b/api/cluster/pdb_test.go @@ -0,0 +1,100 @@ +package cluster + +import ( + "reflect" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + metav1cfg "k8s.io/client-go/applyconfigurations/meta/v1" + policyv1cfg "k8s.io/client-go/applyconfigurations/policy/v1" +) + +func TestPodDisruptionBudget_BuildPDBSpec(t *testing.T) { + defaultInt := 20 + defaultIntOrString := intstr.FromString("20%") + defaultLabels := map[string]string{ + "gojek.com/app": "sklearn-sample-s", + } + + type fields struct { + Name string + Namespace string + Labels map[string]string + MaxUnavailablePercentage *int + MinAvailablePercentage *int + Selector *metav1.LabelSelector + } + tests := []struct { + name string + fields fields + want *policyv1cfg.PodDisruptionBudgetSpecApplyConfiguration + wantErr bool + }{ + { + name: "valid: enabled with min_available", + fields: fields{ + Name: "sklearn-sample-s-1-model-pdb", + Namespace: "pdb-test", + Labels: defaultLabels, + MaxUnavailablePercentage: nil, + MinAvailablePercentage: &defaultInt, + }, + want: &policyv1cfg.PodDisruptionBudgetSpecApplyConfiguration{ + MinAvailable: &defaultIntOrString, + Selector: &metav1cfg.LabelSelectorApplyConfiguration{ + MatchLabels: defaultLabels, + }, + }, + wantErr: false, + }, + { + name: "valid: enabled but both max_unavailable and min_available specified. will use min available", + fields: fields{ + Name: "sklearn-sample-s-1-model-pdb", + Namespace: "pdb-test", + Labels: defaultLabels, + MaxUnavailablePercentage: &defaultInt, + MinAvailablePercentage: &defaultInt, + }, + want: &policyv1cfg.PodDisruptionBudgetSpecApplyConfiguration{ + MinAvailable: &defaultIntOrString, + Selector: &metav1cfg.LabelSelectorApplyConfiguration{ + MatchLabels: defaultLabels, + }, + }, + wantErr: false, + }, + { + name: "invalid: enabled but no max_unavailable and min_available", + fields: fields{ + Name: "sklearn-sample-s-1-model-pdb", + Namespace: "pdb-test", + Labels: map[string]string{}, + MaxUnavailablePercentage: nil, + MinAvailablePercentage: nil, + }, + want: nil, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := PodDisruptionBudget{ + Name: tt.fields.Name, + Namespace: tt.fields.Namespace, + Labels: tt.fields.Labels, + MaxUnavailablePercentage: tt.fields.MaxUnavailablePercentage, + MinAvailablePercentage: tt.fields.MinAvailablePercentage, + } + got, err := cfg.BuildPDBSpec() + if (err != nil) != tt.wantErr { + t.Errorf("PodDisruptionBudget.BuildPDBSpec() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("PodDisruptionBudget.BuildPDBSpec() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/api/config/environment_test.go b/api/config/environment_test.go index 0de264612..6bc1c3d1e 100644 --- a/api/config/environment_test.go +++ b/api/config/environment_test.go @@ -100,7 +100,7 @@ func TestUnmarshalTopologySpreadConstraints(t *testing.T) { } func TestPDBConfig(t *testing.T) { - defaultMaxUnavailablePercentage := 20 + defaultMinAvailablePercentage := 20 testCases := []struct { desc string @@ -112,8 +112,8 @@ func TestPDBConfig(t *testing.T) { desc: "Success: valid pdb config", envConfigPath: "./testdata/valid-environment-1.yaml", expectedPDBConfig: PodDisruptionBudgetConfig{ - Enabled: true, - MaxUnavailablePercentage: &defaultMaxUnavailablePercentage, + Enabled: true, + MinAvailablePercentage: &defaultMinAvailablePercentage, }, }, { diff --git a/api/config/testdata/valid-environment-1.yaml b/api/config/testdata/valid-environment-1.yaml index 5e05f708c..7bb15d947 100644 --- a/api/config/testdata/valid-environment-1.yaml +++ b/api/config/testdata/valid-environment-1.yaml @@ -11,7 +11,7 @@ max_memory: "8Gi" pod_disruption_budget: enabled: true - max_unavailable_percentage: 20 + min_available_percentage: 20 queue_resource_percentage: "20" default_prediction_job_config: executor_replica: 3