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

Add disabled field in rollout spec #155

Merged
merged 1 commit into from
Jul 5, 2023
Merged
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
8 changes: 8 additions & 0 deletions api/v1alpha1/rollout_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ type RolloutSpec struct {
// RolloutID should be changed before each workload revision publication.
// It is to distinguish consecutive multiple workload publications and rollout progress.
DeprecatedRolloutID string `json:"rolloutID,omitempty"`
// if a rollout disabled, then the rollout would not watch changes of workload
//+kubebuilder:validation:Optional
//+kubebuilder:default=false
Disabled bool `json:"disabled"`
}

type ObjectRef struct {
Expand Down Expand Up @@ -257,6 +261,10 @@ const (
RolloutPhaseProgressing RolloutPhase = "Progressing"
// RolloutPhaseTerminating indicates a rollout is terminated
RolloutPhaseTerminating RolloutPhase = "Terminating"
// RolloutPhaseDisabled indicates a rollout is disabled
RolloutPhaseDisabled RolloutPhase = "Disabled"
// RolloutPhaseDisabling indicates a rollout is disabling and releasing resources
RolloutPhaseDisabling RolloutPhase = "Disabling"
)

// +genclient
Expand Down
5 changes: 5 additions & 0 deletions config/crd/bases/rollouts.kruise.io_rollouts.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ spec:
spec:
description: RolloutSpec defines the desired state of Rollout
properties:
disabled:
default: false
description: if a rollout disabled, then the rollout would not watch
changes of workload
type: boolean
objectRef:
description: 'INSERT ADDITIONAL SPEC FIELDS - desired state of cluster
Important: Run "make" to regenerate code after modifying this file
Expand Down
4 changes: 4 additions & 0 deletions pkg/controller/rollout/rollout_controller.go
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ func (r *RolloutReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
if err != nil {
return ctrl.Result{}, err
}

// sync rollout status
retry, newStatus, err := r.calculateRolloutStatus(rollout)
if err != nil {
Expand All @@ -139,11 +140,14 @@ func (r *RolloutReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
return ctrl.Result{RequeueAfter: time.Until(recheckTime)}, nil
}
var recheckTime *time.Time

switch rollout.Status.Phase {
case v1alpha1.RolloutPhaseProgressing:
recheckTime, err = r.reconcileRolloutProgressing(rollout, newStatus)
case v1alpha1.RolloutPhaseTerminating:
recheckTime, err = r.reconcileRolloutTerminating(rollout, newStatus)
case v1alpha1.RolloutPhaseDisabling:
recheckTime, err = r.reconcileRolloutDisabling(rollout, newStatus)
}
if err != nil {
return ctrl.Result{}, err
Expand Down
52 changes: 47 additions & 5 deletions pkg/controller/rollout/rollout_status.go
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,18 @@ func (r *RolloutReconciler) calculateRolloutStatus(rollout *v1alpha1.Rollout) (r
}
return false, newStatus, nil
}

if rollout.Spec.Disabled && newStatus.Phase != v1alpha1.RolloutPhaseDisabled && newStatus.Phase != v1alpha1.RolloutPhaseDisabling {
// if rollout in progressing, indicates a working rollout is disabled, then the rollout should be finalized
if newStatus.Phase == v1alpha1.RolloutPhaseProgressing {
newStatus.Phase = v1alpha1.RolloutPhaseDisabling
newStatus.Message = "Disabling rollout, release resources"
} else {
newStatus.Phase = v1alpha1.RolloutPhaseDisabled
newStatus.Message = "Rollout is disabled"
}
}

if newStatus.Phase == "" {
newStatus.Phase = v1alpha1.RolloutPhaseInitial
}
Expand All @@ -58,12 +70,14 @@ func (r *RolloutReconciler) calculateRolloutStatus(rollout *v1alpha1.Rollout) (r
klog.Errorf("rollout(%s/%s) get workload failed: %s", rollout.Namespace, rollout.Name, err.Error())
return false, nil, err
} else if workload == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

} else if workload == nil && !rollout.Spec.Disabled {

newStatus = &v1alpha1.RolloutStatus{
ObservedGeneration: rollout.Generation,
Phase: v1alpha1.RolloutPhaseInitial,
Message: "Workload Not Found",
if !rollout.Spec.Disabled {
newStatus = &v1alpha1.RolloutStatus{
ObservedGeneration: rollout.Generation,
Phase: v1alpha1.RolloutPhaseInitial,
Message: "Workload Not Found",
}
klog.Infof("rollout(%s/%s) workload not found, and reset status be Initial", rollout.Namespace, rollout.Name)
}
klog.Infof("rollout(%s/%s) workload not found, and reset status be Initial", rollout.Namespace, rollout.Name)
return false, newStatus, nil
}
klog.V(5).Infof("rollout(%s/%s) workload(%s)", rollout.Namespace, rollout.Name, util.DumpJSON(workload))
Expand Down Expand Up @@ -122,6 +136,11 @@ func (r *RolloutReconciler) calculateRolloutStatus(rollout *v1alpha1.Rollout) (r
}
newStatus.Message = "workload deployment is completed"
}
case v1alpha1.RolloutPhaseDisabled:
if !rollout.Spec.Disabled {
newStatus.Phase = v1alpha1.RolloutPhaseHealthy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newStatus.Phase = v1alpha1.RolloutPhaseInitial, Because workload may be not found.

newStatus.Message = "rollout is healthy"
}
}
return false, newStatus, nil
}
Expand Down Expand Up @@ -207,6 +226,29 @@ func (r *RolloutReconciler) reconcileRolloutTerminating(rollout *v1alpha1.Rollou
return c.RecheckTime, nil
}

