From b01195a6173db65e641d524f83869bc0b418999d Mon Sep 17 00:00:00 2001 From: savitaashture Date: Tue, 24 Mar 2020 23:34:48 +0530 Subject: [PATCH 01/13] Add ImageDigests for multi container support --- pkg/apis/serving/v1/revision_types.go | 7 ++++ pkg/apis/serving/v1/zz_generated.deepcopy.go | 7 ++++ .../serving/v1alpha1/revision_conversion.go | 2 ++ pkg/apis/serving/v1alpha1/revision_types.go | 7 ++++ .../serving/v1alpha1/zz_generated.deepcopy.go | 7 ++++ pkg/reconciler/revision/queueing_test.go | 24 +++++++++++++ pkg/reconciler/revision/revision.go | 35 +++++++++++++------ pkg/reconciler/revision/revision_test.go | 34 ++++++++++++++++++ pkg/reconciler/revision/table_test.go | 2 ++ 9 files changed, 114 insertions(+), 11 deletions(-) diff --git a/pkg/apis/serving/v1/revision_types.go b/pkg/apis/serving/v1/revision_types.go index 7b5765a251a1..0b1b8a355400 100644 --- a/pkg/apis/serving/v1/revision_types.go +++ b/pkg/apis/serving/v1/revision_types.go @@ -136,6 +136,13 @@ type RevisionStatus struct { // may be empty if the image comes from a registry listed to skip resolution. // +optional ImageDigest string `json:"imageDigest,omitempty"` + + // ImageDigests holds the resolved digest for the image specified + // within .Spec.Container.Image. The digest is resolved during the creation + // of Revision. ImageDigests holds the digest for all the .Spec.Container.Image both serving and non serving. + // ref: http://bit.ly/image-digests + // +optional + ImageDigests map[string]string `json:"imageDigests,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/pkg/apis/serving/v1/zz_generated.deepcopy.go b/pkg/apis/serving/v1/zz_generated.deepcopy.go index bfff38f19787..ae7f28d4e883 100644 --- a/pkg/apis/serving/v1/zz_generated.deepcopy.go +++ b/pkg/apis/serving/v1/zz_generated.deepcopy.go @@ -230,6 +230,13 @@ func (in *RevisionSpec) DeepCopy() *RevisionSpec { func (in *RevisionStatus) DeepCopyInto(out *RevisionStatus) { *out = *in in.Status.DeepCopyInto(&out.Status) + if in.ImageDigests != nil { + in, out := &in.ImageDigests, &out.ImageDigests + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } return } diff --git a/pkg/apis/serving/v1alpha1/revision_conversion.go b/pkg/apis/serving/v1alpha1/revision_conversion.go index e54ebfeeaa5e..1e8f21536207 100644 --- a/pkg/apis/serving/v1alpha1/revision_conversion.go +++ b/pkg/apis/serving/v1alpha1/revision_conversion.go @@ -80,6 +80,7 @@ func (source *RevisionStatus) ConvertTo(ctx context.Context, sink *v1.RevisionSt sink.ServiceName = source.ServiceName sink.LogURL = source.LogURL sink.ImageDigest = source.ImageDigest + sink.ImageDigests = source.ImageDigests } // ConvertFrom implements apis.Convertible @@ -112,4 +113,5 @@ func (sink *RevisionStatus) ConvertFrom(ctx context.Context, source v1.RevisionS sink.ServiceName = source.ServiceName sink.LogURL = source.LogURL sink.ImageDigest = source.ImageDigest + sink.ImageDigests = source.ImageDigests } diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index e3c4ce8f943e..27ce1e8947b2 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/pkg/apis/serving/v1alpha1/revision_types.go @@ -192,6 +192,13 @@ type RevisionStatus struct { // may be empty if the image comes from a registry listed to skip resolution. // +optional ImageDigest string `json:"imageDigest,omitempty"` + + // ImageDigests holds the resolved digest for the image specified + // within .Spec.Container.Image. The digest is resolved during the creation + // of Revision. ImageDigests holds the digest for all the .Spec.Container.Image both serving and non serving. + // ref: http://bit.ly/image-digests + // +optional + ImageDigests map[string]string `json:"imageDigests,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go index 8d347d821b62..92bff3f318a8 100644 --- a/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go @@ -316,6 +316,13 @@ func (in *RevisionSpec) DeepCopy() *RevisionSpec { func (in *RevisionStatus) DeepCopyInto(out *RevisionStatus) { *out = *in in.Status.DeepCopyInto(&out.Status) + if in.ImageDigests != nil { + in, out := &in.ImageDigests, &out.ImageDigests + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } return } diff --git a/pkg/reconciler/revision/queueing_test.go b/pkg/reconciler/revision/queueing_test.go index 7a10c480080e..461fb6c769c0 100644 --- a/pkg/reconciler/revision/queueing_test.go +++ b/pkg/reconciler/revision/queueing_test.go @@ -111,6 +111,30 @@ func testRevision() *v1.Revision { return rev } +func testRevisionForMultipleContainer() *v1.Revision { + return &v1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rev-multi-container", + Namespace: testNamespace, + }, + Spec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "gcr.io/repo/image", + }, { + Image: "docker.io/repo/image", + }}, + }, + }, + } +} + +func getTestDefaultConfig() *config.Defaults { + c, _ := config.NewDefaultsConfigFromConfigMap(getTestDefaultsConfigMap()) + // ignoring error as test controller is generated + return c +} + func getTestDeploymentConfig() *deployment.Config { c, _ := deployment.NewConfigFromConfigMap(getTestDeploymentConfigMap()) // ignoring error as test controller is generated diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 73bc952a45ba..2cf3fc1df968 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -64,7 +64,7 @@ var _ revisionreconciler.Interface = (*Reconciler)(nil) func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) error { // The image digest has already been resolved. - if rev.Status.ImageDigest != "" { + if rev.Status.ImageDigest != "" || len(rev.Status.ImageDigests) == len(rev.Spec.Containers) { return nil } @@ -78,18 +78,31 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro ServiceAccountName: rev.Spec.ServiceAccountName, ImagePullSecrets: imagePullSecrets, } - digest, err := c.resolver.Resolve(rev.Spec.GetContainer().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( - rev.Spec.GetContainer().Image, err.Error())) - return err + if rev.Status.ImageDigests == nil { + rev.Status.ImageDigests = make(map[string]string, len(rev.Spec.Containers)) } + for _, container := range rev.Spec.Containers { + 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 + } + if len(rev.Spec.Containers) == 1 || len(container.Ports) != 0 { + rev.Status.ImageDigest = digest + } - rev.Status.ImageDigest = digest - + // In order to keep consistency with original behavior of single container handled below scenario + // revision status should not contains imageDigests field in these scenario + // 1. If flag is disabled + // 2. If flag enabled but applied revision has single container + if cfgs.Defaults.EnableMultiContainer && len(rev.Spec.Containers) > 1 { + rev.Status.ImageDigests[container.Name] = digest + } + } return nil } diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index 78af24c3af35..e6138852507d 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -340,6 +340,40 @@ func TestUpdateRevWithWithUpdatedLoggingURL(t *testing.T) { } } +func TestRevWithImageDigests(t *testing.T) { + deploymentConfig := getTestDeploymentConfig() + ctx, _, controller, _ := newTestControllerWithConfig(t, deploymentConfig, []*corev1.ConfigMap{{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "enable-multi-container": "true", + }, + }, + }) + // Flag is enabled and spec contains single container then rev status should not have imageDigests field + rev := getRev(ctx, t, controller, testRevision()) + if len(rev.Status.ImageDigests) != 0 { + t.Errorf("revision status does not have imageDigests") + } + // Flag is enabled and spec contains multiple container so rev status should contain imageDigests field + rev = getRev(ctx, t, controller, testRevisionForMultipleContainer()) + if len(rev.Status.ImageDigests) == 0 { + t.Errorf("revision status does not have imageDigests") + } +} + +func getRev(ctx context.Context, t *testing.T, controller *controller.Impl, rev *v1.Revision) *v1.Revision { + revClient := fakeservingclient.Get(ctx).ServingV1().Revisions(testNamespace) + createRevision(t, ctx, controller, rev) + createdRev, err := revClient.Get(rev.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("couldn't get revision: %v", err) + } + return createdRev +} + // TODO(mattmoor): Remove when we have coverage of EnqueueEndpointsRevision func TestMarkRevReadyUponEndpointBecomesReady(t *testing.T) { ctx, cancel, _, ctl, _ := newTestController(t) diff --git a/pkg/reconciler/revision/table_test.go b/pkg/reconciler/revision/table_test.go index 9655b7abf344..64aa520751c5 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -36,6 +36,7 @@ import ( pkgreconciler "knative.dev/pkg/reconciler" tracingconfig "knative.dev/pkg/tracing/config" asv1a1 "knative.dev/serving/pkg/apis/autoscaling/v1alpha1" + defaultconfig "knative.dev/serving/pkg/apis/config" "knative.dev/serving/pkg/apis/networking" v1 "knative.dev/serving/pkg/apis/serving/v1" autoscalerconfig "knative.dev/serving/pkg/autoscaler/config" @@ -747,5 +748,6 @@ func ReconcilerTestConfig() *config.Config { Logging: &logging.Config{}, Tracing: &tracingconfig.Config{}, Autoscaler: &autoscalerconfig.Config{}, + Defaults: &defaultconfig.Defaults{}, } } From 65ca1a382fac27db140ba2f9c085f798bbb941d5 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Wed, 25 Mar 2020 20:33:23 +0530 Subject: [PATCH 02/13] move imageDigest to Deprecate and fix review comments --- pkg/apis/serving/v1/revision_types.go | 8 +- .../serving/v1alpha1/revision_conversion.go | 4 +- pkg/apis/serving/v1alpha1/revision_types.go | 6 +- pkg/reconciler/revision/queueing_test.go | 86 +++++++------------ pkg/reconciler/revision/resources/deploy.go | 4 +- .../revision/resources/deploy_test.go | 2 +- .../revision/resources/imagecache.go | 2 +- .../revision/resources/imagecache_test.go | 2 +- pkg/reconciler/revision/revision.go | 13 +-- pkg/reconciler/revision/revision_test.go | 48 +++++------ pkg/reconciler/revision/table_test.go | 46 +++++----- pkg/testing/v1/revision.go | 7 ++ test/conformance/api/v1/util.go | 6 +- test/conformance/api/v1alpha1/util.go | 6 +- test/conformance/api/v1beta1/util.go | 6 +- 15 files changed, 113 insertions(+), 133 deletions(-) diff --git a/pkg/apis/serving/v1/revision_types.go b/pkg/apis/serving/v1/revision_types.go index 0b1b8a355400..604287bd8785 100644 --- a/pkg/apis/serving/v1/revision_types.go +++ b/pkg/apis/serving/v1/revision_types.go @@ -129,15 +129,17 @@ type RevisionStatus struct { // +optional LogURL string `json:"logUrl,omitempty"` - // ImageDigest holds the resolved digest for the image specified + // DeprecatedImageDigest holds the resolved digest for the image specified // within .Spec.Container.Image. The digest is resolved during the creation // of Revision. This field holds the digest value regardless of whether // a tag or digest was originally specified in the Container object. It // may be empty if the image comes from a registry listed to skip resolution. + // DEPRECATED Use ImageDigests instead. + // TODO(savitaashture) Remove deprecatedImageDigest in 2 releases(0.16). // +optional - ImageDigest string `json:"imageDigest,omitempty"` + DeprecatedImageDigest string `json:"imageDigest,omitempty"` - // ImageDigests holds the resolved digest for the image specified + // ImageDigests holds the resolved digest for the specified images // within .Spec.Container.Image. The digest is resolved during the creation // of Revision. ImageDigests holds the digest for all the .Spec.Container.Image both serving and non serving. // ref: http://bit.ly/image-digests diff --git a/pkg/apis/serving/v1alpha1/revision_conversion.go b/pkg/apis/serving/v1alpha1/revision_conversion.go index 1e8f21536207..58c9f4453a70 100644 --- a/pkg/apis/serving/v1alpha1/revision_conversion.go +++ b/pkg/apis/serving/v1alpha1/revision_conversion.go @@ -79,7 +79,7 @@ func (source *RevisionStatus) ConvertTo(ctx context.Context, sink *v1.RevisionSt source.Status.ConvertTo(ctx, &sink.Status, v1.IsRevisionCondition) sink.ServiceName = source.ServiceName sink.LogURL = source.LogURL - sink.ImageDigest = source.ImageDigest + sink.DeprecatedImageDigest = source.DeprecatedImageDigest sink.ImageDigests = source.ImageDigests } @@ -112,6 +112,6 @@ func (sink *RevisionStatus) ConvertFrom(ctx context.Context, source v1.RevisionS source.Status.ConvertTo(ctx, &sink.Status, v1.IsRevisionCondition) sink.ServiceName = source.ServiceName sink.LogURL = source.LogURL - sink.ImageDigest = source.ImageDigest + sink.DeprecatedImageDigest = source.DeprecatedImageDigest sink.ImageDigests = source.ImageDigests } diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index 27ce1e8947b2..507fa2eef830 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/pkg/apis/serving/v1alpha1/revision_types.go @@ -185,13 +185,15 @@ type RevisionStatus struct { // +optional LogURL string `json:"logUrl,omitempty"` - // ImageDigest holds the resolved digest for the image specified + // DeprecatedImageDigest holds the resolved digest for the image specified // within .Spec.Container.Image. The digest is resolved during the creation // of Revision. This field holds the digest value regardless of whether // a tag or digest was originally specified in the Container object. It // may be empty if the image comes from a registry listed to skip resolution. + // DEPRECATED Use ImageDigests instead. + // TODO(savitaashture) Remove deprecatedImageDigest in 2 releases(0.16).. // +optional - ImageDigest string `json:"imageDigest,omitempty"` + DeprecatedImageDigest string `json:"imageDigest,omitempty"` // ImageDigests holds the resolved digest for the image specified // within .Spec.Container.Image. The digest is resolved during the creation diff --git a/pkg/reconciler/revision/queueing_test.go b/pkg/reconciler/revision/queueing_test.go index 461fb6c769c0..2dc8cd495c23 100644 --- a/pkg/reconciler/revision/queueing_test.go +++ b/pkg/reconciler/revision/queueing_test.go @@ -60,7 +60,37 @@ const ( testQueueImage = "queueImage" ) -func testRevision() *v1.Revision { +func getPodSpec() corev1.PodSpec { + return corev1.PodSpec{ + // corev1.Container has a lot of setting. We try to pass many + // of them here to verify that we pass through the settings to + // derived objects. + Containers: []corev1.Container{{ + Image: "gcr.io/repo/image", + Command: []string{"echo"}, + Args: []string{"hello", "world"}, + WorkingDir: "/tmp", + Env: []corev1.EnvVar{{ + Name: "EDITOR", + Value: "emacs", + }}, + LivenessProbe: &corev1.Probe{ + TimeoutSeconds: 42, + }, + ReadinessProbe: &corev1.Probe{ + Handler: corev1.Handler{ + HTTPGet: &corev1.HTTPGetAction{ + Path: "health", + }, + }, + TimeoutSeconds: 43, + }, + TerminationMessagePath: "/dev/null", + }}, + } +} + +func testRevision(podSpec corev1.PodSpec) *v1.Revision { rev := &v1.Revision{ ObjectMeta: metav1.ObjectMeta{ SelfLink: "/apis/serving/v1/namespaces/test/revisions/test-rev", @@ -77,33 +107,7 @@ func testRevision() *v1.Revision { UID: "test-rev-uid", }, Spec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - // corev1.Container has a lot of setting. We try to pass many - // of them here to verify that we pass through the settings to - // derived objects. - Containers: []corev1.Container{{ - Image: "gcr.io/repo/image", - Command: []string{"echo"}, - Args: []string{"hello", "world"}, - WorkingDir: "/tmp", - Env: []corev1.EnvVar{{ - Name: "EDITOR", - Value: "emacs", - }}, - LivenessProbe: &corev1.Probe{ - TimeoutSeconds: 42, - }, - ReadinessProbe: &corev1.Probe{ - Handler: corev1.Handler{ - HTTPGet: &corev1.HTTPGetAction{ - Path: "health", - }, - }, - TimeoutSeconds: 43, - }, - TerminationMessagePath: "/dev/null", - }}, - }, + PodSpec: podSpec, TimeoutSeconds: ptr.Int64(60), }, } @@ -111,30 +115,6 @@ func testRevision() *v1.Revision { return rev } -func testRevisionForMultipleContainer() *v1.Revision { - return &v1.Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-rev-multi-container", - Namespace: testNamespace, - }, - Spec: v1.RevisionSpec{ - PodSpec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Image: "gcr.io/repo/image", - }, { - Image: "docker.io/repo/image", - }}, - }, - }, - } -} - -func getTestDefaultConfig() *config.Defaults { - c, _ := config.NewDefaultsConfigFromConfigMap(getTestDefaultsConfigMap()) - // ignoring error as test controller is generated - return c -} - func getTestDeploymentConfig() *deployment.Config { c, _ := deployment.NewConfigFromConfigMap(getTestDeploymentConfigMap()) // ignoring error as test controller is generated @@ -254,7 +234,7 @@ func TestNewRevisionCallsSyncHandler(t *testing.T) { eg := errgroup.Group{} - rev := testRevision() + rev := testRevision(getPodSpec()) servingClient := fakeservingclient.Get(ctx) h := NewHooks() diff --git a/pkg/reconciler/revision/resources/deploy.go b/pkg/reconciler/revision/resources/deploy.go index e56f3b564abe..5c190c4b5e3b 100644 --- a/pkg/reconciler/revision/resources/deploy.go +++ b/pkg/reconciler/revision/resources/deploy.go @@ -159,8 +159,8 @@ func makePodSpec(rev *v1.Revision, loggingConfig *logging.Config, tracingConfig userContainer.TTY = false // Prefer imageDigest from revision if available - if rev.Status.ImageDigest != "" { - userContainer.Image = rev.Status.ImageDigest + if rev.Status.DeprecatedImageDigest != "" { + userContainer.Image = rev.Status.DeprecatedImageDigest } if userContainer.TerminationMessagePolicy == "" { diff --git a/pkg/reconciler/revision/resources/deploy_test.go b/pkg/reconciler/revision/resources/deploy_test.go index c2c1464719e4..ac7f9b56e184 100644 --- a/pkg/reconciler/revision/resources/deploy_test.go +++ b/pkg/reconciler/revision/resources/deploy_test.go @@ -495,7 +495,7 @@ func TestMakePodSpec(t *testing.T) { withContainerConcurrency(1), func(revision *v1.Revision) { revision.Status = v1.RevisionStatus{ - ImageDigest: "busybox@sha256:deadbeef", + DeprecatedImageDigest: "busybox@sha256:deadbeef", } container(revision.Spec.GetContainer(), withTCPReadinessProbe(), diff --git a/pkg/reconciler/revision/resources/imagecache.go b/pkg/reconciler/revision/resources/imagecache.go index 6080721697ee..d2816b89fb73 100644 --- a/pkg/reconciler/revision/resources/imagecache.go +++ b/pkg/reconciler/revision/resources/imagecache.go @@ -28,7 +28,7 @@ import ( // MakeImageCache makes an caching.Image resources from a revision. func MakeImageCache(rev *v1.Revision) *caching.Image { - image := rev.Status.ImageDigest + image := rev.Status.DeprecatedImageDigest if image == "" { image = rev.Spec.GetContainer().Image } diff --git a/pkg/reconciler/revision/resources/imagecache_test.go b/pkg/reconciler/revision/resources/imagecache_test.go index 966a23959974..7015476718cf 100644 --- a/pkg/reconciler/revision/resources/imagecache_test.go +++ b/pkg/reconciler/revision/resources/imagecache_test.go @@ -55,7 +55,7 @@ func TestMakeImageCache(t *testing.T) { }, }, Status: v1.RevisionStatus{ - ImageDigest: "busybox@sha256:deadbeef", + DeprecatedImageDigest: "busybox@sha256:deadbeef", }, }, want: &caching.Image{ diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 2cf3fc1df968..19e440a3b342 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -64,7 +64,7 @@ var _ revisionreconciler.Interface = (*Reconciler)(nil) func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) error { // The image digest has already been resolved. - if rev.Status.ImageDigest != "" || len(rev.Status.ImageDigests) == len(rev.Spec.Containers) { + if rev.Status.DeprecatedImageDigest != "" || len(rev.Status.ImageDigests) == len(rev.Spec.Containers) { return nil } @@ -92,16 +92,9 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro return err } if len(rev.Spec.Containers) == 1 || len(container.Ports) != 0 { - rev.Status.ImageDigest = digest - } - - // In order to keep consistency with original behavior of single container handled below scenario - // revision status should not contains imageDigests field in these scenario - // 1. If flag is disabled - // 2. If flag enabled but applied revision has single container - if cfgs.Defaults.EnableMultiContainer && len(rev.Spec.Containers) > 1 { - rev.Status.ImageDigests[container.Name] = digest + rev.Status.DeprecatedImageDigest = digest } + rev.Status.ImageDigests[container.Name] = digest } return nil } diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index e6138852507d..8355d7521ea8 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -268,7 +268,7 @@ func TestResolutionFailed(t *testing.T) { }) defer cancel() - rev := testRevision() + rev := testRevision(getPodSpec()) config := testConfiguration() rev.OwnerReferences = append(rev.OwnerReferences, *kmeta.NewControllerRef(config)) @@ -313,7 +313,7 @@ func TestUpdateRevWithWithUpdatedLoggingURL(t *testing.T) { }) revClient := fakeservingclient.Get(ctx).ServingV1().Revisions(testNamespace) - rev := testRevision() + rev := testRevision(getPodSpec()) createRevision(t, ctx, controller, rev) // Update controllers logging URL @@ -344,41 +344,37 @@ func TestRevWithImageDigests(t *testing.T) { deploymentConfig := getTestDeploymentConfig() ctx, _, controller, _ := newTestControllerWithConfig(t, deploymentConfig, []*corev1.ConfigMap{{ ObjectMeta: metav1.ObjectMeta{ - Namespace: system.Namespace(), Name: config.DefaultsConfigName, + Namespace: system.Namespace(), }, Data: map[string]string{ - "enable-multi-container": "true", + "container-name-template": "user-container", }, - }, - }) - // Flag is enabled and spec contains single container then rev status should not have imageDigests field - rev := getRev(ctx, t, controller, testRevision()) - if len(rev.Status.ImageDigests) != 0 { - t.Errorf("revision status does not have imageDigests") - } - // Flag is enabled and spec contains multiple container so rev status should contain imageDigests field - rev = getRev(ctx, t, controller, testRevisionForMultipleContainer()) - if len(rev.Status.ImageDigests) == 0 { - t.Errorf("revision status does not have imageDigests") - } -} + }}) -func getRev(ctx context.Context, t *testing.T, controller *controller.Impl, rev *v1.Revision) *v1.Revision { - revClient := fakeservingclient.Get(ctx).ServingV1().Revisions(testNamespace) + rev := testRevision(corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "gcr.io/repo/image", + }, { + Image: "docker.io/repo/image", + }}, + }) createRevision(t, ctx, controller, rev) - createdRev, err := revClient.Get(rev.Name, metav1.GetOptions{}) + revClient := fakeservingclient.Get(ctx).ServingV1().Revisions(testNamespace) + rev, err := revClient.Get(rev.Name, metav1.GetOptions{}) if err != nil { - t.Fatalf("couldn't get revision: %v", err) + t.Fatalf("Couldn't get revision: %v", err) + } + if len(rev.Status.ImageDigests) < 2 { + t.Errorf("Revision status does not have imageDigests") } - return createdRev } // TODO(mattmoor): Remove when we have coverage of EnqueueEndpointsRevision func TestMarkRevReadyUponEndpointBecomesReady(t *testing.T) { ctx, cancel, _, ctl, _ := newTestController(t) defer cancel() - rev := testRevision() + rev := testRevision(getPodSpec()) fakeRecorder := controller.GetEventRecorder(ctx).(*record.FakeRecorder) @@ -444,7 +440,7 @@ func TestNoQueueSidecarImageUpdateFail(t *testing.T) { ctx, cancel, _, controller, watcher := newTestController(t) defer cancel() - rev := testRevision() + rev := testRevision(getPodSpec()) config := testConfiguration() rev.OwnerReferences = append( rev.OwnerReferences, @@ -539,7 +535,7 @@ func TestGlobalResyncOnConfigMapUpdateRevision(t *testing.T) { servingClient := fakeservingclient.Get(ctx) - rev := testRevision() + rev := testRevision(getPodSpec()) revClient := servingClient.ServingV1().Revisions(rev.Namespace) h := NewHooks() @@ -679,7 +675,7 @@ func TestGlobalResyncOnConfigMapUpdateDeployment(t *testing.T) { kubeClient := fakekubeclient.Get(ctx) - rev := testRevision() + rev := testRevision(getPodSpec()) revClient := fakeservingclient.Get(ctx).ServingV1().Revisions(rev.Namespace) h := NewHooks() h.OnUpdate(&kubeClient.Fake, "deployments", test.callback(t)) diff --git a/pkg/reconciler/revision/table_test.go b/pkg/reconciler/revision/table_test.go index 64aa520751c5..6706f36212ca 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -85,7 +85,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "first-reconcile", // The first reconciliation Populates the following status properties. - WithLogURL, AllUnknownConditions, MarkDeploying("Deploying")), + WithLogURL, AllUnknownConditions, MarkDeploying("Deploying"), WithImageDigests), }}, Key: "foo/first-reconcile", }, { @@ -108,7 +108,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "update-status-failure", // Despite failure, the following status properties are set. - WithLogURL, AllUnknownConditions, MarkDeploying("Deploying")), + WithLogURL, AllUnknownConditions, MarkDeploying("Deploying"), WithImageDigests), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for %q: %v", @@ -136,7 +136,7 @@ func TestReconcile(t *testing.T) { Object: rev("foo", "create-pa-failure", // Despite failure, the following status properties are set. WithLogURL, WithInitRevConditions, - MarkDeploying("Deploying")), + MarkDeploying("Deploying"), WithImageDigests), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", `failed to create PA "create-pa-failure": inducing failure for create podautoscalers`), @@ -162,7 +162,7 @@ func TestReconcile(t *testing.T) { Object: rev("foo", "create-user-deploy-failure", // Despite failure, the following status properties are set. WithLogURL, WithInitRevConditions, - MarkDeploying("Deploying")), + MarkDeploying("Deploying"), WithImageDigests), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", @@ -176,7 +176,7 @@ func TestReconcile(t *testing.T) { // state (immediately post-creation), and verify that no changes // are necessary. Objects: []runtime.Object{ - rev("foo", "stable-reconcile", WithLogURL, AllUnknownConditions), + rev("foo", "stable-reconcile", WithLogURL, AllUnknownConditions, WithImageDigests), pa("foo", "stable-reconcile", WithReachability(asv1a1.ReachabilityUnknown)), deploy(t, "foo", "stable-reconcile"), @@ -190,7 +190,7 @@ func TestReconcile(t *testing.T) { // with our desired spec. Objects: []runtime.Object{ rev("foo", "fix-containers", - WithLogURL, AllUnknownConditions), + WithLogURL, AllUnknownConditions, WithImageDigests), pa("foo", "fix-containers", WithReachability(asv1a1.ReachabilityUnknown)), changeContainers(deploy(t, "foo", "fix-containers")), image("foo", "fix-containers"), @@ -208,7 +208,7 @@ func TestReconcile(t *testing.T) { }, Objects: []runtime.Object{ rev("foo", "failure-update-deploy", - withK8sServiceName("whateves"), WithLogURL, AllUnknownConditions), + withK8sServiceName("whateves"), WithLogURL, AllUnknownConditions, WithImageDigests), pa("foo", "failure-update-deploy"), changeContainers(deploy(t, "foo", "failure-update-deploy")), image("foo", "failure-update-deploy"), @@ -229,7 +229,7 @@ func TestReconcile(t *testing.T) { Objects: []runtime.Object{ rev("foo", "stable-deactivation", WithLogURL, MarkRevisionReady, - MarkInactive("NoTraffic", "This thing is inactive.")), + MarkInactive("NoTraffic", "This thing is inactive."), WithImageDigests), pa("foo", "stable-deactivation", WithNoTraffic("NoTraffic", "This thing is inactive.")), deploy(t, "foo", "stable-deactivation"), @@ -250,7 +250,7 @@ func TestReconcile(t *testing.T) { WithLogURL, // When the endpoint and pa are ready, then we will see the // Revision become ready. - MarkRevisionReady), + MarkRevisionReady, WithImageDigests), }}, WantEvents: []string{ Eventf(corev1.EventTypeNormal, "RevisionReady", "Revision becomes ready upon all resources being ready"), @@ -271,7 +271,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pa-not-ready", - WithLogURL, MarkRevisionReady, + WithLogURL, MarkRevisionReady, WithImageDigests, withK8sServiceName("its-not-confidential"), // When we reconcile a ready state and our pa is in an activating // state, we should see the following mutation. @@ -292,7 +292,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pa-inactive", - WithLogURL, MarkRevisionReady, + WithLogURL, MarkRevisionReady, WithImageDigests, // When we reconcile an "all ready" revision when the PA // is inactive, we should see the following change. MarkInactive("NoTraffic", "This thing is inactive.")), @@ -317,7 +317,7 @@ func TestReconcile(t *testing.T) { withK8sServiceName("pa-inactive-svc"), // When we reconcile an "all ready" revision when the PA // is inactive, we should see the following change. - MarkInactive("NoTraffic", "This thing is inactive.")), + MarkInactive("NoTraffic", "This thing is inactive."), WithImageDigests), }}, Key: "foo/pa-inactive", }, { @@ -338,7 +338,7 @@ func TestReconcile(t *testing.T) { WithLogURL, AllUnknownConditions, // When our reconciliation has to change the service // we should see the following mutations to status. - withK8sServiceName("fix-mutated-pa"), WithLogURL, MarkRevisionReady), + withK8sServiceName("fix-mutated-pa"), WithLogURL, MarkRevisionReady, WithImageDigests), }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: pa("foo", "fix-mutated-pa", WithTraffic, @@ -351,7 +351,7 @@ func TestReconcile(t *testing.T) { Objects: []runtime.Object{ rev("foo", "fix-mutated-pa-fail", withK8sServiceName("some-old-stuff"), - WithLogURL, AllUnknownConditions), + WithLogURL, AllUnknownConditions, WithImageDigests), pa("foo", "fix-mutated-pa-fail", WithProtocolType(networking.ProtocolH2C), WithReachability(asv1a1.ReachabilityUnknown)), deploy(t, "foo", "fix-mutated-pa-fail"), image("foo", "fix-mutated-pa-fail"), @@ -386,7 +386,7 @@ func TestReconcile(t *testing.T) { WithLogURL, AllUnknownConditions, // When the revision is reconciled after a Deployment has // timed out, we should see it marked with the PDE state. - MarkProgressDeadlineExceeded("I timed out!")), + MarkProgressDeadlineExceeded("I timed out!"), WithImageDigests), }}, Key: "foo/deploy-timeout", }, { @@ -407,7 +407,7 @@ func TestReconcile(t *testing.T) { WithLogURL, AllUnknownConditions, // When the revision is reconciled after a Deployment has // timed out, we should see it marked with the FailedCreate state. - MarkResourcesUnavailable("FailedCreate", "I replica failed!")), + MarkResourcesUnavailable("FailedCreate", "I replica failed!"), WithImageDigests), }}, Key: "foo/deploy-replica-failure", }, { @@ -424,7 +424,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pull-backoff", WithLogURL, AllUnknownConditions, - MarkResourcesUnavailable("ImagePullBackoff", "can't pull it")), + MarkResourcesUnavailable("ImagePullBackoff", "can't pull it"), WithImageDigests), }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: pa("foo", "pull-backoff", WithReachability(asv1a1.ReachabilityUnreachable)), @@ -446,7 +446,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pod-error", - WithLogURL, AllUnknownConditions, MarkContainerExiting(5, v1.RevisionContainerExitingMessage("I failed man!"))), + WithLogURL, AllUnknownConditions, MarkContainerExiting(5, v1.RevisionContainerExitingMessage("I failed man!")), WithImageDigests), }}, Key: "foo/pod-error", }, { @@ -464,7 +464,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pod-schedule-error", - WithLogURL, AllUnknownConditions, MarkResourcesUnavailable("Insufficient energy", "Unschedulable")), + WithLogURL, AllUnknownConditions, MarkResourcesUnavailable("Insufficient energy", "Unschedulable"), WithImageDigests), }}, Key: "foo/pod-schedule-error", }, { @@ -484,7 +484,7 @@ func TestReconcile(t *testing.T) { Object: rev("foo", "steady-ready", withK8sServiceName("steadier-even"), WithLogURL, // All resources are ready to go, we should see the revision being // marked ready - MarkRevisionReady), + MarkRevisionReady, WithImageDigests), }}, WantEvents: []string{ Eventf(corev1.EventTypeNormal, "RevisionReady", "Revision becomes ready upon all resources being ready"), @@ -504,7 +504,7 @@ func TestReconcile(t *testing.T) { Object: rev("foo", "missing-owners", withK8sServiceName("lesser-revision"), WithLogURL, MarkRevisionReady, // When we're missing the OwnerRef for PodAutoscaler we see this update. - MarkResourceNotOwned("PodAutoscaler", "missing-owners")), + MarkResourceNotOwned("PodAutoscaler", "missing-owners"), WithImageDigests), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", `revision: "missing-owners" does not own PodAutoscaler: "missing-owners"`), @@ -524,7 +524,7 @@ func TestReconcile(t *testing.T) { Object: rev("foo", "missing-owners", withK8sServiceName("youre-gonna-lose"), WithLogURL, MarkRevisionReady, // When we're missing the OwnerRef for Deployment we see this update. - MarkResourceNotOwned("Deployment", "missing-owners-deployment")), + MarkResourceNotOwned("Deployment", "missing-owners-deployment"), WithImageDigests), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", `revision: "missing-owners" does not own Deployment: "missing-owners-deployment"`), @@ -544,7 +544,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "image-pull-secrets", WithImagePullSecrets("foo-secret"), - WithLogURL, AllUnknownConditions, MarkDeploying("Deploying")), + WithLogURL, AllUnknownConditions, MarkDeploying("Deploying"), WithImageDigests), }}, Key: "foo/image-pull-secrets", }} diff --git a/pkg/testing/v1/revision.go b/pkg/testing/v1/revision.go index 40a5594704f4..1cf239a4a7eb 100644 --- a/pkg/testing/v1/revision.go +++ b/pkg/testing/v1/revision.go @@ -182,3 +182,10 @@ func WithRevisionLabel(key, value string) RevisionOption { config.Labels[key] = value } } + +// WithImageDigests sets the .Status.ImageDigests to the Revision. +func WithImageDigests(r *v1.Revision) { + r.Status.ImageDigests = map[string]string{ + "user-container": "", + } +} diff --git a/test/conformance/api/v1/util.go b/test/conformance/api/v1/util.go index 5efc7ad19c8b..1c2a7f126cc3 100644 --- a/test/conformance/api/v1/util.go +++ b/test/conformance/api/v1/util.go @@ -222,11 +222,11 @@ func validateControlPlane(t pkgTest.T, clients *test.Clients, names test.Resourc if ready, err := v1test.IsRevisionReady(r); !ready { return false, fmt.Errorf("revision %s did not become ready to serve traffic: %w", names.Revision, err) } - if r.Status.ImageDigest == "" { + if r.Status.DeprecatedImageDigest == "" { return false, fmt.Errorf("imageDigest not present for revision %s", names.Revision) } - if validDigest, err := validateImageDigest(names.Image, r.Status.ImageDigest); !validDigest { - return false, fmt.Errorf("imageDigest %s is not valid for imageName %s: %w", r.Status.ImageDigest, names.Image, err) + if validDigest, err := validateImageDigest(names.Image, r.Status.DeprecatedImageDigest); !validDigest { + return false, fmt.Errorf("imageDigest %s is not valid for imageName %s: %w", r.Status.DeprecatedImageDigest, names.Image, err) } return true, nil }) diff --git a/test/conformance/api/v1alpha1/util.go b/test/conformance/api/v1alpha1/util.go index 138980e64017..a1ea731249a2 100644 --- a/test/conformance/api/v1alpha1/util.go +++ b/test/conformance/api/v1alpha1/util.go @@ -222,11 +222,11 @@ func validateRunLatestControlPlane(t pkgTest.T, clients *test.Clients, names tes if ready, err := v1a1test.IsRevisionReady(r); !ready { return false, fmt.Errorf("revision %s did not become ready to serve traffic: %w", names.Revision, err) } - if r.Status.ImageDigest == "" { + if r.Status.DeprecatedImageDigest == "" { return false, fmt.Errorf("imageDigest not present for revision %s", names.Revision) } - if validDigest, err := validateImageDigest(names.Image, r.Status.ImageDigest); !validDigest { - return false, fmt.Errorf("imageDigest %s is not valid for imageName %s: %w", r.Status.ImageDigest, names.Image, err) + if validDigest, err := validateImageDigest(names.Image, r.Status.DeprecatedImageDigest); !validDigest { + return false, fmt.Errorf("imageDigest %s is not valid for imageName %s: %w", r.Status.DeprecatedImageDigest, names.Image, err) } return true, nil }) diff --git a/test/conformance/api/v1beta1/util.go b/test/conformance/api/v1beta1/util.go index 923c211ae47f..6add838179c3 100644 --- a/test/conformance/api/v1beta1/util.go +++ b/test/conformance/api/v1beta1/util.go @@ -222,11 +222,11 @@ func validateControlPlane(t pkgTest.T, clients *test.Clients, names test.Resourc if ready, err := v1b1test.IsRevisionReady(r); !ready { return false, fmt.Errorf("revision %s did not become ready to serve traffic: %w", names.Revision, err) } - if r.Status.ImageDigest == "" { + if r.Status.DeprecatedImageDigest == "" { return false, fmt.Errorf("imageDigest not present for revision %s", names.Revision) } - if validDigest, err := validateImageDigest(names.Image, r.Status.ImageDigest); !validDigest { - return false, fmt.Errorf("imageDigest %s is not valid for imageName %s: %w", r.Status.ImageDigest, names.Image, err) + if validDigest, err := validateImageDigest(names.Image, r.Status.DeprecatedImageDigest); !validDigest { + return false, fmt.Errorf("imageDigest %s is not valid for imageName %s: %w", r.Status.DeprecatedImageDigest, names.Image, err) } return true, nil }) From b2230a456527df132201140ce4646f90166774fc Mon Sep 17 00:00:00 2001 From: savitaashture Date: Fri, 27 Mar 2020 12:19:37 +0530 Subject: [PATCH 03/13] Modified reconcileDigest to run parallel --- pkg/reconciler/revision/revision.go | 45 +++++++++++++++++------- pkg/reconciler/revision/revision_test.go | 11 ++++++ pkg/reconciler/revision/table_test.go | 2 +- 3 files changed, 45 insertions(+), 13 deletions(-) diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 19e440a3b342..87da0358280d 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -22,6 +22,7 @@ import ( "strings" "github.com/google/go-containerregistry/pkg/authn/k8schain" + "golang.org/x/sync/errgroup" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" @@ -81,20 +82,40 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro if rev.Status.ImageDigests == nil { rev.Status.ImageDigests = make(map[string]string, len(rev.Spec.Containers)) } - for _, container := range rev.Spec.Containers { - 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 + + var digestGrp errgroup.Group + type digestData struct { + digestValue string + containerName string + containerPort int + } + + digests := make(chan *digestData, len(rev.Spec.Containers)) + digestGrp.Go(func() error { + for _, container := range rev.Spec.Containers { + container = container // Standard Go concurrency pattern. + 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 + } + digests <- &digestData{digestValue: digest, containerName: container.Name, containerPort: len(container.Ports)} } - if len(rev.Spec.Containers) == 1 || len(container.Ports) != 0 { - rev.Status.DeprecatedImageDigest = digest + return nil + }) + if err := digestGrp.Wait(); err != nil { + return err + } + close(digests) + for v := range digests { + if len(rev.Spec.Containers) == 1 || v.containerPort != 0 { + rev.Status.DeprecatedImageDigest = v.digestValue } - rev.Status.ImageDigests[container.Name] = digest + rev.Status.ImageDigests[v.containerName] = v.digestValue } return nil } diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index 8355d7521ea8..a4d7efdc4701 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -355,6 +355,17 @@ func TestRevWithImageDigests(t *testing.T) { rev := testRevision(corev1.PodSpec{ Containers: []corev1.Container{{ Image: "gcr.io/repo/image", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + ReadinessProbe: &corev1.Probe{ + Handler: corev1.Handler{}, + InitialDelaySeconds: 0, + TimeoutSeconds: 0, + PeriodSeconds: 0, + SuccessThreshold: 0, + FailureThreshold: 0, + }, }, { Image: "docker.io/repo/image", }}, diff --git a/pkg/reconciler/revision/table_test.go b/pkg/reconciler/revision/table_test.go index 6706f36212ca..077f60572198 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -515,7 +515,7 @@ func TestReconcile(t *testing.T) { WantErr: true, Objects: []runtime.Object{ rev("foo", "missing-owners", withK8sServiceName("youre-gonna-lose"), WithLogURL, - MarkRevisionReady), + MarkRevisionReady, WithImageDigests), pa("foo", "missing-owners", WithTraffic), noOwner(deploy(t, "foo", "missing-owners")), image("foo", "missing-owners"), From 240c908ccdef2b7d3cc65dba762fb4296442f583 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Fri, 27 Mar 2020 15:11:04 +0530 Subject: [PATCH 04/13] fix review comment --- pkg/reconciler/revision/revision.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 87da0358280d..e706afdfe3fb 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -85,12 +85,12 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro var digestGrp errgroup.Group type digestData struct { - digestValue string - containerName string - containerPort int + digestValue string + containerName string + isServingContainer bool } - digests := make(chan *digestData, len(rev.Spec.Containers)) + digests := make(chan digestData, len(rev.Spec.Containers)) digestGrp.Go(func() error { for _, container := range rev.Spec.Containers { container = container // Standard Go concurrency pattern. @@ -103,7 +103,7 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro container.Image, err.Error())) return err } - digests <- &digestData{digestValue: digest, containerName: container.Name, containerPort: len(container.Ports)} + digests <- digestData{digestValue: digest, containerName: container.Name, isServingContainer: len(container.Ports) != 0} } return nil }) @@ -112,7 +112,7 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro } close(digests) for v := range digests { - if len(rev.Spec.Containers) == 1 || v.containerPort != 0 { + if len(rev.Spec.Containers) == 1 || v.isServingContainer { rev.Status.DeprecatedImageDigest = v.digestValue } rev.Status.ImageDigests[v.containerName] = v.digestValue From f2e77cb62621f658db2a0c505b106da3ddc388c2 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Sat, 28 Mar 2020 00:55:12 +0530 Subject: [PATCH 05/13] fix review comment --- pkg/reconciler/revision/revision.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index e706afdfe3fb..8485b0234571 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -91,9 +91,9 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro } digests := make(chan digestData, len(rev.Spec.Containers)) - digestGrp.Go(func() error { - for _, container := range rev.Spec.Containers { - container = container // Standard Go concurrency pattern. + for _, container := range rev.Spec.Containers { + container := container // Standard Go concurrency pattern. + digestGrp.Go(func() error { digest, err := c.resolver.Resolve(container.Image, opt, cfgs.Deployment.RegistriesSkippingTagResolving) if err != nil { @@ -104,9 +104,9 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro return err } digests <- digestData{digestValue: digest, containerName: container.Name, isServingContainer: len(container.Ports) != 0} - } - return nil - }) + return nil + }) + } if err := digestGrp.Wait(); err != nil { return err } From 3cdfdd66a160f681690623860ef875132b7f810f Mon Sep 17 00:00:00 2001 From: savitaashture Date: Sat, 28 Mar 2020 10:22:56 +0530 Subject: [PATCH 06/13] fix review comment --- pkg/reconciler/revision/revision_test.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index a4d7efdc4701..ad0b4a01d98f 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -358,14 +358,6 @@ func TestRevWithImageDigests(t *testing.T) { Ports: []corev1.ContainerPort{{ ContainerPort: 8888, }}, - ReadinessProbe: &corev1.Probe{ - Handler: corev1.Handler{}, - InitialDelaySeconds: 0, - TimeoutSeconds: 0, - PeriodSeconds: 0, - SuccessThreshold: 0, - FailureThreshold: 0, - }, }, { Image: "docker.io/repo/image", }}, From 0710ebeb106fd2bb4f3a31822a5631510d4244fc Mon Sep 17 00:00:00 2001 From: savitaashture Date: Mon, 6 Apr 2020 20:04:57 +0530 Subject: [PATCH 07/13] updated code to have imageDigests field for old revisions --- pkg/reconciler/revision/revision.go | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 8485b0234571..f8167712782b 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -64,9 +64,20 @@ type Reconciler struct { var _ revisionreconciler.Interface = (*Reconciler)(nil) func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) error { - // The image digest has already been resolved. - if rev.Status.DeprecatedImageDigest != "" || len(rev.Status.ImageDigests) == len(rev.Spec.Containers) { - return nil + if rev.Status.ImageDigests == nil { + rev.Status.ImageDigests = make(map[string]string, len(rev.Spec.Containers)) + } + + if rev.Status.DeprecatedImageDigest != "" { + // The image digest has already been resolved. + if len(rev.Status.ImageDigests) == len(rev.Spec.Containers) { + return nil + } + // Default old revisions to have ImageDigests filled in. + // This path should only be taken by "old" revisions that have exactly one container. + if len(rev.Status.ImageDigests) == 0 { + rev.Status.ImageDigests[rev.Spec.Containers[0].Name] = rev.Status.DeprecatedImageDigest + } } var imagePullSecrets []string @@ -79,9 +90,6 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro ServiceAccountName: rev.Spec.ServiceAccountName, ImagePullSecrets: imagePullSecrets, } - if rev.Status.ImageDigests == nil { - rev.Status.ImageDigests = make(map[string]string, len(rev.Spec.Containers)) - } var digestGrp errgroup.Group type digestData struct { @@ -103,7 +111,11 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro container.Image, err.Error())) return err } - digests <- digestData{digestValue: digest, containerName: container.Name, isServingContainer: len(container.Ports) != 0} + digests <- digestData{ + digestValue: digest, + containerName: container.Name, + isServingContainer: len(rev.Spec.Containers) == 1 || len(container.Ports) != 0, + } return nil }) } @@ -112,7 +124,7 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro } close(digests) for v := range digests { - if len(rev.Spec.Containers) == 1 || v.isServingContainer { + if v.isServingContainer { rev.Status.DeprecatedImageDigest = v.digestValue } rev.Status.ImageDigests[v.containerName] = v.digestValue From 1a6947c9794093ac9e97444e2f49b10272629d9d Mon Sep 17 00:00:00 2001 From: savitaashture Date: Mon, 6 Apr 2020 23:18:00 +0530 Subject: [PATCH 08/13] Updated unit testcase --- pkg/reconciler/revision/revision_test.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index ad0b4a01d98f..c837f0281245 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -369,7 +369,18 @@ func TestRevWithImageDigests(t *testing.T) { t.Fatalf("Couldn't get revision: %v", err) } if len(rev.Status.ImageDigests) < 2 { - t.Errorf("Revision status does not have imageDigests") + t.Error("Revision status does not have imageDigests") + } + + rev.Status.DeprecatedImageDigest = "gcr.io/repo/image" + updateRevision(t, ctx, controller, rev) + if len(rev.Spec.Containers) != len(rev.Status.ImageDigests) { + t.Error("Image digests does not match with the provided containers") + } + rev.Status.ImageDigests = map[string]string{} + updateRevision(t, ctx, controller, rev) + if len(rev.Status.ImageDigests) != 0 { + t.Error("Failed to update revision") } } From 4c31408c92c883982841f912ea110bccbf558eb8 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Wed, 8 Apr 2020 12:15:25 +0530 Subject: [PATCH 09/13] Fix review comment --- pkg/apis/serving/v1/revision_types.go | 11 +++++++---- pkg/apis/serving/v1alpha1/revision_types.go | 11 +++++++---- pkg/reconciler/revision/revision.go | 18 ++++++++++++++---- 3 files changed, 28 insertions(+), 12 deletions(-) diff --git a/pkg/apis/serving/v1/revision_types.go b/pkg/apis/serving/v1/revision_types.go index 604287bd8785..28c3be97913d 100644 --- a/pkg/apis/serving/v1/revision_types.go +++ b/pkg/apis/serving/v1/revision_types.go @@ -134,14 +134,17 @@ type RevisionStatus struct { // of Revision. This field holds the digest value regardless of whether // a tag or digest was originally specified in the Container object. It // may be empty if the image comes from a registry listed to skip resolution. + // If multiple containers specified then DeprecatedImageDigest holds the digest + // for serving container. // DEPRECATED Use ImageDigests instead. - // TODO(savitaashture) Remove deprecatedImageDigest in 2 releases(0.16). + // TODO(savitaashture) Remove deprecatedImageDigest. + // ref https://kubernetes.io/docs/reference/using-api/deprecation-policy for deprecation. // +optional DeprecatedImageDigest string `json:"imageDigest,omitempty"` - // ImageDigests holds the resolved digest for the specified images - // within .Spec.Container.Image. The digest is resolved during the creation - // of Revision. ImageDigests holds the digest for all the .Spec.Container.Image both serving and non serving. + // ImageDigests is a mapping of images present in .Spec.Container[*].Image + // to their respective digests. The digests are resolved during the creation + // of Revision. ImageDigests holds the digests for both serving and non serving container. // ref: http://bit.ly/image-digests // +optional ImageDigests map[string]string `json:"imageDigests,omitempty"` diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index 507fa2eef830..42515ffc17ba 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/pkg/apis/serving/v1alpha1/revision_types.go @@ -190,14 +190,17 @@ type RevisionStatus struct { // of Revision. This field holds the digest value regardless of whether // a tag or digest was originally specified in the Container object. It // may be empty if the image comes from a registry listed to skip resolution. + // If multiple containers specified then DeprecatedImageDigest holds the digest + // for serving container. // DEPRECATED Use ImageDigests instead. - // TODO(savitaashture) Remove deprecatedImageDigest in 2 releases(0.16).. + // TODO(savitaashture) Remove deprecatedImageDigest. + // ref https://kubernetes.io/docs/reference/using-api/deprecation-policy for deprecation. // +optional DeprecatedImageDigest string `json:"imageDigest,omitempty"` - // ImageDigests holds the resolved digest for the image specified - // within .Spec.Container.Image. The digest is resolved during the creation - // of Revision. ImageDigests holds the digest for all the .Spec.Container.Image both serving and non serving. + // ImageDigests is a mapping of images present in .Spec.Container[*].Image + // to their respective digests. The digests are resolved during the creation + // of Revision. ImageDigests holds the digests for both serving and non serving container. // ref: http://bit.ly/image-digests // +optional ImageDigests map[string]string `json:"imageDigests,omitempty"` diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index f8167712782b..3a207203ca97 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -96,6 +96,7 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro digestValue string containerName string isServingContainer bool + image string } digests := make(chan digestData, len(rev.Spec.Containers)) @@ -106,9 +107,7 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro 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())) + digests <- digestData{image: container.Image} return err } digests <- digestData{ @@ -119,10 +118,21 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro return nil }) } + + var setError error if err := digestGrp.Wait(); err != nil { - return err + setError = err } close(digests) + if setError != nil { + for v := range digests { + fmt.Println("Inside for loop", v) + rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, + v1.RevisionContainerMissingMessage( + v.image, setError.Error())) + } + return setError + } for v := range digests { if v.isServingContainer { rev.Status.DeprecatedImageDigest = v.digestValue From ddd8901b149fb491e641d8d32ac2a32152a96b27 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Wed, 8 Apr 2020 20:12:27 +0530 Subject: [PATCH 10/13] fix review commant for adding error to struct --- pkg/reconciler/revision/revision.go | 36 ++++++++++++++--------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 3a207203ca97..f2e6e296525b 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -97,6 +97,7 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro containerName string isServingContainer bool image string + digestError error } digests := make(chan digestData, len(rev.Spec.Containers)) @@ -107,37 +108,34 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro opt, cfgs.Deployment.RegistriesSkippingTagResolving) if err != nil { err = fmt.Errorf("failed to resolve image to digest: %w", err) - digests <- digestData{image: container.Image} - return err - } - digests <- digestData{ - digestValue: digest, - containerName: container.Name, - isServingContainer: len(rev.Spec.Containers) == 1 || len(container.Ports) != 0, + digests <- digestData{ + image: container.Image, + digestError: err, + } + } else { + digests <- digestData{ + digestValue: digest, + containerName: container.Name, + isServingContainer: len(rev.Spec.Containers) == 1 || len(container.Ports) != 0, + } } return nil }) } - - var setError error - if err := digestGrp.Wait(); err != nil { - setError = err - } + digestGrp.Wait() close(digests) - if setError != nil { - for v := range digests { - fmt.Println("Inside for loop", v) + for v := range digests { + if v.digestError != nil { rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, v1.RevisionContainerMissingMessage( - v.image, setError.Error())) + v.image, v.digestError.Error())) + return v.digestError } - return setError - } - for v := range digests { if v.isServingContainer { rev.Status.DeprecatedImageDigest = v.digestValue } rev.Status.ImageDigests[v.containerName] = v.digestValue + } return nil } From 9a2a047bc1cf88a4e2eb81f793544c55861f59b9 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Sat, 11 Apr 2020 03:10:07 +0530 Subject: [PATCH 11/13] Chnaged type of ImageDigests field from map to struct --- pkg/apis/serving/v1/revision_types.go | 8 ++++- pkg/apis/serving/v1/zz_generated.deepcopy.go | 22 ++++++++++--- .../serving/v1alpha1/revision_conversion.go | 22 +++++++++++-- .../v1alpha1/revision_conversion_test.go | 10 +++--- pkg/apis/serving/v1alpha1/revision_types.go | 8 ++++- .../serving/v1alpha1/zz_generated.deepcopy.go | 22 ++++++++++--- pkg/reconciler/revision/revision.go | 31 ++++++++++++++++--- pkg/reconciler/revision/revision_test.go | 2 +- pkg/testing/v1/revision.go | 7 +++-- 9 files changed, 108 insertions(+), 24 deletions(-) diff --git a/pkg/apis/serving/v1/revision_types.go b/pkg/apis/serving/v1/revision_types.go index 28c3be97913d..c68e76fdaa5f 100644 --- a/pkg/apis/serving/v1/revision_types.go +++ b/pkg/apis/serving/v1/revision_types.go @@ -147,7 +147,13 @@ type RevisionStatus struct { // of Revision. ImageDigests holds the digests for both serving and non serving container. // ref: http://bit.ly/image-digests // +optional - ImageDigests map[string]string `json:"imageDigests,omitempty"` + ImageDigests []ImageDigests `json:"imageDigests,omitempty"` +} + +// ImageDigests holds the information of container name and digest value +type ImageDigests struct { + ContainerName string `json:"containerName,omitempty"` + ImageDigest string `json:"imageDigest,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/pkg/apis/serving/v1/zz_generated.deepcopy.go b/pkg/apis/serving/v1/zz_generated.deepcopy.go index ae7f28d4e883..7c17ce18b253 100644 --- a/pkg/apis/serving/v1/zz_generated.deepcopy.go +++ b/pkg/apis/serving/v1/zz_generated.deepcopy.go @@ -138,6 +138,22 @@ func (in *ConfigurationStatusFields) DeepCopy() *ConfigurationStatusFields { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ImageDigests) DeepCopyInto(out *ImageDigests) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ImageDigests. +func (in *ImageDigests) DeepCopy() *ImageDigests { + if in == nil { + return nil + } + out := new(ImageDigests) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Revision) DeepCopyInto(out *Revision) { *out = *in @@ -232,10 +248,8 @@ func (in *RevisionStatus) DeepCopyInto(out *RevisionStatus) { in.Status.DeepCopyInto(&out.Status) if in.ImageDigests != nil { in, out := &in.ImageDigests, &out.ImageDigests - *out = make(map[string]string, len(*in)) - for key, val := range *in { - (*out)[key] = val - } + *out = make([]ImageDigests, len(*in)) + copy(*out, *in) } return } diff --git a/pkg/apis/serving/v1alpha1/revision_conversion.go b/pkg/apis/serving/v1alpha1/revision_conversion.go index 58c9f4453a70..2ca88a901af5 100644 --- a/pkg/apis/serving/v1alpha1/revision_conversion.go +++ b/pkg/apis/serving/v1alpha1/revision_conversion.go @@ -80,7 +80,16 @@ func (source *RevisionStatus) ConvertTo(ctx context.Context, sink *v1.RevisionSt sink.ServiceName = source.ServiceName sink.LogURL = source.LogURL sink.DeprecatedImageDigest = source.DeprecatedImageDigest - sink.ImageDigests = source.ImageDigests + sink.ImageDigests = make([]v1.ImageDigests, len(source.ImageDigests)) + for i := range source.ImageDigests { + source.ImageDigests[i].ConvertTo(&sink.ImageDigests[i]) + } +} + +// ConvertTo helps implement apis.Convertible +func (source *ImageDigests) ConvertTo(sink *v1.ImageDigests) { + sink.ContainerName = source.ContainerName + sink.ImageDigest = source.ImageDigest } // ConvertFrom implements apis.Convertible @@ -113,5 +122,14 @@ func (sink *RevisionStatus) ConvertFrom(ctx context.Context, source v1.RevisionS sink.ServiceName = source.ServiceName sink.LogURL = source.LogURL sink.DeprecatedImageDigest = source.DeprecatedImageDigest - sink.ImageDigests = source.ImageDigests + sink.ImageDigests = make([]ImageDigests, len(source.ImageDigests)) + for i := range sink.ImageDigests { + sink.ImageDigests[i].ConvertFrom(&source.ImageDigests[i]) + } +} + +// ConvertFrom helps implement apis.Convertible +func (sink *ImageDigests) ConvertFrom(source *v1.ImageDigests) { + sink.ContainerName = source.ContainerName + sink.ImageDigest = source.ImageDigest } diff --git a/pkg/apis/serving/v1alpha1/revision_conversion_test.go b/pkg/apis/serving/v1alpha1/revision_conversion_test.go index 5e724255c5e6..fab25db79efb 100644 --- a/pkg/apis/serving/v1alpha1/revision_conversion_test.go +++ b/pkg/apis/serving/v1alpha1/revision_conversion_test.go @@ -89,8 +89,9 @@ func TestRevisionConversion(t *testing.T) { Status: "True", }}, }, - ServiceName: "foo-bar", - LogURL: "http://logger.io", + ServiceName: "foo-bar", + LogURL: "http://logger.io", + ImageDigests: []ImageDigests{}, }, }, }, { @@ -232,8 +233,9 @@ func TestRevisionConversionForMultiContainer(t *testing.T) { Status: "True", }}, }, - ServiceName: "foo-bar", - LogURL: "http://logger.io", + ServiceName: "foo-bar", + LogURL: "http://logger.io", + ImageDigests: []ImageDigests{}, }, } beta := &v1beta1.Revision{} diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index 42515ffc17ba..bf448f072533 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/pkg/apis/serving/v1alpha1/revision_types.go @@ -203,7 +203,13 @@ type RevisionStatus struct { // of Revision. ImageDigests holds the digests for both serving and non serving container. // ref: http://bit.ly/image-digests // +optional - ImageDigests map[string]string `json:"imageDigests,omitempty"` + ImageDigests []ImageDigests `json:"imageDigests,omitempty"` +} + +// ImageDigests holds the information of container name and digest value +type ImageDigests struct { + ContainerName string `json:"containerName,omitempty"` + ImageDigest string `json:"imageDigest,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go index 92bff3f318a8..a86aa6017ed2 100644 --- a/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go @@ -169,6 +169,22 @@ func (in *ConfigurationStatusFields) DeepCopy() *ConfigurationStatusFields { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ImageDigests) DeepCopyInto(out *ImageDigests) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ImageDigests. +func (in *ImageDigests) DeepCopy() *ImageDigests { + if in == nil { + return nil + } + out := new(ImageDigests) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ManualType) DeepCopyInto(out *ManualType) { *out = *in @@ -318,10 +334,8 @@ func (in *RevisionStatus) DeepCopyInto(out *RevisionStatus) { in.Status.DeepCopyInto(&out.Status) if in.ImageDigests != nil { in, out := &in.ImageDigests, &out.ImageDigests - *out = make(map[string]string, len(*in)) - for key, val := range *in { - (*out)[key] = val - } + *out = make([]ImageDigests, len(*in)) + copy(*out, *in) } return } diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index f2e6e296525b..7dc61fa98a17 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -65,7 +65,7 @@ var _ revisionreconciler.Interface = (*Reconciler)(nil) func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) error { if rev.Status.ImageDigests == nil { - rev.Status.ImageDigests = make(map[string]string, len(rev.Spec.Containers)) + rev.Status.ImageDigests = make([]v1.ImageDigests, len(rev.Spec.Containers)) } if rev.Status.DeprecatedImageDigest != "" { @@ -76,7 +76,10 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro // Default old revisions to have ImageDigests filled in. // This path should only be taken by "old" revisions that have exactly one container. if len(rev.Status.ImageDigests) == 0 { - rev.Status.ImageDigests[rev.Spec.Containers[0].Name] = rev.Status.DeprecatedImageDigest + rev.Status.ImageDigests = append(rev.Status.ImageDigests, v1.ImageDigests{ + ContainerName: rev.Spec.Containers[0].Name, + ImageDigest: rev.Status.DeprecatedImageDigest, + }) } } @@ -134,12 +137,32 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro if v.isServingContainer { rev.Status.DeprecatedImageDigest = v.digestValue } - rev.Status.ImageDigests[v.containerName] = v.digestValue - + rev.Status.ImageDigests = append(rev.Status.ImageDigests, v1.ImageDigests{ + ContainerName: v.containerName, + ImageDigest: v.digestValue, + }) } + rev.Status.ImageDigests = removeDuplicateDigests(rev.Status.ImageDigests) return nil } +func removeDuplicateDigests(imageDigests []v1.ImageDigests) []v1.ImageDigests { + digests := make(map[v1.ImageDigests]bool) + list := []v1.ImageDigests{} + for _, d := range imageDigests { + if v := digests[d]; !v { + digests[d] = true + list = append(list, d) + } + } + for i, v := range list { + if v == (v1.ImageDigests{}) { + list = append(list[:i], list[i+1:]...) + } + } + return list +} + func (c *Reconciler) ReconcileKind(ctx context.Context, rev *v1.Revision) pkgreconciler.Event { readyBeforeReconcile := rev.Status.IsReady() diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index c837f0281245..74825a0fb882 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -377,7 +377,7 @@ func TestRevWithImageDigests(t *testing.T) { if len(rev.Spec.Containers) != len(rev.Status.ImageDigests) { t.Error("Image digests does not match with the provided containers") } - rev.Status.ImageDigests = map[string]string{} + rev.Status.ImageDigests = []v1.ImageDigests{} updateRevision(t, ctx, controller, rev) if len(rev.Status.ImageDigests) != 0 { t.Error("Failed to update revision") diff --git a/pkg/testing/v1/revision.go b/pkg/testing/v1/revision.go index 1cf239a4a7eb..b5236e910186 100644 --- a/pkg/testing/v1/revision.go +++ b/pkg/testing/v1/revision.go @@ -185,7 +185,8 @@ func WithRevisionLabel(key, value string) RevisionOption { // WithImageDigests sets the .Status.ImageDigests to the Revision. func WithImageDigests(r *v1.Revision) { - r.Status.ImageDigests = map[string]string{ - "user-container": "", - } + r.Status.ImageDigests = []v1.ImageDigests{{ + ContainerName: "user-container", + ImageDigest: "", + }} } From 4b21cc1bb1de201f9e2d7265e626ac5b76ec8d00 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Fri, 17 Apr 2020 00:55:50 +0530 Subject: [PATCH 12/13] Fix review comments --- pkg/apis/serving/v1/revision_types.go | 18 +++---- pkg/apis/serving/v1/zz_generated.deepcopy.go | 14 +++--- .../serving/v1alpha1/revision_conversion.go | 20 ++++---- .../v1alpha1/revision_conversion_test.go | 12 ++--- pkg/apis/serving/v1alpha1/revision_types.go | 18 +++---- .../serving/v1alpha1/zz_generated.deepcopy.go | 14 +++--- pkg/reconciler/revision/revision.go | 47 ++++++------------- pkg/reconciler/revision/revision_test.go | 8 ++-- pkg/testing/v1/revision.go | 5 +- 9 files changed, 71 insertions(+), 85 deletions(-) diff --git a/pkg/apis/serving/v1/revision_types.go b/pkg/apis/serving/v1/revision_types.go index c68e76fdaa5f..f8bdc77d66e7 100644 --- a/pkg/apis/serving/v1/revision_types.go +++ b/pkg/apis/serving/v1/revision_types.go @@ -142,18 +142,20 @@ type RevisionStatus struct { // +optional DeprecatedImageDigest string `json:"imageDigest,omitempty"` - // ImageDigests is a mapping of images present in .Spec.Container[*].Image - // to their respective digests. The digests are resolved during the creation - // of Revision. ImageDigests holds the digests for both serving and non serving container. + // ContainerStatuses is a slice of images present in .Spec.Container[*].Image + // to their respective digests and their container name. + // The digests are resolved during the creation of Revision. + // ContainerStatuses holds the container name and image digests + // for both serving and non serving container. // ref: http://bit.ly/image-digests // +optional - ImageDigests []ImageDigests `json:"imageDigests,omitempty"` + ContainerStatuses []ContainerStatuses `json:"containerStatuses,omitempty"` } -// ImageDigests holds the information of container name and digest value -type ImageDigests struct { - ContainerName string `json:"containerName,omitempty"` - ImageDigest string `json:"imageDigest,omitempty"` +// ContainerStatuses holds the information of container name and image digest value +type ContainerStatuses struct { + Name string `json:"name,omitempty"` + ImageDigest string `json:"imageDigest,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/pkg/apis/serving/v1/zz_generated.deepcopy.go b/pkg/apis/serving/v1/zz_generated.deepcopy.go index 7c17ce18b253..a8d09cb102bd 100644 --- a/pkg/apis/serving/v1/zz_generated.deepcopy.go +++ b/pkg/apis/serving/v1/zz_generated.deepcopy.go @@ -139,17 +139,17 @@ func (in *ConfigurationStatusFields) DeepCopy() *ConfigurationStatusFields { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ImageDigests) DeepCopyInto(out *ImageDigests) { +func (in *ContainerStatuses) DeepCopyInto(out *ContainerStatuses) { *out = *in return } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ImageDigests. -func (in *ImageDigests) DeepCopy() *ImageDigests { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ContainerStatuses. +func (in *ContainerStatuses) DeepCopy() *ContainerStatuses { if in == nil { return nil } - out := new(ImageDigests) + out := new(ContainerStatuses) in.DeepCopyInto(out) return out } @@ -246,9 +246,9 @@ func (in *RevisionSpec) DeepCopy() *RevisionSpec { func (in *RevisionStatus) DeepCopyInto(out *RevisionStatus) { *out = *in in.Status.DeepCopyInto(&out.Status) - if in.ImageDigests != nil { - in, out := &in.ImageDigests, &out.ImageDigests - *out = make([]ImageDigests, len(*in)) + if in.ContainerStatuses != nil { + in, out := &in.ContainerStatuses, &out.ContainerStatuses + *out = make([]ContainerStatuses, len(*in)) copy(*out, *in) } return diff --git a/pkg/apis/serving/v1alpha1/revision_conversion.go b/pkg/apis/serving/v1alpha1/revision_conversion.go index 2ca88a901af5..9487885a7d32 100644 --- a/pkg/apis/serving/v1alpha1/revision_conversion.go +++ b/pkg/apis/serving/v1alpha1/revision_conversion.go @@ -80,15 +80,15 @@ func (source *RevisionStatus) ConvertTo(ctx context.Context, sink *v1.RevisionSt sink.ServiceName = source.ServiceName sink.LogURL = source.LogURL sink.DeprecatedImageDigest = source.DeprecatedImageDigest - sink.ImageDigests = make([]v1.ImageDigests, len(source.ImageDigests)) - for i := range source.ImageDigests { - source.ImageDigests[i].ConvertTo(&sink.ImageDigests[i]) + sink.ContainerStatuses = make([]v1.ContainerStatuses, len(source.ContainerStatuses)) + for i := range source.ContainerStatuses { + source.ContainerStatuses[i].ConvertTo(ctx, &sink.ContainerStatuses[i]) } } // ConvertTo helps implement apis.Convertible -func (source *ImageDigests) ConvertTo(sink *v1.ImageDigests) { - sink.ContainerName = source.ContainerName +func (source *ContainerStatuses) ConvertTo(ctx context.Context, sink *v1.ContainerStatuses) { + sink.Name = source.Name sink.ImageDigest = source.ImageDigest } @@ -122,14 +122,14 @@ func (sink *RevisionStatus) ConvertFrom(ctx context.Context, source v1.RevisionS sink.ServiceName = source.ServiceName sink.LogURL = source.LogURL sink.DeprecatedImageDigest = source.DeprecatedImageDigest - sink.ImageDigests = make([]ImageDigests, len(source.ImageDigests)) - for i := range sink.ImageDigests { - sink.ImageDigests[i].ConvertFrom(&source.ImageDigests[i]) + sink.ContainerStatuses = make([]ContainerStatuses, len(source.ContainerStatuses)) + for i := range sink.ContainerStatuses { + sink.ContainerStatuses[i].ConvertFrom(ctx, &source.ContainerStatuses[i]) } } // ConvertFrom helps implement apis.Convertible -func (sink *ImageDigests) ConvertFrom(source *v1.ImageDigests) { - sink.ContainerName = source.ContainerName +func (sink *ContainerStatuses) ConvertFrom(ctx context.Context, source *v1.ContainerStatuses) { + sink.Name = source.Name sink.ImageDigest = source.ImageDigest } diff --git a/pkg/apis/serving/v1alpha1/revision_conversion_test.go b/pkg/apis/serving/v1alpha1/revision_conversion_test.go index fab25db79efb..0cca6275451d 100644 --- a/pkg/apis/serving/v1alpha1/revision_conversion_test.go +++ b/pkg/apis/serving/v1alpha1/revision_conversion_test.go @@ -89,9 +89,9 @@ func TestRevisionConversion(t *testing.T) { Status: "True", }}, }, - ServiceName: "foo-bar", - LogURL: "http://logger.io", - ImageDigests: []ImageDigests{}, + ServiceName: "foo-bar", + LogURL: "http://logger.io", + ContainerStatuses: []ContainerStatuses{}, }, }, }, { @@ -233,9 +233,9 @@ func TestRevisionConversionForMultiContainer(t *testing.T) { Status: "True", }}, }, - ServiceName: "foo-bar", - LogURL: "http://logger.io", - ImageDigests: []ImageDigests{}, + ServiceName: "foo-bar", + LogURL: "http://logger.io", + ContainerStatuses: []ContainerStatuses{}, }, } beta := &v1beta1.Revision{} diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index bf448f072533..efa9456ab6c9 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/pkg/apis/serving/v1alpha1/revision_types.go @@ -198,18 +198,20 @@ type RevisionStatus struct { // +optional DeprecatedImageDigest string `json:"imageDigest,omitempty"` - // ImageDigests is a mapping of images present in .Spec.Container[*].Image - // to their respective digests. The digests are resolved during the creation - // of Revision. ImageDigests holds the digests for both serving and non serving container. + // ContainerStatuses is a slice of images present in .Spec.Container[*].Image + // to their respective digests and their container name. + // The digests are resolved during the creation of Revision. + // ContainerStatuses holds the container name and image digests + // for both serving and non serving container. // ref: http://bit.ly/image-digests // +optional - ImageDigests []ImageDigests `json:"imageDigests,omitempty"` + ContainerStatuses []ContainerStatuses `json:"containerStatuses,omitempty"` } -// ImageDigests holds the information of container name and digest value -type ImageDigests struct { - ContainerName string `json:"containerName,omitempty"` - ImageDigest string `json:"imageDigest,omitempty"` +// ContainerStatuses holds the information of container name and image digest value +type ContainerStatuses struct { + Name string `json:"name,omitempty"` + ImageDigest string `json:"imageDigest,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go index a86aa6017ed2..cb524583d384 100644 --- a/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go @@ -170,17 +170,17 @@ func (in *ConfigurationStatusFields) DeepCopy() *ConfigurationStatusFields { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ImageDigests) DeepCopyInto(out *ImageDigests) { +func (in *ContainerStatuses) DeepCopyInto(out *ContainerStatuses) { *out = *in return } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ImageDigests. -func (in *ImageDigests) DeepCopy() *ImageDigests { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ContainerStatuses. +func (in *ContainerStatuses) DeepCopy() *ContainerStatuses { if in == nil { return nil } - out := new(ImageDigests) + out := new(ContainerStatuses) in.DeepCopyInto(out) return out } @@ -332,9 +332,9 @@ func (in *RevisionSpec) DeepCopy() *RevisionSpec { func (in *RevisionStatus) DeepCopyInto(out *RevisionStatus) { *out = *in in.Status.DeepCopyInto(&out.Status) - if in.ImageDigests != nil { - in, out := &in.ImageDigests, &out.ImageDigests - *out = make([]ImageDigests, len(*in)) + if in.ContainerStatuses != nil { + in, out := &in.ContainerStatuses, &out.ContainerStatuses + *out = make([]ContainerStatuses, len(*in)) copy(*out, *in) } return diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 7dc61fa98a17..1b64ff5e25b5 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -64,25 +64,26 @@ type Reconciler struct { var _ revisionreconciler.Interface = (*Reconciler)(nil) func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) error { - if rev.Status.ImageDigests == nil { - rev.Status.ImageDigests = make([]v1.ImageDigests, len(rev.Spec.Containers)) + if rev.Status.ContainerStatuses == nil { + rev.Status.ContainerStatuses = make([]v1.ContainerStatuses, 0, len(rev.Spec.Containers)) } if rev.Status.DeprecatedImageDigest != "" { - // The image digest has already been resolved. - if len(rev.Status.ImageDigests) == len(rev.Spec.Containers) { - return nil - } - // Default old revisions to have ImageDigests filled in. + // Default old revisions to have ContainerStatuses filled in. // This path should only be taken by "old" revisions that have exactly one container. - if len(rev.Status.ImageDigests) == 0 { - rev.Status.ImageDigests = append(rev.Status.ImageDigests, v1.ImageDigests{ - ContainerName: rev.Spec.Containers[0].Name, - ImageDigest: rev.Status.DeprecatedImageDigest, + if len(rev.Status.ContainerStatuses) == 0 { + rev.Status.ContainerStatuses = append(rev.Status.ContainerStatuses, v1.ContainerStatuses{ + Name: rev.Spec.Containers[0].Name, + ImageDigest: rev.Status.DeprecatedImageDigest, }) } } + // The image digest has already been resolved. + if len(rev.Status.ContainerStatuses) == len(rev.Spec.Containers) { + return nil + } + var imagePullSecrets []string for _, s := range rev.Spec.ImagePullSecrets { imagePullSecrets = append(imagePullSecrets, s.Name) @@ -137,32 +138,14 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro if v.isServingContainer { rev.Status.DeprecatedImageDigest = v.digestValue } - rev.Status.ImageDigests = append(rev.Status.ImageDigests, v1.ImageDigests{ - ContainerName: v.containerName, - ImageDigest: v.digestValue, + rev.Status.ContainerStatuses = append(rev.Status.ContainerStatuses, v1.ContainerStatuses{ + Name: v.containerName, + ImageDigest: v.digestValue, }) } - rev.Status.ImageDigests = removeDuplicateDigests(rev.Status.ImageDigests) return nil } -func removeDuplicateDigests(imageDigests []v1.ImageDigests) []v1.ImageDigests { - digests := make(map[v1.ImageDigests]bool) - list := []v1.ImageDigests{} - for _, d := range imageDigests { - if v := digests[d]; !v { - digests[d] = true - list = append(list, d) - } - } - for i, v := range list { - if v == (v1.ImageDigests{}) { - list = append(list[:i], list[i+1:]...) - } - } - return list -} - func (c *Reconciler) ReconcileKind(ctx context.Context, rev *v1.Revision) pkgreconciler.Event { readyBeforeReconcile := rev.Status.IsReady() diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index 74825a0fb882..934d4194e875 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -368,18 +368,18 @@ func TestRevWithImageDigests(t *testing.T) { if err != nil { t.Fatalf("Couldn't get revision: %v", err) } - if len(rev.Status.ImageDigests) < 2 { + if len(rev.Status.ContainerStatuses) < 2 { t.Error("Revision status does not have imageDigests") } rev.Status.DeprecatedImageDigest = "gcr.io/repo/image" updateRevision(t, ctx, controller, rev) - if len(rev.Spec.Containers) != len(rev.Status.ImageDigests) { + if len(rev.Spec.Containers) != len(rev.Status.ContainerStatuses) { t.Error("Image digests does not match with the provided containers") } - rev.Status.ImageDigests = []v1.ImageDigests{} + rev.Status.ContainerStatuses = []v1.ContainerStatuses{} updateRevision(t, ctx, controller, rev) - if len(rev.Status.ImageDigests) != 0 { + if len(rev.Status.ContainerStatuses) != 0 { t.Error("Failed to update revision") } } diff --git a/pkg/testing/v1/revision.go b/pkg/testing/v1/revision.go index b5236e910186..ead895150a33 100644 --- a/pkg/testing/v1/revision.go +++ b/pkg/testing/v1/revision.go @@ -185,8 +185,7 @@ func WithRevisionLabel(key, value string) RevisionOption { // WithImageDigests sets the .Status.ImageDigests to the Revision. func WithImageDigests(r *v1.Revision) { - r.Status.ImageDigests = []v1.ImageDigests{{ - ContainerName: "user-container", - ImageDigest: "", + r.Status.ContainerStatuses = []v1.ContainerStatuses{{ + Name: "user-container", }} } From e8f59784032911a107495ea7931a53c43215a505 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Sat, 18 Apr 2020 10:43:31 +0530 Subject: [PATCH 13/13] update comments for a struct --- pkg/apis/serving/v1/revision_types.go | 2 +- pkg/apis/serving/v1alpha1/revision_types.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/apis/serving/v1/revision_types.go b/pkg/apis/serving/v1/revision_types.go index f8bdc77d66e7..1fa3f7780781 100644 --- a/pkg/apis/serving/v1/revision_types.go +++ b/pkg/apis/serving/v1/revision_types.go @@ -146,7 +146,7 @@ type RevisionStatus struct { // to their respective digests and their container name. // The digests are resolved during the creation of Revision. // ContainerStatuses holds the container name and image digests - // for both serving and non serving container. + // for both serving and non serving containers. // ref: http://bit.ly/image-digests // +optional ContainerStatuses []ContainerStatuses `json:"containerStatuses,omitempty"` diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index efa9456ab6c9..b78d2704bbf2 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/pkg/apis/serving/v1alpha1/revision_types.go @@ -202,7 +202,7 @@ type RevisionStatus struct { // to their respective digests and their container name. // The digests are resolved during the creation of Revision. // ContainerStatuses holds the container name and image digests - // for both serving and non serving container. + // for both serving and non serving containers. // ref: http://bit.ly/image-digests // +optional ContainerStatuses []ContainerStatuses `json:"containerStatuses,omitempty"`