Skip to content

Commit

Permalink
ensure at least one pod is upgraded if cloneset partition != "100%" (o…
Browse files Browse the repository at this point in the history 
…penkruise#954)

Signed-off-by: veophi <[email protected]>
Signed-off-by: Liu Zhenwei <[email protected]>
  • Loading branch information
@veophi @diannaowa
veophi authored and diannaowa committed Sep 14, 2022
1 parent 421958e commit 65efdbd
Show file tree
Hide file tree
Showing 9 changed files with 397 additions and 3 deletions.
4 changes: 4 additions & 0 deletions apis/apps/v1alpha1/cloneset_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,10 @@ type CloneSetStatus struct {
// indicated by updateRevision and have a Ready Condition.
UpdatedReadyReplicas int32 `json:"updatedReadyReplicas"`

// ExpectedUpdatedReplicas is the number of Pods that should be updated by CloneSet controller.
// This field is calculated via Replicas - Partition.
ExpectedUpdatedReplicas int32 `json:"expectUpdatedReplicas,omitempty"`

// UpdateRevision, if not empty, indicates the latest revision of the CloneSet.
UpdateRevision string `json:"updateRevision,omitempty"`

Expand Down
6 changes: 6 additions & 0 deletions config/crd/bases/apps.kruise.io_clonesets.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,12 @@ spec:
description: currentRevision, if not empty, indicates the current
revision version of the CloneSet.
type: string
expectUpdatedReplicas:
description: ExpectedUpdatedReplicas is the number of Pods that should
be updated by CloneSet controller. This field is calculated via
Replicas - Partition.
format: int32
type: integer
labelSelector:
description: LabelSelector is label selectors for query over pods
that should match the replica count used by HPA.
Expand Down
8 changes: 7 additions & 1 deletion pkg/controller/cloneset/cloneset_predownload_image.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1"
clonesetcore "github.com/openkruise/kruise/pkg/controller/cloneset/core"
clonesetutils "github.com/openkruise/kruise/pkg/controller/cloneset/utils"
"github.com/openkruise/kruise/pkg/util"
imagejobutilfunc "github.com/openkruise/kruise/pkg/util/imagejob/utilfunction"
"github.com/openkruise/kruise/pkg/util/inplaceupdate"
apps "k8s.io/api/apps/v1"
Expand Down Expand Up @@ -60,7 +61,12 @@ func (r *ReconcileCloneSet) createImagePullJobsForInPlaceUpdate(cs *appsv1alpha1
// ignore if all Pods update in one batch
var partition, maxUnavailable int
if cs.Spec.UpdateStrategy.Partition != nil {
partition, _ = intstrutil.GetValueFromIntOrPercent(cs.Spec.UpdateStrategy.Partition, int(*cs.Spec.Replicas), true)
if pValue, err := util.CalculatePartitionReplicas(cs.Spec.UpdateStrategy.Partition, cs.Spec.Replicas); err != nil {
klog.Errorf("CloneSet %s/%s partition value is illegal", cs.Namespace, cs.Name)
return err
} else {
partition = pValue
}
}
maxUnavailable, _ = intstrutil.GetValueFromIntOrPercent(
intstrutil.ValueOrDefault(cs.Spec.UpdateStrategy.MaxUnavailable, intstrutil.FromString(appsv1alpha1.DefaultCloneSetMaxUnavailable)), int(*cs.Spec.Replicas), false)
Expand Down
9 changes: 9 additions & 0 deletions pkg/controller/cloneset/cloneset_status.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1"
clonesetcore "github.com/openkruise/kruise/pkg/controller/cloneset/core"
clonesetutils "github.com/openkruise/kruise/pkg/controller/cloneset/utils"
"github.com/openkruise/kruise/pkg/util"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/util/retry"
Expand Down Expand Up @@ -101,4 +102,12 @@ func (r *realStatusUpdater) calculateStatus(cs *appsv1alpha1.CloneSet, newStatus
if newStatus.UpdatedReplicas == newStatus.Replicas {
newStatus.CurrentRevision = newStatus.UpdateRevision
}

if newStatus.UpdateRevision == newStatus.CurrentRevision {
newStatus.ExpectedUpdatedReplicas = *cs.Spec.Replicas
} else {
if partition, err := util.CalculatePartitionReplicas(cs.Spec.UpdateStrategy.Partition, cs.Spec.Replicas); err == nil {
newStatus.ExpectedUpdatedReplicas = *cs.Spec.Replicas - int32(partition)
}
}
}
8 changes: 6 additions & 2 deletions pkg/controller/cloneset/sync/cloneset_sync_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,12 @@ func calculateDiffsWithExpectation(cs *appsv1alpha1.CloneSet, pods []*v1.Pod, cu
replicas := int(*cs.Spec.Replicas)
var partition, maxSurge, maxUnavailable, scaleMaxUnavailable int
if cs.Spec.UpdateStrategy.Partition != nil {
partition, _ = intstrutil.GetValueFromIntOrPercent(cs.Spec.UpdateStrategy.Partition, replicas, true)
partition = integer.IntMin(partition, replicas)
if pValue, err := util.CalculatePartitionReplicas(cs.Spec.UpdateStrategy.Partition, cs.Spec.Replicas); err != nil {
// TODO: maybe, we should block pod update if partition settings is wrong
klog.Errorf("CloneSet %s/%s partition value is illegal", cs.Namespace, cs.Name)
} else {
partition = pValue
}
}
if cs.Spec.UpdateStrategy.MaxSurge != nil {
maxSurge, _ = intstrutil.GetValueFromIntOrPercent(cs.Spec.UpdateStrategy.MaxSurge, replicas, true)
Expand Down
111 changes: 111 additions & 0 deletions pkg/controller/cloneset/sync/cloneset_sync_utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,117 @@ func TestCalculateDiffsWithExpectation(t *testing.T) {
},
expectResult: expectationDiffs{},
},
{
name: "update recreate partition=99% with maxUnavailable=3, maxSurge=2 (step 1/3)",
set: createTestCloneSet(5, intstr.FromString("99%"), intstr.FromInt(3), intstr.FromInt(2)),
pods: []*v1.Pod{
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
},
expectResult: expectationDiffs{scaleNum: 1, useSurge: 1, updateNum: 1, updateMaxUnavailable: 3, scaleUpLimit: 1},
},
{
name: "update recreate partition=99% with maxUnavailable=3, maxSurge=2 (step 2/3)",
set: createTestCloneSet(5, intstr.FromString("99%"), intstr.FromInt(3), intstr.FromInt(2)),
pods: []*v1.Pod{
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
},
expectResult: expectationDiffs{scaleNum: -1, scaleNumOldRevision: -1, deleteReadyLimit: 3},
},
{
name: "update recreate partition=99% with maxUnavailable=3, maxSurge=2 (step 3/3)",
set: createTestCloneSet(5, intstr.FromString("99%"), intstr.FromInt(3), intstr.FromInt(2)),
pods: []*v1.Pod{
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
},
expectResult: expectationDiffs{},
},
{
name: "update recreate partition=99% with maxUnavailable=40%, maxSurge=30% (step 1/3)",
set: createTestCloneSet(5, intstr.FromString("99%"), intstr.FromString("40%"), intstr.FromString("30%")),
pods: []*v1.Pod{
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
},
expectResult: expectationDiffs{scaleNum: 1, useSurge: 1, updateNum: 1, updateMaxUnavailable: 2, scaleUpLimit: 1},
},
{
name: "update recreate partition=99% with maxUnavailable=40%, maxSurge=30% (step 2/3)",
set: createTestCloneSet(5, intstr.FromString("99%"), intstr.FromString("40%"), intstr.FromString("30%")),
pods: []*v1.Pod{
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
},
expectResult: expectationDiffs{scaleNum: -1, scaleNumOldRevision: -1, deleteReadyLimit: 2},
},
{
name: "update recreate partition=99% with maxUnavailable=40%, maxSurge=30% (step 3/3)",
set: createTestCloneSet(5, intstr.FromString("99%"), intstr.FromString("40%"), intstr.FromString("30%")),
pods: []*v1.Pod{
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
},
expectResult: expectationDiffs{},
},
{
name: "update recreate partition=99% with maxUnavailable=30%, maxSurge=30% (step 1/3)",
set: createTestCloneSet(5, intstr.FromString("99%"), intstr.FromString("30%"), intstr.FromString("30%")),
pods: []*v1.Pod{
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
},
expectResult: expectationDiffs{scaleNum: 1, useSurge: 1, updateNum: 1, updateMaxUnavailable: 1, scaleUpLimit: 1},
},
{
name: "update recreate partition=99% with maxUnavailable=30%, maxSurge=30% (step 2/3)",
set: createTestCloneSet(5, intstr.FromString("99%"), intstr.FromString("30%"), intstr.FromString("30%")),
pods: []*v1.Pod{
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
},
expectResult: expectationDiffs{scaleNum: -1, scaleNumOldRevision: -1, deleteReadyLimit: 1},
},
{
name: "update recreate partition=99% with maxUnavailable=30%, maxSurge=30% (step 3/3)",
set: createTestCloneSet(5, intstr.FromString("99%"), intstr.FromString("30%"), intstr.FromString("30%")),
pods: []*v1.Pod{
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(oldRevision, appspub.LifecycleStateNormal, true, false),
createTestPod(newRevision, appspub.LifecycleStateNormal, false, false), // new creation
},
expectResult: expectationDiffs{},
},
{
name: "revision consistent 1",
set: createTestCloneSet(5, intstr.FromInt(0), intstr.FromInt(1), intstr.FromInt(0)),
Expand Down
29 changes: 29 additions & 0 deletions pkg/util/tools.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,3 +145,32 @@ func IsContainerImageEqual(image1, image2 string) bool {

return repo1 == repo2 && tag1 == tag2
}

// CalculatePartitionReplicas returns absolute value of partition for workload. This func can solve some
// corner cases about percentage-type partition, such as:
// - if partition > "0%" and replicas > 0, we will ensure at least 1 old pod is reserved.
// - if partition < "100%" and replicas > 1, we will ensure at least 1 pod is upgraded.
func CalculatePartitionReplicas(partition *intstrutil.IntOrString, replicasPointer *int32) (int, error) {
if partition == nil {
return 0, nil
}

replicas := 1
if replicasPointer != nil {
replicas = int(*replicasPointer)
}

// 'roundUp=true' will ensure at least 1 old pod is reserved if partition > "0%" and replicas > 0.
pValue, err := intstrutil.GetScaledValueFromIntOrPercent(partition, replicas, true)
if err != nil {
return pValue, err
}

// if partition < "100%" and replicas > 1, we will ensure at least 1 pod is upgraded.
if replicas > 1 && pValue == replicas && partition.Type == intstrutil.String && partition.StrVal != "100%" {
pValue = replicas - 1
}

pValue = integer.IntMax(integer.IntMin(pValue, replicas), 0)
return pValue, nil
}
145 changes: 145 additions & 0 deletions pkg/util/tools_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ import (
"fmt"
"sync"
"testing"

"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/utils/pointer"
)

func TestSlowStartBatch(t *testing.T) {
Expand Down Expand Up @@ -154,3 +157,145 @@ func TestIsContainerImageEqual(t *testing.T) {
})
}
}

func TestCalculatePartitionReplicas(t *testing.T) {
cases := []struct {
name string
replicas *int32
partition *intstr.IntOrString
expectedValue int
succeeded bool
}{
{
name: `replicas=0, partition=99%, expected=0`,
replicas: pointer.Int32(0),
partition: &intstr.IntOrString{Type: intstr.String, StrVal: "99%"},
expectedValue: 0,
succeeded: true,
},
{
name: `replicas=10, partition=nil, expected=0`,
replicas: pointer.Int32(10),
partition: nil,
expectedValue: 0,
succeeded: true,
},
{
name: `replicas=9, partition=99%, expected=8`,
replicas: pointer.Int32(9),
partition: &intstr.IntOrString{Type: intstr.String, StrVal: "99%"},
expectedValue: 8,
succeeded: true,
},
{
name: `replicas=10, partition=90%, expected=9`,
replicas: pointer.Int32(10),
partition: &intstr.IntOrString{Type: intstr.String, StrVal: "90%"},
expectedValue: 9,
succeeded: true,
},
{
name: `replicas=10, partition=1%, expected=1`,
replicas: pointer.Int32(10),
partition: &intstr.IntOrString{Type: intstr.String, StrVal: "1%"},
expectedValue: 1,
succeeded: true,
},
{
name: `replicas=99, partition=99%, expected=98`,
replicas: pointer.Int32(99),
partition: &intstr.IntOrString{Type: intstr.String, StrVal: "99%"},
expectedValue: 98,
succeeded: true,
},
{
name: `replicas=99, partition=100%, expected=99`,
replicas: pointer.Int32(99),
partition: &intstr.IntOrString{Type: intstr.String, StrVal: "100%"},
expectedValue: 99,
succeeded: true,
},
{
name: `replicas=99, partition=0%, expected=0`,
replicas: pointer.Int32(99),
partition: &intstr.IntOrString{Type: intstr.String, StrVal: "0%"},
expectedValue: 0,
succeeded: true,
},
{
name: `replicas=nil, partition=100%, expected=1`,
replicas: nil,
partition: &intstr.IntOrString{Type: intstr.String, StrVal: "100%"},
expectedValue: 1,
succeeded: true,
},
{
name: `replicas=nil, partition=nil, expected=0`,
replicas: nil,
partition: nil,
expectedValue: 0,
succeeded: true,
},
{
name: `replicas=nil, partition=1%, expected=1`,
replicas: nil,
partition: &intstr.IntOrString{Type: intstr.String, StrVal: "1%"},
expectedValue: 1,
succeeded: true,
},
{
name: `replicas=nil, partition=1%, expected=1`,
replicas: nil,
partition: &intstr.IntOrString{Type: intstr.String, StrVal: "1%"},
expectedValue: 1,
succeeded: true,
},
{
name: `replicas=10, partition=5, expected=5`,
replicas: pointer.Int32(10),
partition: &intstr.IntOrString{Type: intstr.Int, IntVal: 5},
expectedValue: 5,
succeeded: true,
},
{
name: `replicas=10, partition=1, expected=1`,
replicas: pointer.Int32(10),
partition: &intstr.IntOrString{Type: intstr.Int, IntVal: 1},
expectedValue: 1,
succeeded: true,
},
{
name: `replicas=10, partition=0, expected=0`,
replicas: pointer.Int32(10),
partition: &intstr.IntOrString{Type: intstr.Int, IntVal: 0},
expectedValue: 0,
succeeded: true,
},
{
name: `replicas=10, partition=10, expected=10`,
replicas: pointer.Int32(10),
partition: &intstr.IntOrString{Type: intstr.Int, IntVal: 10},
expectedValue: 10,
succeeded: true,
},
{
name: `replicas=9, partition is illegal, expected=0, err!=nil`,
replicas: pointer.Int32(9),
partition: &intstr.IntOrString{Type: intstr.String, StrVal: "99"},
expectedValue: 0,
succeeded: false,
},
}

for _, cs := range cases {
t.Run(cs.name, func(t *testing.T) {
calculated, err := CalculatePartitionReplicas(cs.partition, cs.replicas)
if (err == nil && !cs.succeeded) || (err != nil && cs.succeeded) {
t.Errorf("got %#v, expect error %#v", err, cs.succeeded)
}
if calculated != cs.expectedValue {
t.Errorf("got %#v, expect %#v", calculated, cs.expectedValue)
}
})
}
}
Loading

0 comments on commit 65efdbd

Please sign in to comment.