Skip to content
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.

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

feat: Implement Issue #1779: add --canary-min-replicas command-line flag #2410

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions cmd/rollouts-controller/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ func newCommand() *cobra.Command {
analysisThreads int
serviceThreads int
ingressThreads int
canaryMinReplicas int32
istioVersion string
trafficSplitVersion string
ambassadorVersion string
Expand Down Expand Up @@ -95,6 +96,7 @@ func newCommand() *cobra.Command {
defaults.SetAmbassadorAPIVersion(ambassadorVersion)
defaults.SetSMIAPIVersion(trafficSplitVersion)
defaults.SetAppMeshCRDVersion(appmeshCRDVersion)
defaults.SetDefaultCanaryMinReplicas(canaryMinReplicas)

config, err := clientConfig.ClientConfig()
checkError(err)
Expand Down Expand Up @@ -225,6 +227,7 @@ func newCommand() *cobra.Command {
command.Flags().IntVar(&analysisThreads, "analysis-threads", controller.DefaultAnalysisThreads, "Set the number of worker threads for the Experiment controller")
command.Flags().IntVar(&serviceThreads, "service-threads", controller.DefaultServiceThreads, "Set the number of worker threads for the Service controller")
command.Flags().IntVar(&ingressThreads, "ingress-threads", controller.DefaultIngressThreads, "Set the number of worker threads for the Ingress controller")
command.Flags().Int32Var(&canaryMinReplicas, "canary-min-replicas", defaults.DefaultCanaryMinReplicas, "Set the minimum number of replicas for Canary ReplicaSets")
command.Flags().StringVar(&istioVersion, "istio-api-version", defaults.DefaultIstioVersion, "Set the default Istio apiVersion that controller should look when manipulating VirtualServices.")
command.Flags().StringVar(&ambassadorVersion, "ambassador-api-version", defaults.DefaultAmbassadorVersion, "Set the Ambassador apiVersion that controller should look when manipulating Ambassador Mappings.")
command.Flags().StringVar(&trafficSplitVersion, "traffic-split-api-version", defaults.DefaultSMITrafficSplitVersion, "Set the default TrafficSplit apiVersion that controller uses when creating TrafficSplits.")
Expand Down
13 changes: 13 additions & 0 deletions utils/defaults/defaults.go
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ const (
// DefaultMetricCleanupDelay is the default time to delay metrics removal upon object removal, gives time for metrics
// to be collected
DefaultMetricCleanupDelay = int32(65)
// DefaultCanaryMinReplicas is the minimum pods in a Replicaset for Canary traffic routing
DefaultCanaryMinReplicas = int32(1)
)

const (
Expand All @@ -66,6 +68,7 @@ var (
targetGroupBindingAPIVersion = DefaultTargetGroupBindingAPIVersion
appmeshCRDVersion = DefaultAppMeshCRDVersion
defaultMetricCleanupDelay = DefaultMetricCleanupDelay
defaultCanaryMinReplicas = DefaultCanaryMinReplicas
)

const (
Expand Down Expand Up @@ -318,3 +321,13 @@ func GetMetricCleanupDelaySeconds() time.Duration {
func SetMetricCleanupDelaySeconds(seconds int32) {
defaultMetricCleanupDelay = seconds
}

// GetDefaultCanaryMinReplicas returns the minimum pods in a Replicaset for Canary traffic routing
func GetDefaultCanaryMinReplicas() int32 {
return defaultCanaryMinReplicas
}

// SetDefaultCanaryMinReplicas sets the minimum pods in a Replicaset for Canary traffic routing
func SetDefaultCanaryMinReplicas(replicas int32) {
defaultCanaryMinReplicas = replicas
}
17 changes: 12 additions & 5 deletions utils/replicaset/canary.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ func allDesiredAreCreated(rs *appsv1.ReplicaSet, desired int32) bool {
return rs != nil && desired == *rs.Spec.Replicas && desired == rs.Status.Replicas
}

func checkMinPods(rollout *v1alpha1.Rollout, replicas int32) int32 {
if replicas > 0 && rollout.Spec.Replicas != nil && *rollout.Spec.Replicas >= defaults.GetDefaultCanaryMinReplicas() {
replicas = max(replicas, defaults.GetDefaultCanaryMinReplicas())
}
return replicas
}

func AtDesiredReplicaCountsForCanary(ro *v1alpha1.Rollout, newRS, stableRS *appsv1.ReplicaSet, olderRSs []*appsv1.ReplicaSet, weights *v1alpha1.TrafficWeights) bool {
var desiredNewRSReplicaCount, desiredStableRSReplicaCount int32
if ro.Spec.Strategy.Canary.TrafficRouting == nil {
Expand Down Expand Up @@ -142,7 +149,7 @@ func CalculateReplicaCountsForBasicCanary(rollout *v1alpha1.Rollout, newRS *apps
if GetReplicaCountForReplicaSets(oldRSs) > 0 {
// If any older ReplicaSets exist, we should scale those down first, before even considering
// scaling down the newRS or stableRS
return newRSReplicaCount, stableRSReplicaCount
return checkMinPods(rollout, newRSReplicaCount), checkMinPods(rollout, stableRSReplicaCount)
}

minAvailableReplicaCount := rolloutSpecReplica - MaxUnavailable(rollout)
Expand All @@ -154,7 +161,7 @@ func CalculateReplicaCountsForBasicCanary(rollout *v1alpha1.Rollout, newRS *apps
replicasToScaleDown := GetReplicasForScaleDown(newRS, !isIncreasing) + GetReplicasForScaleDown(stableRS, isIncreasing)
if replicasToScaleDown <= minAvailableReplicaCount {
// Cannot scale down stableRS or newRS without going below min available replica count
return newRSReplicaCount, stableRSReplicaCount
return checkMinPods(rollout, newRSReplicaCount), checkMinPods(rollout, stableRSReplicaCount)
}

scaleDownCount := replicasToScaleDown - minAvailableReplicaCount
Expand All @@ -167,7 +174,7 @@ func CalculateReplicaCountsForBasicCanary(rollout *v1alpha1.Rollout, newRS *apps
stableRSReplicaCount = calculateScaleDownReplicaCount(stableRS, desiredStableRSReplicaCount, scaleDownCount, stableRSReplicaCount)
stableRSReplicaCount, newRSReplicaCount = adjustReplicaWithinLimits(stableRS, newRS, stableRSReplicaCount, newRSReplicaCount, maxReplicaCountAllowed, minAvailableReplicaCount)
}
return newRSReplicaCount, stableRSReplicaCount
return checkMinPods(rollout, newRSReplicaCount), checkMinPods(rollout, stableRSReplicaCount)
}

// approximateWeightedCanaryStableReplicaCounts approximates the desired canary weight and returns
Expand Down Expand Up @@ -329,7 +336,7 @@ func CalculateReplicaCountsForTrafficRoutedCanary(rollout *v1alpha1.Rollout, wei
if !rollout.Spec.Strategy.Canary.DynamicStableScale {
// Not using dynamic stable scaling. Stable should be left fully scaled (100%), and canary
// will be calculated from setWeight
return canaryCount, rolloutSpecReplica
return checkMinPods(rollout, canaryCount), rolloutSpecReplica
}

// When using dynamic stable scaling, the stable replica count is calculated from the higher of:
Expand All @@ -354,7 +361,7 @@ func CalculateReplicaCountsForTrafficRoutedCanary(rollout *v1alpha1.Rollout, wei
canaryCount = max(trafficWeightReplicaCount, canaryCount)
}
}
return canaryCount, stableCount
return checkMinPods(rollout, canaryCount), checkMinPods(rollout, stableCount)
}

// trafficWeightToReplicas returns the appropriate replicas given the full spec.replicas and a weight
Expand Down
34 changes: 34 additions & 0 deletions utils/replicaset/canary_test.go
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"testing"

"github.com/argoproj/argo-rollouts/utils/defaults"
"github.com/stretchr/testify/assert"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -798,6 +799,39 @@ func TestCalculateReplicaCountsForCanaryTrafficRouting(t *testing.T) {
assert.Equal(t, int32(10), stableRSReplicaCount)
}

func TestCalculateReplicaCountsForCanaryWithMinPods(t *testing.T) {
minPods := defaults.GetDefaultCanaryMinReplicas() // Save initial value
rollout := newRollout(10, 1, intstr.FromInt(0), intstr.FromInt(1), "canary", "stable", nil, nil)

// first test Basic Canary without minPods
stableRS := newRS("stable", 10, 0)
newRS := newRS("canary", 10, 0)
newRSReplicaCount, stableRSReplicaCount := CalculateReplicaCountsForBasicCanary(rollout, newRS, stableRS, nil)
assert.Equal(t, int32(9), newRSReplicaCount)
assert.Equal(t, int32(1), stableRSReplicaCount)

// Now test Basic Canary with minPods
defaults.SetDefaultCanaryMinReplicas(int32(2))
newRSReplicaCount, stableRSReplicaCount = CalculateReplicaCountsForBasicCanary(rollout, newRS, stableRS, nil)
assert.Equal(t, int32(9), newRSReplicaCount)
assert.Equal(t, int32(2), stableRSReplicaCount)

// Now test TrafficRouted Canary
rollout.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{}
newRSReplicaCount, stableRSReplicaCount = CalculateReplicaCountsForTrafficRoutedCanary(rollout, rollout.Status.Canary.Weights)
assert.Equal(t, int32(2), newRSReplicaCount)
assert.Equal(t, int32(10), stableRSReplicaCount)

// Test that we don't use minPods when desired replicas < minPods
rollout = newRollout(1, 1, intstr.FromInt(0), intstr.FromInt(1), "canary", "stable", nil, nil)
rollout.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{}
newRSReplicaCount, stableRSReplicaCount = CalculateReplicaCountsForTrafficRoutedCanary(rollout, rollout.Status.Canary.Weights)
assert.Equal(t, int32(1), newRSReplicaCount)
assert.Equal(t, int32(1), stableRSReplicaCount)

defaults.SetDefaultCanaryMinReplicas(minPods) // Restore initial value
}

func TestCalculateReplicaCountsForCanaryTrafficRoutingDynamicScale(t *testing.T) {
{
// verify we scale down stable
Expand Down