[WIP] PoC for multicontainer support by savitaashture · Pull Request #6739 · knative/serving

savitaashture · 2020-02-05T15:20:54Z

Note:
PR has just code changes adding integration test is in progress in order to make sure changes works as expected

Proposed Changes

Adding support of multiple containers for single Knative service
Design Doc: https://docs.google.com/document/d/1XjIRnOGaq9UGllkZgYXQHuTQmhbECNAOk6TT6RNfJMw/edit?ts=5e25d093#

Release Note

None

knative-prow-robot

@savitaashture: 0 warnings.

Details

In response to this:

Fixes #5822 #3384

Note:
PR has just code changes adding integration test is in progress in order to make sure changes works as expected

Proposed Changes

Adding support of multiple containers for single Knative service
Design Doc: https://docs.google.com/document/d/1XjIRnOGaq9UGllkZgYXQHuTQmhbECNAOk6TT6RNfJMw/edit?ts=5e25d093#

Release Note
None

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-02-05T15:37:10Z

+				if rev.Status.ImageDigests == nil {
+					rev.Status.ImageDigests = make(map[string]string)
+				}
+				rev.Status.ImageDigests[rev.Spec.Containers[i].Name] = digest


I wonder if we should make this "image-as-passed -> digest" rather than "containerName -> digest". That'd even deduplicate things on the degenerate case of having one image used multiple times. We should probably also only resolve those once to not have a race if you've juuuust pushed an update as we've been resolving and to guarantee the same version.

dgerd · 2020-02-05T21:08:02Z


