Add support for multi-containers #1382

dsimansk · 2021-07-13T17:23:59Z

Description

This PR adds support for multiple containers through file or piped cmds.

The container create is not truly create cmd, rather it's printing PodSpec.Containers to stdout in yaml for now. Afterwards it can be combined through unix pipe or stored in file and used as e.g. --containers containers.yaml .

UX examples:

kn container create --image foo.bar | \
kn container create --image bar.foo | \
kn service create --image yet.another --containers -

kn service create --image yet.another --containers containers.yaml

/cc @rhuss @dprotaso

Changes

Add container create command
Add --containers flag to service create/update
TBD tests tests tests

Reference

Fixes #997

knative-prow-robot

@dsimansk: 1 warning.

In response to this:

Description

This PR adds support for multiple containers through file or piped cmds.

The container create is not truly create cmd, rather it's printing PodSpec.Containers to stdout in yaml for now. Afterwards it can be combined through unix pipe or stored in file and used as e.g. --containers containers.yaml .

UX exampleds
kn container create --image foo.bar | \
kn container create --image bar.foo | \
kn service create --image yet.another --containers -
kn service create --image yet.another --containers containers.yaml
/cc @rhuss @dprotaso

Changes

Add container create command

Add --containers flag to service create/update

TBD tests tests tests

Reference

Fixes #997

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/kn/flags/podspec_helper.go

codecov · 2021-07-13T17:27:47Z

Codecov Report

Merging #1382 (075b1bb) into main (c18a55f) will increase coverage by 0.35%.
The diff coverage is 97.36%.

@@            Coverage Diff             @@
##             main    #1382      +/-   ##
==========================================
+ Coverage   78.33%   78.68%   +0.35%     
==========================================
  Files         160      162       +2     
  Lines        8270     8361      +91     
==========================================
+ Hits         6478     6579     +101     
+ Misses       1103     1098       -5     
+ Partials      689      684       -5

Impacted Files	Coverage Δ
pkg/kn/commands/container/add.go	`94.73% <94.73%> (ø)`
pkg/kn/commands/container/container.go	`100.00% <100.00%> (ø)`
pkg/kn/flags/podspec.go	`76.82% <100.00%> (+1.64%)`	⬆️
pkg/kn/flags/podspec_helper.go	`76.23% <100.00%> (+1.73%)`	⬆️
pkg/kn/root/root.go	`87.67% <100.00%> (+0.17%)`	⬆️
pkg/kn/plugin/manager.go	`84.14% <0.00%> (+0.19%)`	⬆️
pkg/eventing/v1/client.go	`82.67% <0.00%> (+0.85%)`	⬆️
pkg/kn/commands/broker/create.go	`71.42% <0.00%> (+3.42%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c18a55f...075b1bb. Read the comment docs.

pkg/kn/flags/podspec_helper.go

dsimansk · 2021-07-14T09:16:20Z

pkg/kn/flags/podspec_helper.go

+// UpdateContainers updates the containers array with additional ones provided from file or os.Stdin
+func UpdateContainers(spec *corev1.PodSpec, containers []corev1.Container) {
+	spec.Containers = append(spec.Containers, containers...)
+}
+


@rhuss the current mode is append additional containers. However, that might not work very well for update operation. I wonder if replacing whatever spec.Containers[1:] is a good solution.
Additionally we could require container's name parameter and match those if Containers array is already populated.

That's a good question. Updating a list is always tricky, but I like the idea with correlating via name. "update" is always an incremental operation, you don't have to specify the full state (that is different to "apply"). But if we would like to update based on position in the list, you would need to be able either to specify the index of the container within this list or provide always all containers to update. I think it is fair to require the container name (and I would even enforce this on creation). If we update a list with containers where not all containers have a name (e.g. on existing services that thas not been created with kn), then we would just ignore those containers on an update.

wdyt ? is this a fair solution ? @dprotaso as kind of stakeholder for this feature ;-) would it be ok to require a name for a container that is managed by kn in a multi-container setup ?

maximilien

Various questions and feedback. Happy to take a second look when you are ready to remove the WIP tag.

And of course, many thanks for the contribution. Cheers 🍻

docs/cmd/kn_container.md

docs/cmd/kn_container_create.md

docs/cmd/kn_service_create.md

pkg/kn/commands/container/container.go

pkg/kn/commands/container/container_test.go

pkg/kn/commands/container/create.go

pkg/kn/commands/container/create_test.go

pkg/kn/flags/podspec_test.go

maximilien · 2021-07-21T00:13:47Z

/ok-to-test

dsimansk · 2021-08-03T18:03:04Z

Unrelated sink test failed.
/retest

dsimansk · 2021-08-09T09:37:37Z

@maximilien @itsmurugappan @rhuss I'd like to kindly ask for another review cycle so we could aim this release for release tomorrow. 🤞

rhuss

Finished a first round of reviews. The only thing that I think is critical is that the update should really correlate via the name over existing containers, so that e.g. kn service apply stays idempotent when the same command is issued twice.

Maybe also add test with updating containers with the same name ?

rhuss · 2021-08-09T10:18:39Z

docs/cmd/kn_container_add.md

+
+  The 'container add' represents utility command that prints YAML container spec to standard output. It's useful for
+  multi-container use cases to create definition with help of standard 'kn' option flags. The command can be chained through
+  Unix pipes to create multiple containers at once.


Maybe mention here that this command supports all container relevant options like kn service create ?

docs/cmd/kn_container_add.md

rhuss · 2021-08-09T10:21:05Z

docs/cmd/kn_container_add.md

+### Options inherited from parent commands
+
+```
+      --cluster string      name of the kubeconfig cluster to use


