From 52eb5a5c3e77e13ffcb1d20317277b82e7d4607c Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Tue, 12 Jun 2018 16:15:09 +0100 Subject: [PATCH] Write whole file updates at a time Up to this point, the update (and release) packages assumed that each file they were dealing with corresponded to a single manifest, because we couldn't deal with updating files with multiple manifests in them. Since we can now update a resource in a multidoc file or List, we can do this correctly and more simply by just overwriting each file after running its contents through the update process, serially. --- cluster/kubernetes/testfiles/data.go | 75 +++++++++++---- release/context.go | 17 ++-- release/releaser_test.go | 136 +++++++++++++++++++++++---- update/automated.go | 6 -- update/release.go | 6 -- update/service.go | 11 +-- 6 files changed, 189 insertions(+), 62 deletions(-) diff --git a/cluster/kubernetes/testfiles/data.go b/cluster/kubernetes/testfiles/data.go index 322be2317..2ec9eb288 100644 --- a/cluster/kubernetes/testfiles/data.go +++ b/cluster/kubernetes/testfiles/data.go @@ -45,11 +45,13 @@ func WriteTestFiles(dir string) error { // ResourceMap is the map of resource names to relative paths, which // must correspond with `Files` below. var ResourceMap = map[flux.ResourceID]string{ - flux.MustParseResourceID("default:deployment/helloworld"): "helloworld-deploy.yaml", - flux.MustParseResourceID("default:deployment/locked-service"): "locked-service-deploy.yaml", - flux.MustParseResourceID("default:deployment/test-service"): "test/test-service-deploy.yaml", - flux.MustParseResourceID("default:deployment/www-example-io"): "multi.yaml", - flux.MustParseResourceID("default:service/www-example-service"): "multi.yaml", + flux.MustParseResourceID("default:deployment/helloworld"): "helloworld-deploy.yaml", + flux.MustParseResourceID("default:deployment/locked-service"): "locked-service-deploy.yaml", + flux.MustParseResourceID("default:deployment/test-service"): "test/test-service-deploy.yaml", + flux.MustParseResourceID("default:deployment/multi-deploy"): "multi.yaml", + flux.MustParseResourceID("default:service/multi-service"): "multi.yaml", + flux.MustParseResourceID("default:deployment/list-deploy"): "list.yaml", + flux.MustParseResourceID("default:service/list-service"): "list.yaml", } // ServiceMap ... given a base path, construct the map representing @@ -60,14 +62,16 @@ func ServiceMap(dir string) map[flux.ResourceID][]string { flux.MustParseResourceID("default:deployment/helloworld"): []string{filepath.Join(dir, "helloworld-deploy.yaml")}, flux.MustParseResourceID("default:deployment/locked-service"): []string{filepath.Join(dir, "locked-service-deploy.yaml")}, flux.MustParseResourceID("default:deployment/test-service"): []string{filepath.Join(dir, "test/test-service-deploy.yaml")}, - flux.MustParseResourceID("default:deployment/www-example-io"): []string{filepath.Join(dir, "multi.yaml")}, + flux.MustParseResourceID("default:deployment/multi-deploy"): []string{filepath.Join(dir, "multi.yaml")}, + flux.MustParseResourceID("default:deployment/list-deploy"): []string{filepath.Join(dir, "list.yaml")}, } } var Files = map[string]string{ "garbage": "This should just be ignored, since it is not YAML", // Some genuine manifests - "helloworld-deploy.yaml": `apiVersion: extensions/v1beta1 + "helloworld-deploy.yaml": `--- +apiVersion: extensions/v1beta1 kind: Deployment metadata: name: helloworld @@ -136,42 +140,75 @@ spec: - containerPort: 80 `, // A multidoc, since we support those now - "multi.yaml": `apiVersion: apps/v1beta1 + "multi.yaml": `--- +apiVersion: apps/v1beta1 kind: Deployment metadata: annotations: flux.weave.works/automated: "true" - name: www-example-io + name: multi-deploy spec: replicas: 1 template: metadata: labels: - app : www-example-io + app : multi-app spec: containers: - - name: www-example-io + - name: hello image: quay.io/weaveworks/helloworld:master-a000001 imagePullPolicy: Always ports: - containerPort: 80 - imagePullSecrets: - - name: imagesecret --- apiVersion: v1 kind: Service metadata: - labels: - app: www-example-io - name: www-example-service + name: multi-service spec: type: NodePort ports: - - name: www-example-io - port: 80 + - port: 80 protocol: TCP selector: - app: www-example-io + app: multi-app +`, + + // A List resource + "list.yaml": `--- +apiVersion: v1 +kind: List +items: +- apiVersion: apps/v1beta1 + kind: Deployment + metadata: + name: list-deploy + spec: + replicas: 1 + template: + metadata: + labels: + app : list-app + spec: + containers: + - name: hello + image: quay.io/weaveworks/helloworld:master-a000001 + imagePullPolicy: Always + ports: + - containerPort: 80 +- apiVersion: v1 + kind: Service + metadata: + labels: + app: list-app + name: list-service + spec: + type: NodePort + ports: + - port: 80 + protocol: TCP + selector: + app: list-app `, // A tricksy chart directory, which should be skipped entirely. Adapted from diff --git a/release/context.go b/release/context.go index fa5953f51..e3282e779 100644 --- a/release/context.go +++ b/release/context.go @@ -42,11 +42,17 @@ func (rc *ReleaseContext) Manifests() cluster.Manifests { func (rc *ReleaseContext) WriteUpdates(updates []*update.ControllerUpdate) error { err := func() error { for _, update := range updates { - fi, err := os.Stat(update.ManifestPath) + manifestBytes, err := ioutil.ReadFile(update.ManifestPath) if err != nil { return err } - if err = ioutil.WriteFile(update.ManifestPath, update.ManifestBytes, fi.Mode()); err != nil { + for _, container := range update.Updates { + manifestBytes, err = rc.manifests.UpdateImage(manifestBytes, update.ResourceID, container.Container, container.Target) + if err != nil { + return err + } + } + if err = ioutil.WriteFile(update.ManifestPath, manifestBytes, os.FileMode(0600)); err != nil { return err } } @@ -129,10 +135,9 @@ func (rc *ReleaseContext) WorkloadsForUpdate() (map[flux.ResourceID]*update.Cont for _, res := range resources { if wl, ok := res.(resource.Workload); ok { defined[res.ResourceID()] = &update.ControllerUpdate{ - ResourceID: res.ResourceID(), - Resource: wl, - ManifestPath: filepath.Join(rc.repo.Dir(), res.Source()), - ManifestBytes: res.Bytes(), + ResourceID: res.ResourceID(), + Resource: wl, + ManifestPath: filepath.Join(rc.repo.Dir(), res.Source()), } } } diff --git a/release/releaser_test.go b/release/releaser_test.go index 4664ac596..5fc2f2718 100644 --- a/release/releaser_test.go +++ b/release/releaser_test.go @@ -196,7 +196,8 @@ func Test_FilterLogic(t *testing.T) { }, flux.MustParseResourceID("default:deployment/locked-service"): ignoredNotIncluded, flux.MustParseResourceID("default:deployment/test-service"): ignoredNotIncluded, - flux.MustParseResourceID("default:deployment/www-example-io"): ignoredNotIncluded, + flux.MustParseResourceID("default:deployment/multi-deploy"): ignoredNotIncluded, + flux.MustParseResourceID("default:deployment/list-deploy"): ignoredNotIncluded, }, }, { Name: "exclude specific service", @@ -226,8 +227,9 @@ func Test_FilterLogic(t *testing.T) { Status: update.ReleaseStatusIgnored, Error: update.Excluded, }, - flux.MustParseResourceID("default:deployment/test-service"): skippedNotInCluster, - flux.MustParseResourceID("default:deployment/www-example-io"): skippedNotInCluster, + flux.MustParseResourceID("default:deployment/test-service"): skippedNotInCluster, + flux.MustParseResourceID("default:deployment/multi-deploy"): skippedNotInCluster, + flux.MustParseResourceID("default:deployment/list-deploy"): skippedNotInCluster, }, }, { Name: "update specific image", @@ -252,14 +254,9 @@ func Test_FilterLogic(t *testing.T) { Status: update.ReleaseStatusIgnored, Error: update.DifferentImage, }, - flux.MustParseResourceID("default:deployment/test-service"): update.ControllerResult{ - Status: update.ReleaseStatusSkipped, - Error: update.NotInCluster, - }, - flux.MustParseResourceID("default:deployment/www-example-io"): update.ControllerResult{ - Status: update.ReleaseStatusSkipped, - Error: update.NotInCluster, - }, + flux.MustParseResourceID("default:deployment/test-service"): skippedNotInCluster, + flux.MustParseResourceID("default:deployment/multi-deploy"): skippedNotInCluster, + flux.MustParseResourceID("default:deployment/list-deploy"): skippedNotInCluster, }, }, // skipped if: not ignored AND (locked or not found in cluster) @@ -292,8 +289,9 @@ func Test_FilterLogic(t *testing.T) { Status: update.ReleaseStatusSkipped, Error: update.Locked, }, - flux.MustParseResourceID("default:deployment/test-service"): skippedNotInCluster, - flux.MustParseResourceID("default:deployment/www-example-io"): skippedNotInCluster, + flux.MustParseResourceID("default:deployment/test-service"): skippedNotInCluster, + flux.MustParseResourceID("default:deployment/multi-deploy"): skippedNotInCluster, + flux.MustParseResourceID("default:deployment/list-deploy"): skippedNotInCluster, }, }, { @@ -324,8 +322,9 @@ func Test_FilterLogic(t *testing.T) { Status: update.ReleaseStatusSkipped, Error: update.Locked, }, - flux.MustParseResourceID("default:deployment/test-service"): skippedNotInCluster, - flux.MustParseResourceID("default:deployment/www-example-io"): skippedNotInCluster, + flux.MustParseResourceID("default:deployment/test-service"): skippedNotInCluster, + flux.MustParseResourceID("default:deployment/multi-deploy"): skippedNotInCluster, + flux.MustParseResourceID("default:deployment/list-deploy"): skippedNotInCluster, }, }, { @@ -340,7 +339,8 @@ func Test_FilterLogic(t *testing.T) { flux.MustParseResourceID("default:deployment/helloworld"): ignoredNotIncluded, flux.MustParseResourceID("default:deployment/locked-service"): ignoredNotIncluded, flux.MustParseResourceID("default:deployment/test-service"): ignoredNotIncluded, - flux.MustParseResourceID("default:deployment/www-example-io"): ignoredNotIncluded, + flux.MustParseResourceID("default:deployment/multi-deploy"): ignoredNotIncluded, + flux.MustParseResourceID("default:deployment/list-deploy"): ignoredNotIncluded, flux.MustParseResourceID(notInRepoService): skippedNotInRepo, }, }, @@ -390,7 +390,8 @@ func Test_ImageStatus(t *testing.T) { Expected: update.Result{ flux.MustParseResourceID("default:deployment/helloworld"): ignoredNotIncluded, flux.MustParseResourceID("default:deployment/locked-service"): ignoredNotIncluded, - flux.MustParseResourceID("default:deployment/www-example-io"): ignoredNotIncluded, + flux.MustParseResourceID("default:deployment/multi-deploy"): ignoredNotIncluded, + flux.MustParseResourceID("default:deployment/list-deploy"): ignoredNotIncluded, flux.MustParseResourceID("default:deployment/test-service"): update.ControllerResult{ Status: update.ReleaseStatusIgnored, Error: update.DoesNotUseImage, @@ -411,7 +412,8 @@ func Test_ImageStatus(t *testing.T) { }, flux.MustParseResourceID("default:deployment/locked-service"): ignoredNotIncluded, flux.MustParseResourceID("default:deployment/test-service"): ignoredNotIncluded, - flux.MustParseResourceID("default:deployment/www-example-io"): ignoredNotIncluded, + flux.MustParseResourceID("default:deployment/multi-deploy"): ignoredNotIncluded, + flux.MustParseResourceID("default:deployment/list-deploy"): ignoredNotIncluded, }, }, } { @@ -429,6 +431,102 @@ func Test_ImageStatus(t *testing.T) { } } +func Test_UpdateMultidoc(t *testing.T) { + egID := flux.MustParseResourceID("default:deployment/multi-deploy") + egSvc := cluster.Controller{ + ID: egID, + Containers: cluster.ContainersOrExcuse{ + Containers: []resource.Container{ + { + Name: "hello", + Image: oldRef, + }, + }, + }, + } + + cluster := mockCluster(hwSvc, lockedSvc, egSvc) // no testsvc in cluster, but it _is_ in repo + checkout, cleanup := setup(t) + defer cleanup() + ctx := &ReleaseContext{ + cluster: cluster, + manifests: mockManifests, + repo: checkout, + registry: mockRegistry, + } + spec := update.ReleaseSpec{ + ServiceSpecs: []update.ResourceSpec{"default:deployment/multi-deploy"}, + ImageSpec: update.ImageSpecLatest, + Kind: update.ReleaseKindExecute, + } + results, err := Release(ctx, spec, log.NewNopLogger()) + if err != nil { + t.Error(err) + } + controllerResult, ok := results[egID] + if !ok { + t.Fatal("controller not found after update") + } + if !reflect.DeepEqual(update.ControllerResult{ + Status: update.ReleaseStatusSuccess, + PerContainer: []update.ContainerUpdate{{ + Container: "hello", + Current: oldRef, + Target: newHwRef, + }}, + }, controllerResult) { + t.Errorf("did not get expected controller result (see test code), got %#v", controllerResult) + } +} + +func Test_UpdateList(t *testing.T) { + egID := flux.MustParseResourceID("default:deployment/list-deploy") + egSvc := cluster.Controller{ + ID: egID, + Containers: cluster.ContainersOrExcuse{ + Containers: []resource.Container{ + { + Name: "hello", + Image: oldRef, + }, + }, + }, + } + + cluster := mockCluster(hwSvc, lockedSvc, egSvc) // no testsvc in cluster, but it _is_ in repo + checkout, cleanup := setup(t) + defer cleanup() + ctx := &ReleaseContext{ + cluster: cluster, + manifests: mockManifests, + repo: checkout, + registry: mockRegistry, + } + spec := update.ReleaseSpec{ + ServiceSpecs: []update.ResourceSpec{"default:deployment/list-deploy"}, + ImageSpec: update.ImageSpecLatest, + Kind: update.ReleaseKindExecute, + } + results, err := Release(ctx, spec, log.NewNopLogger()) + if err != nil { + t.Error(err) + } + controllerResult, ok := results[egID] + if !ok { + t.Fatal("controller not found after update") + } + if !reflect.DeepEqual(update.ControllerResult{ + Status: update.ReleaseStatusSuccess, + PerContainer: []update.ContainerUpdate{{ + Container: "hello", + Current: oldRef, + Target: newHwRef, + }}, + }, controllerResult) { + t.Errorf("did not get expected controller result (see test code), got %#v", controllerResult) + } +} + func testRelease(t *testing.T, ctx *ReleaseContext, spec update.ReleaseSpec, expected update.Result) { results, err := Release(ctx, spec, log.NewNopLogger()) if err != nil { @@ -452,7 +550,7 @@ func (m *badManifests) UpdateImage(def []byte, resourceID flux.ResourceID, conta return def, nil } -func TestBadRelease(t *testing.T) { +func Test_BadRelease(t *testing.T) { cluster := mockCluster(hwSvc) spec := update.ReleaseSpec{ ServiceSpecs: []update.ResourceSpec{update.ResourceSpecAll}, diff --git a/update/automated.go b/update/automated.go index c2d8f74ea..294ef049b 100644 --- a/update/automated.go +++ b/update/automated.go @@ -107,12 +107,6 @@ func (a *Automated) calculateImageUpdates(rc ReleaseContext, candidates []*Contr // the format of the image name as it is in the // resource (e.g., to avoid canonicalising it) newImageID := currentImageID.WithNewTag(change.ImageID.Tag) - var err error - u.ManifestBytes, err = rc.Manifests().UpdateImage(u.ManifestBytes, u.ResourceID, container.Name, newImageID) - if err != nil { - return nil, err - } - containerUpdates = append(containerUpdates, ContainerUpdate{ Container: container.Name, Current: currentImageID, diff --git a/update/release.go b/update/release.go index df52bc574..ff5a23c50 100644 --- a/update/release.go +++ b/update/release.go @@ -251,12 +251,6 @@ func (s ReleaseSpec) calculateImageUpdates(rc ReleaseContext, candidates []*Cont // appears in the manifest, whereas what we have is the // canonical form. newImageID := currentImageID.WithNewTag(latestImage.ID.Tag) - - u.ManifestBytes, err = rc.Manifests().UpdateImage(u.ManifestBytes, u.ResourceID, container.Name, newImageID) - if err != nil { - return nil, err - } - containerUpdates = append(containerUpdates, ContainerUpdate{ Container: container.Name, Current: currentImageID, diff --git a/update/service.go b/update/service.go index 2807da511..073d8b46e 100644 --- a/update/service.go +++ b/update/service.go @@ -7,12 +7,11 @@ import ( ) type ControllerUpdate struct { - ResourceID flux.ResourceID - Controller cluster.Controller - Resource resource.Workload - ManifestPath string - ManifestBytes []byte - Updates []ContainerUpdate + ResourceID flux.ResourceID + Controller cluster.Controller + Resource resource.Workload + ManifestPath string + Updates []ContainerUpdate } type ControllerFilter interface {