From ddc9de8eb93d508f61caa7c35091d3db4c45c7d7 Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Wed, 6 May 2020 10:32:49 -0400 Subject: [PATCH 01/10] Order ContainerStatuses the same as spec.Containers Order ContainerStatuses so it has the same order as spec.Containers --- pkg/reconciler/revision/revision.go | 35 ++++++++++++++++-------- pkg/reconciler/revision/revision_test.go | 10 +++++++ 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index e165410d8ce7..a278d01b0934 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -130,20 +130,31 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro } digestGrp.Wait() close(digests) + + digestSlice := make([]digestData, len(digests)) for v := range digests { - if v.digestError != nil { - rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, - v1.RevisionContainerMissingMessage( - v.image, v.digestError.Error())) - return v.digestError - } - if v.isServingContainer { - rev.Status.DeprecatedImageDigest = v.digestValue + digestSlice = append(digestSlice, v) + } + rev.Status.ContainerStatuses = make([]v1.ContainerStatuses, len(rev.Spec.Containers)) + for i, container := range rev.Spec.Containers { + for _, v := range digestSlice { + if v.digestError != nil { + rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, + v1.RevisionContainerMissingMessage( + v.image, v.digestError.Error())) + return v.digestError + } + if container.Name != v.containerName { + continue + } + if v.isServingContainer { + rev.Status.DeprecatedImageDigest = v.digestValue + } + rev.Status.ContainerStatuses[i] = v1.ContainerStatuses{ + Name: v.containerName, + ImageDigest: v.digestValue, + } } - rev.Status.ContainerStatuses = append(rev.Status.ContainerStatuses, v1.ContainerStatuses{ - Name: v.containerName, - ImageDigest: v.digestValue, - }) } return nil } diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index 4e8925b5fb6c..68cbdc678249 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -346,12 +346,17 @@ func TestRevWithImageDigests(t *testing.T) { rev := testRevision(corev1.PodSpec{ Containers: []corev1.Container{{ + Name: "first", Image: "gcr.io/repo/image", Ports: []corev1.ContainerPort{{ ContainerPort: 8888, }}, }, { + Name: "second", Image: "docker.io/repo/image", + }, { + Name: "third", + Image: "docker.io/anotherrepo/image", }}, }) createRevision(t, ctx, controller, rev) @@ -369,6 +374,11 @@ func TestRevWithImageDigests(t *testing.T) { if len(rev.Spec.Containers) != len(rev.Status.ContainerStatuses) { t.Error("Image digests does not match with the provided containers") } + for i, c := range rev.Spec.Containers { + if c.Name != rev.Status.ContainerStatuses[i].Name { + t.Error("Container statuses do not match the order of containers in spec") + } + } rev.Status.ContainerStatuses = []v1.ContainerStatuses{} updateRevision(t, ctx, controller, rev) if len(rev.Status.ContainerStatuses) != 0 { From 4aaab52e99559846e9d6edf49e48f265128949fe Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Wed, 6 May 2020 13:05:21 -0400 Subject: [PATCH 02/10] Use map for sorting --- pkg/reconciler/revision/revision.go | 42 ++++++++++++++++------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index a278d01b0934..c2d7edeb3a69 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -106,7 +106,9 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro } digests := make(chan digestData, len(rev.Spec.Containers)) - for _, container := range rev.Spec.Containers { + containerIndexMap := map[string]int{} + for i, container := range rev.Spec.Containers { + containerIndexMap[container.Name] = i container := container // Standard Go concurrency pattern. digestGrp.Go(func() error { digest, err := c.resolver.Resolve(container.Image, @@ -136,24 +138,26 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro digestSlice = append(digestSlice, v) } rev.Status.ContainerStatuses = make([]v1.ContainerStatuses, len(rev.Spec.Containers)) - for i, container := range rev.Spec.Containers { - for _, v := range digestSlice { - if v.digestError != nil { - rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, - v1.RevisionContainerMissingMessage( - v.image, v.digestError.Error())) - return v.digestError - } - if container.Name != v.containerName { - continue - } - if v.isServingContainer { - rev.Status.DeprecatedImageDigest = v.digestValue - } - rev.Status.ContainerStatuses[i] = v1.ContainerStatuses{ - Name: v.containerName, - ImageDigest: v.digestValue, - } + for _, v := range digestSlice { + if v.digestError != nil { + rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, + v1.RevisionContainerMissingMessage( + v.image, v.digestError.Error())) + return v.digestError + } + if v.containerName == "" { + continue + } + if v.isServingContainer { + rev.Status.DeprecatedImageDigest = v.digestValue + } + index, ok := containerIndexMap[v.containerName] + if !ok { + return fmt.Errorf("error sorting container statuses") + } + rev.Status.ContainerStatuses[index] = v1.ContainerStatuses{ + Name: v.containerName, + ImageDigest: v.digestValue, } } return nil From 070922da0da36165233a4358f87a0f20b9463166 Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Wed, 6 May 2020 13:06:49 -0400 Subject: [PATCH 03/10] Review --- pkg/reconciler/revision/revision.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index c2d7edeb3a69..8c0f890ed081 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -133,7 +133,7 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro digestGrp.Wait() close(digests) - digestSlice := make([]digestData, len(digests)) + digestSlice := make([]digestData, 0, len(digests)) for v := range digests { digestSlice = append(digestSlice, v) } From bfce7f290bed831d66008a427650589ef3f8e072 Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Wed, 6 May 2020 14:20:38 -0400 Subject: [PATCH 04/10] combine loops --- pkg/reconciler/revision/revision.go | 67 +++++++---------------------- 1 file changed, 16 insertions(+), 51 deletions(-) diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 8c0f890ed081..5c21369347da 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -97,70 +97,35 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro } var digestGrp errgroup.Group - type digestData struct { - digestValue string - containerName string - isServingContainer bool - image string - digestError error - } - - digests := make(chan digestData, len(rev.Spec.Containers)) - containerIndexMap := map[string]int{} + rev.Status.ContainerStatuses = make([]v1.ContainerStatuses, len(rev.Spec.Containers)) for i, container := range rev.Spec.Containers { - containerIndexMap[container.Name] = i container := container // Standard Go concurrency pattern. + i := i digestGrp.Go(func() error { digest, err := c.resolver.Resolve(container.Image, opt, cfgs.Deployment.RegistriesSkippingTagResolving) if err != nil { err = fmt.Errorf("failed to resolve image to digest: %w", err) - digests <- digestData{ - image: container.Image, - digestError: err, - } + rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, + v1.RevisionContainerMissingMessage( + container.Image, err.Error())) + return err } else { - isServingContainer := len(rev.Spec.Containers) == 1 || len(container.Ports) != 0 - digests <- digestData{ - digestValue: digest, - containerName: container.Name, - isServingContainer: isServingContainer, + if container.Name == "" { + return nil + } + if len(rev.Spec.Containers) == 1 || len(container.Ports) != 0 { + rev.Status.DeprecatedImageDigest = digest + } + rev.Status.ContainerStatuses[i] = v1.ContainerStatuses{ + Name: container.Name, + ImageDigest: digest, } } return nil }) } - digestGrp.Wait() - close(digests) - - digestSlice := make([]digestData, 0, len(digests)) - for v := range digests { - digestSlice = append(digestSlice, v) - } - rev.Status.ContainerStatuses = make([]v1.ContainerStatuses, len(rev.Spec.Containers)) - for _, v := range digestSlice { - if v.digestError != nil { - rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, - v1.RevisionContainerMissingMessage( - v.image, v.digestError.Error())) - return v.digestError - } - if v.containerName == "" { - continue - } - if v.isServingContainer { - rev.Status.DeprecatedImageDigest = v.digestValue - } - index, ok := containerIndexMap[v.containerName] - if !ok { - return fmt.Errorf("error sorting container statuses") - } - rev.Status.ContainerStatuses[index] = v1.ContainerStatuses{ - Name: v.containerName, - ImageDigest: v.digestValue, - } - } - return nil + return digestGrp.Wait() } func (c *Reconciler) ReconcileKind(ctx context.Context, rev *v1.Revision) pkgreconciler.Event { From c2abddd7ca55fd09dbe4070838e0fde4b5e1291f Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Wed, 6 May 2020 14:38:21 -0400 Subject: [PATCH 05/10] Review --- pkg/reconciler/revision/revision.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 5c21369347da..abfece6c7fe4 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -98,6 +98,7 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro var digestGrp errgroup.Group rev.Status.ContainerStatuses = make([]v1.ContainerStatuses, len(rev.Spec.Containers)) + var failedContainerImage string for i, container := range rev.Spec.Containers { container := container // Standard Go concurrency pattern. i := i @@ -105,11 +106,8 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro digest, err := c.resolver.Resolve(container.Image, opt, cfgs.Deployment.RegistriesSkippingTagResolving) if err != nil { - err = fmt.Errorf("failed to resolve image to digest: %w", err) - rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, - v1.RevisionContainerMissingMessage( - container.Image, err.Error())) - return err + failedContainerImage = container.Image + return fmt.Errorf("failed to resolve image to digest: %w", err) } else { if container.Name == "" { return nil @@ -125,7 +123,12 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro return nil }) } - return digestGrp.Wait() + if err := digestGrp.Wait(); err != nil { + rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, + v1.RevisionContainerMissingMessage(failedContainerImage, err.Error())) + return err + } + return nil } func (c *Reconciler) ReconcileKind(ctx context.Context, rev *v1.Revision) pkgreconciler.Event { From 7d7f3314fa2d2fd69fcc0e3b37b3da6f92db768b Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Thu, 7 May 2020 09:26:46 -0400 Subject: [PATCH 06/10] Remove failedContainerImage --- pkg/reconciler/revision/revision.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index abfece6c7fe4..6c03d825d827 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -98,7 +98,6 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro var digestGrp errgroup.Group rev.Status.ContainerStatuses = make([]v1.ContainerStatuses, len(rev.Spec.Containers)) - var failedContainerImage string for i, container := range rev.Spec.Containers { container := container // Standard Go concurrency pattern. i := i @@ -106,8 +105,7 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro digest, err := c.resolver.Resolve(container.Image, opt, cfgs.Deployment.RegistriesSkippingTagResolving) if err != nil { - failedContainerImage = container.Image - return fmt.Errorf("failed to resolve image to digest: %w", err) + return fmt.Errorf(v1.RevisionContainerMissingMessage(container.Image, err.Error())) } else { if container.Name == "" { return nil @@ -124,8 +122,7 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro }) } if err := digestGrp.Wait(); err != nil { - rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, - v1.RevisionContainerMissingMessage(failedContainerImage, err.Error())) + rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, err.Error()) return err } return nil From 79e63683b91d37f7b1eb64f4a1354769b720a73c Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Thu, 7 May 2020 09:37:36 -0400 Subject: [PATCH 07/10] Fix err message --- pkg/reconciler/revision/revision.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 6c03d825d827..6c13c83b7901 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -105,7 +105,7 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro digest, err := c.resolver.Resolve(container.Image, opt, cfgs.Deployment.RegistriesSkippingTagResolving) if err != nil { - return fmt.Errorf(v1.RevisionContainerMissingMessage(container.Image, err.Error())) + return fmt.Errorf(v1.RevisionContainerMissingMessage(container.Image, fmt.Sprintf("failed to resolve image to digest: %v", err))) } else { if container.Name == "" { return nil From df66e171c4d503690fcd3767456654fd321e51a3 Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Thu, 7 May 2020 09:44:03 -0400 Subject: [PATCH 08/10] Review --- pkg/reconciler/revision/revision.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 6c13c83b7901..862b9dbaba6c 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -18,6 +18,7 @@ package revision import ( "context" + "errors" "fmt" "strings" @@ -105,7 +106,7 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro digest, err := c.resolver.Resolve(container.Image, opt, cfgs.Deployment.RegistriesSkippingTagResolving) if err != nil { - return fmt.Errorf(v1.RevisionContainerMissingMessage(container.Image, fmt.Sprintf("failed to resolve image to digest: %v", err))) + return errors.New(v1.RevisionContainerMissingMessage(container.Image, fmt.Sprintf("failed to resolve image to digest: %v", err))) } else { if container.Name == "" { return nil From 19ffa323bfe3fc5a5c995fe512041f83b8970501 Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Thu, 7 May 2020 12:14:33 -0400 Subject: [PATCH 09/10] Clear container statuses if there's an error pulling digest --- pkg/reconciler/revision/revision.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 862b9dbaba6c..bff28800f962 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -108,9 +108,6 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro if err != nil { return errors.New(v1.RevisionContainerMissingMessage(container.Image, fmt.Sprintf("failed to resolve image to digest: %v", err))) } else { - if container.Name == "" { - return nil - } if len(rev.Spec.Containers) == 1 || len(container.Ports) != 0 { rev.Status.DeprecatedImageDigest = digest } @@ -124,6 +121,7 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro } if err := digestGrp.Wait(); err != nil { rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, err.Error()) + rev.Status.ContainerStatuses = make([]v1.ContainerStatuses, 0) return err } return nil From ac7acb08cd26dfc231cc7254e8b9390021554057 Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Thu, 7 May 2020 15:54:48 -0400 Subject: [PATCH 10/10] Review --- pkg/reconciler/revision/revision.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index bff28800f962..caec982272e1 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -98,7 +98,7 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro } var digestGrp errgroup.Group - rev.Status.ContainerStatuses = make([]v1.ContainerStatuses, len(rev.Spec.Containers)) + containerStatuses := make([]v1.ContainerStatuses, len(rev.Spec.Containers)) for i, container := range rev.Spec.Containers { container := container // Standard Go concurrency pattern. i := i @@ -107,23 +107,22 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro opt, cfgs.Deployment.RegistriesSkippingTagResolving) if err != nil { return errors.New(v1.RevisionContainerMissingMessage(container.Image, fmt.Sprintf("failed to resolve image to digest: %v", err))) - } else { - if len(rev.Spec.Containers) == 1 || len(container.Ports) != 0 { - rev.Status.DeprecatedImageDigest = digest - } - rev.Status.ContainerStatuses[i] = v1.ContainerStatuses{ - Name: container.Name, - ImageDigest: digest, - } + } + if len(rev.Spec.Containers) == 1 || len(container.Ports) != 0 { + rev.Status.DeprecatedImageDigest = digest + } + containerStatuses[i] = v1.ContainerStatuses{ + Name: container.Name, + ImageDigest: digest, } return nil }) } if err := digestGrp.Wait(); err != nil { rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, err.Error()) - rev.Status.ContainerStatuses = make([]v1.ContainerStatuses, 0) return err } + rev.Status.ContainerStatuses = containerStatuses return nil }