Skip to content

fix(k8s): default PodDisruptionBudget to maxUnavailable:1 #307

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Cre-eD wants to merge 1 commit into from
Closed

fix(k8s): default PodDisruptionBudget to maxUnavailable:1 #307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 52 additions & 37 deletions pkg/clouds/pulumi/kubernetes/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,43 +220,41 @@ func DeploySimpleContainer(ctx *sdk.Context, args Args, opts ...sdk.ResourceOpti

args.Params.Log.Warn(ctx.Context(), "configure simple container deployment for %q in %q", stackName, stackEnv)
sc, err := NewSimpleContainer(ctx, &SimpleContainerArgs{
KubeProvider: args.KubeProvider,
ComputeContext: args.ComputeContext,
ServiceType: args.ServiceType,
ExternalTrafficPolicy: args.ExternalTrafficPolicy,
UseSSL: args.UseSSL,
ProvisionIngress: args.ProvisionIngress,
Namespace: namespace,
Service: deploymentName,
Deployment: deploymentName,
ScEnv: stackEnv,
IngressContainer: args.Deployment.IngressContainer,
Domain: lo.FromPtr(args.Deployment.StackConfig).Domain,
Prefix: lo.FromPtr(args.Deployment.StackConfig).Prefix,
ProxyKeepPrefix: lo.FromPtr(args.Deployment.StackConfig).ProxyKeepPrefix,
ParentStack: lo.If(args.Params.ParentStack != nil, lo.ToPtr(lo.FromPtr(args.Params.ParentStack).FullReference)).Else(nil),
ParentEnv: lo.If(parentEnv != "", lo.ToPtr(parentEnv)).Else(nil),
Replicas: replicas,
Headers: args.Deployment.Headers,
SecretEnvs: mergedSecretEnvs,
LbConfig: args.Deployment.StackConfig.LBConfig,
Volumes: args.Deployment.TextVolumes,
PersistentVolumes: pvs,
EphemeralVolumes: args.Deployment.EphemeralVolumes, // Pass generic ephemeral volumes configuration
PriorityClassName: args.Deployment.PriorityClassName,
Containers: containers,
ServiceAccountName: args.ServiceAccountName,
InitContainers: args.InitContainers,
GenerateCaddyfileEntry: args.GenerateCaddyfileEntry,
Annotations: args.Annotations,
NodeSelector: args.NodeSelector,
Affinity: args.Affinity,
Sidecars: args.Sidecars,
VPA: args.VPA, // Pass VPA configuration to SimpleContainer
Scale: args.Deployment.Scale, // Pass Scale configuration to SimpleContainer
PodDisruption: lo.If(args.Deployment.DisruptionBudget != nil, args.Deployment.DisruptionBudget).Else(&k8s.DisruptionBudget{
MinAvailable: lo.ToPtr(1),
}),
KubeProvider: args.KubeProvider,
ComputeContext: args.ComputeContext,
ServiceType: args.ServiceType,
ExternalTrafficPolicy: args.ExternalTrafficPolicy,
UseSSL: args.UseSSL,
ProvisionIngress: args.ProvisionIngress,
Namespace: namespace,
Service: deploymentName,
Deployment: deploymentName,
ScEnv: stackEnv,
IngressContainer: args.Deployment.IngressContainer,
Domain: lo.FromPtr(args.Deployment.StackConfig).Domain,
Prefix: lo.FromPtr(args.Deployment.StackConfig).Prefix,
ProxyKeepPrefix: lo.FromPtr(args.Deployment.StackConfig).ProxyKeepPrefix,
ParentStack: lo.If(args.Params.ParentStack != nil, lo.ToPtr(lo.FromPtr(args.Params.ParentStack).FullReference)).Else(nil),
ParentEnv: lo.If(parentEnv != "", lo.ToPtr(parentEnv)).Else(nil),
Replicas: replicas,
Headers: args.Deployment.Headers,
SecretEnvs: mergedSecretEnvs,
LbConfig: args.Deployment.StackConfig.LBConfig,
Volumes: args.Deployment.TextVolumes,
PersistentVolumes: pvs,
EphemeralVolumes: args.Deployment.EphemeralVolumes, // Pass generic ephemeral volumes configuration
PriorityClassName: args.Deployment.PriorityClassName,
Containers: containers,
ServiceAccountName: args.ServiceAccountName,
InitContainers: args.InitContainers,
GenerateCaddyfileEntry: args.GenerateCaddyfileEntry,
Annotations: args.Annotations,
NodeSelector: args.NodeSelector,
Affinity: args.Affinity,
Sidecars: args.Sidecars,
VPA: args.VPA, // Pass VPA configuration to SimpleContainer
Scale: args.Deployment.Scale, // Pass Scale configuration to SimpleContainer
PodDisruption: defaultDisruptionBudget(args.Deployment.DisruptionBudget),
RollingUpdate: lo.If(args.Deployment.RollingUpdate != nil, toRollingUpdateArgs(args.Deployment.RollingUpdate)).Else(nil),
SecurityContext: nil, // TODO
Log: args.Params.Log,
Expand Down Expand Up @@ -304,6 +302,23 @@ func buildPreStopLifecycle(preStopSleepSeconds *int) *corev1.LifecycleArgs {
}
}

// defaultDisruptionBudget returns the PodDisruptionBudget to apply to a deployment.
// An explicitly configured budget is used as-is; otherwise we default to
// maxUnavailable:1 instead of minAvailable:1.
//
// minAvailable:1 on a single-replica deployment resolves to disruptionsAllowed:0,
// which forbids the eviction API and so blocks the cluster autoscaler from ever
// draining the pod's node. On GKE Autopilot that means the node can never be
// consolidated and stays pinned regardless of utilization. maxUnavailable:1 yields
// disruptionsAllowed:1 (the single pod can be evicted and rescheduled), while still
// bounding voluntary disruptions to one pod at a time for multi-replica deployments.
func defaultDisruptionBudget(db *k8s.DisruptionBudget) *k8s.DisruptionBudget {
if db != nil {
return db
}
return &k8s.DisruptionBudget{MaxUnavailable: lo.ToPtr(1)}
}

func toRollingUpdateArgs(update *k8s.RollingUpdate) *v1.RollingUpdateDeploymentArgs {
return &v1.RollingUpdateDeploymentArgs{
MaxUnavailable: lo.If(lo.FromPtr(update).MaxUnavailable != nil, sdk.IntPtrFromPtr(lo.FromPtr(update).MaxUnavailable)).Else(nil),
Expand Down
56 changes: 56 additions & 0 deletions pkg/clouds/pulumi/kubernetes/deployment_disruption_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package kubernetes

import (
"testing"

"github.com/samber/lo"

"github.com/simple-container-com/api/pkg/clouds/k8s"
)

// TestDefaultDisruptionBudget guards the PodDisruptionBudget default. The default
// must be maxUnavailable:1 (not minAvailable:1): minAvailable:1 on a single-replica
// deployment yields disruptionsAllowed:0, which blocks the cluster autoscaler from
// draining the pod's node and pins underutilized nodes on GKE Autopilot.
func TestDefaultDisruptionBudget(t *testing.T) {
t.Run("nil defaults to maxUnavailable:1 so single-replica nodes stay drainable", func(t *testing.T) {
got := defaultDisruptionBudget(nil)
if got == nil {
t.Fatal("expected a default budget, got nil")
}
if got.MinAvailable != nil {
t.Errorf("default must not set MinAvailable (it pins single-replica nodes); got %d", *got.MinAvailable)
}
if got.MaxUnavailable == nil {
t.Fatal("default must set MaxUnavailable")
}
if *got.MaxUnavailable != 1 {
t.Errorf("default MaxUnavailable = %d, want 1", *got.MaxUnavailable)
}
})

t.Run("explicit minAvailable is honored unchanged", func(t *testing.T) {
in := &k8s.DisruptionBudget{MinAvailable: lo.ToPtr(2)}
got := defaultDisruptionBudget(in)
if got != in {
t.Fatal("explicit budget must be returned as-is")
}
if got.MinAvailable == nil || *got.MinAvailable != 2 {
t.Errorf("MinAvailable = %v, want 2", got.MinAvailable)
}
if got.MaxUnavailable != nil {
t.Errorf("explicit minAvailable budget must not gain a MaxUnavailable; got %d", *got.MaxUnavailable)
}
})

t.Run("explicit maxUnavailable is honored unchanged", func(t *testing.T) {
in := &k8s.DisruptionBudget{MaxUnavailable: lo.ToPtr(3)}
got := defaultDisruptionBudget(in)
if got != in {
t.Fatal("explicit budget must be returned as-is")
}
if got.MaxUnavailable == nil || *got.MaxUnavailable != 3 {
t.Errorf("MaxUnavailable = %v, want 3", got.MaxUnavailable)
}
})
}
Loading