From dff25169bde1e497d4cb644a36ba0224898c2285 Mon Sep 17 00:00:00 2001 From: Aaron Kirkbride Date: Tue, 8 May 2018 15:09:53 +0100 Subject: [PATCH 1/7] Refactor imagesMap functions to make clear we are handling maps of image repos --- daemon/daemon.go | 10 ++-- daemon/images.go | 4 +- registry/cache/memcached/integration_test.go | 4 +- registry/cache/registry.go | 4 +- registry/cache/warming_test.go | 2 +- registry/mock/mock.go | 2 +- registry/monitoring.go | 4 +- registry/registry.go | 2 +- update/images.go | 53 ++++++++++---------- update/images_test.go | 8 +-- update/release.go | 10 ++-- 11 files changed, 52 insertions(+), 51 deletions(-) diff --git a/daemon/daemon.go b/daemon/daemon.go index c9dfca056..92fbcc208 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -143,17 +143,17 @@ func (d *Daemon) ListImages(ctx context.Context, spec update.ResourceSpec) ([]v6 services, err = d.Cluster.SomeControllers([]flux.ResourceID{id}) } - images, err := update.CollectAvailableImages(d.Registry, clusterContainers(services), d.Logger) + imageRepos, err := update.FetchImageRepos(d.Registry, clusterContainers(services), d.Logger) if err != nil { return nil, errors.Wrap(err, "getting images for services") } var res []v6.ImageStatus for _, service := range services { - containers := containersWithAvailable(service, images) + serviceContainers := getServiceContainers(service, imageRepos) res = append(res, v6.ImageStatus{ ID: service.ID, - Containers: containers, + Containers: serviceContainers, }) } @@ -544,9 +544,9 @@ func containers2containers(cs []resource.Container) []v6.Container { return res } -func containersWithAvailable(service cluster.Controller, images update.ImageMap) (res []v6.Container) { +func getServiceContainers(service cluster.Controller, imageRepos update.ImageRepos) (res []v6.Container) { for _, c := range service.ContainersOrNil() { - available := images.Available(c.Image.Name) + available := imageRepos.Available(c.Image.Name) availableErr := "" if available == nil { availableErr = registry.ErrNoImageData.Error() diff --git a/daemon/images.go b/daemon/images.go index cddba1f69..17a8e7cf3 100644 --- a/daemon/images.go +++ b/daemon/images.go @@ -34,7 +34,7 @@ func (d *Daemon) pollForNewImages(logger log.Logger) { return } // Check the latest available image(s) for each service - imageMap, err := update.CollectAvailableImages(d.Registry, clusterContainers(services), logger) + imageRepos, err := update.FetchImageRepos(d.Registry, clusterContainers(services), logger) if err != nil { logger.Log("error", errors.Wrap(err, "fetching image updates")) return @@ -55,7 +55,7 @@ func (d *Daemon) pollForNewImages(logger log.Logger) { repo := currentImageID.Name logger.Log("repo", repo, "pattern", pattern) - if latest, ok := imageMap.LatestImage(repo, pattern); ok && latest.ID != currentImageID { + if latest, ok := imageRepos.LatestImage(repo, pattern); ok && latest.ID != currentImageID { if latest.ID.Tag == "" { logger.Log("msg", "untagged image in available images", "action", "skip", "available", repo) continue diff --git a/registry/cache/memcached/integration_test.go b/registry/cache/memcached/integration_test.go index 92b9b4ffc..b3e212d9a 100644 --- a/registry/cache/memcached/integration_test.go +++ b/registry/cache/memcached/integration_test.go @@ -66,14 +66,14 @@ Loop: case <-timeout.C: t.Fatal("Cache timeout") case <-tick.C: - _, err := r.GetRepository(id.Name) + _, err := r.GetSortedRepositoryImages(id.Name) if err == nil { break Loop } } } - img, err := r.GetRepository(id.Name) + img, err := r.GetSortedRepositoryImages(id.Name) if err != nil { t.Fatal(err) } diff --git a/registry/cache/registry.go b/registry/cache/registry.go index 9d27e1a7a..ea55ec575 100644 --- a/registry/cache/registry.go +++ b/registry/cache/registry.go @@ -31,9 +31,9 @@ type Cache struct { Reader Reader } -// GetRepository returns the list of image manifests in an image +// GetSortedRepositoryImages returns the list of image manifests in an image // repository (e.g,. at "quay.io/weaveworks/flux") -func (c *Cache) GetRepository(id image.Name) ([]image.Info, error) { +func (c *Cache) GetSortedRepositoryImages(id image.Name) ([]image.Info, error) { repoKey := NewRepositoryKey(id.CanonicalName()) bytes, _, err := c.Reader.GetKey(repoKey) if err != nil { diff --git a/registry/cache/warming_test.go b/registry/cache/warming_test.go index 29159a55d..7ad49c68a 100644 --- a/registry/cache/warming_test.go +++ b/registry/cache/warming_test.go @@ -71,7 +71,7 @@ func TestWarm(t *testing.T) { warmer.warm(context.TODO(), logger, repo, registry.NoCredentials()) registry := &Cache{Reader: c} - repoInfo, err := registry.GetRepository(ref.Name) + repoInfo, err := registry.GetSortedRepositoryImages(ref.Name) if err != nil { t.Error(err) } diff --git a/registry/mock/mock.go b/registry/mock/mock.go index b1c202d63..1f243fec8 100644 --- a/registry/mock/mock.go +++ b/registry/mock/mock.go @@ -40,7 +40,7 @@ type Registry struct { Err error } -func (m *Registry) GetRepository(id image.Name) ([]image.Info, error) { +func (m *Registry) GetSortedRepositoryImages(id image.Name) ([]image.Info, error) { var imgs []image.Info for _, i := range m.Images { // include only if it's the same repository in the same place diff --git a/registry/monitoring.go b/registry/monitoring.go index 2d6be8662..fcdef5444 100644 --- a/registry/monitoring.go +++ b/registry/monitoring.go @@ -46,9 +46,9 @@ func NewInstrumentedRegistry(next Registry) Registry { } } -func (m *instrumentedRegistry) GetRepository(id image.Name) (res []image.Info, err error) { +func (m *instrumentedRegistry) GetSortedRepositoryImages(id image.Name) (res []image.Info, err error) { start := time.Now() - res, err = m.next.GetRepository(id) + res, err = m.next.GetSortedRepositoryImages(id) registryDuration.With( fluxmetrics.LabelSuccess, strconv.FormatBool(err == nil), ).Observe(time.Since(start).Seconds()) diff --git a/registry/registry.go b/registry/registry.go index f0768671f..5794ee503 100644 --- a/registry/registry.go +++ b/registry/registry.go @@ -12,7 +12,7 @@ var ( // Registry is a store of image metadata. type Registry interface { - GetRepository(image.Name) ([]image.Info, error) + GetSortedRepositoryImages(image.Name) ([]image.Info, error) GetImage(image.Ref) (image.Info, error) } diff --git a/update/images.go b/update/images.go index b4d9eae59..2d80947f1 100644 --- a/update/images.go +++ b/update/images.go @@ -14,10 +14,11 @@ import ( "github.com/weaveworks/flux/resource" ) -type infoMap map[image.CanonicalName][]image.Info +type imageReposMap map[image.CanonicalName][]image.Info -type ImageMap struct { - images infoMap +// ImageRepos contains a map of image repositories to their images +type ImageRepos struct { + imageRepos imageReposMap } // LatestImage returns the latest releasable image for a repository @@ -26,8 +27,8 @@ type ImageMap struct { // in descending order of latestness.) If no such image exists, // returns a zero value and `false`, and the caller can decide whether // that's an error or not. -func (m ImageMap) LatestImage(repo image.Name, tagGlob string) (image.Info, bool) { - for _, available := range m.images[repo.CanonicalName()] { +func (r ImageRepos) LatestImage(repo image.Name, tagGlob string) (image.Info, bool) { + for _, available := range r.imageRepos[repo.CanonicalName()] { tag := available.ID.Tag // Ignore latest if and only if it's not what the user wants. if !strings.EqualFold(tagGlob, "latest") && strings.EqualFold(tag, "latest") { @@ -45,8 +46,8 @@ func (m ImageMap) LatestImage(repo image.Name, tagGlob string) (image.Info, bool // Available returns image.Info entries for all the images in the // named image repository. -func (m ImageMap) Available(repo image.Name) []image.Info { - if canon, ok := m.images[repo.CanonicalName()]; ok { +func (r ImageRepos) Available(repo image.Name) []image.Info { + if canon, ok := r.imageRepos[repo.CanonicalName()]; ok { infos := make([]image.Info, len(canon)) for i := range canon { infos[i] = canon[i] @@ -73,50 +74,50 @@ func (cs controllerContainers) Containers(i int) []resource.Container { return cs[i].Controller.ContainersOrNil() } -// collectUpdateImages is a convenient shim to -// `CollectAvailableImages`. -func collectUpdateImages(registry registry.Registry, updateable []*ControllerUpdate, logger log.Logger) (ImageMap, error) { - return CollectAvailableImages(registry, controllerContainers(updateable), logger) +// fetchUpdatableImageRepos is a convenient shim to +// `FetchImageRepos`. +func fetchUpdatableImageRepos(registry registry.Registry, updateable []*ControllerUpdate, logger log.Logger) (ImageRepos, error) { + return FetchImageRepos(registry, controllerContainers(updateable), logger) } -// CollectAvailableImages finds all the known image metadata for +// FetchImageRepos finds all the known image metadata for // containers in the controllers given. -func CollectAvailableImages(reg registry.Registry, cs containers, logger log.Logger) (ImageMap, error) { - images := infoMap{} +func FetchImageRepos(reg registry.Registry, cs containers, logger log.Logger) (ImageRepos, error) { + imageRepos := imageReposMap{} for i := 0; i < cs.Len(); i++ { for _, container := range cs.Containers(i) { - images[container.Image.CanonicalName()] = nil + imageRepos[container.Image.CanonicalName()] = nil } } - for name := range images { - imageRepo, err := reg.GetRepository(name.Name) + for repo := range imageRepos { + sortedRepoImages, err := reg.GetSortedRepositoryImages(repo.Name) if err != nil { // Not an error if missing. Use empty images. if !fluxerr.IsMissing(err) { - logger.Log("err", errors.Wrapf(err, "fetching image metadata for %s", name)) + logger.Log("err", errors.Wrapf(err, "fetching image metadata for %s", repo)) continue } } - images[name] = imageRepo + imageRepos[repo] = sortedRepoImages } - return ImageMap{images}, nil + return ImageRepos{imageRepos}, nil } -// Create a map of images. It will check that each image exists. -func exactImages(reg registry.Registry, images []image.Ref) (ImageMap, error) { - m := infoMap{} +// Create a map of image repos to images. It will check that each image exists. +func exactImageRepos(reg registry.Registry, images []image.Ref) (ImageRepos, error) { + m := imageReposMap{} for _, id := range images { // We must check that the exact images requested actually exist. Otherwise we risk pushing invalid images to git. exist, err := imageExists(reg, id) if err != nil { - return ImageMap{}, errors.Wrap(image.ErrInvalidImageID, err.Error()) + return ImageRepos{}, errors.Wrap(image.ErrInvalidImageID, err.Error()) } if !exist { - return ImageMap{}, errors.Wrap(image.ErrInvalidImageID, fmt.Sprintf("image %q does not exist", id)) + return ImageRepos{}, errors.Wrap(image.ErrInvalidImageID, fmt.Sprintf("image %q does not exist", id)) } m[id.CanonicalName()] = []image.Info{{ID: id}} } - return ImageMap{m}, nil + return ImageRepos{m}, nil } // Checks whether the given image exists in the repository. diff --git a/update/images_test.go b/update/images_test.go index ed2aafaf9..82f2fe712 100644 --- a/update/images_test.go +++ b/update/images_test.go @@ -20,18 +20,18 @@ var ( // names (e.g., `index.docker.io/library/alpine`), but we ask // questions in terms of everyday names (e.g., `alpine`). func TestDecanon(t *testing.T) { - m := ImageMap{infoMap{ + m := ImageRepos{imageReposMap{ name: infos, }} - latest, ok := m.LatestImage(mustParseName("weaveworks/helloworld"), "*") + latest, ok := m.LatestFilteredImage(mustParseName("weaveworks/helloworld"), "*") if !ok { t.Error("did not find latest image") } else if latest.ID.Name != mustParseName("weaveworks/helloworld") { t.Error("name did not match what was asked") } - latest, ok = m.LatestImage(mustParseName("index.docker.io/weaveworks/helloworld"), "*") + latest, ok = m.LatestFilteredImage(mustParseName("index.docker.io/weaveworks/helloworld"), "*") if !ok { t.Error("did not find latest image") } else if latest.ID.Name != mustParseName("index.docker.io/weaveworks/helloworld") { @@ -50,7 +50,7 @@ func TestDecanon(t *testing.T) { } func TestAvail(t *testing.T) { - m := ImageMap{infoMap{name: infos}} + m := ImageRepos{imageReposMap{name: infos}} avail := m.Available(mustParseName("weaveworks/goodbyeworld")) if len(avail) > 0 { t.Errorf("did not expect available images, but got %#v", avail) diff --git a/update/release.go b/update/release.go index 4dae03ef3..4509d702e 100644 --- a/update/release.go +++ b/update/release.go @@ -188,20 +188,20 @@ func (s ReleaseSpec) markSkipped(results Result) { // if not, it indicates there's likely some problem with the running // system vs the definitions given in the repo.) func (s ReleaseSpec) calculateImageUpdates(rc ReleaseContext, candidates []*ControllerUpdate, results Result, logger log.Logger) ([]*ControllerUpdate, error) { - // Compile an `ImageMap` of all relevant images - var images ImageMap + // Compile an `ImageRepos` of all relevant images + var imageRepos ImageRepos var singleRepo image.CanonicalName var err error switch s.ImageSpec { case ImageSpecLatest: - images, err = collectUpdateImages(rc.Registry(), candidates, logger) + imageRepos, err = fetchUpdatableImageRepos(rc.Registry(), candidates, logger) default: var ref image.Ref ref, err = s.ImageSpec.AsRef() if err == nil { singleRepo = ref.CanonicalName() - images, err = exactImages(rc.Registry(), []image.Ref{ref}) + imageRepos, err = exactImageRepos(rc.Registry(), []image.Ref{ref}) } } @@ -231,7 +231,7 @@ func (s ReleaseSpec) calculateImageUpdates(rc ReleaseContext, candidates []*Cont for _, container := range containers { currentImageID := container.Image - latestImage, ok := images.LatestImage(currentImageID.Name, "*") + latestImage, ok := imageRepos.LatestImage(currentImageID.Name, "*") if !ok { if currentImageID.CanonicalName() != singleRepo { ignoredOrSkipped = ReleaseStatusIgnored From 617937f1c925bfe6fc726a4ef94017d9927fab97 Mon Sep 17 00:00:00 2001 From: Aaron Kirkbride Date: Tue, 8 May 2018 17:22:21 +0100 Subject: [PATCH 2/7] Include available and filtered image summaries in ListImages api --- api/v6/api.go | 13 ++++++- daemon/daemon.go | 93 +++++++++++++++++++++++++++++++++++------------ daemon/images.go | 16 +++++--- update/images.go | 36 +++++++++++++++--- update/release.go | 2 +- 5 files changed, 122 insertions(+), 38 deletions(-) diff --git a/api/v6/api.go b/api/v6/api.go index 9befabd71..1d0b6808d 100644 --- a/api/v6/api.go +++ b/api/v6/api.go @@ -44,8 +44,17 @@ type ControllerStatus struct { type Container struct { Name string Current image.Info - Available []image.Info - AvailableError string `json:",omitempty"` + LatestFiltered image.Info + + // All available images (ignoring tag filters) + Available []image.Info + AvailableError string `json:",omitempty"` + AvailableImagesCount int + NewAvailableImagesCount int + + // Filtered available images (matching tag filters) + FilteredImagesCount int + NewFilteredImagesCount int } // --- config types diff --git a/daemon/daemon.go b/daemon/daemon.go index 92fbcc208..f0e1f940d 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -70,35 +70,43 @@ func (d *Daemon) Export(ctx context.Context) ([]byte, error) { return d.Cluster.Export() } -func (d *Daemon) ListServices(ctx context.Context, namespace string) ([]v6.ControllerStatus, error) { - clusterServices, err := d.Cluster.AllControllers(namespace) - if err != nil { - return nil, errors.Wrap(err, "getting services from cluster") - } - +func (d *Daemon) getPolicyResourceMap(ctx context.Context) (policy.ResourceMap, v6.ReadOnlyReason, error) { var services policy.ResourceMap var globalReadOnly v6.ReadOnlyReason - err = d.WithClone(ctx, func(checkout *git.Checkout) error { + err := d.WithClone(ctx, func(checkout *git.Checkout) error { var err error services, err = d.Manifests.ServicesWithPolicies(checkout.ManifestDir()) return err }) + + // Capture errors related to read-only repositories switch { case err == git.ErrNotReady: globalReadOnly = v6.ReadOnlyNotReady case err == git.ErrNoConfig: globalReadOnly = v6.ReadOnlyNoRepo case err != nil: - return nil, errors.Wrap(err, "getting service policies") + return nil, globalReadOnly, errors.Wrap(err, "getting service policies") + } + + return services, globalReadOnly, nil +} + +func (d *Daemon) ListServices(ctx context.Context, namespace string) ([]v6.ControllerStatus, error) { + clusterServices, err := d.Cluster.AllControllers(namespace) + if err != nil { + return nil, errors.Wrap(err, "getting services from cluster") + } + + policyResourceMap, readOnly, err := d.getPolicyResourceMap(ctx) + if err != nil { + return nil, err } var res []v6.ControllerStatus for _, service := range clusterServices { - var readOnly v6.ReadOnlyReason - policies, ok := services[service.ID] + policies, ok := policyResourceMap[service.ID] switch { - case globalReadOnly != "": - readOnly = globalReadOnly case !ok: readOnly = v6.ReadOnlyMissing case service.IsSystem: @@ -148,9 +156,14 @@ func (d *Daemon) ListImages(ctx context.Context, spec update.ResourceSpec) ([]v6 return nil, errors.Wrap(err, "getting images for services") } + policyResourceMap, _, err := d.getPolicyResourceMap(ctx) + if err != nil { + return nil, err + } + var res []v6.ImageStatus for _, service := range services { - serviceContainers := getServiceContainers(service, imageRepos) + serviceContainers := getServiceContainers(service, imageRepos, policyResourceMap) res = append(res, v6.ImageStatus{ ID: service.ID, Containers: serviceContainers, @@ -544,20 +557,52 @@ func containers2containers(cs []resource.Container) []v6.Container { return res } -func getServiceContainers(service cluster.Controller, imageRepos update.ImageRepos) (res []v6.Container) { +func getServiceContainers(service cluster.Controller, imageRepos update.ImageRepos, policyResourceMap policy.ResourceMap) (res []v6.Container) { for _, c := range service.ContainersOrNil() { - available := imageRepos.Available(c.Image.Name) - availableErr := "" - if available == nil { - availableErr = registry.ErrNoImageData.Error() + imageRepo := c.Image.Name + tagPattern := getTagPattern(policyResourceMap, service.ID, c.Name) + + currentImage := imageRepos.FindImageInfo(imageRepo, c.Image) + latestFilteredImage, _ := imageRepos.LatestFilteredImage(imageRepo, tagPattern) + + // All available images + availableImages := imageRepos.Available(imageRepo) + availableImagesCount := len(availableImages) + availableImagesErr := "" + if availableImages == nil { + availableImagesErr = registry.ErrNoImageData.Error() + } + var newAvailableImages []image.Info + for _, img := range availableImages { + if img.CreatedAt.After(currentImage.CreatedAt) { + newAvailableImages = append(newAvailableImages, img) + } } + newAvailableImagesCount := len(newAvailableImages) + + // Filtered available images + filteredImages := imageRepos.FilteredAvailable(imageRepo, tagPattern) + filteredImagesCount := len(filteredImages) + var newFilteredImages []image.Info + for _, img := range filteredImages { + if img.CreatedAt.After(currentImage.CreatedAt) { + newFilteredImages = append(newFilteredImages, img) + } + } + newFilteredImagesCount := len(newFilteredImages) + res = append(res, v6.Container{ - Name: c.Name, - Current: image.Info{ - ID: c.Image, - }, - Available: available, - AvailableError: availableErr, + Name: c.Name, + Current: currentImage, + LatestFiltered: latestFilteredImage, + + Available: availableImages, + AvailableError: availableImagesErr, + AvailableImagesCount: availableImagesCount, + NewAvailableImagesCount: newAvailableImagesCount, + + FilteredImagesCount: filteredImagesCount, + NewFilteredImagesCount: newFilteredImagesCount, }) } return res diff --git a/daemon/images.go b/daemon/images.go index 17a8e7cf3..f91461dfc 100644 --- a/daemon/images.go +++ b/daemon/images.go @@ -18,17 +18,17 @@ func (d *Daemon) pollForNewImages(logger log.Logger) { ctx := context.Background() - candidateServices, err := d.unlockedAutomatedServices(ctx) + candidateServicesPolicyMap, err := d.getUnlockedAutomatedServicesPolicyMap(ctx) if err != nil { logger.Log("error", errors.Wrap(err, "getting unlocked automated services")) return } - if len(candidateServices) == 0 { + if len(candidateServicesPolicyMap) == 0 { logger.Log("msg", "no automated services") return } // Find images to check - services, err := d.Cluster.SomeControllers(candidateServices.ToSlice()) + services, err := d.Cluster.SomeControllers(candidateServicesPolicyMap.ToSlice()) if err != nil { logger.Log("error", errors.Wrap(err, "checking services for new images")) return @@ -51,11 +51,11 @@ func (d *Daemon) pollForNewImages(logger log.Logger) { continue } - pattern := getTagPattern(candidateServices, service.ID, container.Name) + pattern := getTagPattern(candidateServicesPolicyMap, service.ID, container.Name) repo := currentImageID.Name logger.Log("repo", repo, "pattern", pattern) - if latest, ok := imageRepos.LatestImage(repo, pattern); ok && latest.ID != currentImageID { + if latest, ok := imageRepos.LatestFilteredImage(repo, pattern); ok && latest.ID != currentImageID { if latest.ID.Tag == "" { logger.Log("msg", "untagged image in available images", "action", "skip", "available", repo) continue @@ -73,6 +73,9 @@ func (d *Daemon) pollForNewImages(logger log.Logger) { } func getTagPattern(services policy.ResourceMap, service flux.ResourceID, container string) string { + if services == nil { + return "*" + } policies := services[service] if pattern, ok := policies.Get(policy.TagPrefix(container)); ok { return strings.TrimPrefix(pattern, "glob:") @@ -80,7 +83,8 @@ func getTagPattern(services policy.ResourceMap, service flux.ResourceID, contain return "*" } -func (d *Daemon) unlockedAutomatedServices(ctx context.Context) (policy.ResourceMap, error) { +// getUnlockedAutomatedServicesPolicyMap returns a resource policy map for all unlocked automated services +func (d *Daemon) getUnlockedAutomatedServicesPolicyMap(ctx context.Context) (policy.ResourceMap, error) { var services policy.ResourceMap err := d.WithClone(ctx, func(checkout *git.Checkout) error { var err error diff --git a/update/images.go b/update/images.go index 2d80947f1..e03242040 100644 --- a/update/images.go +++ b/update/images.go @@ -21,14 +21,40 @@ type ImageRepos struct { imageRepos imageReposMap } -// LatestImage returns the latest releasable image for a repository +// FindImageInfo retruns image.Info given an image ref. If the image cannot be +// found, return the image.Info with only rhe ID. +func (r ImageRepos) FindImageInfo(repo image.Name, ref image.Ref) image.Info { + images, ok := r.imageRepos[ref.CanonicalName()] + if !ok { + return image.Info{ID: ref} + } + for _, img := range images { + if img.ID == ref { + return img + } + } + return image.Info{ID: ref} +} + +// LatestFilteredImage returns the latest releasable image for a repository // for which the tag matches a given pattern. A releasable image is // one that is not tagged "latest". (Assumes the available images are // in descending order of latestness.) If no such image exists, // returns a zero value and `false`, and the caller can decide whether // that's an error or not. -func (r ImageRepos) LatestImage(repo image.Name, tagGlob string) (image.Info, bool) { - for _, available := range r.imageRepos[repo.CanonicalName()] { +func (r ImageRepos) LatestFilteredImage(repo image.Name, tagGlob string) (image.Info, bool) { + filtered := r.FilteredAvailable(repo, tagGlob) + if len(filtered) > 0 { + return filtered[0], true + } + return image.Info{}, false +} + +// FilteredAvailable returns image.Info engtries for all the images in the +// names image repository which match the tagGlob. +func (r ImageRepos) FilteredAvailable(repo image.Name, tagGlob string) []image.Info { + var filtered []image.Info + for _, available := range r.Available(repo) { tag := available.ID.Tag // Ignore latest if and only if it's not what the user wants. if !strings.EqualFold(tagGlob, "latest") && strings.EqualFold(tag, "latest") { @@ -38,10 +64,10 @@ func (r ImageRepos) LatestImage(repo image.Name, tagGlob string) (image.Info, bo var im image.Info im = available im.ID = repo.ToRef(tag) - return im, true + filtered = append(filtered, im) } } - return image.Info{}, false + return filtered } // Available returns image.Info entries for all the images in the diff --git a/update/release.go b/update/release.go index 4509d702e..a905e43c9 100644 --- a/update/release.go +++ b/update/release.go @@ -231,7 +231,7 @@ func (s ReleaseSpec) calculateImageUpdates(rc ReleaseContext, candidates []*Cont for _, container := range containers { currentImageID := container.Image - latestImage, ok := imageRepos.LatestImage(currentImageID.Name, "*") + latestImage, ok := imageRepos.LatestFilteredImage(currentImageID.Name, "*") if !ok { if currentImageID.CanonicalName() != singleRepo { ignoredOrSkipped = ReleaseStatusIgnored From 6f921b6e232ce9bbf91cee1db398d9a1d32498d0 Mon Sep 17 00:00:00 2001 From: Aaron Kirkbride Date: Thu, 17 May 2018 13:48:07 +0100 Subject: [PATCH 3/7] Add containerFields query parameter to ListImages --- api/v6/api.go | 24 +++++----- cmd/fluxctl/list_images_cmd.go | 2 +- daemon/daemon.go | 81 ++++++++++++++++++++++++++-------- daemon/daemon_test.go | 4 +- http/client/client.go | 6 +-- http/daemon/server.go | 21 +++++++-- http/transport.go | 4 +- image/image.go | 8 ++-- remote/logging.go | 4 +- remote/metrics.go | 4 +- remote/mock.go | 6 +-- remote/rpc/baseclient.go | 2 +- remote/rpc/clientV6.go | 2 +- remote/rpc/clientV7.go | 2 +- remote/rpc/clientV8.go | 2 +- remote/rpc/server.go | 2 +- 16 files changed, 118 insertions(+), 56 deletions(-) diff --git a/api/v6/api.go b/api/v6/api.go index 1d0b6808d..c3c10950a 100644 --- a/api/v6/api.go +++ b/api/v6/api.go @@ -42,19 +42,19 @@ type ControllerStatus struct { } type Container struct { - Name string - Current image.Info - LatestFiltered image.Info + Name string `json:",omitempty"` + Current image.Info `json:",omitempty"` + LatestFiltered image.Info `json:",omitempty"` // All available images (ignoring tag filters) - Available []image.Info - AvailableError string `json:",omitempty"` - AvailableImagesCount int - NewAvailableImagesCount int + Available []image.Info `json:",omitempty"` + AvailableError string `json:",omitempty"` + AvailableImagesCount int `json:",omitempty"` + NewAvailableImagesCount int `json:",omitempty"` // Filtered available images (matching tag filters) - FilteredImagesCount int - NewFilteredImagesCount int + FilteredImagesCount int `json:",omitempty"` + NewFilteredImagesCount int `json:",omitempty"` } // --- config types @@ -75,13 +75,17 @@ type Deprecated interface { SyncNotify(context.Context) error } +type ListImagesOptions struct { + OverrideContainerFields []string +} + type NotDeprecated interface { // from v5 Export(context.Context) ([]byte, error) // v6 ListServices(ctx context.Context, namespace string) ([]ControllerStatus, error) - ListImages(context.Context, update.ResourceSpec) ([]ImageStatus, error) + ListImages(ctx context.Context, spec update.ResourceSpec, opts ListImagesOptions) ([]ImageStatus, error) UpdateManifests(context.Context, update.Spec) (job.ID, error) SyncStatus(ctx context.Context, ref string) ([]string, error) JobStatus(context.Context, job.ID) (job.Status, error) diff --git a/cmd/fluxctl/list_images_cmd.go b/cmd/fluxctl/list_images_cmd.go index e9bb7a115..138fff09e 100644 --- a/cmd/fluxctl/list_images_cmd.go +++ b/cmd/fluxctl/list_images_cmd.go @@ -67,7 +67,7 @@ func (opts *controllerShowOpts) RunE(cmd *cobra.Command, args []string) error { ctx := context.Background() - controllers, err := opts.API.ListImages(ctx, resourceSpec) + controllers, err := opts.API.ListImages(ctx, resourceSpec, v6.ListImagesOptions{}) if err != nil { return err } diff --git a/daemon/daemon.go b/daemon/daemon.go index f0e1f940d..98866295a 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -138,7 +138,7 @@ func (cs clusterContainers) Containers(i int) []resource.Container { } // List the images available for set of services -func (d *Daemon) ListImages(ctx context.Context, spec update.ResourceSpec) ([]v6.ImageStatus, error) { +func (d *Daemon) ListImages(ctx context.Context, spec update.ResourceSpec, opts v6.ListImagesOptions) ([]v6.ImageStatus, error) { var services []cluster.Controller var err error if spec == update.ResourceSpecAll { @@ -163,7 +163,10 @@ func (d *Daemon) ListImages(ctx context.Context, spec update.ResourceSpec) ([]v6 var res []v6.ImageStatus for _, service := range services { - serviceContainers := getServiceContainers(service, imageRepos, policyResourceMap) + serviceContainers, err := getServiceContainers(service, imageRepos, policyResourceMap, opts.OverrideContainerFields) + if err != nil { + return nil, err + } res = append(res, v6.ImageStatus{ ID: service.ID, Containers: serviceContainers, @@ -557,13 +560,37 @@ func containers2containers(cs []resource.Container) []v6.Container { return res } -func getServiceContainers(service cluster.Controller, imageRepos update.ImageRepos, policyResourceMap policy.ResourceMap) (res []v6.Container) { +func getServiceContainers(service cluster.Controller, imageRepos update.ImageRepos, policyResourceMap policy.ResourceMap, overrideFields []string) (res []v6.Container, err error) { + fields := map[string]struct{}{ + "Name": struct{}{}, + "Current": struct{}{}, + "LatestFiltered": struct{}{}, + "Available": struct{}{}, + "AvailableError": struct{}{}, + "AvailableImagesCount": struct{}{}, + "NewAvailableImagesCount": struct{}{}, + "FilteredImagesCount": struct{}{}, + "NewFilteredImagesCount": struct{}{}, + } + + // If overrideFields is provided, override the default fields to return + if len(overrideFields) > 0 { + newFieldsMap := make(map[string]struct{}) + for _, f := range overrideFields { + if _, ok := fields[f]; !ok { + return nil, errors.Errorf("%s is an invalid field", f) + } + newFieldsMap[f] = struct{}{} + } + fields = newFieldsMap + } + for _, c := range service.ContainersOrNil() { + var container v6.Container + imageRepo := c.Image.Name tagPattern := getTagPattern(policyResourceMap, service.ID, c.Name) - currentImage := imageRepos.FindImageInfo(imageRepo, c.Image) - latestFilteredImage, _ := imageRepos.LatestFilteredImage(imageRepo, tagPattern) // All available images availableImages := imageRepos.Available(imageRepo) @@ -591,21 +618,37 @@ func getServiceContainers(service cluster.Controller, imageRepos update.ImageRep } newFilteredImagesCount := len(newFilteredImages) - res = append(res, v6.Container{ - Name: c.Name, - Current: currentImage, - LatestFiltered: latestFilteredImage, - - Available: availableImages, - AvailableError: availableImagesErr, - AvailableImagesCount: availableImagesCount, - NewAvailableImagesCount: newAvailableImagesCount, - - FilteredImagesCount: filteredImagesCount, - NewFilteredImagesCount: newFilteredImagesCount, - }) + if _, ok := fields["Name"]; ok { + container.Name = c.Name + } + if _, ok := fields["Current"]; ok { + container.Current = currentImage + } + if _, ok := fields["LatestFiltered"]; ok { + container.LatestFiltered, _ = imageRepos.LatestFilteredImage(imageRepo, tagPattern) + } + if _, ok := fields["Available"]; ok { + container.Available = availableImages + } + if _, ok := fields["AvailableError"]; ok { + container.AvailableError = availableImagesErr + } + if _, ok := fields["AvailableImagesCount"]; ok { + container.AvailableImagesCount = availableImagesCount + } + if _, ok := fields["NewAvailableImagesCount"]; ok { + container.NewAvailableImagesCount = newAvailableImagesCount + } + if _, ok := fields["FilteredImagesCount"]; ok { + container.FilteredImagesCount = filteredImagesCount + } + if _, ok := fields["NewFilteredImagesCount"]; ok { + container.NewFilteredImagesCount = newFilteredImagesCount + } + res = append(res, container) } - return res + + return res, nil } func policyCommitMessage(us policy.Updates, cause update.Cause) string { diff --git a/daemon/daemon_test.go b/daemon/daemon_test.go index eeac66319..909cb7ba7 100644 --- a/daemon/daemon_test.go +++ b/daemon/daemon_test.go @@ -139,7 +139,7 @@ func TestDaemon_ListImages(t *testing.T) { // List all images for services ss := update.ResourceSpec(update.ResourceSpecAll) - is, err := d.ListImages(ctx, ss) + is, err := d.ListImages(ctx, ss, v6.ListImagesOptions{}) if err != nil { t.Fatalf("Error: %s", err.Error()) } @@ -150,7 +150,7 @@ func TestDaemon_ListImages(t *testing.T) { // List images for specific service ss = update.ResourceSpec(svc) - is, err = d.ListImages(ctx, ss) + is, err = d.ListImages(ctx, ss, v6.ListImagesOptions{}) if err != nil { t.Fatalf("Error: %s", err.Error()) } diff --git a/http/client/client.go b/http/client/client.go index ccafac535..a4acaea0b 100644 --- a/http/client/client.go +++ b/http/client/client.go @@ -57,9 +57,9 @@ func (c *Client) ListServices(ctx context.Context, namespace string) ([]v6.Contr return res, err } -func (c *Client) ListImages(ctx context.Context, s update.ResourceSpec) ([]v6.ImageStatus, error) { +func (c *Client) ListImages(ctx context.Context, s update.ResourceSpec, opts v6.ListImagesOptions) ([]v6.ImageStatus, error) { var res []v6.ImageStatus - err := c.Get(ctx, &res, transport.ListImages, "service", string(s)) + err := c.Get(ctx, &res, transport.ListImages, "service", string(s), "containerFields", strings.Join(opts.OverrideContainerFields, ",")) return res, err } @@ -210,7 +210,7 @@ func (c *Client) executeRequest(req *http.Request) (*http.Response, error) { if err := json.Unmarshal(body, &niceError); err != nil { return resp, errors.Wrap(err, "decoding response body of error") } - // just in case it's JSON but not one of our own errors + // just in case it's JSON but not one of our own errors if niceError.Err != nil { return resp, &niceError } diff --git a/http/daemon/server.go b/http/daemon/server.go index 5f22231e3..7380272ae 100644 --- a/http/daemon/server.go +++ b/http/daemon/server.go @@ -3,6 +3,7 @@ package daemon import ( "encoding/json" "net/http" + "strings" "github.com/gorilla/mux" "github.com/pkg/errors" @@ -11,6 +12,7 @@ import ( "github.com/weaveworks/flux" "github.com/weaveworks/flux/api" + "github.com/weaveworks/flux/api/v6" transport "github.com/weaveworks/flux/http" "github.com/weaveworks/flux/job" fluxmetrics "github.com/weaveworks/flux/metrics" @@ -91,14 +93,27 @@ func (s HTTPServer) SyncStatus(w http.ResponseWriter, r *http.Request) { } func (s HTTPServer) ListImages(w http.ResponseWriter, r *http.Request) { - service := mux.Vars(r)["service"] + queryValues := r.URL.Query() + + // service - Select services to update. + service := queryValues.Get("service") + if service == "" { + service = string(update.ResourceSpecAll) + } spec, err := update.ParseResourceSpec(service) if err != nil { transport.WriteError(w, r, http.StatusBadRequest, errors.Wrapf(err, "parsing service spec %q", service)) return } - d, err := s.server.ListImages(r.Context(), spec) + // containerFields - Override which fields to return in the container struct. + var opts v6.ListImagesOptions + containerFields := queryValues.Get("containerFields") + if containerFields != "" { + opts.OverrideContainerFields = strings.Split(containerFields, ",") + } + + d, err := s.server.ListImages(r.Context(), spec, opts) if err != nil { transport.ErrorResponse(w, r, err) return @@ -122,7 +137,7 @@ func (s HTTPServer) UpdateManifests(w http.ResponseWriter, r *http.Request) { } func (s HTTPServer) ListServices(w http.ResponseWriter, r *http.Request) { - namespace := mux.Vars(r)["namespace"] + namespace := r.URL.Query().Get("namespace") res, err := s.server.ListServices(r.Context(), namespace) if err != nil { transport.ErrorResponse(w, r, err) diff --git a/http/transport.go b/http/transport.go index 7b9d764c9..74d019519 100644 --- a/http/transport.go +++ b/http/transport.go @@ -29,8 +29,8 @@ func DeprecateVersions(r *mux.Router, versions ...string) { func NewAPIRouter() *mux.Router { r := mux.NewRouter() - r.NewRoute().Name(ListServices).Methods("GET").Path("/v6/services").Queries("namespace", "{namespace}") // optional namespace! - r.NewRoute().Name(ListImages).Methods("GET").Path("/v6/images").Queries("service", "{service}") + r.NewRoute().Name(ListServices).Methods("GET").Path("/v6/services") + r.NewRoute().Name(ListImages).Methods("GET").Path("/v6/images") r.NewRoute().Name(UpdateManifests).Methods("POST").Path("/v9/update-manifests") r.NewRoute().Name(JobStatus).Methods("GET").Path("/v6/jobs").Queries("id", "{id}") diff --git a/image/image.go b/image/image.go index ac283165a..fbb6e179d 100644 --- a/image/image.go +++ b/image/image.go @@ -225,16 +225,16 @@ func (i Ref) WithNewTag(t string) Ref { // from its registry. type Info struct { // the reference to this image; probably a tagged image name - ID Ref + ID Ref `json:",omitempty"` // the digest we got when fetching the metadata, which will be // different each time a manifest is uploaded for the reference - Digest string + Digest string `json:",omitempty"` // an identifier for the *image* this reference points to; this // will be the same for references that point at the same image // (but does not necessarily equal Docker's image ID) - ImageID string + ImageID string `json:",omitempty"` // the time at which the image pointed at was created - CreatedAt time.Time + CreatedAt time.Time `json:",omitempty"` } // MarshalJSON returns the Info value in JSON (as bytes). It is diff --git a/remote/logging.go b/remote/logging.go index b2deed8a6..86226bdaa 100644 --- a/remote/logging.go +++ b/remote/logging.go @@ -43,13 +43,13 @@ func (p *ErrorLoggingServer) ListServices(ctx context.Context, maybeNamespace st return p.server.ListServices(ctx, maybeNamespace) } -func (p *ErrorLoggingServer) ListImages(ctx context.Context, spec update.ResourceSpec) (_ []v6.ImageStatus, err error) { +func (p *ErrorLoggingServer) ListImages(ctx context.Context, spec update.ResourceSpec, opts v6.ListImagesOptions) (_ []v6.ImageStatus, err error) { defer func() { if err != nil { p.logger.Log("method", "ListImages", "error", err) } }() - return p.server.ListImages(ctx, spec) + return p.server.ListImages(ctx, spec, opts) } func (p *ErrorLoggingServer) JobStatus(ctx context.Context, jobID job.ID) (_ job.Status, err error) { diff --git a/remote/metrics.go b/remote/metrics.go index 2938f188e..50a98282a 100644 --- a/remote/metrics.go +++ b/remote/metrics.go @@ -56,14 +56,14 @@ func (i *instrumentedServer) ListServices(ctx context.Context, namespace string) return i.s.ListServices(ctx, namespace) } -func (i *instrumentedServer) ListImages(ctx context.Context, spec update.ResourceSpec) (_ []v6.ImageStatus, err error) { +func (i *instrumentedServer) ListImages(ctx context.Context, spec update.ResourceSpec, opts v6.ListImagesOptions) (_ []v6.ImageStatus, err error) { defer func(begin time.Time) { requestDuration.With( fluxmetrics.LabelMethod, "ListImages", fluxmetrics.LabelSuccess, fmt.Sprint(err == nil), ).Observe(time.Since(begin).Seconds()) }(time.Now()) - return i.s.ListImages(ctx, spec) + return i.s.ListImages(ctx, spec, opts) } func (i *instrumentedServer) UpdateManifests(ctx context.Context, spec update.Spec) (_ job.ID, err error) { diff --git a/remote/mock.go b/remote/mock.go index 0b5fac128..e23a41540 100644 --- a/remote/mock.go +++ b/remote/mock.go @@ -65,7 +65,7 @@ func (p *MockServer) ListServices(ctx context.Context, ns string) ([]v6.Controll return p.ListServicesAnswer, p.ListServicesError } -func (p *MockServer) ListImages(context.Context, update.ResourceSpec) ([]v6.ImageStatus, error) { +func (p *MockServer) ListImages(context.Context, update.ResourceSpec, v6.ListImagesOptions) ([]v6.ImageStatus, error) { return p.ListImagesAnswer, p.ListImagesError } @@ -194,7 +194,7 @@ func ServerTestBattery(t *testing.T, wrap func(mock api.UpstreamServer) api.Upst t.Error("expected error from ListServices, got nil") } - ims, err := client.ListImages(ctx, update.ResourceSpecAll) + ims, err := client.ListImages(ctx, update.ResourceSpecAll, v6.ListImagesOptions{}) if err != nil { t.Error(err) } @@ -202,7 +202,7 @@ func ServerTestBattery(t *testing.T, wrap func(mock api.UpstreamServer) api.Upst t.Error(fmt.Errorf("expected:\n%#v\ngot:\n%#v", mock.ListImagesAnswer, ims)) } mock.ListImagesError = fmt.Errorf("list images error") - if _, err = client.ListImages(ctx, update.ResourceSpecAll); err == nil { + if _, err = client.ListImages(ctx, update.ResourceSpecAll, v6.ListImagesOptions{}); err == nil { t.Error("expected error from ListImages, got nil") } diff --git a/remote/rpc/baseclient.go b/remote/rpc/baseclient.go index 9b7dfa4b4..5375d1526 100644 --- a/remote/rpc/baseclient.go +++ b/remote/rpc/baseclient.go @@ -33,7 +33,7 @@ func (bc baseClient) ListServices(context.Context, string) ([]v6.ControllerStatu return nil, remote.UpgradeNeededError(errors.New("ListServices method not implemented")) } -func (bc baseClient) ListImages(context.Context, update.ResourceSpec) ([]v6.ImageStatus, error) { +func (bc baseClient) ListImages(context.Context, update.ResourceSpec, v6.ListImagesOptions) ([]v6.ImageStatus, error) { return nil, remote.UpgradeNeededError(errors.New("ListImages method not implemented")) } diff --git a/remote/rpc/clientV6.go b/remote/rpc/clientV6.go index 1cbfb6b85..d7206af69 100644 --- a/remote/rpc/clientV6.go +++ b/remote/rpc/clientV6.go @@ -99,7 +99,7 @@ func (p *RPCClientV6) ListServices(ctx context.Context, namespace string) ([]v6. return services, err } -func (p *RPCClientV6) ListImages(ctx context.Context, spec update.ResourceSpec) ([]v6.ImageStatus, error) { +func (p *RPCClientV6) ListImages(ctx context.Context, spec update.ResourceSpec, opts v6.ListImagesOptions) ([]v6.ImageStatus, error) { var images []v6.ImageStatus if err := requireServiceSpecKinds(spec, supportedKindsV6); err != nil { return images, remote.UpgradeNeededError(err) diff --git a/remote/rpc/clientV7.go b/remote/rpc/clientV7.go index b87a6c65d..074b5f46d 100644 --- a/remote/rpc/clientV7.go +++ b/remote/rpc/clientV7.go @@ -62,7 +62,7 @@ func (p *RPCClientV7) ListServices(ctx context.Context, namespace string) ([]v6. return resp.Result, err } -func (p *RPCClientV7) ListImages(ctx context.Context, spec update.ResourceSpec) ([]v6.ImageStatus, error) { +func (p *RPCClientV7) ListImages(ctx context.Context, spec update.ResourceSpec, opts v6.ListImagesOptions) ([]v6.ImageStatus, error) { var resp ListImagesResponse if err := requireServiceSpecKinds(spec, supportedKindsV7); err != nil { return resp.Result, remote.UpgradeNeededError(err) diff --git a/remote/rpc/clientV8.go b/remote/rpc/clientV8.go index dddb3eba8..bfcdaf3eb 100644 --- a/remote/rpc/clientV8.go +++ b/remote/rpc/clientV8.go @@ -32,7 +32,7 @@ func NewClientV8(conn io.ReadWriteCloser) *RPCClientV8 { return &RPCClientV8{NewClientV7(conn)} } -func (p *RPCClientV8) ListImages(ctx context.Context, spec update.ResourceSpec) ([]v6.ImageStatus, error) { +func (p *RPCClientV8) ListImages(ctx context.Context, spec update.ResourceSpec, opts v6.ListImagesOptions) ([]v6.ImageStatus, error) { var resp ListImagesResponse if err := requireServiceSpecKinds(spec, supportedKindsV8); err != nil { return resp.Result, remote.UnsupportedResourceKind(err) diff --git a/remote/rpc/server.go b/remote/rpc/server.go index ebe2ba531..c12905d96 100644 --- a/remote/rpc/server.go +++ b/remote/rpc/server.go @@ -89,7 +89,7 @@ type ListImagesResponse struct { } func (p *RPCServer) ListImages(spec update.ResourceSpec, resp *ListImagesResponse) error { - v, err := p.s.ListImages(context.Background(), spec) + v, err := p.s.ListImages(context.Background(), spec, v6.ListImagesOptions{}) resp.Result = v if err != nil { if err, ok := errors.Cause(err).(*fluxerr.Error); ok { From 40c293f752731406a9b340d0a67c2538b55cb152 Mon Sep 17 00:00:00 2001 From: Aaron Kirkbride Date: Thu, 17 May 2018 16:42:27 +0100 Subject: [PATCH 4/7] Add tests for ListImages and getTagPattern --- daemon/daemon_test.go | 180 +++++++++++++++++++++++++++++++++++------- daemon/images_test.go | 58 ++++++++++++++ 2 files changed, 210 insertions(+), 28 deletions(-) create mode 100644 daemon/images_test.go diff --git a/daemon/daemon_test.go b/daemon/daemon_test.go index 909cb7ba7..da6025867 100644 --- a/daemon/daemon_test.go +++ b/daemon/daemon_test.go @@ -12,6 +12,7 @@ import ( "time" "github.com/go-kit/kit/log" + "github.com/stretchr/testify/assert" "github.com/weaveworks/flux" "github.com/weaveworks/flux/api/v6" @@ -40,6 +41,10 @@ const ( newHelloImage = "quay.io/weaveworks/helloworld:2" currentHelloImage = "quay.io/weaveworks/helloworld:master-a000001" + anotherSvc = "another:deployment/service" + anotherContainer = "it-doesn't-matter" + anotherImage = "another/service:latest" + invalidNS = "adsajkfldsa" testVersion = "test" ) @@ -137,26 +142,155 @@ func TestDaemon_ListImages(t *testing.T) { ctx := context.Background() - // List all images for services - ss := update.ResourceSpec(update.ResourceSpecAll) - is, err := d.ListImages(ctx, ss, v6.ListImagesOptions{}) - if err != nil { - t.Fatalf("Error: %s", err.Error()) - } - ids := imageIDs(is) - if 3 != len(ids) { - t.Fatalf("Expected %v but got %v", 3, len(ids)) + specAll := update.ResourceSpec(update.ResourceSpecAll) + + // Service 1 + svcID, err := flux.ParseResourceID(svc) + assert.NoError(t, err) + currentImageRef, err := image.ParseRef(currentHelloImage) + assert.NoError(t, err) + newImageRef, err := image.ParseRef(newHelloImage) + assert.NoError(t, err) + + // Service 2 + anotherSvcID, err := flux.ParseResourceID(anotherSvc) + assert.NoError(t, err) + anotherImageRef, err := image.ParseRef(anotherImage) + assert.NoError(t, err) + + tests := []struct { + name string + spec update.ResourceSpec + opts v6.ListImagesOptions + + expectedImages []v6.ImageStatus + expectedNumImages int + shouldError bool + }{ + { + name: "All services", + spec: specAll, + opts: v6.ListImagesOptions{}, + expectedImages: []v6.ImageStatus{ + { + ID: svcID, + Containers: []v6.Container{ + { + Name: container, + Current: image.Info{ID: currentImageRef}, + LatestFiltered: image.Info{ID: currentImageRef}, + Available: []image.Info{ + {ID: currentImageRef}, + {ID: newImageRef}, + }, + AvailableImagesCount: 2, + NewAvailableImagesCount: 1, + FilteredImagesCount: 2, + NewFilteredImagesCount: 1, + }, + }, + }, + { + ID: anotherSvcID, + Containers: []v6.Container{ + { + Name: anotherContainer, + Current: image.Info{ID: anotherImageRef}, + LatestFiltered: image.Info{}, + Available: []image.Info{ + {ID: anotherImageRef}, + }, + AvailableImagesCount: 1, + NewAvailableImagesCount: 0, + FilteredImagesCount: 0, // Excludes latest + NewFilteredImagesCount: 0, + }, + }, + }, + }, + shouldError: false, + }, + { + name: "Specific service", + spec: update.ResourceSpec(svc), + opts: v6.ListImagesOptions{}, + expectedImages: []v6.ImageStatus{ + { + ID: svcID, + Containers: []v6.Container{ + { + Name: container, + Current: image.Info{ID: currentImageRef}, + LatestFiltered: image.Info{ID: currentImageRef}, + Available: []image.Info{ + {ID: currentImageRef}, + {ID: newImageRef}, + }, + AvailableImagesCount: 2, + NewAvailableImagesCount: 1, + FilteredImagesCount: 2, + NewFilteredImagesCount: 1, + }, + }, + }, + }, + shouldError: false, + }, + { + name: "Override container field selection", + spec: specAll, + opts: v6.ListImagesOptions{OverrideContainerFields: []string{"Name", "Current", "NewAvailableImagesCount"}}, + expectedImages: []v6.ImageStatus{ + { + ID: svcID, + Containers: []v6.Container{ + { + Name: container, + Current: image.Info{ID: currentImageRef}, + NewAvailableImagesCount: 1, + }, + }, + }, + { + ID: anotherSvcID, + Containers: []v6.Container{ + { + Name: anotherContainer, + Current: image.Info{ID: anotherImageRef}, + NewAvailableImagesCount: 0, + }, + }, + }, + }, + shouldError: false, + }, + { + name: "Override container field selection with invalid field", + spec: specAll, + opts: v6.ListImagesOptions{OverrideContainerFields: []string{"InvalidField"}}, + expectedImages: nil, + shouldError: true, + }, } - // List images for specific service - ss = update.ResourceSpec(svc) - is, err = d.ListImages(ctx, ss, v6.ListImagesOptions{}) - if err != nil { - t.Fatalf("Error: %s", err.Error()) - } - ids = imageIDs(is) - if 2 != len(ids) { - t.Fatalf("Expected %v but got %v", 2, len(ids)) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + is, err := d.ListImages(ctx, tt.spec, tt.opts) + assert.Equal(t, tt.shouldError, err != nil) + + // Clear CreatedAt fields for testing + for ri, r := range is { + for ci, c := range r.Containers { + is[ri].Containers[ci].Current.CreatedAt = time.Time{} + is[ri].Containers[ci].LatestFiltered.CreatedAt = time.Time{} + for ai := range c.Available { + is[ri].Containers[ci].Available[ai].CreatedAt = time.Time{} + } + } + } + + assert.Equal(t, tt.expectedImages, is) + }) } } @@ -558,16 +692,6 @@ func (w *wait) ForSyncStatus(d *Daemon, rev string, expectedNumCommits int) []st return revs } -func imageIDs(status []v6.ImageStatus) []image.Info { - var availableImgs []image.Info - for _, i := range status { - for _, c := range i.Containers { - availableImgs = append(availableImgs, c.Available...) - } - } - return availableImgs -} - func updateImage(ctx context.Context, d *Daemon, t *testing.T) job.ID { return updateManifest(ctx, t, d, update.Spec{ Type: update.Images, diff --git a/daemon/images_test.go b/daemon/images_test.go new file mode 100644 index 000000000..cc2da131c --- /dev/null +++ b/daemon/images_test.go @@ -0,0 +1,58 @@ +package daemon + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/weaveworks/flux" + "github.com/weaveworks/flux/policy" +) + +func Test_getTagPattern(t *testing.T) { + resourceID, err := flux.ParseResourceID("default:deployment/helloworld") + assert.NoError(t, err) + container := "helloContainer" + + type args struct { + services policy.ResourceMap + service flux.ResourceID + container string + } + tests := []struct { + name string + args args + want string + }{ + { + name: "Nil policies", + args: args{services: nil}, + want: "*", + }, + { + name: "No match", + args: args{services: policy.ResourceMap{}}, + want: "*", + }, + { + name: "Match", + args: args{ + services: policy.ResourceMap{ + resourceID: policy.Set{ + policy.Policy(fmt.Sprintf("tag.%s", container)): "glob:master-*", + }, + }, + service: resourceID, + container: container, + }, + want: "master-*", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := getTagPattern(tt.args.services, tt.args.service, tt.args.container); got != tt.want { + t.Errorf("getTagPattern() = %v, want %v", got, tt.want) + } + }) + } +} From dafb214650ff783550e30fd94a4eee7f425d00f0 Mon Sep 17 00:00:00 2001 From: Aaron Kirkbride Date: Fri, 18 May 2018 15:40:34 +0100 Subject: [PATCH 5/7] Fix comment spelling --- update/images.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/update/images.go b/update/images.go index e03242040..6d5f38133 100644 --- a/update/images.go +++ b/update/images.go @@ -21,8 +21,8 @@ type ImageRepos struct { imageRepos imageReposMap } -// FindImageInfo retruns image.Info given an image ref. If the image cannot be -// found, return the image.Info with only rhe ID. +// FindImageInfo returns image.Info given an image ref. If the image cannot be +// found, return the image.Info with only the ID. func (r ImageRepos) FindImageInfo(repo image.Name, ref image.Ref) image.Info { images, ok := r.imageRepos[ref.CanonicalName()] if !ok { @@ -50,8 +50,8 @@ func (r ImageRepos) LatestFilteredImage(repo image.Name, tagGlob string) (image. return image.Info{}, false } -// FilteredAvailable returns image.Info engtries for all the images in the -// names image repository which match the tagGlob. +// FilteredAvailable returns image.Info entries for all the images in the +// named image repository which match the tagGlob. func (r ImageRepos) FilteredAvailable(repo image.Name, tagGlob string) []image.Info { var filtered []image.Info for _, available := range r.Available(repo) { From 90d5c6bca9d9d12c4d59f1bb1e75aaef09a23c7f Mon Sep 17 00:00:00 2001 From: Aaron Kirkbride Date: Fri, 18 May 2018 15:48:12 +0100 Subject: [PATCH 6/7] Use switch for iterating fields --- daemon/daemon.go | 83 ++++++++++++++++++++---------------------------- 1 file changed, 35 insertions(+), 48 deletions(-) diff --git a/daemon/daemon.go b/daemon/daemon.go index 98866295a..a736779db 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -560,29 +560,19 @@ func containers2containers(cs []resource.Container) []v6.Container { return res } -func getServiceContainers(service cluster.Controller, imageRepos update.ImageRepos, policyResourceMap policy.ResourceMap, overrideFields []string) (res []v6.Container, err error) { - fields := map[string]struct{}{ - "Name": struct{}{}, - "Current": struct{}{}, - "LatestFiltered": struct{}{}, - "Available": struct{}{}, - "AvailableError": struct{}{}, - "AvailableImagesCount": struct{}{}, - "NewAvailableImagesCount": struct{}{}, - "FilteredImagesCount": struct{}{}, - "NewFilteredImagesCount": struct{}{}, - } - - // If overrideFields is provided, override the default fields to return - if len(overrideFields) > 0 { - newFieldsMap := make(map[string]struct{}) - for _, f := range overrideFields { - if _, ok := fields[f]; !ok { - return nil, errors.Errorf("%s is an invalid field", f) - } - newFieldsMap[f] = struct{}{} +func getServiceContainers(service cluster.Controller, imageRepos update.ImageRepos, policyResourceMap policy.ResourceMap, fields []string) (res []v6.Container, err error) { + if len(fields) == 0 { + fields = []string{ + "Name", + "Current", + "LatestFiltered", + "Available", + "AvailableError", + "AvailableImagesCount", + "NewAvailableImagesCount", + "FilteredImagesCount", + "NewFilteredImagesCount", } - fields = newFieldsMap } for _, c := range service.ContainersOrNil() { @@ -618,32 +608,29 @@ func getServiceContainers(service cluster.Controller, imageRepos update.ImageRep } newFilteredImagesCount := len(newFilteredImages) - if _, ok := fields["Name"]; ok { - container.Name = c.Name - } - if _, ok := fields["Current"]; ok { - container.Current = currentImage - } - if _, ok := fields["LatestFiltered"]; ok { - container.LatestFiltered, _ = imageRepos.LatestFilteredImage(imageRepo, tagPattern) - } - if _, ok := fields["Available"]; ok { - container.Available = availableImages - } - if _, ok := fields["AvailableError"]; ok { - container.AvailableError = availableImagesErr - } - if _, ok := fields["AvailableImagesCount"]; ok { - container.AvailableImagesCount = availableImagesCount - } - if _, ok := fields["NewAvailableImagesCount"]; ok { - container.NewAvailableImagesCount = newAvailableImagesCount - } - if _, ok := fields["FilteredImagesCount"]; ok { - container.FilteredImagesCount = filteredImagesCount - } - if _, ok := fields["NewFilteredImagesCount"]; ok { - container.NewFilteredImagesCount = newFilteredImagesCount + for _, field := range fields { + switch field { + case "Name": + container.Name = c.Name + case "Current": + container.Current = currentImage + case "LatestFiltered": + container.LatestFiltered, _ = imageRepos.LatestFilteredImage(imageRepo, tagPattern) + case "Available": + container.Available = availableImages + case "AvailableError": + container.AvailableError = availableImagesErr + case "AvailableImagesCount": + container.AvailableImagesCount = availableImagesCount + case "NewAvailableImagesCount": + container.NewAvailableImagesCount = newAvailableImagesCount + case "FilteredImagesCount": + container.FilteredImagesCount = filteredImagesCount + case "NewFilteredImagesCount": + container.NewFilteredImagesCount = newFilteredImagesCount + default: + return nil, errors.Errorf("%s is an invalid field", field) + } } res = append(res, container) } From 25d9aaf9615e690f01df399afebf8c808ac5d417 Mon Sep 17 00:00:00 2001 From: Aaron Kirkbride Date: Fri, 18 May 2018 16:55:34 +0100 Subject: [PATCH 7/7] Refactor available image functions --- daemon/daemon.go | 39 ++++++++++----------- daemon/images.go | 4 ++- update/images.go | 79 ++++++++++++++++++++----------------------- update/images_test.go | 10 +++--- update/release.go | 3 +- 5 files changed, 67 insertions(+), 68 deletions(-) diff --git a/daemon/daemon.go b/daemon/daemon.go index a736779db..a3c778e4b 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -580,25 +580,26 @@ func getServiceContainers(service cluster.Controller, imageRepos update.ImageRep imageRepo := c.Image.Name tagPattern := getTagPattern(policyResourceMap, service.ID, c.Name) - currentImage := imageRepos.FindImageInfo(imageRepo, c.Image) - - // All available images - availableImages := imageRepos.Available(imageRepo) - availableImagesCount := len(availableImages) - availableImagesErr := "" - if availableImages == nil { - availableImagesErr = registry.ErrNoImageData.Error() + + images := imageRepos.GetRepoImages(imageRepo) + currentImage := images.FindWithRef(c.Image) + + // All images + imagesCount := len(images) + imagesErr := "" + if images == nil { + imagesErr = registry.ErrNoImageData.Error() } - var newAvailableImages []image.Info - for _, img := range availableImages { + var newImages []image.Info + for _, img := range images { if img.CreatedAt.After(currentImage.CreatedAt) { - newAvailableImages = append(newAvailableImages, img) + newImages = append(newImages, img) } } - newAvailableImagesCount := len(newAvailableImages) + newImagesCount := len(newImages) - // Filtered available images - filteredImages := imageRepos.FilteredAvailable(imageRepo, tagPattern) + // Filtered images + filteredImages := images.Filter(tagPattern) filteredImagesCount := len(filteredImages) var newFilteredImages []image.Info for _, img := range filteredImages { @@ -615,15 +616,15 @@ func getServiceContainers(service cluster.Controller, imageRepos update.ImageRep case "Current": container.Current = currentImage case "LatestFiltered": - container.LatestFiltered, _ = imageRepos.LatestFilteredImage(imageRepo, tagPattern) + container.LatestFiltered, _ = filteredImages.Latest() case "Available": - container.Available = availableImages + container.Available = images case "AvailableError": - container.AvailableError = availableImagesErr + container.AvailableError = imagesErr case "AvailableImagesCount": - container.AvailableImagesCount = availableImagesCount + container.AvailableImagesCount = imagesCount case "NewAvailableImagesCount": - container.NewAvailableImagesCount = newAvailableImagesCount + container.NewAvailableImagesCount = newImagesCount case "FilteredImagesCount": container.FilteredImagesCount = filteredImagesCount case "NewFilteredImagesCount": diff --git a/daemon/images.go b/daemon/images.go index f91461dfc..eb933b5c4 100644 --- a/daemon/images.go +++ b/daemon/images.go @@ -55,7 +55,9 @@ func (d *Daemon) pollForNewImages(logger log.Logger) { repo := currentImageID.Name logger.Log("repo", repo, "pattern", pattern) - if latest, ok := imageRepos.LatestFilteredImage(repo, pattern); ok && latest.ID != currentImageID { + filteredImages := imageRepos.GetRepoImages(repo).Filter(pattern) + + if latest, ok := filteredImages.Latest(); ok && latest.ID != currentImageID { if latest.ID.Tag == "" { logger.Log("msg", "untagged image in available images", "action", "skip", "available", repo) continue diff --git a/update/images.go b/update/images.go index 6d5f38133..42e870a64 100644 --- a/update/images.go +++ b/update/images.go @@ -14,74 +14,67 @@ import ( "github.com/weaveworks/flux/resource" ) -type imageReposMap map[image.CanonicalName][]image.Info +type imageReposMap map[image.CanonicalName]ImageInfos // ImageRepos contains a map of image repositories to their images type ImageRepos struct { imageRepos imageReposMap } -// FindImageInfo returns image.Info given an image ref. If the image cannot be -// found, return the image.Info with only the ID. -func (r ImageRepos) FindImageInfo(repo image.Name, ref image.Ref) image.Info { - images, ok := r.imageRepos[ref.CanonicalName()] - if !ok { - return image.Info{ID: ref} - } - for _, img := range images { - if img.ID == ref { - return img +// GetRepoImages returns image.Info entries for all the images in the +// named image repository. +func (r ImageRepos) GetRepoImages(repo image.Name) ImageInfos { + if canon, ok := r.imageRepos[repo.CanonicalName()]; ok { + infos := make([]image.Info, len(canon)) + for i := range canon { + infos[i] = canon[i] + infos[i].ID = repo.ToRef(infos[i].ID.Tag) } + return infos } - return image.Info{ID: ref} + return nil } -// LatestFilteredImage returns the latest releasable image for a repository -// for which the tag matches a given pattern. A releasable image is -// one that is not tagged "latest". (Assumes the available images are -// in descending order of latestness.) If no such image exists, -// returns a zero value and `false`, and the caller can decide whether -// that's an error or not. -func (r ImageRepos) LatestFilteredImage(repo image.Name, tagGlob string) (image.Info, bool) { - filtered := r.FilteredAvailable(repo, tagGlob) - if len(filtered) > 0 { - return filtered[0], true - } - return image.Info{}, false -} +// ImageInfos is a list of image.Info which can be filtered. +type ImageInfos []image.Info -// FilteredAvailable returns image.Info entries for all the images in the -// named image repository which match the tagGlob. -func (r ImageRepos) FilteredAvailable(repo image.Name, tagGlob string) []image.Info { - var filtered []image.Info - for _, available := range r.Available(repo) { - tag := available.ID.Tag +// Filter returns only the images which match the tagGlob. +func (ii ImageInfos) Filter(tagGlob string) ImageInfos { + var filtered ImageInfos + for _, i := range ii { + tag := i.ID.Tag // Ignore latest if and only if it's not what the user wants. if !strings.EqualFold(tagGlob, "latest") && strings.EqualFold(tag, "latest") { continue } if glob.Glob(tagGlob, tag) { var im image.Info - im = available - im.ID = repo.ToRef(tag) + im = i filtered = append(filtered, im) } } return filtered } -// Available returns image.Info entries for all the images in the -// named image repository. -func (r ImageRepos) Available(repo image.Name) []image.Info { - if canon, ok := r.imageRepos[repo.CanonicalName()]; ok { - infos := make([]image.Info, len(canon)) - for i := range canon { - infos[i] = canon[i] - infos[i].ID = repo.ToRef(infos[i].ID.Tag) +// Latest returns the latest image from ImageInfos. If no such image exists, +// returns a zero value and `false`, and the caller can decide whether +// that's an error or not. +func (ii ImageInfos) Latest() (image.Info, bool) { + if len(ii) > 0 { + return ii[0], true + } + return image.Info{}, false +} + +// FindWithRef returns image.Info given an image ref. If the image cannot be +// found, it returns the image.Info with the ID provided. +func (ii ImageInfos) FindWithRef(ref image.Ref) image.Info { + for _, img := range ii { + if img.ID == ref { + return img } - return infos } - return nil + return image.Info{ID: ref} } // containers represents a collection of things that have containers diff --git a/update/images_test.go b/update/images_test.go index 82f2fe712..79e3c929e 100644 --- a/update/images_test.go +++ b/update/images_test.go @@ -24,21 +24,23 @@ func TestDecanon(t *testing.T) { name: infos, }} - latest, ok := m.LatestFilteredImage(mustParseName("weaveworks/helloworld"), "*") + filteredImages := m.GetRepoImages(mustParseName("weaveworks/helloworld")).Filter("*") + latest, ok := filteredImages.Latest() if !ok { t.Error("did not find latest image") } else if latest.ID.Name != mustParseName("weaveworks/helloworld") { t.Error("name did not match what was asked") } - latest, ok = m.LatestFilteredImage(mustParseName("index.docker.io/weaveworks/helloworld"), "*") + filteredImages = m.GetRepoImages(mustParseName("index.docker.io/weaveworks/helloworld")).Filter("*") + latest, ok = filteredImages.Latest() if !ok { t.Error("did not find latest image") } else if latest.ID.Name != mustParseName("index.docker.io/weaveworks/helloworld") { t.Error("name did not match what was asked") } - avail := m.Available(mustParseName("weaveworks/helloworld")) + avail := m.GetRepoImages(mustParseName("weaveworks/helloworld")) if len(avail) != len(infos) { t.Errorf("expected %d available images, got %d", len(infos), len(avail)) } @@ -51,7 +53,7 @@ func TestDecanon(t *testing.T) { func TestAvail(t *testing.T) { m := ImageRepos{imageReposMap{name: infos}} - avail := m.Available(mustParseName("weaveworks/goodbyeworld")) + avail := m.GetRepoImages(mustParseName("weaveworks/goodbyeworld")) if len(avail) > 0 { t.Errorf("did not expect available images, but got %#v", avail) } diff --git a/update/release.go b/update/release.go index a905e43c9..3b021eaac 100644 --- a/update/release.go +++ b/update/release.go @@ -231,7 +231,8 @@ func (s ReleaseSpec) calculateImageUpdates(rc ReleaseContext, candidates []*Cont for _, container := range containers { currentImageID := container.Image - latestImage, ok := imageRepos.LatestFilteredImage(currentImageID.Name, "*") + filteredImages := imageRepos.GetRepoImages(currentImageID.Name).Filter("*") + latestImage, ok := filteredImages.Latest() if !ok { if currentImageID.CanonicalName() != singleRepo { ignoredOrSkipped = ReleaseStatusIgnored