Add validation and flag to support multi container by savitaashture · Pull Request #7167 · knative/serving

savitaashture · 2020-03-06T14:44:27Z

Fixes:
Parts of #5822 #3384

Design Doc: https://docs.google.com/document/d/1XjIRnOGaq9UGllkZgYXQHuTQmhbECNAOk6TT6RNfJMw/edit?ts=5e25d093#

Proposed Changes

Divided PR to multiple parts in order to cover all the functionality with sub PR's

Part1:

Add flag enable-multi-container in order to allow multi container support
default value is false
Add validation check to support multi container
Add default values to support multi container

Release Note

None

/assign @mattmoor @dprotaso @markusthoemmes

knative-prow-robot

@savitaashture: 5 warnings.

Details

In response to this:

Fixes:
Parts of #5822 #3384

Proposed Changes

Divided PR to multiple parts in order to cover all the functionality with sub PR's

Part1:

Add flag enable-multi-container in order to allow multi container support
default value is false

Add validation check to support multi container

Add default values to support multi container

Release Note
None
/assign @mattmoor @dprotaso @markusthoemmes

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.

mattmoor

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

knative-test-reporter-robot · 2020-03-09T12:42:06Z

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

I must have missed this because the MD link checked is dying here: knative/serving#7167

vagababov

Mostly nits from me.
Generally looks good.
Thanks a lot!

…isabled

dprotaso · 2020-03-10T18:34:56Z

 	nc := defaultConfig()

+	// Process bool field.
+	nc.EnableMultiContainer = strings.EqualFold(data["enable-multi-container"], "true")


nit: If we ever change the default for this property to true this will override it to false if the property is not set in a defaults config map

not important now

I think that's WAI? Alpha=Opt-In, Beta=Opt-Out, GA=remove the option.

Alpha=Opt-In, Beta=Opt-Out, GA=remove the option.

Yeah but data["enable-multi-container"] will return an empty string - so there's an explicit change that'll be needed when we swap from alpha to beta

not something that's necessary right now

Yeah for beta this will require explicit code change, but I think that's by design, no? :)

yeah - that's why I just marked it a nit

markusthoemmes

A few nits. I think it might've been worthwhile to even break the flag handling out into its own PR. Not strictly necessary to do that now though.

vagababov

/lgtm
But I'll let Dave or Matt do the final approval (nor I can in practice 😄 )

dprotaso

few minor things

After this PR - I'd prioritize _conversion.go changes between v1alpha1, v1beta1 & v1 - otherwise the conversion webhook will return errors.

dprotaso · 2020-03-12T19:41:35Z

