Add ImageDigests for multi container support by savitaashture · Pull Request #7373 · knative/serving

savitaashture · 2020-03-24T18:26:15Z

Fixes:
Parts of #5822 #3384

Proposed Changes

Added new field ImageDigests for multicontainer
Modified reconcileDigest to support multiple containers

Pending:

Need to update new field ImageDigests to doc after the PR merge

knative-prow-robot

@savitaashture: 0 warnings.

Details

In response to this:

Fixes:
Parts of #5822 #3384

Proposed Changes

Added new field ImageDigests for multicontainer

Modified reconcileDigest to support multiple containers

Pending:

Need to update new field ImageDigests to doc after the PR merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vagababov · 2020-03-24T18:32:17Z

@@ -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"`


Mark this as deprecated, add a todo to remove in 2 releases?

We don't remove stuff from the API, I think?

🤷‍♂ we did before

While discussing during POC we decided like we will keep existing one to not to break the functionality and along with ImageDigest we will add new field called ImageDigests to support multiple containers

I am not suggesting we remove it now. But in N knative releases (N=2, I think) this stops being of interest. So we if we rename it deprecatedImageDigest and a todo to remove it in future (0.16 I guess) it should be alright.

Please note that this also forces clients that link against the API types to be updated. This immediately breaks any code (not only in two versions). E.g. see the kn client failure in https://storage.googleapis.com/knative-prow/pr-logs/pull/knative_client/801/pull-knative-client-unit-tests/1255135293493743616/build-log.txt:

Running tests with 'go test -v -race -mod=vendor ./... ' # knative.dev/client/pkg/serving pkg/serving/config_changes.go:305:24: baseRevision.Status.ImageDigest undefined (type "knative.dev/serving/pkg/apis/serving/v1".RevisionStatus has no field or method ImageDigest) pkg/serving/config_changes.go:306:51: baseRevision.Status.ImageDigest undefined (type "knative.dev/serving/pkg/apis/serving/v1".RevisionStatus has no field or method ImageDigest) # knative.dev/client/pkg/serving [knative.dev/client/pkg/serving.test] pkg/serving/config_changes.go:305:24: baseRevision.Status.ImageDigest undefined (type "knative.dev/serving/pkg/apis/serving/v1".RevisionStatus has no field or method ImageDigest) pkg/serving/config_changes.go:306:51: baseRevision.Status.ImageDigest undefined (type "knative.dev/serving/pkg/apis/serving/v1".RevisionStatus has no field or method ImageDigest) pkg/serving/config_changes_test.go:128:17: revision.Status.ImageDigest undefined (type "knative.dev/serving/pkg/apis/serving/v1".RevisionStatus has no field or method ImageDigest)

This raises the question of what "deprecation" should mean. Only on the API (yaml) level or also on the code level ? Currently renaming fields to a Deprecated prefix is just a sledge-hammer for any client using a code dependency (also, this is not necessarily needed, or ?) and is, despite the naming, not a "deprecation" but a breaking change.

vagababov · 2020-03-24T18:32:57Z

 	// +optional
 	ImageDigest string `json:"imageDigest,omitempty"`
+
+	// ImageDigests holds the resolved digest for the image specified


The comment needs pluralization.

vagababov · 2020-03-24T18:33:43Z

 	return rev
 }

