Skip to content

Commit 2e34e48

Browse files
AiRanthemzmberg
AiRanthem
authored and
zmberg
committed
Feature: Rewrite step jump logic (#311)
* rewrite step jump logic Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com> * refactor UnifiedStatus Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com> * resolve some conversations Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com> * move function doStepJump from rollout_canary.go to rollout_status.go Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com> --------- Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com>
1 parent e1ccef1 commit 2e34e48

File tree

9 files changed

+683
-88
lines changed

9 files changed

+683
-88
lines changed

‎api/v1beta1/rollout_types.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -456,7 +456,8 @@ type BlueGreenStatus struct {
456456
UpdatedReadyReplicas int32 `json:"updatedReadyReplicas"`
457457
}
458458

459-
// GetSubStatus returns the ethier canary or bluegreen status
459+
// GetSubStatus returns the either canary or blueGreen status
460+
// TODO: It will be gradually deprecated in the future, moving towards using UnifiedStatus to read status uniformly
460461
func (r *RolloutStatus) GetSubStatus() *CommonStatus {
461462
if r.CanaryStatus == nil && r.BlueGreenStatus == nil {
462463
return nil

‎pkg/controller/batchrelease/labelpatch/patcher.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -147,10 +147,18 @@ func (r *realPatcher) patchPodBatchLabel(pods []*corev1.Pod, ctx *batchcontext.B
147147

148148
podBatchID, err := strconv.Atoi(labels[v1beta1.RolloutBatchIDLabel])
149149
if err != nil {
150-
klog.InfoS("Pod batchID is not a number, skip patching", "pod", klog.KObj(pod), "rollout", r.logKey)
150+
klog.InfoS("Pod batchID is not a number, will overwrite it", "pod", klog.KObj(pod),
151+
"rollout", r.logKey, "batchID", labels[v1beta1.RolloutBatchIDLabel])
152+
updatedButUnpatchedPods = append(updatedButUnpatchedPods, pod)
151153
continue
152154
}
153-
plannedUpdatedReplicasForBatches[podBatchID-1]--
155+
if podBatchID > 0 && podBatchID <= len(plannedUpdatedReplicasForBatches) {
156+
plannedUpdatedReplicasForBatches[podBatchID-1]--
157+
} else {
158+
klog.InfoS("Pod batchID is not valid, will overwrite it", "pod", klog.KObj(pod),
159+
"rollout", r.logKey, "batchID", labels[v1beta1.RolloutBatchIDLabel])
160+
updatedButUnpatchedPods = append(updatedButUnpatchedPods, pod)
161+
}
154162
}
155163
klog.InfoS("updatedButUnpatchedPods amount calculated", "amount", len(updatedButUnpatchedPods),
156164
"rollout", r.logKey, "plan", plannedUpdatedReplicasForBatches)

‎pkg/controller/batchrelease/labelpatch/patcher_test.go‎

Lines changed: 45 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ func TestLabelPatcher(t *testing.T) {
7272
},
7373
Batches: []v1beta1.ReleaseBatch{
7474
{
75-
CanaryReplicas: intstr.FromInt(5),
75+
CanaryReplicas: intstr.FromInt32(5),
7676
},
7777
},
7878
expectedPatched: []int{5},
@@ -92,7 +92,7 @@ func TestLabelPatcher(t *testing.T) {
9292
},
9393
Batches: []v1beta1.ReleaseBatch{
9494
{
95-
CanaryReplicas: intstr.FromInt(5),
95+
CanaryReplicas: intstr.FromInt32(5),
9696
},
9797
},
9898
expectedPatched: []int{5},
@@ -112,7 +112,7 @@ func TestLabelPatcher(t *testing.T) {
112112
},
113113
Batches: []v1beta1.ReleaseBatch{
114114
{
115-
CanaryReplicas: intstr.FromInt(5),
115+
CanaryReplicas: intstr.FromInt32(5),
116116
},
117117
},
118118
expectedPatched: []int{5},
@@ -132,7 +132,7 @@ func TestLabelPatcher(t *testing.T) {
132132
},
133133
Batches: []v1beta1.ReleaseBatch{
134134
{
135-
CanaryReplicas: intstr.FromInt(5),
135+
CanaryReplicas: intstr.FromInt32(5),
136136
},
137137
},
138138
expectedPatched: []int{7},
@@ -152,7 +152,7 @@ func TestLabelPatcher(t *testing.T) {
152152
},
153153
Batches: []v1beta1.ReleaseBatch{
154154
{
155-
CanaryReplicas: intstr.FromInt(5),
155+
CanaryReplicas: intstr.FromInt32(5),
156156
},
157157
},
158158
expectedPatched: []int{2},
@@ -172,7 +172,7 @@ func TestLabelPatcher(t *testing.T) {
172172
},
173173
Batches: []v1beta1.ReleaseBatch{
174174
{
175-
CanaryReplicas: intstr.FromInt(5),
175+
CanaryReplicas: intstr.FromInt32(5),
176176
},
177177
},
178178
expectedPatched: []int{5},
@@ -192,8 +192,8 @@ func TestLabelPatcher(t *testing.T) {
192192
return ctx
193193
},
194194
Batches: []v1beta1.ReleaseBatch{
195-
{CanaryReplicas: intstr.FromInt(2)},
196-
{CanaryReplicas: intstr.FromInt(5)},
195+
{CanaryReplicas: intstr.FromInt32(2)},
196+
{CanaryReplicas: intstr.FromInt32(5)},
197197
},
198198
expectedPatched: []int{2, 3},
199199
},
@@ -212,8 +212,8 @@ func TestLabelPatcher(t *testing.T) {
212212
return ctx
213213
},
214214
Batches: []v1beta1.ReleaseBatch{
215-
{CanaryReplicas: intstr.FromInt(2)},
216-
{CanaryReplicas: intstr.FromInt(5)},
215+
{CanaryReplicas: intstr.FromInt32(2)},
216+
{CanaryReplicas: intstr.FromInt32(5)},
217217
},
218218
expectedPatched: []int{2, 3},
219219
},
@@ -232,11 +232,31 @@ func TestLabelPatcher(t *testing.T) {
232232
return ctx
233233
},
234234
Batches: []v1beta1.ReleaseBatch{
235-
{CanaryReplicas: intstr.FromInt(2)},
236-
{CanaryReplicas: intstr.FromInt(5)},
235+
{CanaryReplicas: intstr.FromInt32(2)},
236+
{CanaryReplicas: intstr.FromInt32(5)},
237237
},
238238
expectedPatched: []int{3, 2},
239239
},
240+
"batch-id out of range, equal to no pods are labelled": {
241+
batchContext: func() *batchcontext.BatchContext {
242+
ctx := &batchcontext.BatchContext{
243+
RolloutID: "rollout-1",
244+
UpdateRevision: "version-1",
245+
PlannedUpdatedReplicas: 5,
246+
CurrentBatch: 1,
247+
Replicas: 10,
248+
}
249+
pods := generatePods(1, 5, 3,
250+
"rollout-1", strconv.Itoa(3), ctx.UpdateRevision)
251+
ctx.Pods = pods
252+
return ctx
253+
},
254+
Batches: []v1beta1.ReleaseBatch{
255+
{CanaryReplicas: intstr.FromInt32(2)},
256+
{CanaryReplicas: intstr.FromInt32(5)},
257+
},
258+
expectedPatched: []int{2, 3},
259+
},
240260
}
241261