func (r *RolloutReconciler) reconcileRolloutDisabling(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus) (*time.Time, error) {
workload, err := r.finder.GetWorkloadForRef(rollout)
if err != nil {
klog.Errorf("rollout(%s/%s) get workload failed: %s", rollout.Namespace, rollout.Name, err.Error())
return nil, err
}
c := &RolloutContext{Rollout: rollout, NewStatus: newStatus, Workload: workload}
done, err := r.doFinalising(c)
if err != nil {
return nil, err
} else if done {
klog.Infof("rollout(%s/%s) is disabled", rollout.Namespace, rollout.Name)
newStatus.Phase = v1alpha1.RolloutPhaseDisabled
newStatus.Message = "Rollout is disabled"
} else {
// Incomplete, recheck
expectedTime := time.Now().Add(time.Duration(defaultGracePeriodSeconds) * time.Second)
c.RecheckTime = &expectedTime
klog.Infof("rollout(%s/%s) disabling is incomplete, and recheck(%s)", rollout.Namespace, rollout.Name, expectedTime.String())
}
return c.RecheckTime, nil
}

// handle adding and handle finalizer logic, it turns if we should continue to reconcile
func (r *RolloutReconciler) handleFinalizer(rollout *v1alpha1.Rollout) error {
// delete rollout crd, remove finalizer
Expand Down
68 changes: 68 additions & 0 deletions pkg/controller/rollout/rollout_status_test.go
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package rollout

import (
"context"
"testing"

"github.com/openkruise/rollouts/api/v1alpha1"
Expand Down Expand Up @@ -104,3 +105,70 @@ func TestCalculateRolloutHash(t *testing.T) {
})
}
}