+func testRevisionForMultipleContainer() *v1.Revision {


can we call regular testRevision here and just annotate podspec?
remove duplicate boilerplate.

vagababov · 2020-03-24T18:38:12Z

+		// 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 {


Why wouldn't we switch to the map basically always? Even if the MC is disabled. The map will just have a single entry.

Yes if MC is disabled map will have just one entry.

But as per the current behavior the revision status don't have ImageDigests field
In that case if we don't add that particular condition even if MC disabled revision will have ImageDigests field .

So in order to keep consistency with current behavior of revision status added this logic

Let me know allowing ImageDigests field even if MC disabled is okay then i will update code

Yeah, I think it is totally fine to have this map populated as source of truth always going forward.

I agree, let's add it always.

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

vagababov

/lgtm

vagababov · 2020-03-25T17:56:37Z

+		rev.Status.ImageDigests = make(map[string]string, len(rev.Spec.Containers))
+	}
+	for _, container := range rev.Spec.Containers {
+		digest, err := c.resolver.Resolve(container.Image,


Do we want to do this asyncronously?
I remember @markusthoemmes having problem with each taking 10s?

na, my issues was entirely separate. This is fine usually. I suppose we could run them in parallel though?

yeah, that's what I meant in go routines.

Modified
Thank you @markusthoemmes @vagababov

vagababov · 2020-03-27T19:39:05Z

+	}
+	close(digests)
+	for v := range digests {
+		if len(rev.Spec.Containers) == 1 || v.isServingContainer {


I could see len==1 case be specialcased, but
🤷‍♂

Sorry i think i don't understand your comment fully
Do you mean len(rev.Spec.Containers) == 1 should be handled as special case?
or do you mean we should handled same way like v.isServingContainer ?

In theory, since we don't need to do any of the parallel stuff. But in practice probably doesn't matter.

savitaashture · 2020-03-28T07:55:08Z

/retest

markusthoemmes

One more comment on the upgrade path

savitaashture · 2020-04-06T19:23:38Z

Thank you @markusthoemmes @vagababov
Addressed all the review comments and ready for another round of review
Please take a look at it

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

dprotaso

It's probably worth talking about the ContainerStatus as the WG meeting/slack to get alignment.

dprotaso · 2020-04-13T21:21:38Z

+		if v.digestError != nil {
+			rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing,
+				v1.RevisionContainerMissingMessage(
+					v.image, v.digestError.Error()))


Now I wonder if individual digest resolution errors should appear in each ContainerStatus

I would think so
because instead of giving single failure information for more than one images it would be better to give failure error respective to that

let's leave it for a separate PR/issue

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

savitaashture · 2020-04-17T18:07:18Z

@dprotaso @mattmoor @vagababov @markusthoemmes PR is ready for another round of review
Please take a look
Thank you

vagababov

It's mostly good by me as before.

knative-metrics-robot · 2020-04-18T05:19:27Z

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/revision/revision.go	93.9%	98.1%	4.2

dprotaso

/lgtm
/approve
/hold

holding because of the nit - if you want to do it in a separate PR feel free to drop it

dprotaso · 2020-04-20T14:27:29Z

+		if v.isServingContainer {
+			rev.Status.DeprecatedImageDigest = v.digestValue
+		}
+		rev.Status.ContainerStatuses = append(rev.Status.ContainerStatuses, v1.ContainerStatuses{


nit: can we have the order of the containers in the status be the same as the spec?

Hi @dprotaso I feel it will be little bit messy because we don't have any comparison element between spec and status in order to have the ordering of the status same like spec

I was thinking of using the container name - so the first container in the spec would be the first container in the status etc.

If you want to do it in another PR or create a good first issue (for someone else) feel free to unblock this PR with an /unhold

Okay I have created an issue to handle that in another PR
#7658

knative-prow-robot · 2020-04-20T14:30:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, savitaashture

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/apis/OWNERS~~ [dprotaso]
~~pkg/reconciler/OWNERS~~ [dprotaso]
~~pkg/testing/OWNERS~~ [dprotaso]
~~test/OWNERS~~ [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

savitaashture · 2020-04-20T20:24:21Z

/unhold

knative-test-reporter-robot · 2020-04-20T20:33:17Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-integration-tests	pull-knative-serving-integration-tests	1/3
pull-knative-serving-unit-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests:

pkg/reconciler/autoscaling/kpa.TestControllerSynchronizesCreatesAndDeletes

savitaashture · 2020-04-20T20:39:29Z

/test pull-knative-serving-unit-tests

rhuss · 2020-04-28T15:02:00Z

 	// 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.


Where is that "ImageDigests" field ?

Add ImageDigests for multi container support

b01195a

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 24, 2020

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 24, 2020

knative-prow-robot requested review from dgerd and markusthoemmes March 24, 2020 18:26

knative-prow-robot reviewed Mar 24, 2020

View reviewed changes

knative-prow-robot added the area/API API objects and controllers label Mar 24, 2020

vagababov reviewed Mar 24, 2020

View reviewed changes

knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Mar 25, 2020

mattmoor reviewed Mar 25, 2020

View reviewed changes

move imageDigest to Deprecate and fix review comments

65ca1a3

savitaashture force-pushed the imageDigest branch from 0f0daff to 65ca1a3 Compare March 25, 2020 15:08

vagababov reviewed Mar 25, 2020

View reviewed changes

knative-prow-robot assigned vagababov Mar 25, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2020

Modified reconcileDigest to run parallel

b2230a4

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2020

markusthoemmes reviewed Mar 27, 2020

View reviewed changes

Comment thread pkg/reconciler/revision/revision.go Outdated

Comment thread pkg/reconciler/revision/revision.go Outdated

fix review comment

240c908

vagababov reviewed Mar 27, 2020

View reviewed changes

Comment thread pkg/reconciler/revision/revision.go Outdated

Comment thread pkg/reconciler/revision/revision.go Outdated

fix review comment

f2e77cb

vagababov reviewed Mar 27, 2020

View reviewed changes

fix review comment

3cdfdd6

markusthoemmes reviewed Apr 6, 2020

View reviewed changes

Comment thread pkg/reconciler/revision/revision.go Outdated

markusthoemmes reviewed Apr 6, 2020

View reviewed changes

Comment thread pkg/reconciler/revision/revision.go Outdated

savitaashture added 2 commits April 6, 2020 20:04

updated code to have imageDigests field for old revisions

0710ebe

Updated unit testcase

1a6947c

knative-prow-robot assigned mattmoor Apr 6, 2020

markusthoemmes reviewed Apr 8, 2020

View reviewed changes

Comment thread pkg/reconciler/revision/revision.go

fix review commant for adding error to struct

ddd8901

dprotaso mentioned this pull request Apr 8, 2020

Add new field ImageDigests to Revision status for multi container support #7183

Closed

mattmoor reviewed Apr 10, 2020

View reviewed changes

Chnaged type of ImageDigests field from map to struct

9a2a047

savitaashture force-pushed the imageDigest branch from f93ad7d to 9a2a047 Compare April 10, 2020 21:48

dprotaso reviewed Apr 13, 2020

View reviewed changes

mattmoor reviewed Apr 16, 2020

View reviewed changes

Comment thread pkg/apis/serving/v1alpha1/revision_conversion.go Outdated

Comment thread pkg/apis/serving/v1alpha1/revision_conversion.go Outdated

Fix review comments

4b21cc1

savitaashture force-pushed the imageDigest branch from 6fd05bf to 4b21cc1 Compare April 16, 2020 19:33

vagababov reviewed Apr 17, 2020

View reviewed changes

Comment thread pkg/apis/serving/v1alpha1/revision_types.go Outdated

update comments for a struct

e8f5978

dprotaso approved these changes Apr 20, 2020

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 20, 2020

knative-prow-robot assigned dprotaso Apr 20, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 20, 2020

savitaashture mentioned this pull request Apr 20, 2020

Modify order of the revision ContainerStatus in sync with Spec #7658

Closed

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 20, 2020

knative-prow-robot merged commit 25daf8a into knative:master Apr 20, 2020

savitaashture mentioned this pull request Apr 21, 2020

Update Docs to include new field ContainerStatus to the revision status knative/docs#2405

Merged

rhuss reviewed Apr 28, 2020

View reviewed changes

savitaashture mentioned this pull request Jun 3, 2020

[WIP] PoC for multicontainer support #6739

Closed

dprotaso mentioned this pull request Jun 11, 2020

Add domain and realm CRDs knative/networking#12

Merged

Conversation

savitaashture commented Mar 24, 2020

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

savitaashture Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

savitaashture Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

savitaashture Mar 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

savitaashture commented Mar 28, 2020

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

savitaashture commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

savitaashture Mar 25, 2020 •

edited

Loading

savitaashture Mar 27, 2020 •

edited

Loading

savitaashture Mar 28, 2020 •

edited

Loading

savitaashture commented Apr 6, 2020 •

edited

Loading

dprotaso Apr 20, 2020 •

edited

Loading

savitaashture Apr 20, 2020 •

edited

Loading

dprotaso Apr 20, 2020 •

edited

Loading