+	}
+	if _, ok := container.Resources.Requests[corev1.ResourceMemory]; !ok {
+		if rm := cfg.Defaults.RevisionMemoryRequest; rm != nil {
+			container.Resources.Requests[corev1.ResourceMemory] = *rm


Does applying this default still make sense?

I'd think RevisionCPURequest & RevisionMemoryRequest would imply totals for all user containers combined?

Now the Pod's requests/limits are a multiple of the number of containers

I assume each user containers also should have Resource information

I made an issue for this discussion: #7240

knative-metrics-robot · 2020-03-13T05:53:07Z

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/apis/config/defaults.go	97.1%	97.1%	0.1
pkg/apis/serving/k8s_validation.go	99.1%	99.2%	0.1

dprotaso · 2020-03-13T14:14:11Z

I made an issue for one more minor thing: #7241

dprotaso · 2020-03-13T14:15:45Z

Can we prioritize landing the conversion changes next?

thanks for PR @savitaashture 🎉
/lgtm
/approve

knative-prow-robot · 2020-03-13T14:15:58Z

[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:

~~config/OWNERS~~ [dprotaso]
~~pkg/apis/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-03-13T14:56:05Z

Can we prioritize landing the conversion changes next?

@dprotaso Sure i will send conversion changes

knative-prow-robot assigned dprotaso, markusthoemmes and mattmoor Mar 6, 2020

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

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 6, 2020

knative-prow-robot reviewed Mar 6, 2020

View reviewed changes

knative-prow-robot requested review from dprotaso and mattmoor March 6, 2020 14:44

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

mattmoor reviewed Mar 6, 2020

View reviewed changes

Comment thread pkg/apis/serving/k8s_validation_test.go

Comment thread pkg/apis/serving/k8s_validation_test.go

savitaashture force-pushed the multicontainer-part1 branch from 54c97e6 to d8c2768 Compare March 6, 2020 14:56

vagababov reviewed Mar 6, 2020

View reviewed changes

savitaashture added 2 commits March 7, 2020 16:19

Add validation and flag to support multi container

fa171a9

fix review comments

35c9fb7

savitaashture force-pushed the multicontainer-part1 branch from d8c2768 to 35c9fb7 Compare March 7, 2020 11:15

savitaashture mentioned this pull request Mar 7, 2020

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

Closed

vagababov reviewed Mar 7, 2020

View reviewed changes

Comment thread pkg/apis/config/defaults.go Outdated

fix review comment

c6efde9

mattmoor added a commit to mattmoor/docs that referenced this pull request Mar 9, 2020

Add a minishift redirect

79eb2cc

I must have missed this because the MD link checked is dying here: knative/serving#7167

mattmoor mentioned this pull request Mar 9, 2020

Add a minishift redirect knative/docs#2286

Merged

knative-prow-robot pushed a commit to knative/docs that referenced this pull request Mar 9, 2020

Add a minishift redirect (#2286)

229160f

I must have missed this because the MD link checked is dying here: knative/serving#7167

mattmoor added a commit to mattmoor/docs that referenced this pull request Mar 9, 2020

Add a minishift redirect (knative#2286)

d3e8c30

I must have missed this because the MD link checked is dying here: knative/serving#7167

mattmoor mentioned this pull request Mar 9, 2020

[RELEASE 0.13] Add a minishift redirect knative/docs#2287

Merged

knative-prow-robot pushed a commit to knative/docs that referenced this pull request Mar 9, 2020

Add a minishift redirect (#2286) (#2287)

af56491

I must have missed this because the MD link checked is dying here: knative/serving#7167

vagababov reviewed Mar 9, 2020

View reviewed changes

fix nit and changed error message for multiple conatiners when flag d…

50f1397

…isabled

dprotaso reviewed Mar 10, 2020

View reviewed changes

vagababov reviewed Mar 11, 2020

View reviewed changes

Comment thread pkg/apis/serving/v1/revision_defaults.go Outdated

add logic to generate unique names for containers

04f589b

savitaashture force-pushed the multicontainer-part1 branch from a4e34e1 to 04f589b Compare March 12, 2020 09:04

markusthoemmes reviewed Mar 12, 2020

View reviewed changes

Comment thread pkg/apis/serving/k8s_validation.go Outdated

Comment thread pkg/apis/serving/k8s_validation.go Outdated

Comment thread pkg/apis/serving/k8s_validation.go

Comment thread pkg/apis/serving/k8s_validation.go Outdated

Comment thread pkg/apis/serving/k8s_validation.go Outdated

fix review comments

ee44718

vagababov reviewed Mar 12, 2020

View reviewed changes

knative-prow-robot assigned vagababov Mar 12, 2020

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

dprotaso requested changes Mar 12, 2020

View reviewed changes

fix review comments and removed duplicate testcases

47e0ee4

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 13, 2020

This was referenced Mar 13, 2020

[multi-container] How should Revision[CPU|Memory]Request defaults apply to revisions with multiple containers #7240

Open

[multi-container] toggling the default enable-multi-container from true to false will break things #7241

Closed

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

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

knative-prow-robot merged commit 6194d05 into knative:master Mar 13, 2020

savitaashture mentioned this pull request Jun 3, 2020

[WIP] PoC for multicontainer support #6739

Closed

Conversation

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

Uh oh!

Proposed Changes

Part1:

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Part1:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

knative-test-reporter-robot commented Mar 9, 2020

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dprotaso Mar 10, 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!

mattmoor Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

dprotaso Mar 12, 2020

Choose a reason for hiding this comment

Uh oh!

vagababov Mar 12, 2020

Choose a reason for hiding this comment

Uh oh!

dprotaso Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

dprotaso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dprotaso Mar 12, 2020

Choose a reason for hiding this comment

Uh oh!

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

savitaashture commented Mar 6, 2020 •

edited

Loading

dprotaso Mar 10, 2020 •

edited

Loading

savitaashture Mar 13, 2020 •

edited

Loading

dprotaso Mar 13, 2020 •

edited

Loading

savitaashture commented Mar 13, 2020 •

edited

Loading