Merge pull request #495 from ta924/addFinalize

Add canary finalizers

artifacts/flagger/crd.yaml

@@ -502,6 +502,9 @@ spec:
  skipAnalysis:
  description: Skip analysis and promote canary
  type: boolean
+ revertOnDeletion:
+ description: Revert mutated resources to original spec on deletion
+ type: boolean
  analysis:
  description: Canary analysis for this canary
  type: object
@@ -650,6 +653,8 @@ spec:
  - Finalising
  - Succeeded
  - Failed
+ - Terminating
+ - Terminated
  canaryWeight:
  description: Traffic weight percentage routed to canary
  type: number

charts/flagger/crds/crd.yaml

@@ -502,6 +502,9 @@ spec:
  skipAnalysis:
  description: Skip analysis and promote canary
  type: boolean
+ revertOnDeletion:
+ description: Revert mutated resources to original spec on deletion
+ type: boolean
  analysis:
  description: Canary analysis for this canary
  type: object
@@ -650,6 +653,8 @@ spec:
  - Finalising
  - Succeeded
  - Failed
+ - Terminating
+ - Terminated
  canaryWeight:
  description: Traffic weight percentage routed to canary
  type: number

docs/gitbook/usage/how-it-works.md

@@ -240,6 +240,32 @@ kubectl wait canary/podinfo --for=condition=promoted --timeout=5m
 kubectl get canary/podinfo | grep Succeeded
 ```
 
+### Canary finalizers
+
+The default behavior of Flagger on canary deletion is to leave resources that aren't owned by the controller 
+in their current state. This simplifies the deletion action and avoids possible deadlocks during resource 
+finalization. In the event the canary was introduced with existing resource(s) (i.e. service, virtual service, etc.),
+they would be mutated during the initialization phase and no longer reflect their initial state. If the desired
+functionality upon deletion is to revert the resources to their initial state, the `revertOnDeletion` attribute
+can be enabled. 
+
+```yaml
+spec:
+ revertOnDeletion: true
+```
+
+When a deletion action is submitted to the cluster, Flagger will attempt to revert the following resources:
+
+* [Canary target](#canary-target) replicas will be updated to the primary replica count
+* [Canary service](#canary-service) selector will be reverted
+* Mesh/Ingress traffic routed to the target 
+
+The recommended approach to disable canary analysis would be utilization of the `skipAnalysis`
+attribute, which limits the need for resource reconciliation. Utilizing the `revertOnDeletion` attribute should be 
+enabled when you no longer plan to rely on Flagger for deployment management.
+
+**Note** When this feature is enabled expect a delay in the delete action due to the reconciliation. 
+
 ### Canary analysis
 
 The canary analysis defines:

go.sum

@@ -322,4 +322,4 @@ modernc.org/strutil v1.0.0/go.mod h1:lstksw84oURvj9y3tn8lGvRxyRC1S2+g5uuIzNfIOBs
 modernc.org/xc v1.0.0/go.mod h1:mRNCo0bvLjGhHO9WsyuKVU4q0ceiDDDoEeWDJHrNx8I=
 sigs.k8s.io/structured-merge-diff v0.0.0-20190525122527-15d366b2352e/go.mod h1:wWxsB5ozmmv/SG7nM11ayaAW51xMvak/t1r0CSlcokI=
 sigs.k8s.io/yaml v1.1.0 h1:4A07+ZFc2wgJwo8YNlQpr1rVlgUDlxXHhPJciaPY5gs=
-sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o=
+sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o=

kustomize/base/flagger/crd.yaml

@@ -502,6 +502,9 @@ spec:
  skipAnalysis:
  description: Skip analysis and promote canary
  type: boolean
+ revertOnDeletion:
+ description: Revert mutated resources to original spec on deletion
+ type: boolean
  analysis:
  description: Canary analysis for this canary
  type: object
@@ -650,6 +653,8 @@ spec:
  - Finalising
  - Succeeded
  - Failed
+ - Terminating
+ - Terminated
  canaryWeight:
  description: Traffic weight percentage routed to canary
  type: number

pkg/apis/flagger/v1beta1/canary.go

@@ -94,6 +94,10 @@ type CanarySpec struct {
  // SkipAnalysis promotes the canary without analysing it
  // +optional
  SkipAnalysis bool `json:"skipAnalysis,omitempty"`
+
+ // revert canary mutation on deletion of canary resource
+ // +optional
+ RevertOnDeletion bool `json:"revertOnDeletion,omitempty"`
 }
 
 // CanaryService defines how ClusterIP services, service mesh or ingress routing objects are generated
@@ -298,7 +302,7 @@ type CanaryWebhook struct {
  URL string `json:"url"`
 
  // Request timeout for this webhook
- Timeout string `json:"timeout"`
+ Timeout string `json:"timeout,omitempty"`
 
  // Metadata (key-value pairs) for this webhook
  // +optional

pkg/apis/flagger/v1beta1/status.go

@@ -57,6 +57,12 @@ const (
  // CanaryPhaseFailed means the canary analysis failed
  // and the canary deployment has been scaled to zero
  CanaryPhaseFailed CanaryPhase = "Failed"
+ // CanaryPhaseTerminating means the canary has been marked
+ // for deletion and in the finalizing state
+ CanaryPhaseTerminating CanaryPhase = "Terminating"
+ // CanaryPhaseTerminated means the canary has been finalized
+ // and successfully deleted
+ CanaryPhaseTerminated CanaryPhase = "Terminated"
 )
 
 // CanaryStatus is used for state persistence (read-only)

pkg/canary/controller.go

@@ -19,4 +19,5 @@ type Controller interface {
  HaveDependenciesChanged(canary *flaggerv1.Canary) (bool, error)
  ScaleToZero(canary *flaggerv1.Canary) error
  ScaleFromZero(canary *flaggerv1.Canary) error
+ Finalize(canary *flaggerv1.Canary) error
 }

pkg/canary/daemonset_controller.go

@@ -296,3 +296,12 @@ func (c *DaemonSetController) getSelectorLabel(daemonSet *appsv1.DaemonSet) (str
 func (c *DaemonSetController) HaveDependenciesChanged(cd *flaggerv1.Canary) (bool, error) {
  return c.configTracker.HasConfigChanged(cd)
 }
+
+//Finalize scale the reference instance from zero
+func (c *DaemonSetController) Finalize(cd *flaggerv1.Canary) error {
+
+ if err := c.ScaleFromZero(cd); err != nil {
+ return err
+ }
+ return nil
+}

pkg/canary/daemonset_controller_test.go

@@ -194,3 +194,19 @@ func TestDaemonSetController_Scale(t *testing.T) {
  }
  })
 }
+
+func TestDaemonSetController_Finalize(t *testing.T) {
+ mocks := newDaemonSetFixture()
+ err := mocks.controller.Initialize(mocks.canary, true)
+ require.NoError(t, err)
+
+ err = mocks.controller.Finalize(mocks.canary)
+ require.NoError(t, err)
+
+ dep, err := mocks.kubeClient.AppsV1().DaemonSets("default").Get("podinfo", metav1.GetOptions{})
+ require.NoError(t, err)
+
+ _, ok := dep.Spec.Template.Spec.NodeSelector["flagger.app/scale-to-zero"]
+
+ assert.False(t, ok)
+}

pkg/canary/deployment_controller.go

@@ -179,6 +179,27 @@ func (c *DeploymentController) ScaleFromZero(cd *flaggerv1.Canary) error {
  return nil
 }
 
+// Scale sets the canary deployment replicas
+func (c *DeploymentController) Scale(cd *flaggerv1.Canary, replicas int32) error {
+ targetName := cd.Spec.TargetRef.Name
+ dep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(targetName, metav1.GetOptions{})
+ if err != nil {
+ if errors.IsNotFound(err) {
+ return fmt.Errorf("deployment %s.%s not found", targetName, cd.Namespace)
+ }
+ return fmt.Errorf("deployment %s.%s query error %v", targetName, cd.Namespace, err)
+ }
+
+ depCopy := dep.DeepCopy()
+ depCopy.Spec.Replicas = int32p(replicas)
+
+ _, err = c.kubeClient.AppsV1().Deployments(dep.Namespace).Update(depCopy)
+ if err != nil {
+ return fmt.Errorf("scaling %s.%s to %v failed: %v", depCopy.GetName(), depCopy.Namespace, replicas, err)
+ }
+ return nil
+}
+
 // GetMetadata returns the pod label selector and svc ports
 func (c *DeploymentController) GetMetadata(cd *flaggerv1.Canary) (string, map[string]int32, error) {
  targetName := cd.Spec.TargetRef.Name
@@ -376,3 +397,39 @@ func (c *DeploymentController) getSelectorLabel(deployment *appsv1.Deployment) (
 func (c *DeploymentController) HaveDependenciesChanged(cd *flaggerv1.Canary) (bool, error) {
  return c.configTracker.HasConfigChanged(cd)
 }
+
+// Finalize will set the replica count from the primary to the reference instance. This method is used
+// during a delete to attempt to revert the deployment back to the original state. Error is returned if unable
+// update the reference deployment replicas to the primary replicas
+func (c *DeploymentController) Finalize(cd *flaggerv1.Canary) error {
+
+ primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name)
+
+ //Get ref deployment
+ refDep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(cd.Spec.TargetRef.Name, metav1.GetOptions{})
+ if err != nil {
+ return err
+ }
+
+ //2. Get primary if possible, if not scale from zero
+ primaryDep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(primaryName, metav1.GetOptions{})
+ if err != nil {
+ if errors.IsNotFound(err) {
+ if err := c.ScaleFromZero(cd); err != nil {
+ return err
+ }
+ return nil
+ }
+ return err
+ }
+
+ //3. If both ref and primary present update the replicas of the ref to match the primary
+ if refDep.Spec.Replicas != primaryDep.Spec.Replicas {
+ //3. Set the replicas value on the original reference deployment
+ if err := c.Scale(cd, int32Default(primaryDep.Spec.Replicas)); err != nil {
+ return err
+ }
+ }
+
+ return nil
+}

pkg/canary/deployment_controller_test.go

@@ -188,3 +188,48 @@ func TestDeploymentController_HasTargetChanged(t *testing.T) {
  require.NoError(t, err)
  assert.True(t, isNew)
 }
+
+func TestDeploymentController_Finalize(t *testing.T) {
+
+ mocks := newDeploymentFixture()
+
+ tables := []struct {
+ mocks deploymentControllerFixture
+ callInitialize bool
+ shouldError bool
+ expectedReplicas int32
+ canary *flaggerv1.Canary
+ }{
+ //Primary not found returns error
+ {mocks, false, false, 1, mocks.canary},
+ //Happy path
+ {mocks, true, false, 1, mocks.canary},
+ }
+
+ for _, table := range tables {
+ if table.callInitialize {
+ err := mocks.controller.Initialize(table.canary, true)
+ if err != nil {
+ t.Fatal(err.Error())
+ }
+ }
+
+ err := mocks.controller.Finalize(table.canary)
+
+ if table.shouldError && err == nil {
+ t.Error("Expected error while calling Finalize, but none was returned")
+ } else if !table.shouldError && err != nil {
+ t.Errorf("Expected no error would be returned while calling Finalize, but returned %s", err)
+ }
+
+ if table.expectedReplicas > 0 {
+ c, err := mocks.kubeClient.AppsV1().Deployments(mocks.canary.Namespace).Get(mocks.canary.Name, metav1.GetOptions{})
+ if err != nil {
+ t.Fatal(err.Error())
+ }
+ if int32Default(c.Spec.Replicas) != table.expectedReplicas {
+ t.Errorf("Expected replicas %d recieved replicas %d", table.expectedReplicas, c.Spec.Replicas)
+ }
+ }
+ }
+}

pkg/canary/service_controller.go

@@ -221,3 +221,7 @@ func (c *ServiceController) IsPrimaryReady(_ *flaggerv1.Canary) error {
 func (c *ServiceController) IsCanaryReady(_ *flaggerv1.Canary) (bool, error) {
  return true, nil
 }
+
+func (c *ServiceController) Finalize(cd *flaggerv1.Canary) error {
+ return nil
+}

pkg/controller/controller.go

@@ -128,6 +128,21 @@ func NewController(
  }
 
  ctrl.enqueue(new)
+ } else if !newCanary.DeletionTimestamp.IsZero() && hasFinalizer(&newCanary, finalizer) ||
+ !hasFinalizer(&newCanary, finalizer) && newCanary.Spec.RevertOnDeletion {
+ //If this was marked for deletion and has finalizers enqueue for finalizing or
+ //If this canary doesn't have finalizers and RevertOnDeletion is true updated speck enqueue
+ ctrl.enqueue(new)
+ }
+
+ //If canary no longer desires reverting, finalizers should be removed
+ if oldCanary.Spec.RevertOnDeletion && !newCanary.Spec.RevertOnDeletion {
+ ctrl.logger.Infof("%s.%s opting out, deleting finalizers", newCanary.Name, newCanary.Namespace)
+ err := ctrl.removeFinalizer(&newCanary, finalizer)
+ if err != nil {
+ ctrl.logger.Warnf("Failed to remove finalizers for %s.%s", oldCanary.Name, oldCanary.Namespace)
+ return
+ }
  }
  },
  DeleteFunc: func(old interface{}) {
@@ -217,6 +232,33 @@ func (c *Controller) syncHandler(key string) error {
  return nil
  }
 
+ //Finalize if canary has been marked for deletion and revert is desired
+ if cd.Spec.RevertOnDeletion && cd.ObjectMeta.DeletionTimestamp != nil {
+
+ //If finalizers have been previously removed proceed
+ if !hasFinalizer(cd, finalizer) {
+ c.logger.Infof("Canary %s.%s has been finalized", cd.Name, cd.Namespace)
+ return nil
+ }
+
+ if cd.Status.Phase != flaggerv1.CanaryPhaseTerminated {
+ if err := c.finalize(cd); err != nil {
+ return fmt.Errorf("unable to finalize to canary %s.%s error %s", cd.Name, cd.Namespace, err)
+ }
+ }
+
+ //Remove finalizer from Canary
+ if err := c.removeFinalizer(cd, finalizer); err != nil {
+ return fmt.Errorf("unable to remove finalizer for canary %s.%s", cd.Name, cd.Namespace)
+ }
+
+ //record event
+ c.recordEventInfof(cd, "Terminated canary %s.%s", cd.Name, cd.Namespace)
+
+ c.logger.Infof("Canary %s.%s has been successfully processed and marked for deletion", cd.Name, cd.Namespace)
+ return nil
+ }
+
  // set status condition for new canaries
  if cd.Status.Conditions == nil {
  if ok, conditions := canary.MakeStatusConditions(cd, flaggerv1.CanaryPhaseInitializing); ok {
@@ -233,6 +275,14 @@ func (c *Controller) syncHandler(key string) error {
  }
 
  c.canaries.Store(fmt.Sprintf("%s.%s", cd.Name, cd.Namespace), cd)
+
+ //If opt in for revertOnDeletion add finaliers if not present
+ if cd.Spec.RevertOnDeletion && !hasFinalizer(cd, finalizer) {
+ if err := c.addFinalizer(cd, finalizer); err != nil {
+ return fmt.Errorf("unable to add finalizer to canary %s.%s", cd.Name, cd.Namespace)
+ }
+
+ }
  c.logger.Infof("Synced %s", key)
 
  return nil