From 37268d2ff74a9de7d6954ae707ce26bfb4cd145a Mon Sep 17 00:00:00 2001 From: Roland Schilter Date: Tue, 10 Jul 2018 19:20:31 -0700 Subject: [PATCH] Improvements - Renames `IgnoreFailedController` to `IgnoreMismatches` - IgnoreMismatches now ignores any non-matching spec (also if controller cannot be found) - Allows empty image spec `Current` value to disable precondition check --- release/releaser_test.go | 188 +++++++++++++++++++++------------------ update/containers.go | 112 ++++++++++++++++------- update/filter.go | 4 +- 3 files changed, 185 insertions(+), 119 deletions(-) diff --git a/release/releaser_test.go b/release/releaser_test.go index 371fb015a2..de826fa666 100644 --- a/release/releaser_test.go +++ b/release/releaser_test.go @@ -2,7 +2,6 @@ package release import ( "encoding/json" - "fmt" "reflect" "testing" "time" @@ -532,62 +531,6 @@ func Test_UpdateList(t *testing.T) { } } -func Test_UpdateContainers_someFail(t *testing.T) { - cluster := mockCluster(hwSvc, testSvc) - checkout, cleanup := setup(t) - defer cleanup() - - ctx := &ReleaseContext{ - cluster: cluster, - manifests: mockManifests, - repo: checkout, - registry: mockRegistry, - } - spec := update.ContainerSpecs{ - ContainerSpecs: map[flux.ResourceID][]update.ContainerUpdate{ - hwSvcID: { // success - { - Container: helloContainer, - Current: oldRef, - Target: newHwRef, - }, - }, - testSvcID: { // fail - { - Container: testContainer, - Current: oldRef, - Target: oldRef, - }, - }, - }, - Kind: update.ReleaseKindExecute, - } - - _, err := Release(ctx, spec, log.NewNopLogger()) - assert.Error(t, err) - - spec.IgnoreFailedControllers = true - results, err := Release(ctx, spec, log.NewNopLogger()) - assert.NoError(t, err) - - expected := update.Result{ - hwSvcID: update.ControllerResult{ - Status: update.ReleaseStatusSuccess, - PerContainer: []update.ContainerUpdate{{ - Container: helloContainer, - Current: oldRef, - Target: newHwRef, - }}, - }, - testSvcID: update.ControllerResult{ - Status: update.ReleaseStatusFailed, - Error: fmt.Sprintf(update.ContainerTagMismatch, testContainer), - }, - } - assert.Equal(t, expected[hwSvcID], results[hwSvcID]) - assert.Equal(t, expected[testSvcID], results[testSvcID]) -} - func Test_UpdateContainers(t *testing.T) { cluster := mockCluster(hwSvc, lockedSvc) checkout, cleanup := setup(t) @@ -599,10 +542,13 @@ func Test_UpdateContainers(t *testing.T) { registry: mockRegistry, } for _, tst := range []struct { - Name string - Spec []update.ContainerUpdate - Expected update.ControllerResult - Commit string + Name string + Spec []update.ContainerUpdate + + // true|false for `IgnoreMismatches` + Commit map[bool]string + Expected map[bool]update.ControllerResult + Err map[bool]error }{ { Name: "multiple containers", @@ -618,19 +564,36 @@ func Test_UpdateContainers(t *testing.T) { Target: newSidecarRef, }, }, - Expected: update.ControllerResult{ - Status: update.ReleaseStatusSuccess, - PerContainer: []update.ContainerUpdate{{ - Container: helloContainer, - Current: oldRef, - Target: newHwRef, - }, { - Container: sidecarContainer, - Current: sidecarRef, - Target: newSidecarRef, - }}, + Expected: map[bool]update.ControllerResult{ + true: { + Status: update.ReleaseStatusSuccess, + PerContainer: []update.ContainerUpdate{{ + Container: helloContainer, + Current: oldRef, + Target: newHwRef, + }, { + Container: sidecarContainer, + Current: sidecarRef, + Target: newSidecarRef, + }}, + }, + false: { + Status: update.ReleaseStatusSuccess, + PerContainer: []update.ContainerUpdate{{ + Container: helloContainer, + Current: oldRef, + Target: newHwRef, + }, { + Container: sidecarContainer, + Current: sidecarRef, + Target: newSidecarRef, + }}, + }, + }, + Commit: map[bool]string{ + true: "Release containers\n\ndefault:deployment/helloworld\n- quay.io/weaveworks/helloworld:master-a000002\n- weaveworks/sidecar:master-a000002\n", + false: "Release containers\n\ndefault:deployment/helloworld\n- quay.io/weaveworks/helloworld:master-a000002\n- weaveworks/sidecar:master-a000002\n", }, - Commit: "Release containers\n\ndefault:deployment/helloworld\n- quay.io/weaveworks/helloworld:master-a000002\n- weaveworks/sidecar:master-a000002\n", }, { Name: "container tag mismatch", @@ -646,37 +609,88 @@ func Test_UpdateContainers(t *testing.T) { Target: newSidecarRef, }, }, - Expected: update.ControllerResult{ - Status: update.ReleaseStatusFailed, - Error: fmt.Sprintf(update.ContainerTagMismatch, helloContainer), + Expected: map[bool]update.ControllerResult{ + true: { + Status: update.ReleaseStatusSuccess, + PerContainer: []update.ContainerUpdate{ + { + Container: sidecarContainer, + Current: sidecarRef, + Target: newSidecarRef, + }, + }, + }, + false: {}, // Errors. + }, + Commit: map[bool]string{ + true: "Release containers\n\ndefault:deployment/helloworld\n- weaveworks/sidecar:master-a000002\n", }, - Commit: "Release containers\n\ndefault:deployment/helloworld failed: container tag mismatch: greeter", }, { Name: "container not found", Spec: []update.ContainerUpdate{ { - Container: "foo", + Container: helloContainer, + Current: oldRef, + Target: newHwRef, + }, + { + Container: "foo", // not found Current: oldRef, Target: newHwRef, }, }, - Expected: update.ControllerResult{ - Status: update.ReleaseStatusFailed, - Error: fmt.Sprintf(update.ContainerNotFound, "foo"), + Expected: map[bool]update.ControllerResult{ + true: { + Status: update.ReleaseStatusSuccess, + PerContainer: []update.ContainerUpdate{ + { + Container: helloContainer, + Current: oldRef, + Target: newHwRef, + }, + }, + }, + false: {}, // Errors. + }, + Commit: map[bool]string{ + true: "Release containers\n\ndefault:deployment/helloworld\n- quay.io/weaveworks/helloworld:master-a000002\n", + }, + }, + { + Name: "no changes", + Spec: []update.ContainerUpdate{ + { + Container: helloContainer, + Current: newHwRef, // mismatch + Target: newHwRef, + }, + }, + Expected: map[bool]update.ControllerResult{ + true: {}, + false: {}, }, - Commit: "Release containers\n\ndefault:deployment/helloworld failed: container not found: foo", }, } { specs := update.ContainerSpecs{ ContainerSpecs: map[flux.ResourceID][]update.ContainerUpdate{hwSvcID: tst.Spec}, Kind: update.ReleaseKindExecute, - IgnoreFailedControllers: true, } - results, err := Release(ctx, specs, log.NewNopLogger()) - assert.NoError(t, err, tst.Name) - assert.Equal(t, tst.Expected, results[hwSvcID], tst.Name) - assert.Equal(t, tst.Commit, specs.CommitMessage(results), tst.Name) + + for _, ignoreMismatches := range []bool{true, false} { + name := tst.Name + if ignoreMismatches { + name += " (IgnoreMismatches)" + } + specs.IgnoreMismatches = ignoreMismatches + results, err := Release(ctx, specs, log.NewNopLogger()) + if tst.Expected[ignoreMismatches].Status != "" { + assert.Equal(t, tst.Expected[ignoreMismatches], results[hwSvcID], name) + assert.Equal(t, tst.Commit[ignoreMismatches], specs.CommitMessage(results), name) + } else { + assert.Error(t, err, name) + } + } } } diff --git a/update/containers.go b/update/containers.go index a3bea6fab3..24c1186cfb 100644 --- a/update/containers.go +++ b/update/containers.go @@ -4,28 +4,53 @@ import ( "bytes" "errors" "fmt" + "strings" "github.com/go-kit/kit/log" "github.com/weaveworks/flux" + "github.com/weaveworks/flux/image" "github.com/weaveworks/flux/resource" ) // ContainerSpecs defines the spec for a `containers` manifest update. type ContainerSpecs struct { - Kind ReleaseKind - ContainerSpecs map[flux.ResourceID][]ContainerUpdate - IgnoreFailedControllers bool + Kind ReleaseKind + ContainerSpecs map[flux.ResourceID][]ContainerUpdate + IgnoreMismatches bool } -var errCannotSatisfySpecs = errors.New("cannot satisfy specs") - // CalculateRelease computes required controller updates to satisfy this specification. -// It returns an error if any spec calculation fails unless `IgnoreFailedControllers` is true. +// It returns an error if any spec calculation fails unless `IgnoreMismatches` is true. func (s ContainerSpecs) CalculateRelease(rc ReleaseContext, logger log.Logger) ([]*ControllerUpdate, Result, error) { - results := Result{} + all, results, err := s.selectServices(rc) + if err != nil { + return nil, results, err + } + updates := s.controllerUpdates(results, all) + + failures := 0 + successes := 0 + for _, res := range results { + switch res.Status { + case ReleaseStatusFailed: + failures++ + case ReleaseStatusSuccess: + successes++ + } + } + if failures > 0 { + return updates, results, errors.New("cannot satisfy specs") + } + if successes == 0 { + return updates, results, errors.New("no changes found") + } - // Collect data from services in spec + return updates, results, nil +} + +func (s ContainerSpecs) selectServices(rc ReleaseContext) ([]*ControllerUpdate, Result, error) { + results := Result{} var rids []flux.ResourceID for rid := range s.ContainerSpecs { rids = append(rids, rid) @@ -34,7 +59,10 @@ func (s ContainerSpecs) CalculateRelease(rc ReleaseContext, logger log.Logger) ( if err != nil { return nil, results, err } + return all, results, nil +} +func (s ContainerSpecs) controllerUpdates(results Result, all []*ControllerUpdate) []*ControllerUpdate { // Look at all controllers of services var updates []*ControllerUpdate for _, u := range all { @@ -52,29 +80,61 @@ func (s ContainerSpecs) CalculateRelease(rc ReleaseContext, logger log.Logger) ( containers[spec.Name] = spec } - // Go through specs and collect updates + var mismatch, notfound []string var containerUpdates []ContainerUpdate for _, spec := range s.ContainerSpecs[u.ResourceID] { container, ok := containers[spec.Container] if !ok { - results[u.ResourceID] = ControllerResult{ - Status: ReleaseStatusFailed, - Error: fmt.Sprintf(ContainerNotFound, spec.Container), - } - break // go to next controller + notfound = append(notfound, spec.Container) + continue + } + + // An empty spec for the current image skips the precondition + empty := image.Ref{} + if spec.Current != empty && container.Image != spec.Current { + mismatch = append(mismatch, spec.Container) + continue } - if container.Image != spec.Current { - results[u.ResourceID] = ControllerResult{ - Status: ReleaseStatusFailed, - Error: fmt.Sprintf(ContainerTagMismatch, spec.Container), - } - break // go to next controller + if container.Image == spec.Target { + // Nothing to update + continue } + containerUpdates = append(containerUpdates, spec) } - if res := results[u.ResourceID]; res.Status == "" { + skippedError := ImageUpToDate + notFoundError := fmt.Sprintf(ContainerNotFound, strings.Join(notfound, ", ")) + mismatchError := fmt.Sprintf(ContainerTagMismatch, strings.Join(mismatch, ", ")) + + if s.IgnoreMismatches { + // Ignoring mismatches means we set the controller result status + // to Skipped instead of Failed. We still want to provide an error + // message that conveys why we skipped it. + if len(notfound) > 0 { + skippedError = notFoundError + } else if len(mismatch) > 0 { + skippedError = mismatchError + } + } + + if !s.IgnoreMismatches && len(notfound) > 0 { + results[u.ResourceID] = ControllerResult{ + Status: ReleaseStatusFailed, + Error: fmt.Sprintf(ContainerNotFound, strings.Join(notfound, ", ")), + } + } else if !s.IgnoreMismatches && len(mismatch) > 0 { + results[u.ResourceID] = ControllerResult{ + Status: ReleaseStatusFailed, + Error: fmt.Sprintf(ContainerTagMismatch, strings.Join(mismatch, ", ")), + } + } else if len(containerUpdates) == 0 { + results[u.ResourceID] = ControllerResult{ + Status: ReleaseStatusSkipped, + Error: skippedError, + } + } else { u.Updates = containerUpdates updates = append(updates, u) results[u.ResourceID] = ControllerResult{ @@ -84,15 +144,7 @@ func (s ContainerSpecs) CalculateRelease(rc ReleaseContext, logger log.Logger) ( } } - if !s.IgnoreFailedControllers { - for _, res := range results { - if res.Status == ReleaseStatusFailed { - return updates, results, errCannotSatisfySpecs - } - } - } - - return updates, results, nil + return updates } func (s ContainerSpecs) ReleaseKind() ReleaseKind { diff --git a/update/filter.go b/update/filter.go index 50f206b00c..35c7acf446 100644 --- a/update/filter.go +++ b/update/filter.go @@ -15,8 +15,8 @@ const ( ImageNotFound = "cannot find one or more images" ImageUpToDate = "image(s) up to date" DoesNotUseImage = "does not use image(s)" - ContainerNotFound = "container not found: %s" - ContainerTagMismatch = "container tag mismatch: %s" + ContainerNotFound = "container(s) not found: %s" + ContainerTagMismatch = "container(s) tag mismatch: %s" ) type SpecificImageFilter struct {