Add multi container changes to deploy PodSpec by savitaashture · Pull Request #7801 · knative/serving

savitaashture · 2020-04-30T17:26:26Z

Fixes:
Parts of #5822 #3384

Proposed Changes

Add changes to PodSpec in order to support multiple containers

Release Note

knative-prow-robot

@savitaashture: 0 warnings.

Details

In response to this:

Fixes:
Parts of #5822 #3384

Proposed Changes

Add changes to PodSpec in order to support multiple containers

Release Note

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.

pkg/reconciler/revision/resources/deploy.go

pkg/reconciler/revision/revision.go

savitaashture · 2020-05-02T18:40:15Z

/retest

vagababov · 2020-05-02T19:46:03Z

pkg/reconciler/revision/revision.go

 	}

 	digests := make(chan digestData, len(rev.Spec.Containers))
+	d := digestData{}


it seems what you really want here is:

an error variable (you don't really need the whole digest object)

sync.Once to enure the error is just set once. Right now the last error wins, but any of them should do.

pkg/apis/serving/v1/revision_helpers.go

pkg/reconciler/revision/resources/deploy.go

markusthoemmes · 2020-05-04T09:54:29Z

pkg/reconciler/revision/revision.go

 	digestGrp.Wait()
 	close(digests)
+	// If there is a digest error no need to create a containerStatus object
+	if d.digestError != nil {


This is a little weird indeed. One would expect this to be the usecase of the errgroup, but I get you need more state than just the error.

What about this:

Remove the redundant error check here.

Build ContainerStatuses below outside of the struct.

As soon as the loop below hits an error, exit early.

If the foor loop below passes just fine, assign the ContainerStatuses to the struct.

Pseudo-code:

var ( servingDigest string containerStatuses = make([]v1.ContainerStatuses, 0, len(digests)) ) for v := range digests { if v.digestError != nil { rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, v1.RevisionContainerMissingMessage( v.image, v.digestError.Error())) return v.digestError } if v.isServingContainer { servingDigests = v.digestValue } containerStatuses = append(containerStatuses, v1.ContainerStatuses{ Name: v.containerName, ImageDigest: v.digestValue, }) } // If we reached here, no errors happened. rev.Status.DeprecatedImageDigest = servingDigest rev.Status.ContainerStatuses = containerStatuses

That should allow the same semantics without the extra variable and mutex above.

yeah also errgroup.Wait() returns an error

@markusthoemmes Thank you updated PR

@dprotaso the errgroup returns nil so that's why not verifying error as part of errgroup.Wait()

markusthoemmes

/assign @markusthoemmes @dprotaso

pkg/reconciler/revision/resources/deploy.go

pkg/testing/v1/revision.go

pkg/reconciler/revision/resources/deploy_test.go

dprotaso · 2020-05-04T15:18:34Z

pkg/reconciler/revision/resources/deploy_test.go

 	}{{
 		name: "user-defined user port, queue proxy have PORT env",
 		rev: revision("bar", "foo",
+			withPodSpec([]corev1.Container{{


These fixtures here have become hard to understand with the additional of withPodSpec since further down there's another revisionOption that's modifying the container that you've built here

dprotaso · 2020-05-04T15:19:46Z

pkg/reconciler/revision/resources/deploy_test.go

 	}
 }

+func withPodSpec(containers []corev1.Container) revisionOption {


Since this doesn't really take any podspec properties maybe we should call this withContainers

dprotaso · 2020-05-04T15:36:22Z

pkg/reconciler/revision/revision.go

 	digestGrp.Wait()
 	close(digests)
+	// If there is a digest error no need to create a containerStatus object
+	if d.digestError != nil {


yeah also errgroup.Wait() returns an error

pkg/apis/serving/v1/revision_helpers.go

pkg/reconciler/revision/resources/deploy.go

taragu · 2020-05-07T14:18:24Z

pkg/reconciler/revision/resources/deploy.go

+		} else {
+			container = makeContainer(rev.Spec.PodSpec.Containers[i], rev)
+		}
+		updateImage(rev, &container)


After #7866 is merged, we won't need to iterate through ContainerStatuses anymore since the same container should have the same index as rev.Spec.PodSpec.Containers

Should prolly ditch the entire function then.

Updated changes based on #7866 PR

taragu · 2020-05-08T14:08:28Z

pkg/reconciler/revision/resources/deploy_test.go

 	}
+)

-	defaultRevision = &v1.Revision{


Is there a reason why we are removing the serving container from the default revision and instead adding them in the test cases?

The existing defaultRevision supports single container and to add testcase for multiple container need to write another function just with the podSpec change.

So in order to avoid code duplication moved container section to another function and called in the testcases

An option could be to create a RevisionOption func to add the extra container to the existing revision.

ie.

func WithSidecar(c v1.Container) RevisionOption { return func (r v1.Revision) { r.Containers = append(r.Containers, c) // potential validation } }

Added func withContainers(containers []corev1.Container) RevisionOption { function as that contains containers for both serving and non-serving

pkg/reconciler/revision/resources/deploy.go

pkg/reconciler/revision/resources/deploy_test.go

pkg/webhook/podspec_dryrun.go

markusthoemmes

The tests are quite beasty to review, sorry for the delay 🙈

pkg/reconciler/revision/resources/deploy_test.go

pkg/reconciler/revision/table_test.go

pkg/reconciler/revision/resources/deploy.go

dprotaso

looks great - one last thing

pkg/reconciler/revision/table_test.go

savitaashture · 2020-06-01T18:49:09Z

/test pull-knative-serving-upgrade-tests

markusthoemmes · 2020-06-03T09:43:55Z

@savitaashture this needs a rebase.

knative-metrics-robot · 2020-06-03T13:51:14Z

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/v1alpha1/revision_lifecycle.go	89.6%	90.0%	0.4
pkg/webhook/podspec_dryrun.go	91.7%	91.3%	-0.4

dprotaso · 2020-06-03T17:58:56Z

/lgtm
/approve

knative-prow-robot · 2020-06-03T17:59:09Z

[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/webhook/OWNERS~~ [dprotaso]

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

vagababov · 2020-06-03T18:09:48Z

🎉

dprotaso · 2020-06-03T18:14:30Z

thanks for all the work @savitaashture !

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

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/API API objects and controllers area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Apr 30, 2020

knative-prow-robot reviewed Apr 30, 2020

View reviewed changes

knative-prow-robot requested review from dprotaso, markusthoemmes and vagababov April 30, 2020 17:26

savitaashture force-pushed the multi-deploy branch from ec63e8b to 30d6c65 Compare May 1, 2020 12:54

markusthoemmes reviewed May 1, 2020

View reviewed changes

pkg/reconciler/revision/resources/deploy.go Outdated Show resolved Hide resolved

vagababov reviewed May 1, 2020

View reviewed changes

vagababov reviewed May 2, 2020

View reviewed changes

markusthoemmes reviewed May 4, 2020

View reviewed changes

knative-prow-robot assigned dprotaso and markusthoemmes May 4, 2020

dprotaso reviewed May 4, 2020

View reviewed changes

savitaashture force-pushed the multi-deploy branch from d78568c to fcf2444 Compare May 5, 2020 13:48

julz reviewed May 5, 2020

View reviewed changes

pkg/apis/serving/v1/revision_helpers.go Outdated Show resolved Hide resolved

julz reviewed May 6, 2020

View reviewed changes

pkg/reconciler/revision/resources/deploy.go Outdated Show resolved Hide resolved

julz reviewed May 6, 2020

View reviewed changes

pkg/reconciler/revision/resources/deploy.go Outdated Show resolved Hide resolved

taragu reviewed May 8, 2020

View reviewed changes

savitaashture force-pushed the multi-deploy branch from 807838b to 569fec7 Compare May 10, 2020 18:01

markusthoemmes reviewed May 12, 2020

View reviewed changes

savitaashture force-pushed the multi-deploy branch from 23511e3 to 9910567 Compare May 13, 2020 07:56

markusthoemmes reviewed May 13, 2020

View reviewed changes

savitaashture force-pushed the multi-deploy branch from ba41468 to 45fc323 Compare May 13, 2020 10:56

julz reviewed May 14, 2020

View reviewed changes

pkg/reconciler/revision/resources/deploy.go Outdated Show resolved Hide resolved

julz reviewed May 14, 2020

View reviewed changes

pkg/reconciler/revision/resources/deploy.go Outdated Show resolved Hide resolved

dprotaso reviewed May 29, 2020

View reviewed changes

pkg/reconciler/revision/table_test.go Outdated Show resolved Hide resolved

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

savitaashture mentioned this pull request Jun 3, 2020

[WIP] PoC for multicontainer support #6739

Closed

savitaashture added 15 commits June 3, 2020 16:55

Add multi container changes to deploy PodSpec

d541d3c

code refactor and added lock to make structure thread safe

8ac6e95

fix review comment

de804ef

fix review comments

e96a3aa

fix some nits

693b3d9

Fix review comments

4cfb1b2

addressed review comments

5fcc729

Refactored testcase

c38974f

Add testcase for missing scenario

29615e3

reverted back to use DeepCopy for containers

47dafd1

fix image override when digest value is empty

9ab79ea

update comments

a3b5059

Add len check instead of nil

71ae1e1

addressed review comments

e153672

Fix merge conflict

85d51c1

savitaashture force-pushed the multi-deploy branch from a465642 to 85d51c1 Compare June 3, 2020 13:45

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

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

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

knative-prow-robot merged commit d74d622 into knative:master Jun 3, 2020

Conversation

savitaashture commented Apr 30, 2020

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

savitaashture commented May 2, 2020

Uh oh!

Choose a reason for hiding this comment

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!

markusthoemmes left a comment

Choose a reason for hiding this comment

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!

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 May 9, 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!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dprotaso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

savitaashture May 9, 2020 •

edited

Loading