Add ImageCache for multi container support by savitaashture · Pull Request #7661 · knative/serving

savitaashture · 2020-04-21T11:00:27Z

Fixes:
Parts of #5822 #3384

Proposed Changes

Added image cache changes to have cache for multiple containers

Result:

apiVersion: serving.knative.dev/v1
kind: Service
metadata:
  name: multicontainer
spec:
  template:
    metadata:
      name: multicontainer-v1
    spec:
      containers:
      - image: container-a:v2
        ports:
        - containerPort: 8111
      - image: container-c:v2
        name: second
      - image: container-b:v2

image cache

kubectl get images.caching.internal.knative.dev
NAME                                                              AGE
multicontainer-v1-cache-second                                    90s
multicontainer-v1-cachec0cb79286e5fb748b173d92990b0b9fe-user-co   90s
multicontainer-v1-cached3e27505a93cf979d4ed256e644bb629-user-co   90s

knative-prow-robot

@savitaashture: 0 warnings.

Details

In response to this:

Fixes:
Parts of #5822 #3384

Proposed Changes

Added image cache changes to have cache for multiple containers

Result:

apiVersion: serving.knative.dev/v1
kind: Service
metadata:
 name: multicontainer
spec:
 template:
   metadata:
     name: multicontainer-v1
   spec:
     containers:
     - image: container-a:v2
       ports:
       - containerPort: 8111
     - image: container-c:v2
       name: second
     - image: container-b:v2

image cache

kubectl get images.caching.internal.knative.dev
NAME                                                              AGE
multicontainer-v1-cache-second                                    90s
multicontainer-v1-cachec0cb79286e5fb748b173d92990b0b9fe-user-co   90s
multicontainer-v1-cached3e27505a93cf979d4ed256e644bb629-user-co   90s

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.

markusthoemmes · 2020-04-21T11:03:51Z

 // MakeImageCache makes an caching.Image resources from a revision.
-func MakeImageCache(rev *v1.Revision) *caching.Image {
-	image := rev.Status.DeprecatedImageDigest
+func MakeImageCache(rev *v1.Revision, name string) *caching.Image {


Shall we pass in containerName, imageDigest string to not need to iterate twice?

Currently passing only containerName not the digest
because here I don't get the digest value
when i do container.Image

If you iterate over status, you'd get both, right?

Ah got it modified.
Please take a look :)

markusthoemmes · 2020-04-21T11:03:59Z

+		}
+	}
 	if image == "" {
 		image = rev.Spec.GetContainer().Image


This seems wrong now?

mattmoor

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

mattmoor

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

mattmoor · 2020-04-22T04:26:36Z


 	ns := rev.Namespace
-	imageName := resourcenames.ImageCache(rev)
-	_, err := c.imageLister.Images(ns).Get(imageName)


Are we not updating the Image resource when the digest changes?

knative-metrics-robot · 2020-04-27T19:13:09Z

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/reconcile_resources.go	92.8%	92.7%	-0.1

savitaashture · 2020-04-28T15:26:17Z

@markusthoemmes @vagababov @dprotaso @mattmoor
PR is ready for another review

Thank you :)

mattmoor

/lgtm
/approve

knative-prow-robot · 2020-04-28T15:57:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, 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/reconciler/OWNERS~~ [mattmoor]
~~pkg/testing/OWNERS~~ [mattmoor]

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-28T19:37:21Z

/retest

savitaashture · 2020-04-29T04:58:12Z

/retest

knative-test-reporter-robot · 2020-04-29T06:30:11Z

The following jobs failed:

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

Automatically retrying due to test flakiness...
/test pull-knative-serving-integration-tests

markusthoemmes · 2020-04-29T10:17:39Z

@googlebot rescan

Add ImageCache for multi container support

fffc03e

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

knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 21, 2020

knative-prow-robot reviewed Apr 21, 2020

View reviewed changes

knative-prow-robot requested review from chaodaiG and mattmoor April 21, 2020 11:00

knative-prow-robot added area/API API objects and controllers area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Apr 21, 2020

markusthoemmes reviewed Apr 21, 2020

View reviewed changes

mattmoor reviewed Apr 21, 2020

View reviewed changes

savitaashture force-pushed the imageCache branch from b58ee48 to 93d8aef Compare April 21, 2020 15:07

mattmoor reviewed Apr 21, 2020

View reviewed changes

Comment thread pkg/reconciler/revision/reconcile_resources.go

Fix review comments

3d476b3

savitaashture force-pushed the imageCache branch from 93d8aef to 3d476b3 Compare April 21, 2020 15:18

mattmoor reviewed Apr 22, 2020

View reviewed changes

julz reviewed Apr 23, 2020

View reviewed changes

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

taragu reviewed Apr 23, 2020

View reviewed changes

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

savitaashture added 2 commits April 24, 2020 20:16

fix review comments

7a885a2

Add comments for clear information

88df5eb

julz reviewed Apr 27, 2020

View reviewed changes

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

Remove using the image value when there are no digests

a75a254

mattmoor reviewed Apr 28, 2020

View reviewed changes

knative-prow-robot assigned mattmoor Apr 28, 2020

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

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

knative-prow-robot merged commit 0744ccb into knative:master Apr 29, 2020

savitaashture mentioned this pull request Jun 3, 2020

[WIP] PoC for multicontainer support #6739

Closed

Conversation

savitaashture commented Apr 21, 2020

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

markusthoemmes Apr 21, 2020

Choose a reason for hiding this comment

Uh oh!

savitaashture Apr 21, 2020

Choose a reason for hiding this comment

Uh oh!

markusthoemmes Apr 21, 2020

Choose a reason for hiding this comment

Uh oh!

savitaashture Apr 21, 2020

Choose a reason for hiding this comment

Uh oh!

markusthoemmes Apr 21, 2020

Choose a reason for hiding this comment

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattmoor Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

knative-metrics-robot commented Apr 27, 2020

Uh oh!

savitaashture commented Apr 28, 2020

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Apr 28, 2020

Uh oh!

savitaashture commented Apr 28, 2020

Uh oh!

savitaashture commented Apr 29, 2020

Uh oh!

knative-test-reporter-robot commented Apr 29, 2020

Uh oh!

markusthoemmes commented Apr 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants