From 09dd89a4681148e7bc70f2fbbf0e78dbc64f7f6c Mon Sep 17 00:00:00 2001 From: Jesse Suen Date: Fri, 19 Jul 2019 13:27:44 -0700 Subject: [PATCH] Do not allow app-of-app child app's Missing status to affect parent (#1954) --- Gopkg.lock | 1 + util/health/health.go | 25 +++++++++++++-- util/health/health_test.go | 63 ++++++++++++++++++++++++++++++++++---- util/kube/kube.go | 3 +- 4 files changed, 82 insertions(+), 10 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 52ae1ac218..82576d24ee 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -1606,6 +1606,7 @@ "github.com/gogo/protobuf/proto", "github.com/gogo/protobuf/protoc-gen-gofast", "github.com/gogo/protobuf/protoc-gen-gogofast", + "github.com/gogo/protobuf/sortkeys", "github.com/golang/protobuf/proto", "github.com/golang/protobuf/protoc-gen-go", "github.com/golang/protobuf/ptypes/empty", diff --git a/util/health/health.go b/util/health/health.go index c505a54b81..3db58e44c2 100644 --- a/util/health/health.go +++ b/util/health/health.go @@ -42,8 +42,7 @@ func SetApplicationHealth(resStatuses []appv1.ResourceStatus, liveObjs []*unstru } if resHealth != nil { resStatuses[i].Health = resHealth - // Don't allow resource hooks to affect health status - ignore := liveObj != nil && (hookutil.IsHook(liveObj) || resource.Ignore(liveObj)) + ignore := ignoreLiveObjectHealth(liveObj, *resHealth) if !ignore && IsWorse(appHealth.Status, resHealth.Status) { appHealth.Status = resHealth.Status } @@ -52,6 +51,28 @@ func SetApplicationHealth(resStatuses []appv1.ResourceStatus, liveObjs []*unstru return &appHealth, savedErr } +// ignoreLiveObjectHealth determines if we should not allow the live object to affect the overall +// health of the application (e.g. hooks, missing child applications) +func ignoreLiveObjectHealth(liveObj *unstructured.Unstructured, resHealth appv1.HealthStatus) bool { + if liveObj != nil { + if hookutil.IsHook(liveObj) { + // Don't allow resource hooks to affect health status + return true + } + if resource.Ignore(liveObj) { + return true + } + gvk := liveObj.GroupVersionKind() + if gvk.Group == "argoproj.io" && gvk.Kind == "Application" && resHealth.Status == appv1.HealthStatusMissing { + // Covers the app-of-apps corner case where child app is deployed but that app itself + // has a status of 'Missing', which we don't want to cause the parent's health status + // to also be Missing + return true + } + } + return false +} + // GetResourceHealth returns the health of a k8s resource func GetResourceHealth(obj *unstructured.Unstructured, resourceOverrides map[string]appv1.ResourceOverride) (*appv1.HealthStatus, error) { diff --git a/util/health/health_test.go b/util/health/health_test.go index 4b64a3b72d..aca7c731b1 100644 --- a/util/health/health_test.go +++ b/util/health/health_test.go @@ -6,10 +6,12 @@ import ( "github.com/ghodss/yaml" "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "github.com/argoproj/argo-cd/common" appv1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1" + "github.com/argoproj/argo-cd/util/kube" ) func assertAppHealth(t *testing.T, yamlPath string, expectedStatus appv1.HealthStatusCode) { @@ -87,6 +89,55 @@ func TestApplication(t *testing.T) { assertAppHealth(t, "./testdata/application-degraded.yaml", appv1.HealthStatusDegraded) } +func TestAppOfAppsHealth(t *testing.T) { + newAppLiveObj := func(name string, status appv1.HealthStatusCode) (*unstructured.Unstructured, appv1.ResourceStatus) { + app := appv1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + }, + TypeMeta: metav1.TypeMeta{ + APIVersion: "argoproj.io/v1alpha1", + Kind: "Application", + }, + Status: appv1.ApplicationStatus{ + Health: appv1.HealthStatus{ + Status: status, + }, + }, + } + resStatus := appv1.ResourceStatus{ + Group: "argoproj.io", + Version: "v1alpha1", + Kind: "Application", + Name: name, + } + return kube.MustToUnstructured(&app), resStatus + } + + missingApp, missingStatus := newAppLiveObj("foo", appv1.HealthStatusMissing) + healthyApp, healthyStatus := newAppLiveObj("bar", appv1.HealthStatusHealthy) + degradedApp, degradedStatus := newAppLiveObj("baz", appv1.HealthStatusDegraded) + + // verify missing child app does not affect app health + { + missingAndHealthyStatuses := []appv1.ResourceStatus{missingStatus, healthyStatus} + missingAndHealthyLiveObjects := []*unstructured.Unstructured{missingApp, healthyApp} + healthStatus, err := SetApplicationHealth(missingAndHealthyStatuses, missingAndHealthyLiveObjects, nil, noFilter) + assert.NoError(t, err) + assert.Equal(t, appv1.HealthStatusHealthy, healthStatus.Status) + } + + // verify degraded does affect + { + degradedAndHealthyStatuses := []appv1.ResourceStatus{degradedStatus, healthyStatus} + degradedAndHealthyLiveObjects := []*unstructured.Unstructured{degradedApp, healthyApp} + healthStatus, err := SetApplicationHealth(degradedAndHealthyStatuses, degradedAndHealthyLiveObjects, nil, noFilter) + assert.NoError(t, err) + assert.Equal(t, appv1.HealthStatusDegraded, healthStatus.Status) + } + +} + func TestSetApplicationHealth(t *testing.T) { yamlBytes, err := ioutil.ReadFile("./testdata/job-failed.yaml") assert.Nil(t, err) @@ -118,17 +169,13 @@ func TestSetApplicationHealth(t *testing.T) { &runningPod, &failedJob, } - healthStatus, err := SetApplicationHealth(resources, liveObjs, nil, func(obj *unstructured.Unstructured) bool { - return true - }) + healthStatus, err := SetApplicationHealth(resources, liveObjs, nil, noFilter) assert.NoError(t, err) assert.Equal(t, appv1.HealthStatusDegraded, healthStatus.Status) // now mark the job as a hook and retry. it should ignore the hook and consider the app healthy failedJob.SetAnnotations(map[string]string{common.AnnotationKeyHook: "PreSync"}) - healthStatus, err = SetApplicationHealth(resources, liveObjs, nil, func(obj *unstructured.Unstructured) bool { - return true - }) + healthStatus, err = SetApplicationHealth(resources, liveObjs, nil, noFilter) assert.NoError(t, err) assert.Equal(t, appv1.HealthStatusHealthy, healthStatus.Status) } @@ -139,3 +186,7 @@ func TestAPIService(t *testing.T) { assertAppHealth(t, "./testdata/apiservice-v1beta1-true.yaml", appv1.HealthStatusHealthy) assertAppHealth(t, "./testdata/apiservice-v1beta1-false.yaml", appv1.HealthStatusProgressing) } + +func noFilter(obj *unstructured.Unstructured) bool { + return true +} diff --git a/util/kube/kube.go b/util/kube/kube.go index 97407c63e1..009a10e32b 100644 --- a/util/kube/kube.go +++ b/util/kube/kube.go @@ -8,10 +8,9 @@ import ( "strings" "time" - v1 "k8s.io/api/core/v1" - "github.com/ghodss/yaml" log "github.com/sirupsen/logrus" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" apierr "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"