Skip to content

Commit

Permalink
fix: Add precautionary logic for PDB (#353)
Browse files Browse the repository at this point in the history 
* Add precautionary logic for PDB

* Add unit test for PDB creation

* Update integration tests

* Fix lint error
  • Loading branch information
@krithika369
krithika369 authored Jul 27, 2023
1 parent 8c764ed commit 0302624
Show file tree
Hide file tree
Showing 2 changed files with 252 additions and 22 deletions.
51 changes: 41 additions & 10 deletions api/turing/service/router_deployment_service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"encoding/json"
"fmt"
"math"
"os"
"strings"
"sync"
Expand Down Expand Up @@ -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,
Expand All @@ -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.DockerConfig != nil &&
routerVersion.Ensembler.DockerConfig.ResourceRequest != nil &&
math.Ceil(float64(routerVersion.Ensembler.DockerConfig.ResourceRequest.MinReplica)*
minAvailablePercent) < float64(routerVersion.Ensembler.DockerConfig.ResourceRequest.MinReplica)) ||
(routerVersion.Ensembler.PyfuncConfig != nil &&
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,
Expand All @@ -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
}
Expand Down
223 changes: 211 additions & 12 deletions api/turing/service/router_deployment_service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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{}
Expand Down Expand Up @@ -447,6 +438,10 @@ func TestDeleteEndpoint(t *testing.T) {
testEnv: controller,
},
svcBuilder: svcBuilder,
pdbConfig: config.PodDisruptionBudgetConfig{
Enabled: true,
MinAvailablePercentage: &defaultMinAvailablePercentage,
},
}

eventsCh := NewEventChannel()
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -548,3 +543,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{
{
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",
},
},
},
{
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",
},
},
},
{
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{
{
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",
},
},
},
{
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",
},
},
},
{
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{
{
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)
})
}
}

0 comments on commit 0302624

Please sign in to comment.