From 078b2339bdae0cba691575ebe7b90ea933397bdd Mon Sep 17 00:00:00 2001 From: Jesse Suen Date: Fri, 13 Jul 2018 10:03:56 -0700 Subject: [PATCH] Apply logic was ignoring `kubectl apply` failures (#395) --- controller/sync.go | 28 ++++++++++++-------------- pkg/apis/application/v1alpha1/types.go | 4 ++++ util/kube/kube.go | 13 ++++++++++-- util/kube/kube_test.go | 5 +++++ 4 files changed, 33 insertions(+), 17 deletions(-) diff --git a/controller/sync.go b/controller/sync.go index 41875a34d9..0a1b26338c 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -298,7 +298,7 @@ func (sc *syncContext) setOperationPhase(phase appv1.OperationPhase, message str } // applyObject performs a `kubectl apply` of a single resource -func (sc *syncContext) applyObject(targetObj *unstructured.Unstructured, dryRun bool, force bool) (appv1.ResourceDetails, bool) { +func (sc *syncContext) applyObject(targetObj *unstructured.Unstructured, dryRun bool, force bool) appv1.ResourceDetails { resDetails := appv1.ResourceDetails{ Name: targetObj.GetName(), Kind: targetObj.GetKind(), @@ -308,21 +308,20 @@ func (sc *syncContext) applyObject(targetObj *unstructured.Unstructured, dryRun if err != nil { resDetails.Message = err.Error() resDetails.Status = appv1.ResourceDetailsSyncFailed - return resDetails, false + return resDetails } resDetails.Message = message resDetails.Status = appv1.ResourceDetailsSynced - return resDetails, true + return resDetails } // pruneObject deletes the object if both prune is true and dryRun is false. Otherwise appropriate message -func (sc *syncContext) pruneObject(liveObj *unstructured.Unstructured, prune, dryRun bool) (appv1.ResourceDetails, bool) { +func (sc *syncContext) pruneObject(liveObj *unstructured.Unstructured, prune, dryRun bool) appv1.ResourceDetails { resDetails := appv1.ResourceDetails{ Name: liveObj.GetName(), Kind: liveObj.GetKind(), Namespace: liveObj.GetNamespace(), } - successful := true if prune { if dryRun { resDetails.Message = "pruned (dry run)" @@ -332,7 +331,6 @@ func (sc *syncContext) pruneObject(liveObj *unstructured.Unstructured, prune, dr if err != nil { resDetails.Message = err.Error() resDetails.Status = appv1.ResourceDetailsSyncFailed - successful = false } else { resDetails.Message = "pruned" resDetails.Status = appv1.ResourceDetailsSyncedAndPruned @@ -342,33 +340,33 @@ func (sc *syncContext) pruneObject(liveObj *unstructured.Unstructured, prune, dr resDetails.Message = "ignored (requires pruning)" resDetails.Status = appv1.ResourceDetailsPruningRequired } - return resDetails, successful + return resDetails } // performs a apply based sync of the given sync tasks (possibly pruning the objects). -// Optionally updates the resource details with the result +// If update is true, will updates the resource details with the result. +// Or if the prune/apply failed, will also update the result. func (sc *syncContext) doApplySync(syncTasks []syncTask, dryRun, force, update bool) bool { syncSuccessful := true // apply all resources in parallel var wg sync.WaitGroup for _, task := range syncTasks { - defer func(t syncTask) { - wg.Add(1) + wg.Add(1) + go func(t syncTask) { defer wg.Done() var resDetails appv1.ResourceDetails - var successful bool if t.targetObj == nil { - resDetails, successful = sc.pruneObject(t.liveObj, sc.syncOp.Prune, dryRun) + resDetails = sc.pruneObject(t.liveObj, sc.syncOp.Prune, dryRun) } else { if isHook(t.targetObj) { return } - resDetails, successful = sc.applyObject(t.targetObj, dryRun, force) + resDetails = sc.applyObject(t.targetObj, dryRun, force) } - if !successful { + if !resDetails.Status.Successful() { syncSuccessful = false } - if update { + if update || !resDetails.Status.Successful() { sc.setResourceDetails(&resDetails) } }(task) diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go index 0dc1609f5b..5bb45356e7 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/pkg/apis/application/v1alpha1/types.go @@ -148,6 +148,10 @@ const ( ResourceDetailsPruningRequired ResourceSyncStatus = "PruningRequired" ) +func (s ResourceSyncStatus) Successful() bool { + return s != ResourceDetailsSyncFailed +} + type ResourceDetails struct { Name string `json:"name" protobuf:"bytes,1,opt,name=name"` Kind string `json:"kind" protobuf:"bytes,2,opt,name=kind"` diff --git a/util/kube/kube.go b/util/kube/kube.go index 46823fceae..28381efe65 100644 --- a/util/kube/kube.go +++ b/util/kube/kube.go @@ -508,8 +508,7 @@ func ApplyResource(config *rest.Config, obj *unstructured.Unstructured, namespac out, err := cmd.Output() if err != nil { if exErr, ok := err.(*exec.ExitError); ok { - // this makes the output a little better to read - errMsg := strings.Replace(strings.TrimSpace(string(exErr.Stderr)), ": error when creating \"STDIN\"", "", -1) + errMsg := cleanKubectlOutput(string(exErr.Stderr)) return "", errors.New(errMsg) } return "", err @@ -517,6 +516,16 @@ func ApplyResource(config *rest.Config, obj *unstructured.Unstructured, namespac return strings.TrimSpace(string(out)), nil } +// cleanKubectlOutput makes the error output of kubectl a little better to read +func cleanKubectlOutput(s string) string { + s = strings.TrimSpace(s) + s = strings.Replace(s, ": error validating \"STDIN\"", "", -1) + s = strings.Replace(s, ": error when creating \"STDIN\"", "", -1) + s = strings.Replace(s, "; if you choose to ignore these errors, turn validation off with --validate=false", "", -1) + s = strings.Replace(s, "error: error", "error", -1) + return s +} + // WriteKubeConfig takes a rest.Config and writes it as a kubeconfig at the specified path func WriteKubeConfig(restConfig *rest.Config, namespace, filename string) error { var kubeConfig = clientcmdapi.Config{ diff --git a/util/kube/kube_test.go b/util/kube/kube_test.go index d7810b79f4..2dfa7c7e66 100644 --- a/util/kube/kube_test.go +++ b/util/kube/kube_test.go @@ -232,3 +232,8 @@ func TestSetLabels(t *testing.T) { } } + +func TestCleanKubectlOutput(t *testing.T) { + testString := `error: error validating "STDIN": error validating data: ValidationError(Deployment.spec): missing required field "selector" in io.k8s.api.apps.v1beta2.DeploymentSpec; if you choose to ignore these errors, turn validation off with --validate=false` + assert.Equal(t, cleanKubectlOutput(testString), `error validating data: ValidationError(Deployment.spec): missing required field "selector" in io.k8s.api.apps.v1beta2.DeploymentSpec`) +}