242262
for name, cs := range cases {
@@ -256,7 +276,7 @@ func TestLabelPatcher(t *testing.T) {
256276
patched := make([]int, ctx.CurrentBatch+1)
257277
for _, pod := range podList.Items {
258278
if pod.Labels[v1beta1.RolloutIDLabel] == ctx.RolloutID {
259-
if batchID, err := strconv.Atoi(pod.Labels[v1beta1.RolloutBatchIDLabel]); err == nil {
279+
if batchID, err := strconv.Atoi(pod.Labels[v1beta1.RolloutBatchIDLabel]); err == nil && batchID <= len(patched) {
260280
patched[batchID-1]++
261281
}
262282
}
@@ -352,7 +372,7 @@ func TestDeploymentPatch(t *testing.T) {
352372
},
353373
Batches: []v1beta1.ReleaseBatch{
354374
{
355-
CanaryReplicas: intstr.FromInt(5),
375+
CanaryReplicas: intstr.FromInt32(5),
356376
},
357377
},
358378
expectedPatched: []int{5},
@@ -372,7 +392,7 @@ func TestDeploymentPatch(t *testing.T) {
372392
},
373393
Batches: []v1beta1.ReleaseBatch{
374394
{
375-
CanaryReplicas: intstr.FromInt(5),
395+
CanaryReplicas: intstr.FromInt32(5),
376396
},
377397
},
378398
expectedPatched: []int{5},
@@ -392,7 +412,7 @@ func TestDeploymentPatch(t *testing.T) {
392412
},
393413
Batches: []v1beta1.ReleaseBatch{
394414
{
395-
CanaryReplicas: intstr.FromInt(5),
415+
CanaryReplicas: intstr.FromInt32(5),
396416
},
397417
},
398418
expectedPatched: []int{5},
@@ -412,7 +432,7 @@ func TestDeploymentPatch(t *testing.T) {
412432
},
413433
Batches: []v1beta1.ReleaseBatch{
414434
{
415-
CanaryReplicas: intstr.FromInt(5),
435+
CanaryReplicas: intstr.FromInt32(5),
416436
},
417437
},
418438
expectedPatched: []int{7},
@@ -432,7 +452,7 @@ func TestDeploymentPatch(t *testing.T) {
432452
},
433453
Batches: []v1beta1.ReleaseBatch{
434454
{
435-
CanaryReplicas: intstr.FromInt(5),
455+
CanaryReplicas: intstr.FromInt32(5),
436456
},
437457
},
438458
expectedPatched: []int{2},
@@ -452,7 +472,7 @@ func TestDeploymentPatch(t *testing.T) {
452472
},
453473
Batches: []v1beta1.ReleaseBatch{
454474
{
455-
CanaryReplicas: intstr.FromInt(5),
475+
CanaryReplicas: intstr.FromInt32(5),
456476
},
457477
},
458478
expectedPatched: []int{5},
@@ -472,8 +492,8 @@ func TestDeploymentPatch(t *testing.T) {
472492
return ctx
473493
},
474494
Batches: []v1beta1.ReleaseBatch{
475-
{CanaryReplicas: intstr.FromInt(2)},
476-
{CanaryReplicas: intstr.FromInt(5)},
495+
{CanaryReplicas: intstr.FromInt32(2)},
496+
{CanaryReplicas: intstr.FromInt32(5)},
477497
},
478498
expectedPatched: []int{2, 3},
479499
},
@@ -492,8 +512,8 @@ func TestDeploymentPatch(t *testing.T) {
492512
return ctx
493513
},
494514
Batches: []v1beta1.ReleaseBatch{
495-
{CanaryReplicas: intstr.FromInt(2)},
496-
{CanaryReplicas: intstr.FromInt(5)},
515+
{CanaryReplicas: intstr.FromInt32(2)},
516+
{CanaryReplicas: intstr.FromInt32(5)},
497517
},
498518
expectedPatched: []int{2, 3},
499519
},
@@ -512,8 +532,8 @@ func TestDeploymentPatch(t *testing.T) {
512532
return ctx
513533
},
514534
Batches: []v1beta1.ReleaseBatch{
515-
{CanaryReplicas: intstr.FromInt(2)},
516-
{CanaryReplicas: intstr.FromInt(5)},
535+
{CanaryReplicas: intstr.FromInt32(2)},
536+
{CanaryReplicas: intstr.FromInt32(5)},
517537
},
518538
expectedPatched: []int{3, 2},
519539
},

‎pkg/controller/rollout/rollout_bluegreen.go‎

Lines changed: 3 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ package rollout
1919
import (
2020
"context"
2121
"fmt"
22-
"reflect"
2322
"time"
2423

2524
"github.com/openkruise/rollouts/api/v1alpha1"
@@ -247,31 +246,10 @@ func (m *blueGreenReleaseManager) doCanaryPaused(c *RolloutContext) (bool, error
247246
}
248247

249248
func (m *blueGreenReleaseManager) doCanaryJump(c *RolloutContext) (jumped bool) {
250-
bluegreenStatus := c.NewStatus.BlueGreenStatus
251-
// since we forbid adding or removing steps, currentStepIndex should always be valid
252-
currentStep := c.Rollout.Spec.Strategy.BlueGreen.Steps[bluegreenStatus.CurrentStepIndex-1]
253-
// nextIndex=-1 means the release is done, nextIndex=0 is not used
254-
if nextIndex := bluegreenStatus.NextStepIndex; nextIndex != util.NextBatchIndex(c.Rollout, bluegreenStatus.CurrentStepIndex) && nextIndex > 0 {
255-
currentIndexBackup := bluegreenStatus.CurrentStepIndex
256-
currentStepStateBackup := bluegreenStatus.CurrentStepState
257-
// update the current and next stepIndex
258-
bluegreenStatus.CurrentStepIndex = nextIndex
259-
bluegreenStatus.NextStepIndex = util.NextBatchIndex(c.Rollout, nextIndex)
260-
nextStep := c.Rollout.Spec.Strategy.BlueGreen.Steps[nextIndex-1]
261-
// compare next step and current step to decide the state we should go
262-
if reflect.DeepEqual(nextStep.Replicas, currentStep.Replicas) {
263-
bluegreenStatus.CurrentStepState = v1beta1.CanaryStepStateTrafficRouting
264-
} else {
265-
bluegreenStatus.CurrentStepState = v1beta1.CanaryStepStateInit
266-
}
267-
bluegreenStatus.LastUpdateTime = &metav1.Time{Time: time.Now()}
268-
klog.Infof("rollout(%s/%s) step(%d->%d) state from(%s -> %s)",
269-
c.Rollout.Namespace, c.Rollout.Name,
270-
currentIndexBackup, bluegreenStatus.CurrentStepIndex,
271-
currentStepStateBackup, bluegreenStatus.CurrentStepState)
272-
return true
249+
if c.Workload == nil {
250+
return false
273251
}
274-
return false
252+
return doStepJump(c.Rollout, c.NewStatus, c.Rollout.Spec.Strategy.BlueGreen.Steps, int(c.Workload.Replicas))
275253
}
276254

277255
// cleanup after rollout is completed or finished

‎pkg/controller/rollout/rollout_canary.go‎

Lines changed: 3 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ package rollout
1919
import (
2020
"context"
2121
"fmt"
22-
"reflect"
2322
"time"
2423

2524
"github.com/openkruise/rollouts/api/v1alpha1"
@@ -313,31 +312,10 @@ func (m *canaryReleaseManager) doCanaryPaused(c *RolloutContext) (bool, error) {
313312
}
314313

315314
func (m *canaryReleaseManager) doCanaryJump(c *RolloutContext) (jumped bool) {
316-
canaryStatus := c.NewStatus.CanaryStatus
317-
// since we forbid adding or removing steps, currentStepIndex should always be valid
318-
currentStep := c.Rollout.Spec.Strategy.Canary.Steps[canaryStatus.CurrentStepIndex-1]
319-
// nextIndex=-1 means the release is done, nextIndex=0 is not used
320-
if nextIndex := canaryStatus.NextStepIndex; nextIndex != util.NextBatchIndex(c.Rollout, canaryStatus.CurrentStepIndex) && nextIndex > 0 {
321-
currentIndexBackup := canaryStatus.CurrentStepIndex
322-
currentStepStateBackup := canaryStatus.CurrentStepState
323-
// update the current and next stepIndex
324-
canaryStatus.CurrentStepIndex = nextIndex
325-
canaryStatus.NextStepIndex = util.NextBatchIndex(c.Rollout, nextIndex)
326-
nextStep := c.Rollout.Spec.Strategy.Canary.Steps[nextIndex-1]
327-
// compare next step and current step to decide the state we should go
328-
if reflect.DeepEqual(nextStep.Replicas, currentStep.Replicas) {
329-
canaryStatus.CurrentStepState = v1beta1.CanaryStepStateTrafficRouting
330-
} else {
331-
canaryStatus.CurrentStepState = v1beta1.CanaryStepStateInit
332-
}
333-
canaryStatus.LastUpdateTime = &metav1.Time{Time: time.Now()}
334-
klog.Infof("rollout(%s/%s) step(%d->%d) state from(%s -> %s)",
335-
c.Rollout.Namespace, c.Rollout.Name,
336-
currentIndexBackup, canaryStatus.CurrentStepIndex,
337-
currentStepStateBackup, canaryStatus.CurrentStepState)
338-
return true
315+
if c.Workload == nil {
316+
return false
339317
}
340-
return false
318+
return doStepJump(c.Rollout, c.NewStatus, c.Rollout.Spec.Strategy.Canary.Steps, int(c.Workload.Replicas))
341319
}
342320

343321
// cleanup after rollout is completed or finished

0 commit comments

Comments
 (0)