func TestCalculateRolloutStatus(t *testing.T) {
cases := []struct {
name string
getRollout func() *v1alpha1.Rollout
expectPhase v1alpha1.RolloutPhase
}{
{
name: "apply an enabled rollout",
getRollout: func() *v1alpha1.Rollout {
obj := rolloutDemo.DeepCopy()
obj.Name = "Rollout-demo1"
obj.Status = v1alpha1.RolloutStatus{}
obj.Spec.Disabled = false
return obj
},
expectPhase: v1alpha1.RolloutPhaseInitial,
},
{
name: "disable an working rollout",
getRollout: func() *v1alpha1.Rollout {
obj := rolloutDemo.DeepCopy()
obj.Name = "Rollout-demo1"
obj.Status = v1alpha1.RolloutStatus{}
obj.Spec.Disabled = true
return obj
},
expectPhase: v1alpha1.RolloutPhaseDisabled,
},
{
name: "enable an disabled rollout",
getRollout: func() *v1alpha1.Rollout {
obj := rolloutDemo.DeepCopy()
obj.Name = "Rollout-demo2"
obj.Status = v1alpha1.RolloutStatus{}
obj.Spec.Disabled = false
return obj
},
expectPhase: v1alpha1.RolloutPhaseInitial,
},
}

t.Run("RolloutStatus test", func(t *testing.T) {
fc := fake.NewClientBuilder().WithScheme(scheme).Build()
r := &RolloutReconciler{
Client: fc,
Scheme: scheme,
Recorder: record.NewFakeRecorder(10),
finder: util.NewControllerFinder(fc),
trafficRoutingManager: trafficrouting.NewTrafficRoutingManager(fc),
}
r.canaryManager = &canaryReleaseManager{
Client: fc,
trafficRoutingManager: r.trafficRoutingManager,
recorder: r.Recorder,
}
for _, cs := range cases {
rollout := cs.getRollout()
fc.Create(context.TODO(), rollout)
_, newStatus, _ := r.calculateRolloutStatus(rollout)
r.updateRolloutStatusInternal(rollout, *newStatus)
if cs.expectPhase != newStatus.Phase {
t.Fatalf("expect phase %s, get %s, for rollout %s", cs.expectPhase, newStatus.Phase, rollout.Name)
}
}
})
}
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,7 @@ func TestRolloutValidateCreate(t *testing.T) {
object3 := rollout.DeepCopy()
object3.Name = "object-3"
object3.Spec.ObjectRef.WorkloadRef.Kind = "another"

return []client.Object{
object, object1, object2, object3,
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/webhook/workload/mutating/workload_update_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,10 @@ func (h *WorkloadHandler) fetchMatchedRollout(obj client.Object) (*appsv1alpha1.
if !rollout.DeletionTimestamp.IsZero() || rollout.Spec.ObjectRef.WorkloadRef == nil {
continue
}
if rollout.Status.Phase == appsv1alpha1.RolloutPhaseDisabled {
klog.Infof("Disabled rollout(%s/%s) fetched when fetching matched rollout", rollout.Namespace, rollout.Name)
continue
}
ref := rollout.Spec.ObjectRef.WorkloadRef
gv, err := schema.ParseGroupVersion(ref.APIVersion)
if err != nil {
Expand Down
82 changes: 82 additions & 0 deletions test/e2e/rollout_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5485,6 +5485,88 @@ var _ = SIGDescribe("Rollout", func() {
})

})

KruiseDescribe("Disabled rollout tests", func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add an ut about enable a disabled rollout

rollout := &v1alpha1.Rollout{}
Expect(ReadYamlToObject("./test_data/rollout/rollout_disabled.yaml", rollout)).ToNot(HaveOccurred())
It("Rollout status tests", func() {
By("Create an enabled rollout")
rollout1 := rollout.DeepCopy()
rollout1.Name = "rollout-demo1"
rollout1.Spec.Disabled = false
CreateObject(rollout1)
time.Sleep(1 * time.Second)

By("Create another enabled rollout")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if there are multiple Rollout creations, the Webhook should reject the later rollout creations.

rollout2 := rollout.DeepCopy()
rollout2.Name = "rollout-demo2"
rollout2.Spec.Disabled = false
rollout2.SetNamespace(namespace)
Expect(k8sClient.Create(context.TODO(), rollout2)).Should(HaveOccurred())

By("Creating a disabled rollout")
rollout3 := rollout.DeepCopy()
rollout3.Name = "rollout-demo3"
rollout3.Spec.Disabled = true
rollout2.SetNamespace(namespace)
Expect(k8sClient.Create(context.TODO(), rollout2)).Should(HaveOccurred())
// wait for reconciling
time.Sleep(3 * time.Second)
Expect(GetObject(rollout1.Name, rollout1)).NotTo(HaveOccurred())
Expect(rollout1.Status.Phase).Should(Equal(v1alpha1.RolloutPhaseInitial))

By("Create workload")
deploy := &apps.Deployment{}
Expect(ReadYamlToObject("./test_data/rollout/deployment_disabled.yaml", deploy)).ToNot(HaveOccurred())
CreateObject(deploy)
WaitDeploymentAllPodsReady(deploy)
Expect(GetObject(rollout1.Name, rollout1)).NotTo(HaveOccurred())
Expect(rollout1.Status.Phase).Should(Equal(v1alpha1.RolloutPhaseHealthy))

By("Updating deployment version-1 to version-2")
Expect(GetObject(deploy.Name, deploy)).NotTo(HaveOccurred())
newEnvs := mergeEnvVar(deploy.Spec.Template.Spec.Containers[0].Env, v1.EnvVar{Name: "VERSION", Value: "version-2"})
deploy.Spec.Template.Spec.Containers[0].Env = newEnvs
UpdateDeployment(deploy)
WaitRolloutCanaryStepPaused(rollout1.Name, 1)
Expect(GetObject(rollout1.Name, rollout1)).NotTo(HaveOccurred())
Expect(rollout1.Status.CanaryStatus.CanaryReplicas).Should(BeNumerically("==", 2))
Expect(rollout1.Status.CanaryStatus.CanaryReadyReplicas).Should(BeNumerically("==", 2))
Expect(GetObject(deploy.Name, deploy)).NotTo(HaveOccurred())
Expect(deploy.Spec.Paused).Should(BeTrue())

By("Disable a rolling rollout")
rollout1.Spec.Disabled = true
UpdateRollout(rollout1)
time.Sleep(5 * time.Second)

By("Rolling should be resumed")
Expect(GetObject(deploy.Name, deploy)).NotTo(HaveOccurred())
Expect(deploy.Spec.Paused).Should(BeFalse())

By("Batchrelease should be deleted")
key := types.NamespacedName{Namespace: namespace, Name: rollout1.Name}
Expect(k8sClient.Get(context.TODO(), key, &v1alpha1.BatchRelease{})).Should(HaveOccurred())
Expect(GetObject(rollout1.Name, rollout1)).NotTo(HaveOccurred())
Expect(rollout1.Status.Phase).Should(Equal(v1alpha1.RolloutPhaseDisabled))

By("Updating deployment version-2 to version-3")
Expect(GetObject(deploy.Name, deploy)).NotTo(HaveOccurred())
newEnvs = mergeEnvVar(deploy.Spec.Template.Spec.Containers[0].Env, v1.EnvVar{Name: "VERSION", Value: "version-3"})
deploy.Spec.Template.Spec.Containers[0].Env = newEnvs
UpdateDeployment(deploy)
time.Sleep(3 * time.Second)
Expect(GetObject(deploy.Name, deploy)).NotTo(HaveOccurred())
Expect(deploy.Spec.Paused).Should(BeFalse())

By("Enable a disabled rollout")
rollout1.Spec.Disabled = false
UpdateRollout(rollout1)
time.Sleep(3 * time.Second)
Expect(GetObject(rollout1.Name, rollout1)).NotTo(HaveOccurred())
Expect(rollout1.Status.Phase).Should(Equal(v1alpha1.RolloutPhaseHealthy))
})
})
})

func mergeEnvVar(original []v1.EnvVar, add v1.EnvVar) []v1.EnvVar {
Expand Down
23 changes: 23 additions & 0 deletions test/e2e/test_data/rollout/deployment_disabled.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
apiVersion: apps/v1
kind: Deployment
metadata:
name: workload-demo
namespace: default
spec:
replicas: 10
selector:
matchLabels:
app: demo
template:
metadata:
labels:
app: demo
spec:
containers:
- name: busybox
image: busybox:latest
imagePullPolicy: IfNotPresent
command: ["/bin/sh", "-c", "sleep 100d"]
env:
- name: VERSION
value: "version-1"
19 changes: 19 additions & 0 deletions test/e2e/test_data/rollout/rollout_disabled.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
apiVersion: rollouts.kruise.io/v1alpha1
kind: Rollout
metadata:
name: rollouts-demo
namespace: default
annotations:
rollouts.kruise.io/rolling-style: partition
spec:
disabled: false
objectRef:
workloadRef:
apiVersion: apps/v1
kind: Deployment
name: workload-demo
strategy:
canary:
steps:
- replicas: 2
- replicas: 50%