Take into account number of unavailable replicas to decided if deployment is healthy or not (#270)

* Take into account number of unavailable replicas to decided if deployment is healthy or not

* Run one controller for all e2e tests to reduce tests duration

* Apply reviewer notes: use logic from kubectl/rollout_status.go to check deployment health

controller/health.go

@@ -60,30 +60,54 @@ func (ctrl *kubeAppHealthManager) getDeploymentHealth(config *rest.Config, names
 	if err != nil {
 		return nil, err
 	}
-	deploy, err := clientSet.AppsV1().Deployments(namespace).Get(name, metav1.GetOptions{})
+	deployment, err := clientSet.AppsV1().Deployments(namespace).Get(name, metav1.GetOptions{})
 	if err != nil {
 		return nil, err
 	}
-	health := appv1.HealthStatus{
-		Status: appv1.HealthStatusUnknown,
-	}
-	for _, condition := range deploy.Status.Conditions {
-		// deployment is healthy is it successfully progressed
-		if condition.Type == v1.DeploymentProgressing && condition.Status == "True" {
-			health.Status = appv1.HealthStatusHealthy
-		} else if condition.Type == v1.DeploymentReplicaFailure && condition.Status == "True" {
-			health.Status = appv1.HealthStatusDegraded
-		} else if condition.Type == v1.DeploymentProgressing && condition.Status == "False" {
-			health.Status = appv1.HealthStatusDegraded
-		} else if condition.Type == v1.DeploymentAvailable && condition.Status == "False" {
-			health.Status = appv1.HealthStatusDegraded
+
+	if deployment.Generation <= deployment.Status.ObservedGeneration {
+		cond := getDeploymentCondition(deployment.Status, v1.DeploymentProgressing)
+		if cond != nil && cond.Reason == "ProgressDeadlineExceeded" {
+			return &appv1.HealthStatus{
+				Status:        appv1.HealthStatusDegraded,
+				StatusDetails: fmt.Sprintf("Deployment %q exceeded its progress deadline", name),
+			}, nil
+		} else if deployment.Spec.Replicas != nil && deployment.Status.UpdatedReplicas < *deployment.Spec.Replicas {
+			return &appv1.HealthStatus{
+				Status:        appv1.HealthStatusProgressing,
+				StatusDetails: fmt.Sprintf("Waiting for rollout to finish: %d out of %d new replicas have been updated...\n", deployment.Status.UpdatedReplicas, *deployment.Spec.Replicas),
+			}, nil
+		} else if deployment.Status.Replicas > deployment.Status.UpdatedReplicas {
+			return &appv1.HealthStatus{
+				Status:        appv1.HealthStatusProgressing,
+				StatusDetails: fmt.Sprintf("Waiting for rollout to finish: %d old replicas are pending termination...\n", deployment.Status.Replicas-deployment.Status.UpdatedReplicas),
+			}, nil
+		} else if deployment.Status.AvailableReplicas < deployment.Status.UpdatedReplicas {
+			return &appv1.HealthStatus{
+				Status:        appv1.HealthStatusProgressing,
+				StatusDetails: fmt.Sprintf("Waiting for rollout to finish: %d of %d updated replicas are available...\n", deployment.Status.AvailableReplicas, deployment.Status.UpdatedReplicas),
+			}, nil
 		}
-		if health.Status != appv1.HealthStatusUnknown {
-			health.StatusDetails = fmt.Sprintf("%s:%s", condition.Reason, condition.Message)
-			break
+	} else {
+		return &appv1.HealthStatus{
+			Status:        appv1.HealthStatusProgressing,
+			StatusDetails: "Waiting for rollout to finish: observed deployment generation less then desired generation",
+		}, nil
+	}
+
+	return &appv1.HealthStatus{
+		Status: appv1.HealthStatusHealthy,
+	}, nil
+}
+
+func getDeploymentCondition(status v1.DeploymentStatus, condType v1.DeploymentConditionType) *v1.DeploymentCondition {
+	for i := range status.Conditions {
+		c := status.Conditions[i]
+		if c.Type == condType {
+			return &c
 		}
 	}
-	return &health, nil
+	return nil
 }
 
 func (ctrl *kubeAppHealthManager) GetAppHealth(server string, namespace string, comparisonResult *appv1.ComparisonResult) (*appv1.HealthStatus, error) {

examples/guestbook/components/guestbook-ui.jsonnet

@@ -23,7 +23,7 @@ local appDeployment = deployment
     params.replicas,
     container
       .new(params.name, params.image)
-      .withPorts(containerPort.new(targetPort)),
+      .withPorts(containerPort.new(targetPort)) + if params.command != null then { command: [ params.command ] } else {},
     labels);
 
 k.core.v1.list.new([appService, appDeployment])

examples/guestbook/components/params.libsonnet

@@ -12,7 +12,8 @@
       name: "guestbook-ui",
       replicas: 1,
       servicePort: 80,
-      type: "LoadBalancer",
+      type: "ClusterIP",
+      command: null,
     },
   },
 }

server/application/application.go

@@ -561,7 +561,7 @@ func (s *Server) setAppOperation(ctx context.Context, appName string, operationC
 		}
 		a.Operation = op
 		a.Status.OperationState = nil
-		_, err = s.Update(ctx, &ApplicationUpdateRequest{Application: a})
+		_, err = s.appclientset.ArgoprojV1alpha1().Applications(s.ns).Update(a)
 		if err != nil && apierr.IsConflict(err) {
 			log.Warnf("Failed to set operation for app '%s' due to update conflict. Retrying again...", appName)
 		} else {

test/e2e/app_management_test.go

@@ -1,7 +1,6 @@
 package e2e
 
 import (
-	"context"
 	"strconv"
 	"testing"
 	"time"
@@ -18,9 +17,7 @@ import (
 )
 
 func TestAppManagement(t *testing.T) {
-
 	testApp := &v1alpha1.Application{
-		ObjectMeta: metav1.ObjectMeta{GenerateName: "e2e-test"},
 		Spec: v1alpha1.ApplicationSpec{
 			Source: v1alpha1.ApplicationSource{
 				RepoURL: "https://github.com/argoproj/argo-cd.git", Path: ".", Environment: "minikube",
@@ -72,20 +69,12 @@ func TestAppManagement(t *testing.T) {
 	})
 
 	t.Run("TestTrackAppStateAndSyncApp", func(t *testing.T) {
-		ctrl := fixture.CreateController()
-		ctx, cancel := context.WithCancel(context.Background())
-		go ctrl.Run(ctx, 1, 1)
-		defer cancel()
-
-		// create app and ensure it reaches OutOfSync state
 		app := fixture.CreateApp(t, testApp)
 		WaitUntil(t, func() (done bool, err error) {
 			app, err = fixture.AppClient.ArgoprojV1alpha1().Applications(fixture.Namespace).Get(app.ObjectMeta.Name, metav1.GetOptions{})
 			return err == nil && app.Status.ComparisonResult.Status != v1alpha1.ComparisonStatusUnknown, err
 		})
 
-		assert.Equal(t, v1alpha1.ComparisonStatusOutOfSync, app.Status.ComparisonResult.Status)
-
 		// sync app and make sure it reaches InSync state
 		_, err := fixture.RunCli("app", "sync", app.Name)
 		if err != nil {
@@ -103,30 +92,30 @@ func TestAppManagement(t *testing.T) {
 
 	t.Run("TestAppRollbackSuccessful", func(t *testing.T) {
 		appWithHistory := testApp.DeepCopy()
-		appWithHistory.Status.History = []v1alpha1.DeploymentInfo{{
-			ID:       1,
-			Revision: "abc",
-		}, {
-			ID:       2,
-			Revision: "cdb",
-		}}
 
-		ctrl := fixture.CreateController()
-		ctx, cancel := context.WithCancel(context.Background())
-		go ctrl.Run(ctx, 1, 1)
-		defer cancel()
-
-		// create app and ensure it reaches OutOfSync state
+		// create app and ensure it's comparion status is not ComparisonStatusUnknown
 		app := fixture.CreateApp(t, appWithHistory)
+		app.Status.History = []v1alpha1.DeploymentInfo{{
+			ID:                          1,
+			Revision:                    "abc",
+			ComponentParameterOverrides: app.Spec.Source.ComponentParameterOverrides,
+		}, {
+			ID:                          2,
+			Revision:                    "cdb",
+			ComponentParameterOverrides: app.Spec.Source.ComponentParameterOverrides,
+		}}
+		app, err := fixture.AppClient.ArgoprojV1alpha1().Applications(fixture.Namespace).Update(app)
+		if err != nil {
+			t.Fatalf("Unable to update app %v", err)
+		}
+
 		WaitUntil(t, func() (done bool, err error) {
 			app, err = fixture.AppClient.ArgoprojV1alpha1().Applications(fixture.Namespace).Get(app.ObjectMeta.Name, metav1.GetOptions{})
 			return err == nil && app.Status.ComparisonResult.Status != v1alpha1.ComparisonStatusUnknown, err
 		})
 
-		assert.Equal(t, v1alpha1.ComparisonStatusOutOfSync, app.Status.ComparisonResult.Status)
-
 		// sync app and make sure it reaches InSync state
-		_, err := fixture.RunCli("app", "rollback", app.Name, "1")
+		_, err = fixture.RunCli("app", "rollback", app.Name, "1")
 		if err != nil {
 			t.Fatalf("Unable to sync app %v", err)
 		}
@@ -146,11 +135,6 @@ func TestAppManagement(t *testing.T) {
 		invalidApp := testApp.DeepCopy()
 		invalidApp.Spec.Destination.Server = "https://not-registered-cluster/api"
 
-		ctrl := fixture.CreateController()
-		ctx, cancel := context.WithCancel(context.Background())
-		go ctrl.Run(ctx, 1, 1)
-		defer cancel()
-
 		app := fixture.CreateApp(t, invalidApp)
 
 		WaitUntil(t, func() (done bool, err error) {
@@ -165,4 +149,39 @@ func TestAppManagement(t *testing.T) {
 
 		assert.Equal(t, v1alpha1.ComparisonStatusError, app.Status.ComparisonResult.Status)
 	})
+
+	t.Run("TestArgoCDWaitEnsureAppIsNotCrashing", func(t *testing.T) {
+		updatedApp := testApp.DeepCopy()
+
+		// deploy app and make sure it is healthy
+		app := fixture.CreateApp(t, updatedApp)
+		_, err := fixture.RunCli("app", "sync", app.Name)
+		if err != nil {
+			t.Fatalf("Unable to sync app %v", err)
+		}
+
+		WaitUntil(t, func() (done bool, err error) {
+			app, err = fixture.AppClient.ArgoprojV1alpha1().Applications(fixture.Namespace).Get(app.ObjectMeta.Name, metav1.GetOptions{})
+			return err == nil && app.Status.ComparisonResult.Status == v1alpha1.ComparisonStatusSynced && app.Status.Health.Status == v1alpha1.HealthStatusHealthy, err
+		})
+
+		// deploy app which fails and make sure it became unhealthy
+		app.Spec.Source.ComponentParameterOverrides = append(
+			app.Spec.Source.ComponentParameterOverrides,
+			v1alpha1.ComponentParameter{Name: "command", Value: "wrong-command", Component: "guestbook-ui"})
+		_, err = fixture.AppClient.ArgoprojV1alpha1().Applications(fixture.Namespace).Update(app)
+		if err != nil {
+			t.Fatalf("Unable to set app parameter %v", err)
+		}
+
+		_, err = fixture.RunCli("app", "sync", app.Name)
+		if err != nil {
+			t.Fatalf("Unable to sync app %v", err)
+		}
+
+		WaitUntil(t, func() (done bool, err error) {
+			app, err = fixture.AppClient.ArgoprojV1alpha1().Applications(fixture.Namespace).Get(app.ObjectMeta.Name, metav1.GetOptions{})
+			return err == nil && app.Status.ComparisonResult.Status == v1alpha1.ComparisonStatusSynced && app.Status.Health.Status == v1alpha1.HealthStatusDegraded, err
+		})
+	})
 }

test/e2e/fixture.go

@@ -1,7 +1,6 @@
 package e2e
 
 import (
-	"bytes"
 	"context"
 	"encoding/json"
 	"fmt"
@@ -21,6 +20,8 @@ import (
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"
 
+	"strings"
+
 	"github.com/argoproj/argo-cd/cmd/argocd/commands"
 	"github.com/argoproj/argo-cd/common"
 	"github.com/argoproj/argo-cd/controller"
@@ -143,22 +144,11 @@ func (f *Fixture) setup() error {
 	})
 
 	ctx, cancel := context.WithCancel(context.Background())
-	go func() {
-		err = repoServerGRPC.Serve(repoServerListener)
-	}()
 	go func() {
 		apiServer.Run(ctx, apiServerPort)
 	}()
 
-	f.tearDownCallback = func() {
-		cancel()
-		repoServerGRPC.Stop()
-	}
-	if err != nil {
-		return err
-	}
-
-	return waitUntilE(func() (done bool, err error) {
+	err = waitUntilE(func() (done bool, err error) {
 		clientset, err := f.NewApiClientset()
 		if err != nil {
 			return false, nil
@@ -171,6 +161,22 @@ func (f *Fixture) setup() error {
 		_, err = appClient.List(context.Background(), &application.ApplicationQuery{})
 		return err == nil, nil
 	})
+
+	ctrl := f.createController()
+	ctrlCtx, cancelCtrl := context.WithCancel(context.Background())
+	go ctrl.Run(ctrlCtx, 1, 1)
+
+	go func() {
+		err = repoServerGRPC.Serve(repoServerListener)
+	}()
+
+	f.tearDownCallback = func() {
+		cancel()
+		cancelCtrl()
+		repoServerGRPC.Stop()
+	}
+
+	return err
 }
 
 func (f *Fixture) ensureClusterRegistered() error {
@@ -270,6 +276,8 @@ func NewFixture() (*Fixture, error) {
 
 // CreateApp creates application with appropriate controller instance id.
 func (f *Fixture) CreateApp(t *testing.T, application *v1alpha1.Application) *v1alpha1.Application {
+	application = application.DeepCopy()
+	application.Name = fmt.Sprintf("e2e-test-%v", time.Now().Unix())
 	labels := application.ObjectMeta.Labels
 	if labels == nil {
 		labels = make(map[string]string)
@@ -277,6 +285,10 @@ func (f *Fixture) CreateApp(t *testing.T, application *v1alpha1.Application) *v1
 	}
 	labels[common.LabelKeyApplicationControllerInstanceID] = f.InstanceID
 
+	application.Spec.Source.ComponentParameterOverrides = append(
+		application.Spec.Source.ComponentParameterOverrides,
+		v1alpha1.ComponentParameter{Name: "name", Value: application.Name, Component: "guestbook-ui"})
+
 	app, err := f.AppClient.ArgoprojV1alpha1().Applications(f.Namespace).Create(application)
 	if err != nil {
 		t.Fatal(fmt.Sprintf("Unable to create app %v", err))
@@ -284,8 +296,8 @@ func (f *Fixture) CreateApp(t *testing.T, application *v1alpha1.Application) *v1
 	return app
 }
 
-// CreateController creates new controller instance
-func (f *Fixture) CreateController() *controller.ApplicationController {
+// createController creates new controller instance
+func (f *Fixture) createController() *controller.ApplicationController {
 	appStateManager := controller.NewAppStateManager(
 		f.DB, f.AppClient, reposerver.NewRepositoryServerClientset(f.RepoServerAddress), f.Namespace)
 
@@ -311,12 +323,21 @@ func (f *Fixture) NewApiClientset() (argocdclient.Client, error) {
 }
 
 func (f *Fixture) RunCli(args ...string) (string, error) {
-	cmd := commands.NewCommand()
-	cmd.SetArgs(append(args, "--server", f.ApiServerAddress, "--plaintext"))
-	output := new(bytes.Buffer)
-	cmd.SetOutput(output)
-	err := cmd.Execute()
-	return output.String(), err
+	args = append([]string{"run", "../../cmd/argocd/main.go"}, args...)
+	cmd := exec.Command("go", append(args, "--server", f.ApiServerAddress, "--plaintext")...)
+	outBytes, err := cmd.Output()
+	if err != nil {
+		exErr, ok := err.(*exec.ExitError)
+		if !ok {
+			return "", err
+		}
+		errOutput := string(exErr.Stderr)
+		if outBytes != nil {
+			errOutput = string(outBytes) + "\n" + errOutput
+		}
+		return "", fmt.Errorf(strings.TrimSpace(errOutput))
+	}
+	return string(outBytes), nil
 }
 
 func waitUntilE(condition wait.ConditionFunc) error {

util/git/git.go

@@ -98,10 +98,12 @@ func TestRepo(repo, username, password string, sshPrivateKey string) error {
 	cmd.Env = env
 	_, err = cmd.Output()
 	if err != nil {
-		exErr := err.(*exec.ExitError)
-		errOutput := strings.Split(string(exErr.Stderr), "\n")[0]
-		errOutput = redactPassword(errOutput, password)
-		return fmt.Errorf("%s: %s", repo, errOutput)
+		if exErr, ok := err.(*exec.ExitError); ok {
+			errOutput := strings.Split(string(exErr.Stderr), "\n")[0]
+			errOutput = redactPassword(errOutput, password)
+			return fmt.Errorf("%s: %s", repo, errOutput)
+		}
+		return err
 	}
 	return nil
 }