From d84cb86504b0c43b31bf21864277226465e51b26 Mon Sep 17 00:00:00 2001 From: Krithika Sundararajan Date: Thu, 27 Jul 2023 10:25:39 +0800 Subject: [PATCH 1/4] Add precautionary logic for PDB --- .../service/router_deployment_service.go | 51 +++++++++++++++---- 1 file changed, 41 insertions(+), 10 deletions(-) diff --git a/api/turing/service/router_deployment_service.go b/api/turing/service/router_deployment_service.go index 51f8d3239..a81dccaf1 100644 --- a/api/turing/service/router_deployment_service.go +++ b/api/turing/service/router_deployment_service.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "math" "os" "strings" "sync" @@ -748,8 +749,26 @@ func (ds *deploymentService) createPodDisruptionBudgets( ) []*cluster.PodDisruptionBudget { pdbs := []*cluster.PodDisruptionBudget{} + // Only create PDB if: ceil(minReplica * minAvailablePercent) < minReplica + // If not, the replicas may be unable to be removed. + // Note that we only care about minReplica here because, for active replicas > minReplica, + // the condition will be satisfied if it was satisfied for the minReplica case. + + var minAvailablePercent float64 + if ds.pdbConfig.MinAvailablePercentage != nil { + minAvailablePercent = float64(*ds.pdbConfig.MinAvailablePercentage) / 100.0 + } else if ds.pdbConfig.MaxUnavailablePercentage != nil { + minAvailablePercent = float64(100-*ds.pdbConfig.MaxUnavailablePercentage) / 100.0 + } else { + // PDB config is not set properly, we can't apply it. + return pdbs + } + // Enricher's PDB - if routerVersion.Enricher != nil { + if routerVersion.Enricher != nil && + routerVersion.Enricher.ResourceRequest != nil && + math.Ceil(float64(routerVersion.Enricher.ResourceRequest.MinReplica)* + minAvailablePercent) < float64(routerVersion.Enricher.ResourceRequest.MinReplica) { enricherPdb := ds.svcBuilder.NewPodDisruptionBudget( routerVersion, project, @@ -759,8 +778,16 @@ func (ds *deploymentService) createPodDisruptionBudgets( pdbs = append(pdbs, enricherPdb) } - // Ensembler's PDB - if routerVersion.Enricher != nil { + // Ensembler's PDB - create for Docker / Pyfunc ensemblers only + if routerVersion.Ensembler != nil && + ((routerVersion.Ensembler.Type == models.EnsemblerDockerType && + routerVersion.Ensembler.DockerConfig.ResourceRequest != nil && + math.Ceil(float64(routerVersion.Ensembler.DockerConfig.ResourceRequest.MinReplica)* + minAvailablePercent) < float64(routerVersion.Ensembler.DockerConfig.ResourceRequest.MinReplica)) || + (routerVersion.Ensembler.Type == models.EnsemblerPyFuncType && + routerVersion.Ensembler.PyfuncConfig.ResourceRequest != nil && + math.Ceil(float64(routerVersion.Ensembler.PyfuncConfig.ResourceRequest.MinReplica)* + minAvailablePercent) < float64(routerVersion.Ensembler.PyfuncConfig.ResourceRequest.MinReplica))) { ensemblerPdb := ds.svcBuilder.NewPodDisruptionBudget( routerVersion, project, @@ -771,13 +798,17 @@ func (ds *deploymentService) createPodDisruptionBudgets( } // Router's PDB - routerPdb := ds.svcBuilder.NewPodDisruptionBudget( - routerVersion, - project, - servicebuilder.ComponentTypes.Router, - ds.pdbConfig, - ) - pdbs = append(pdbs, routerPdb) + if routerVersion.ResourceRequest != nil && + math.Ceil(float64(routerVersion.ResourceRequest.MinReplica)* + minAvailablePercent) < float64(routerVersion.ResourceRequest.MinReplica) { + routerPdb := ds.svcBuilder.NewPodDisruptionBudget( + routerVersion, + project, + servicebuilder.ComponentTypes.Router, + ds.pdbConfig, + ) + pdbs = append(pdbs, routerPdb) + } return pdbs } From 39cda9b5b3d20c445dd8982b5e093a7ecd08e236 Mon Sep 17 00:00:00 2001 From: Krithika Sundararajan Date: Thu, 27 Jul 2023 11:09:15 +0800 Subject: [PATCH 2/4] Add unit test for PDB creation --- .../service/router_deployment_service.go | 4 +- .../service/router_deployment_service_test.go | 204 ++++++++++++++++++ 2 files changed, 206 insertions(+), 2 deletions(-) diff --git a/api/turing/service/router_deployment_service.go b/api/turing/service/router_deployment_service.go index a81dccaf1..86edaa5b0 100644 --- a/api/turing/service/router_deployment_service.go +++ b/api/turing/service/router_deployment_service.go @@ -780,11 +780,11 @@ func (ds *deploymentService) createPodDisruptionBudgets( // Ensembler's PDB - create for Docker / Pyfunc ensemblers only if routerVersion.Ensembler != nil && - ((routerVersion.Ensembler.Type == models.EnsemblerDockerType && + ((routerVersion.Ensembler.DockerConfig != nil && routerVersion.Ensembler.DockerConfig.ResourceRequest != nil && math.Ceil(float64(routerVersion.Ensembler.DockerConfig.ResourceRequest.MinReplica)* minAvailablePercent) < float64(routerVersion.Ensembler.DockerConfig.ResourceRequest.MinReplica)) || - (routerVersion.Ensembler.Type == models.EnsemblerPyFuncType && + (routerVersion.Ensembler.PyfuncConfig != nil && routerVersion.Ensembler.PyfuncConfig.ResourceRequest != nil && math.Ceil(float64(routerVersion.Ensembler.PyfuncConfig.ResourceRequest.MinReplica)* minAvailablePercent) < float64(routerVersion.Ensembler.PyfuncConfig.ResourceRequest.MinReplica))) { diff --git a/api/turing/service/router_deployment_service_test.go b/api/turing/service/router_deployment_service_test.go index d3d41e826..5a0adba84 100644 --- a/api/turing/service/router_deployment_service_test.go +++ b/api/turing/service/router_deployment_service_test.go @@ -548,3 +548,207 @@ func TestBuildEnsemblerServiceImage(t *testing.T) { Env: routerVersion.Ensembler.PyfuncConfig.Env, }) } + +func TestCreatePodDisruptionBudgets(t *testing.T) { + twenty, eighty := 20, 80 + testRouterLabels := map[string]string{ + "app": "test", + "environment": "", + "orchestrator": "turing", + "stream": "", + "team": "", + } + + tests := map[string]struct { + rv *models.RouterVersion + pdbConfig config.PodDisruptionBudgetConfig + expected []*cluster.PodDisruptionBudget + }{ + "bad pdb config": { + rv: &models.RouterVersion{ + ResourceRequest: &models.ResourceRequest{ + MinReplica: 5, + }, + }, + pdbConfig: config.PodDisruptionBudgetConfig{ + Enabled: true, + }, + expected: []*cluster.PodDisruptionBudget{}, + }, + "all pdbs | minAvailablePercentage": { + rv: &models.RouterVersion{ + Router: &models.Router{ + Name: "test", + }, + Version: 3, + ResourceRequest: &models.ResourceRequest{ + MinReplica: 5, + }, + Enricher: &models.Enricher{ + ResourceRequest: &models.ResourceRequest{ + MinReplica: 3, + }, + }, + Ensembler: &models.Ensembler{ + DockerConfig: &models.EnsemblerDockerConfig{ + ResourceRequest: &models.ResourceRequest{ + MinReplica: 2, + }, + }, + }, + }, + pdbConfig: config.PodDisruptionBudgetConfig{ + Enabled: true, + MinAvailablePercentage: &twenty, + }, + expected: []*cluster.PodDisruptionBudget{ + &cluster.PodDisruptionBudget{ + Name: "test-turing-enricher-3-pdb", + Namespace: "ns", + Labels: testRouterLabels, + MinAvailablePercentage: &twenty, + Selector: &apimetav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "test-turing-enricher-3-0", + }, + }, + }, + &cluster.PodDisruptionBudget{ + Name: "test-turing-ensembler-3-pdb", + Namespace: "ns", + Labels: testRouterLabels, + MinAvailablePercentage: &twenty, + Selector: &apimetav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "test-turing-ensembler-3-0", + }, + }, + }, + &cluster.PodDisruptionBudget{ + Name: "test-turing-router-3-pdb", + Namespace: "ns", + Labels: testRouterLabels, + MinAvailablePercentage: &twenty, + Selector: &apimetav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "test-turing-router-3-0", + }, + }, + }, + }, + }, + "all pdbs | maxUnavailablePercentage": { + rv: &models.RouterVersion{ + Router: &models.Router{ + Name: "test", + }, + Version: 3, + ResourceRequest: &models.ResourceRequest{ + MinReplica: 5, + }, + Enricher: &models.Enricher{ + ResourceRequest: &models.ResourceRequest{ + MinReplica: 3, + }, + }, + Ensembler: &models.Ensembler{ + DockerConfig: &models.EnsemblerDockerConfig{ + ResourceRequest: &models.ResourceRequest{ + MinReplica: 2, + }, + }, + }, + }, + pdbConfig: config.PodDisruptionBudgetConfig{ + Enabled: true, + MaxUnavailablePercentage: &eighty, + }, + expected: []*cluster.PodDisruptionBudget{ + &cluster.PodDisruptionBudget{ + Name: "test-turing-enricher-3-pdb", + Namespace: "ns", + Labels: testRouterLabels, + MaxUnavailablePercentage: &eighty, + Selector: &apimetav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "test-turing-enricher-3-0", + }, + }, + }, + &cluster.PodDisruptionBudget{ + Name: "test-turing-ensembler-3-pdb", + Namespace: "ns", + Labels: testRouterLabels, + MaxUnavailablePercentage: &eighty, + Selector: &apimetav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "test-turing-ensembler-3-0", + }, + }, + }, + &cluster.PodDisruptionBudget{ + Name: "test-turing-router-3-pdb", + Namespace: "ns", + Labels: testRouterLabels, + MaxUnavailablePercentage: &eighty, + Selector: &apimetav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "test-turing-router-3-0", + }, + }, + }, + }, + }, + "pyfunc ensembler": { + rv: &models.RouterVersion{ + Router: &models.Router{ + Name: "test", + }, + Version: 3, + ResourceRequest: &models.ResourceRequest{ + MinReplica: 1, + }, + Ensembler: &models.Ensembler{ + PyfuncConfig: &models.EnsemblerPyfuncConfig{ + ResourceRequest: &models.ResourceRequest{ + MinReplica: 10, + }, + }, + }, + }, + pdbConfig: config.PodDisruptionBudgetConfig{ + Enabled: true, + MinAvailablePercentage: &twenty, + }, + expected: []*cluster.PodDisruptionBudget{ + &cluster.PodDisruptionBudget{ + Name: "test-turing-ensembler-3-pdb", + Namespace: "ns", + Labels: testRouterLabels, + MinAvailablePercentage: &twenty, + Selector: &apimetav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "test-turing-ensembler-3-0", + }, + }, + }, + }, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + ds := &deploymentService{ + pdbConfig: tt.pdbConfig, + svcBuilder: servicebuilder.NewClusterServiceBuilder( + resource.MustParse("200m"), + resource.MustParse("200Mi"), + 10, + []corev1.TopologySpreadConstraint{}, + ), + } + pdbs := ds.createPodDisruptionBudgets(tt.rv, &mlp.Project{Name: "ns"}) + assert.Equal(t, tt.expected, pdbs) + }) + } +} From 677bffd5692cba918e25ae6e86f5e443bab92d7a Mon Sep 17 00:00:00 2001 From: Krithika Sundararajan Date: Thu, 27 Jul 2023 11:44:36 +0800 Subject: [PATCH 3/4] Update integration tests --- .../service/router_deployment_service_test.go | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/api/turing/service/router_deployment_service_test.go b/api/turing/service/router_deployment_service_test.go index 5a0adba84..333a743e4 100644 --- a/api/turing/service/router_deployment_service_test.go +++ b/api/turing/service/router_deployment_service_test.go @@ -364,16 +364,6 @@ func TestDeployEndpoint(t *testing.T) { }, }, }) - controller.AssertCalled(t, "ApplyPodDisruptionBudget", mock.Anything, testNamespace, cluster.PodDisruptionBudget{ - Name: "test-svc-turing-enricher-1-pdb", - Namespace: testNamespace, - MinAvailablePercentage: &defaultMinAvailablePercentage, - Selector: &apimetav1.LabelSelector{ - MatchLabels: map[string]string{ - "app": "test-svc-turing-enricher-1-0", - }, - }, - }) controller.AssertCalled(t, "ApplyPodDisruptionBudget", mock.Anything, testNamespace, cluster.PodDisruptionBudget{ Name: "test-svc-turing-ensembler-1-pdb", Namespace: testNamespace, @@ -384,7 +374,7 @@ func TestDeployEndpoint(t *testing.T) { }, }, }) - controller.AssertNumberOfCalls(t, "ApplyPodDisruptionBudget", 3) + controller.AssertNumberOfCalls(t, "ApplyPodDisruptionBudget", 2) // Verify endpoint for upi routers routerVersion.Protocol = routerConfig.UPI @@ -409,6 +399,7 @@ func TestDeleteEndpoint(t *testing.T) { testEnv := "test-env" testNs := "test-namespace" timeout := time.Second * 5 + defaultMinAvailablePercentage := 10 // Create mock controller controller := &mocks.Controller{} @@ -447,6 +438,10 @@ func TestDeleteEndpoint(t *testing.T) { testEnv: controller, }, svcBuilder: svcBuilder, + pdbConfig: config.PodDisruptionBudgetConfig{ + Enabled: true, + MinAvailablePercentage: &defaultMinAvailablePercentage, + }, } eventsCh := NewEventChannel() @@ -479,7 +474,7 @@ func TestDeleteEndpoint(t *testing.T) { controller.AssertCalled(t, "DeletePersistentVolumeClaim", mock.Anything, "pvc", testNs, false) controller.AssertCalled(t, "DeletePodDisruptionBudget", mock.Anything, testNs, mock.Anything) controller.AssertNumberOfCalls(t, "DeleteKnativeService", 3) - controller.AssertNumberOfCalls(t, "DeletePodDisruptionBudget", 3) + controller.AssertNumberOfCalls(t, "DeletePodDisruptionBudget", 2) } func TestBuildEnsemblerServiceImage(t *testing.T) { From a7b75e7162b9950a83a6ea8b95e7e7ab1b48a3e5 Mon Sep 17 00:00:00 2001 From: Krithika Sundararajan Date: Thu, 27 Jul 2023 12:08:05 +0800 Subject: [PATCH 4/4] Fix lint error --- .../service/router_deployment_service_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/api/turing/service/router_deployment_service_test.go b/api/turing/service/router_deployment_service_test.go index 333a743e4..b6e33e89f 100644 --- a/api/turing/service/router_deployment_service_test.go +++ b/api/turing/service/router_deployment_service_test.go @@ -597,7 +597,7 @@ func TestCreatePodDisruptionBudgets(t *testing.T) { MinAvailablePercentage: &twenty, }, expected: []*cluster.PodDisruptionBudget{ - &cluster.PodDisruptionBudget{ + { Name: "test-turing-enricher-3-pdb", Namespace: "ns", Labels: testRouterLabels, @@ -608,7 +608,7 @@ func TestCreatePodDisruptionBudgets(t *testing.T) { }, }, }, - &cluster.PodDisruptionBudget{ + { Name: "test-turing-ensembler-3-pdb", Namespace: "ns", Labels: testRouterLabels, @@ -619,7 +619,7 @@ func TestCreatePodDisruptionBudgets(t *testing.T) { }, }, }, - &cluster.PodDisruptionBudget{ + { Name: "test-turing-router-3-pdb", Namespace: "ns", Labels: testRouterLabels, @@ -659,7 +659,7 @@ func TestCreatePodDisruptionBudgets(t *testing.T) { MaxUnavailablePercentage: &eighty, }, expected: []*cluster.PodDisruptionBudget{ - &cluster.PodDisruptionBudget{ + { Name: "test-turing-enricher-3-pdb", Namespace: "ns", Labels: testRouterLabels, @@ -670,7 +670,7 @@ func TestCreatePodDisruptionBudgets(t *testing.T) { }, }, }, - &cluster.PodDisruptionBudget{ + { Name: "test-turing-ensembler-3-pdb", Namespace: "ns", Labels: testRouterLabels, @@ -681,7 +681,7 @@ func TestCreatePodDisruptionBudgets(t *testing.T) { }, }, }, - &cluster.PodDisruptionBudget{ + { Name: "test-turing-router-3-pdb", Namespace: "ns", Labels: testRouterLabels, @@ -716,7 +716,7 @@ func TestCreatePodDisruptionBudgets(t *testing.T) { MinAvailablePercentage: &twenty, }, expected: []*cluster.PodDisruptionBudget{ - &cluster.PodDisruptionBudget{ + { Name: "test-turing-ensembler-3-pdb", Namespace: "ns", Labels: testRouterLabels,