diff --git a/api/cluster/controller.go b/api/cluster/controller.go index 1e64a7b0d..7a838e520 100644 --- a/api/cluster/controller.go +++ b/api/cluster/controller.go @@ -251,7 +251,8 @@ func (c *controller) Deploy(ctx context.Context, modelService *models.Service) ( } if c.deploymentConfig.PodDisruptionBudget.Enabled { - pdbs := createPodDisruptionBudgets(modelService, c.deploymentConfig.PodDisruptionBudget) + // Create / update pdb + pdbs := generatePDBSpecs(modelService, c.deploymentConfig.PodDisruptionBudget) if err := c.deployPodDisruptionBudgets(ctx, pdbs); err != nil { log.Errorf("unable to create pdb: %v", err) return nil, errors.Wrapf(err, fmt.Sprintf("%v", ErrUnableToCreatePDB)) @@ -320,19 +321,22 @@ func (c *controller) Delete(ctx context.Context, modelService *models.Service) ( } if c.deploymentConfig.PodDisruptionBudget.Enabled { - pdbs := createPodDisruptionBudgets(modelService, c.deploymentConfig.PodDisruptionBudget) + pdbs := generatePDBSpecs(modelService, c.deploymentConfig.PodDisruptionBudget) if err := c.deletePodDisruptionBudgets(ctx, pdbs); err != nil { log.Errorf("unable to delete pdb %v", err) return nil, ErrUnableToDeletePDB } } - if modelService.RevisionID > 1 { - vsName := fmt.Sprintf("%s-%s-%s", modelService.ModelName, modelService.ModelVersion, models.VirtualServiceComponentType) - if err := c.deleteVirtualService(ctx, vsName, modelService.Namespace); err != nil { - log.Errorf("unable to delete virtual service %v", err) - return nil, ErrUnableToDeleteVirtualService - } + vsCfg, err := NewVirtualService(modelService, "") + if err != nil { + log.Errorf("unable to initialize virtual service builder: %v", err) + return nil, errors.Wrapf(err, fmt.Sprintf("%v", ErrUnableToDeleteVirtualService)) + } + + if err := c.deleteVirtualService(ctx, vsCfg); err != nil { + log.Errorf("unable to delete virtual service %v", err) + return nil, ErrUnableToDeleteVirtualService } return modelService, nil diff --git a/api/cluster/pdb.go b/api/cluster/pdb.go index 5b49e47cf..ac595b996 100644 --- a/api/cluster/pdb.go +++ b/api/cluster/pdb.go @@ -2,14 +2,15 @@ package cluster import ( "context" + "encoding/json" "fmt" "math" + policyv1 "k8s.io/api/policy/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" - metav1cfg "k8s.io/client-go/applyconfigurations/meta/v1" - policyv1cfg "k8s.io/client-go/applyconfigurations/policy/v1" "github.com/caraml-dev/merlin/config" "github.com/caraml-dev/merlin/models" @@ -29,7 +30,7 @@ func NewPodDisruptionBudget(modelService *models.Service, componentType string, labels["serving.kserve.io/inferenceservice"] = modelService.Name return &PodDisruptionBudget{ - Name: fmt.Sprintf("%s-%s-%s", modelService.Name, componentType, models.PDBComponentType), + Name: fmt.Sprintf("%s-%s-%s-%s", modelService.ModelName, modelService.ModelVersion, componentType, models.PDBComponentType), Namespace: modelService.Namespace, Labels: labels, MaxUnavailablePercentage: pdbConfig.MaxUnavailablePercentage, @@ -37,14 +38,25 @@ func NewPodDisruptionBudget(modelService *models.Service, componentType string, } } -func (cfg PodDisruptionBudget) BuildPDBSpec() (*policyv1cfg.PodDisruptionBudgetSpecApplyConfiguration, error) { +func (cfg PodDisruptionBudget) BuildPDBSpec() (*policyv1.PodDisruptionBudget, error) { if cfg.MaxUnavailablePercentage == nil && cfg.MinAvailablePercentage == nil { return nil, fmt.Errorf("one of maxUnavailable and minAvailable must be specified") } - pdbSpec := &policyv1cfg.PodDisruptionBudgetSpecApplyConfiguration{ - Selector: &metav1cfg.LabelSelectorApplyConfiguration{ - MatchLabels: cfg.Labels, + pdb := &policyv1.PodDisruptionBudget{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "policy/v1", + Kind: "PodDisruptionBudget", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: cfg.Name, + Namespace: cfg.Namespace, + Labels: cfg.Labels, + }, + Spec: policyv1.PodDisruptionBudgetSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: cfg.Labels, + }, }, } @@ -52,16 +64,16 @@ func (cfg PodDisruptionBudget) BuildPDBSpec() (*policyv1cfg.PodDisruptionBudgetS // https://kubernetes.io/docs/tasks/run-application/configure-pdb/#specifying-a-poddisruptionbudget if cfg.MinAvailablePercentage != nil { minAvailable := intstr.FromString(fmt.Sprintf("%d%%", *cfg.MinAvailablePercentage)) - pdbSpec.MinAvailable = &minAvailable + pdb.Spec.MinAvailable = &minAvailable } else if cfg.MaxUnavailablePercentage != nil { maxUnavailable := intstr.FromString(fmt.Sprintf("%d%%", *cfg.MaxUnavailablePercentage)) - pdbSpec.MaxUnavailable = &maxUnavailable + pdb.Spec.MaxUnavailable = &maxUnavailable } - return pdbSpec, nil + return pdb, nil } -func createPodDisruptionBudgets(modelService *models.Service, pdbConfig config.PodDisruptionBudgetConfig) []*PodDisruptionBudget { +func generatePDBSpecs(modelService *models.Service, pdbConfig config.PodDisruptionBudgetConfig) []*PodDisruptionBudget { pdbs := []*PodDisruptionBudget{} // Only create PDB if: ceil(minReplica * minAvailablePercent) < minReplica @@ -113,11 +125,15 @@ func (c *controller) deployPodDisruptionBudget(ctx context.Context, pdb *PodDisr return err } - pdbCfg := policyv1cfg.PodDisruptionBudget(pdb.Name, pdb.Namespace) - pdbCfg.WithLabels(pdb.Labels) - pdbCfg.WithSpec(pdbSpec) + pdbJSON, err := json.Marshal(pdbSpec) + if err != nil { + return err + } + + forceEnabled := true - _, err = c.policyClient.PodDisruptionBudgets(pdb.Namespace).Apply(ctx, pdbCfg, metav1.ApplyOptions{FieldManager: "application/apply-patch"}) + _, err = c.policyClient.PodDisruptionBudgets(pdb.Namespace). + Patch(ctx, pdb.Name, types.ApplyPatchType, pdbJSON, metav1.PatchOptions{FieldManager: "application/apply-patch", Force: &forceEnabled}) if err != nil { return err } diff --git a/api/cluster/pdb_test.go b/api/cluster/pdb_test.go index 6ac5eb3ff..668433425 100644 --- a/api/cluster/pdb_test.go +++ b/api/cluster/pdb_test.go @@ -5,10 +5,9 @@ import ( "testing" "github.com/stretchr/testify/assert" + policyv1 "k8s.io/api/policy/v1" 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" "github.com/caraml-dev/merlin/config" "github.com/caraml-dev/merlin/models" @@ -32,7 +31,7 @@ func TestPodDisruptionBudget_BuildPDBSpec(t *testing.T) { tests := []struct { name string fields fields - want *policyv1cfg.PodDisruptionBudgetSpecApplyConfiguration + want *policyv1.PodDisruptionBudget wantErr bool }{ { @@ -44,10 +43,21 @@ func TestPodDisruptionBudget_BuildPDBSpec(t *testing.T) { MaxUnavailablePercentage: nil, MinAvailablePercentage: &defaultInt, }, - want: &policyv1cfg.PodDisruptionBudgetSpecApplyConfiguration{ - MinAvailable: &defaultIntOrString, - Selector: &metav1cfg.LabelSelectorApplyConfiguration{ - MatchLabels: defaultLabels, + want: &policyv1.PodDisruptionBudget{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "policy/v1", + Kind: "PodDisruptionBudget", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "sklearn-sample-s-1-model-pdb", + Namespace: "pdb-test", + Labels: defaultLabels, + }, + Spec: policyv1.PodDisruptionBudgetSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: defaultLabels, + }, + MinAvailable: &defaultIntOrString, }, }, wantErr: false, @@ -61,10 +71,21 @@ func TestPodDisruptionBudget_BuildPDBSpec(t *testing.T) { MaxUnavailablePercentage: &defaultInt, MinAvailablePercentage: &defaultInt, }, - want: &policyv1cfg.PodDisruptionBudgetSpecApplyConfiguration{ - MinAvailable: &defaultIntOrString, - Selector: &metav1cfg.LabelSelectorApplyConfiguration{ - MatchLabels: defaultLabels, + want: &policyv1.PodDisruptionBudget{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "policy/v1", + Kind: "PodDisruptionBudget", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "sklearn-sample-s-1-model-pdb", + Namespace: "pdb-test", + Labels: defaultLabels, + }, + Spec: policyv1.PodDisruptionBudgetSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: defaultLabels, + }, + MinAvailable: &defaultIntOrString, }, }, wantErr: false, @@ -103,7 +124,7 @@ func TestPodDisruptionBudget_BuildPDBSpec(t *testing.T) { } } -func TestCreatePodDisruptionBudgets(t *testing.T) { +func Test_generatePDBSpecs(t *testing.T) { err := models.InitKubernetesLabeller("gojek.com/", "dev") assert.Nil(t, err) @@ -322,7 +343,7 @@ func TestCreatePodDisruptionBudgets(t *testing.T) { for name, tt := range tests { t.Run(name, func(t *testing.T) { - pdbs := createPodDisruptionBudgets(tt.modelService, tt.pdbConfig) + pdbs := generatePDBSpecs(tt.modelService, tt.pdbConfig) assert.Equal(t, tt.expected, pdbs) }) } diff --git a/api/cluster/virtual_service.go b/api/cluster/virtual_service.go index d7e07a84b..353195595 100644 --- a/api/cluster/virtual_service.go +++ b/api/cluster/virtual_service.go @@ -222,6 +222,6 @@ func (c *controller) deployVirtualService(ctx context.Context, vsCfg *VirtualSer Patch(ctx, vsCfg.Name, types.ApplyPatchType, vsJSON, metav1.PatchOptions{FieldManager: "application/apply-patch", Force: &forceEnabled}) } -func (c *controller) deleteVirtualService(ctx context.Context, name, namespace string) error { - return c.istioClient.VirtualServices(namespace).Delete(ctx, name, metav1.DeleteOptions{}) +func (c *controller) deleteVirtualService(ctx context.Context, vsCfg *VirtualService) error { + return c.istioClient.VirtualServices(vsCfg.Namespace).Delete(ctx, vsCfg.Name, metav1.DeleteOptions{}) }