Fix rollback testing scenarios

api/v1alpha1/appgroup_types.go

@@ -16,6 +16,18 @@ import (
 
 type WorkflowType string
 
+const (
+	Forward  WorkflowType = "forward"
+	Reverse  WorkflowType = "reverse"
+	Rollback WorkflowType = "rollback"
+)
+
+var WorkflowConditionMap = map[WorkflowType]string{
+	Forward:  meta.ForwardWorkflowSucceededCondition,
+	Reverse:  meta.ReverseWorkflowSucceededCondition,
+	Rollback: meta.RollbackWorkflowSucceededCondition,
+}
+
 const (
 	DefaultProgressingRequeue = 5 * time.Second
 	DefaultSucceededRequeue   = 5 * time.Minute
@@ -270,6 +282,17 @@ func (in *ApplicationGroup) GetReadyCondition() string {
 	return condition.Reason
 }
 
+func (in *ApplicationGroup) GetWorkflowCondition(wfType WorkflowType) string {
+	var condition *metav1.Condition
+	if wfCondition, ok := WorkflowConditionMap[wfType]; ok {
+		condition = meta.GetResourceCondition(in, wfCondition)
+	}
+	if condition == nil {
+		return meta.ProgressingReason
+	}
+	return condition.Reason
+}
+
 // GetStatusConditions gets the status conditions from the
 // ApplicationGroup status
 func (in *ApplicationGroup) GetStatusConditions() *[]metav1.Condition {
@@ -300,7 +323,7 @@ func (in *ApplicationGroup) SetLastSuccessful() {
 }
 
 // +kubebuilder:object:root=true
-// +kubebuilder:resource:path=applicationgroups,scope=Cluster,shortName=ag
+// +kubebuilder:resource:path=applicationgroups,scope=Cluster,shortName={"ag","appgroup"}
 // +kubebuilder:subresource:status
 // +kubebuilder:printcolumn:name="Ready",type="string",JSONPath=".status.conditions[?(@.type==\"Ready\")].status"
 // +kubebuilder:printcolumn:name="Reason",type="string",JSONPath=".status.conditions[?(@.type==\"Ready\")].reason"

chart/orkestra/crds/orkestra.azure.microsoft.com_applicationgroups.yaml

@@ -15,6 +15,7 @@ spec:
     plural: applicationgroups
     shortNames:
     - ag
+    - appgroup
     singular: applicationgroup
   scope: Cluster
   versions:

config/crd/bases/orkestra.azure.microsoft.com_applicationgroups.yaml

@@ -15,6 +15,7 @@ spec:
     plural: applicationgroups
     shortNames:
     - ag
+    - appgroup
     singular: applicationgroup
   scope: Cluster
   versions:

controllers/appgroup_controller.go

@@ -6,6 +6,7 @@ package controllers
 import (
 	"context"
 	"fmt"
+
 	"github.com/Azure/Orkestra/pkg/helpers"
 
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -161,7 +162,7 @@ func (r *ApplicationGroupReconciler) SetupWithManager(mgr ctrl.Manager) error {
 }
 
 func (r *ApplicationGroupReconciler) ShouldRemediate(ctx context.Context, instance *v1alpha1.ApplicationGroup) (bool, error) {
-	forwardClient, _ := r.WorkflowClientBuilder.Forward(instance).Build()
+	forwardClient := r.WorkflowClientBuilder.Forward(instance).Build()
 
 	isFailed, err := workflow.IsFailed(ctx, forwardClient)
 	if err != nil {

controllers/appgroup_controller_test.go

@@ -299,7 +299,7 @@ var _ = Describe("ApplicationGroup Controller", func() {
 			}, DefaultTimeout, time.Second).Should(BeTrue())
 		})
 
-		It("should succeed to rollback helm chart versions on failure", func() {
+		FIt("should succeed to rollback helm chart versions on failure", func() {
 			applicationGroup := defaultAppGroup(name)
 			applicationGroup.Namespace = DefaultNamespace
 			applicationGroup.Spec.Applications[1].Spec.Chart.Version = ambassadorOldChartVersion
@@ -366,7 +366,8 @@ var _ = Describe("ApplicationGroup Controller", func() {
 				}
 				return ambassadorHelmRelease.Spec.Chart.Spec.Version == ambassadorOldChartVersion &&
 					meta.GetResourceCondition(ambassadorHelmRelease, meta.ReadyCondition).Reason == meta2.ReconciliationSucceededReason &&
-					applicationGroup.GetReadyCondition() == meta.SucceededReason
+					applicationGroup.GetReadyCondition() == meta.WorkflowFailedReason &&
+					applicationGroup.GetWorkflowCondition(v1alpha1.Rollback) == meta.SucceededReason
 			}, DefaultTimeout, time.Second).Should(BeTrue())
 		})
 

controllers/testutils.go

@@ -39,7 +39,7 @@ func defaultAppGroup(targetNamespace string) *v1alpha1.ApplicationGroup {
 		},
 	}
 	g.Spec.Applications = make([]v1alpha1.Application, 0)
-	g.Spec.Applications = append(g.Spec.Applications, ambassadorApplication(targetNamespace), bookinfoApplication(targetNamespace))
+	g.Spec.Applications = append(g.Spec.Applications, bookinfoApplication(targetNamespace), ambassadorApplication(targetNamespace))
 	return g
 }
 

pkg/helpers/reconcile.go

@@ -7,8 +7,6 @@ import (
 	"os"
 	"strings"
 
-	kerrors "k8s.io/apimachinery/pkg/api/errors"
-
 	"github.com/Azure/Orkestra/api/v1alpha1"
 	"github.com/Azure/Orkestra/pkg/meta"
 	"github.com/Azure/Orkestra/pkg/registry"
@@ -64,8 +62,8 @@ func (helper *ReconcileHelper) CreateOrUpdate(ctx context.Context) error {
 		return fmt.Errorf("failed to reconcile the applications : %w", err)
 	}
 	// Generate the Workflow object to submit to Argo
-	engine, _ := helper.WorkflowClientBuilder.Forward(helper.Instance).Build()
-	if err := workflow.Run(ctx, engine); err != nil {
+	forwardClient := helper.WorkflowClientBuilder.Forward(helper.Instance).Build()
+	if err := workflow.Run(ctx, forwardClient); err != nil {
 		helper.StatusHelper.MarkTemplateGenerationFailed(helper.Instance, err)
 		helper.Error(err, "failed to reconcile ApplicationGroup instance")
 		return err
@@ -107,50 +105,47 @@ func (helper *ReconcileHelper) Rollback(ctx context.Context, patch client.Patch,
 	}
 
 	helper.Info("Rolling back to last successful application group spec")
-	rollbackInstance := helper.Instance.DeepCopy()
-	rollbackInstance.Spec = *helper.Instance.GetLastSuccessful()
-	rollbackClient, _ := helper.WorkflowClientBuilder.Rollback(rollbackInstance).Build()
+	rollbackClient := helper.WorkflowClientBuilder.Rollback(helper.Instance).Build()
+
+	// Re-running the workflow will not re-generate it since we check if we have already started it
 	if err := workflow.Run(ctx, rollbackClient); err != nil {
 		helper.Error(err, "failed to create the workflow for rollback")
 		return ctrl.Result{}, err
 	}
-
+	if isSucceeded, err := workflow.IsSucceeded(ctx, rollbackClient); err != nil {
+		helper.Error(err, "failed to validate if the workflow is succeeded")
+		return ctrl.Result{}, err
+	} else if isSucceeded {
+		return ctrl.Result{}, nil
+	}
 	return reconcile.Result{RequeueAfter: v1alpha1.DefaultProgressingRequeue}, nil
 }
 
 func (helper *ReconcileHelper) Reverse(ctx context.Context) (ctrl.Result, error) {
-	reverseClient, _ := helper.WorkflowClientBuilder.Reverse(helper.Instance).Build()
-	forwardClient, _ := helper.WorkflowClientBuilder.Forward(helper.Instance).Build()
-	if rwf, err := reverseClient.GetWorkflow(ctx); kerrors.IsNotFound(err) {
-		helper.Info("Reversing the workflow")
-		if err := workflow.Run(ctx, reverseClient); err != nil && strings.Contains(err.Error(), meta.ErrForwardWorkflowNotFound.Error()) {
-			// Forward workflow wasn't found so we just return
-			return ctrl.Result{}, nil
-		} else if err != nil {
-			helper.Error(err, "failed to generate reverse workflow")
-			// if generation of reverse workflow failed, delete the forward workflow and return
-			if err := workflow.DeleteWorkflow(ctx, forwardClient); err != nil {
-				helper.Error(err, "failed to delete workflow CRO")
-				return ctrl.Result{}, err
-			}
-			return ctrl.Result{}, nil
-		}
-		return ctrl.Result{Requeue: true}, nil
+	reverseClient := helper.WorkflowClientBuilder.Reverse(helper.Instance).Build()
+	forwardClient := helper.WorkflowClientBuilder.Forward(helper.Instance).Build()
+	helper.Info("Reversing the workflow")
+
+	// Re-running the workflow will not re-generate it since we check if we have already started it
+	if err := workflow.Run(ctx, reverseClient); err != nil && strings.Contains(err.Error(), meta.ErrForwardWorkflowNotFound.Error()) {
+		// Forward workflow wasn't found so we just return
+		return ctrl.Result{}, nil
 	} else if err != nil {
-		helper.Error(err, "failed to GET workflow object with an unrecoverable error")
-		return ctrl.Result{}, err
-	} else {
-		if !rwf.Status.FinishedAt.IsZero() {
-			helper.Info("reverse workflow is finished")
-			if err := workflow.DeleteWorkflow(ctx, reverseClient); err != nil {
-				helper.Error(err, "failed to delete workflow CRO")
-				return ctrl.Result{}, err
-			}
-		} else {
-			return ctrl.Result{RequeueAfter: v1alpha1.DefaultProgressingRequeue}, nil
+		helper.Error(err, "failed to generate reverse workflow")
+		// if generation of reverse workflow failed, delete the forward workflow and return
+		if err := workflow.DeleteWorkflow(ctx, forwardClient); err != nil {
+			helper.Error(err, "failed to delete workflow CRO")
+			return ctrl.Result{}, err
 		}
+		return ctrl.Result{}, nil
+	}
+	if isSucceeded, err := workflow.IsSucceeded(ctx, reverseClient); err != nil {
+		helper.Error(err, "failed to validate if the workflow is succeeded")
+		return ctrl.Result{}, err
+	} else if isSucceeded {
+		return ctrl.Result{}, nil
 	}
-	return ctrl.Result{}, nil
+	return ctrl.Result{RequeueAfter: v1alpha1.DefaultProgressingRequeue}, nil
 }
 
 func (helper *ReconcileHelper) reconcileApplications() error {

pkg/helpers/status.go

@@ -40,10 +40,11 @@ func (helper *StatusHelper) UpdateStatus(ctx context.Context, instance *v1alpha1
 }
 
 func (helper *StatusHelper) updateWorkflowStatus(ctx context.Context, instance *v1alpha1.ApplicationGroup) (ctrl.Result, error) {
-	forwardClient, _ := helper.WorkflowClientBuilder.Forward(instance).Build()
-	reverseClient, _ := helper.WorkflowClientBuilder.Reverse(instance).Build()
+	forwardClient := helper.WorkflowClientBuilder.Forward(instance).Build()
+	reverseClient := helper.WorkflowClientBuilder.Reverse(instance).Build()
+	rollbackClient := helper.WorkflowClientBuilder.Rollback(instance).Build()
 
-	for _, wfClient := range []workflow.Client{forwardClient, reverseClient} {
+	for _, wfClient := range []workflow.Client{forwardClient, reverseClient, rollbackClient} {
 		if err := workflow.UpdateStatus(ctx, wfClient); err != nil {
 			return ctrl.Result{}, err
 		}

pkg/meta/errors.go

@@ -10,4 +10,5 @@ var (
 	ErrHelmReleaseStatusFailure = errors.New("helmrelease in failure status")
 
 	ErrForwardWorkflowNotFound = errors.New("forward workflow not found")
+	ErrPreviousSpecNotSet      = errors.New("failed to generate rollback workflow, previous spec is unset")
 )

pkg/workflow/templates.go

@@ -3,6 +3,7 @@ package workflow
 import (
 	"encoding/base64"
 	"fmt"
+
 	"github.com/Azure/Orkestra/api/v1alpha1"
 	"github.com/Azure/Orkestra/pkg/utils"
 	v1alpha12 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
@@ -12,13 +13,11 @@ import (
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-func generateTemplates(wfClient Client) (*v1alpha12.Template, []v1alpha12.Template, error) {
-	options := wfClient.GetOptions()
-	if wfClient.GetAppGroup() == nil {
+func generateTemplates(instance *v1alpha1.ApplicationGroup, options ClientOptions) (*v1alpha12.Template, []v1alpha12.Template, error) {
+	if instance == nil {
 		return nil, nil, fmt.Errorf("applicationGroup cannot be nil")
 	}
-
-	templates, err := generateAppDAGTemplates(wfClient.GetAppGroup(), wfClient.GetNamespace(), options.parallelism)
+	templates, err := generateAppDAGTemplates(instance, options.namespace, options.parallelism)
 	if err != nil {
 		return nil, nil, fmt.Errorf("failed to generate application DAG templates : %w", err)
 	}
@@ -27,15 +26,15 @@ func generateTemplates(wfClient Client) (*v1alpha12.Template, []v1alpha12.Templa
 	entryTemplate := &v1alpha12.Template{
 		Name: EntrypointTemplateName,
 		DAG: &v1alpha12.DAGTemplate{
-			Tasks: make([]v1alpha12.DAGTask, len(wfClient.GetAppGroup().Spec.Applications)),
+			Tasks: make([]v1alpha12.DAGTask, len(instance.Spec.Applications)),
 		},
 		Parallelism: options.parallelism,
 	}
 	for i, tpl := range templates {
 		entryTemplate.DAG.Tasks[i] = v1alpha12.DAGTask{
 			Name:         utils.ConvertToDNS1123(tpl.Name),
 			Template:     utils.ConvertToDNS1123(tpl.Name),
-			Dependencies: utils.ConvertSliceToDNS1123(wfClient.GetAppGroup().Spec.Applications[i].Dependencies),
+			Dependencies: utils.ConvertSliceToDNS1123(instance.Spec.Applications[i].Dependencies),
 		}
 	}
 	return entryTemplate, templates, nil