It's probably out of scope for this PR, but cluster/config configuration doesn't make any sense for this offline-online command (and might even confuse users)

pkg/kn/commands/container/add.go

docs/cmd/kn_container_add.md

hack/tools.go

pkg/kn/commands/container/container_test.go

rhuss · 2021-08-09T10:32:59Z

pkg/kn/flags/podspec_helper.go

+	c := containerOfPodSpec(spec)
+	spec.Containers = []corev1.Container{}
+	spec.Containers = append(spec.Containers, *c)
+	spec.Containers = append(spec.Containers, containers...)


I haven't dived deep enough into the code, but shouldn't you correlated via the container name instead of just appending ? I.e. lookup whether the extra containers' name are already present in the existing containers and replace those in case instead of just appending ?

Currently we're in drop&replace as I was aiming to have a solution to replace all sidecars at once. E.g. when Ksvc is created from file without named containers etc.

Is it better now? I can confirm that kn apply works with the change. I'll add more tests if it's accepted.
21567d3

dsimansk · 2021-08-09T14:14:01Z

/retest

dsimansk · 2021-08-09T14:59:48Z

/retest

rhuss · 2021-08-09T16:34:21Z

/retest

pkg/kn/commands/container/add.go

pkg/kn/flags/podspec_helper.go

docs/cmd/kn_container_add.md

pkg/kn/flags/podspec.go

rhuss

thanks !

/approve
/lgtm

rhuss · 2021-08-09T18:07:29Z

Looks good, but I think some test needs to be updated, that still uses --containers.

dsimansk · 2021-08-09T18:28:13Z

Looks good, but I think some test needs to be updated, that still uses --containers.

Yep, sorry I should have checked it better first time. Updated.

pkg/kn/flags/podspec_test.go

Co-authored-by: Roland Huß <rhuss@redhat.com>

knative-metrics-robot · 2021-08-09T18:57:34Z

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

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/container/add.go	Do not exist	96.0%
pkg/kn/commands/container/container.go	Do not exist	100.0%
pkg/kn/flags/podspec.go	83.0%	84.3%	1.3
pkg/kn/flags/podspec_helper.go	83.3%	84.6%	1.3

rhuss · 2021-08-09T19:21:02Z

/approve
/lgtm

knative-prow-robot · 2021-08-09T19:21:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, rhuss

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dsimansk,rhuss]

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

itsmurugappan · 2021-08-09T21:39:50Z

sorry @dsimansk I could not get to this as there was a power outage in my area for most of the day and just got it back.

knative-prow-robot requested review from dprotaso and rhuss July 13, 2021 17:24

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 Jul 13, 2021

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 13, 2021

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

knative-prow-robot reviewed Jul 13, 2021

View reviewed changes

pkg/kn/flags/podspec_helper.go Outdated Show resolved Hide resolved

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 13, 2021

dsimansk force-pushed the pr/multicontainers branch 2 times, most recently from dadaf22 to c210272 Compare July 13, 2021 17:51

dsimansk commented Jul 13, 2021

View reviewed changes

pkg/kn/flags/podspec_helper.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 Jul 13, 2021

dsimansk force-pushed the pr/multicontainers branch from 9d7c9cd to 9a99645 Compare July 13, 2021 20:49

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

dsimansk force-pushed the pr/multicontainers branch from 9a99645 to dd0840b Compare July 13, 2021 22:01

dsimansk commented Jul 14, 2021

View reviewed changes

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 14, 2021

maximilien suggested changes Jul 21, 2021

View reviewed changes

knative-prow-robot assigned maximilien Jul 21, 2021

knative-prow-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jul 21, 2021

knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 3, 2021

dsimansk changed the title ~~WIP: Add support for multi-containers~~ Add support for multi-containers Aug 3, 2021

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 3, 2021

dsimansk force-pushed the pr/multicontainers branch from cd15149 to 092e617 Compare August 3, 2021 13:42

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