+//ValidateMultiContainerPorts validates port when specified multiple containers
+func ValidateMultiContainerPorts(containers []corev1.Container) *apis.FieldError {
+	cPort := []int32{}


You don't use the port number. Just use a count.

dgerd · 2020-02-05T21:11:55Z

+	var errs *apis.FieldError
+	for i := range containers {
+		for j := range containers[i].Ports {
+			cPort = append(cPort, containers[i].Ports[j].ContainerPort)


This seems to be doing two checks here:

Multiple ports on a single container

Multiple containers with a ports specified

However, validateContainerPorts already validates if multiple ports are specified per container and does so with a different error message.

Handled port validation for both single and multi container

Thank you

dgerd · 2020-02-05T21:15:06Z

 	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. This will be filled if there are multiple container specified.


We should fill this in all cases.

dgerd · 2020-02-05T21:15:30Z

+	// ImageDigests holds the resolved digest for the image specified
+	// within .Spec.Container.Image. The digest is resolved during the creation
+	// of Revision. This will be filled if there are multiple container specified.
+	// ImageDigests holds the digest for all the .Spec.Container.Image which are non serving.


We should put all images in this map -- not just the non-serving images.

So do you mean

Whether if its single container or multiple container we need to fill ImageDigests and ImageDigest for backward compatibility
ex:

Single container
ImageDigest: digestValue
ImageDigests:
containerName: digestValue

Multiple container (consider 2 container)
ImageDigest: digestValue
ImageDigests:
servingContainerName: digestValue
nonServingContainerName: digestValue

dgerd · 2020-02-05T21:19:15Z

+func validate(container corev1.Container, volumes sets.String) *apis.FieldError {
 	if equality.Semantic.DeepEqual(container, corev1.Container{}) {
 		return apis.ErrMissingField(apis.CurrentField)
 	}


Related to this change, but tangential: we should expand the list of reservedContainerNames in case we need to add more system sidecars later. Perhaps something with a prefix of knative-serving-system-*?

This is potentially breaking, but given that this requires named containers means that the risk of breaking only goes up after this lands.

It doesn't require named containers, does it? Aren't we inferring names from the indices? @savitaashture

Yes if user don't provide name for sidecar container we inferring names from the indices
https://github.com/knative/serving/pull/6739/files#diff-daca68b8123c709e8127c6c834c5c023R56

markusthoemmes

Mostly code hygiene stuff, but that makes me go in and question code flow the most so I figured I might as well leave comments about it. Very nice looking so far!

markusthoemmes · 2020-02-06T12:17:28Z

@@ -71,7 +71,13 @@ func (rs *RevisionSpec) GetContainer() *corev1.Container {
 	if rs.DeprecatedContainer != nil {


Have we had a look through all of our code to triage if this is okay for all places this helper is used? On a cursory look it seems okay but maybe we should rename to GetServingContainer()?

I also thought initially but dint change it because its used in lot of places
But if that is okay to change then i will rename it

@dgerd might have an opinion.

skaslev · 2020-02-07T14:33:10Z

+			if rev.Status.ImageDigest != "" {
+				servingContainer.Image = rev.Status.ImageDigest
+			}
+			containers = appendContainer(containers, servingContainer)


Calling appendContainer() here results in O(n^2) time complexity. Probably not an issue since the number of containers are expected to be small.

Can container name uniqueness be done during validation and skipped here?
Alternatively can we use a map here instead of iterating over containers checking for name collisions?

name check validation not added to check name uniqueness
rather validation added because container name will be unique so it will be easy to decide whether containers need to be appended or not
I mean the logic of appendContainer is to append the containers only if there is change in name.

Why is that even needed here? Haven't we made sure there are no name-clashes in validation? If not, we should add that there.

Modified thank you 👍

skaslev · 2020-02-07T14:35:47Z

-	return podSpec, nil
+func makeContainer(container *corev1.Container, rev *v1alpha1.Revision) corev1.Container {
+	container.VolumeMounts = append(container.VolumeMounts, varLogVolumeMount)
+	container.Lifecycle = userLifecycle


Shouldn't userLifecycle be set just for the serving container?

The purpose of userLifecycle is to run PreStop hook and which helps to block the user-container from exiting before the queue-proxy is ready
So IMO it should be applicable for all the containers?
Because even serving containers serve the request but serving container may depends on sidecar container and in any case if the sidecar exit then there will be failure in functionality.

I agree, all containers should "hang" until the PreStop hook finishes successfully.

skaslev · 2020-02-07T14:41:06Z

Prow is complaining that ./hack/update-codegen.sh hasn't been run after making changes to the apis (the new ImageDigests field in RevisionStatus).

savitaashture · 2020-02-08T07:24:00Z

Prow is complaining that ./hack/update-codegen.sh hasn't been run after making changes to the apis (the new ImageDigests field in RevisionStatus).

Thank you @skaslev update PR by running ./hack/update-codegen.sh

knative-prow-robot · 2020-02-26T11:46:30Z

@savitaashture: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-serving-build-tests-go114	`c396b9b`	link	`/test pull-knative-serving-build-tests-go114`
pull-knative-serving-integration-tests-go114	`c396b9b`	link	`/test pull-knative-serving-integration-tests-go114`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

knative-test-reporter-robot · 2020-02-26T11:46:53Z

The following jobs failed:

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

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

pkg/apis/serving.TestPodSpecValidation
pkg/apis/serving.TestPodSpecValidation/with_volume_name_collision
pkg/apis/serving.TestPodSpecValidation/too_many_containers
pkg/apis/serving.TestPodSpecValidation/extra_field
pkg/apis/serving.TestContainerValidation
pkg/apis/serving.TestContainerValidation/has_more_than_one_ports_with_valid_names
pkg/apis/serving.TestContainerValidation/has_more_than_one_unnamed_port
pkg/apis/serving/v1.TestRevisionDefaulting

and 13 more.

knative-prow-robot · 2020-02-29T15:49:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: savitaashture
To complete the pull request process, please assign mattmoor
You can assign the PR to them by writing /assign @mattmoor in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

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

mattmoor

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

knative-metrics-robot · 2020-02-29T16:01:50Z

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/serving/k8s_validation.go	99.1%	99.2%	0.1
pkg/apis/serving/v1alpha1/revision_lifecycle.go	89.3%	88.5%	-0.9
pkg/reconciler/revision/reconcile_resources.go	92.8%	92.7%	-0.1
pkg/reconciler/revision/revision.go	93.8%	94.6%	0.8

knative-prow-robot · 2020-03-16T02:53:51Z

@savitaashture: PR needs rebase.

Details

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-27T09:54:44Z

@savitaashture this one is now vastly outdated? Does it serve an purpose anymore at this point?

xvzf · 2020-04-27T13:53:27Z

@markusthoemmes This is actually kind of important, if the creator of this PR doesn't respond and it is on the timeline, I'd start working on it

markusthoemmes · 2020-04-27T14:03:00Z

@xvzf Savita is working on it, but in smaller self-contained PRs. The first of them already landed.

xvzf · 2020-04-27T14:24:32Z

@savitaashture if you need assistance just ping me :)

savitaashture · 2020-06-03T10:10:15Z

@savitaashture if you need assistance just ping me :)

Hi @xvzf Sure thank you
Yes as @markusthoemmes said we divided a feature into small sub tasks which are covered as part of these PRs
#7167
#7245
#7373
#7661
#7866
#7801

markusthoemmes · 2020-06-03T10:28:17Z

Closing as per chat.

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 5, 2020

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

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

knative-prow-robot reviewed Feb 5, 2020

View reviewed changes

knative-prow-robot added the area/API API objects and controllers label Feb 5, 2020

knative-prow-robot requested review from dprotaso and markusthoemmes February 5, 2020 15:21

markusthoemmes reviewed Feb 5, 2020

View reviewed changes

taragu reviewed Feb 5, 2020

View reviewed changes

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

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 Outdated

dgerd reviewed Feb 5, 2020

View reviewed changes

savitaashture force-pushed the wippoc branch from ce9d194 to 5e8e969 Compare February 6, 2020 07:21

markusthoemmes reviewed Feb 6, 2020

View reviewed changes

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

markusthoemmes reviewed Feb 6, 2020

View reviewed changes

skaslev reviewed Feb 7, 2020

View reviewed changes

savitaashture force-pushed the wippoc branch from c1cf553 to 40d0d8a Compare February 8, 2020 07:23

savitaashture force-pushed the wippoc branch from 40d0d8a to 98363b0 Compare February 26, 2020 11:41

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/test-and-release It flags unit/e2e/conformance/perf test issues for product features and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 29, 2020

mattmoor reviewed Feb 29, 2020

View reviewed changes

Comment thread pkg/reconciler/revision/table_test.go

Comment thread pkg/reconciler/revision/table_test.go

savitaashture force-pushed the wippoc branch from 5ab7de6 to e55cd76 Compare February 29, 2020 15:53

savitaashture added 4 commits February 29, 2020 21:25

[WIP] PoC for multicontainer support

0217cb3

fix review comments

8b7c77c

handle port validation for both single and multicontainer

22a6b25

fix review comments

29b8ab8

savitaashture added 3 commits February 29, 2020 21:25

update update-codegen.sh for ImageDigests

68736d7

fix conflict issue

478fb07

Add unit test cases

2f91fb9

savitaashture force-pushed the wippoc branch from e55cd76 to 2f91fb9 Compare February 29, 2020 15:56

This was referenced Mar 6, 2020

Add validation and flag to support multi container #7167

Merged

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

Closed

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2020

markusthoemmes closed this Jun 3, 2020

		@@ -71,7 +71,13 @@ func (rs RevisionSpec) GetContainer() corev1.Container {
		if rs.DeprecatedContainer != nil {

Conversation

savitaashture commented Feb 5, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

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!

skaslev commented Feb 7, 2020

Uh oh!

savitaashture commented Feb 8, 2020

Uh oh!

knative-prow-robot commented Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-test-reporter-robot commented Feb 26, 2020

Uh oh!

knative-prow-robot commented Feb 29, 2020

Uh oh!

knative-prow-robot commented Feb 26, 2020 •

edited

Loading