Support k8s Deployment in play kube #6256

theunrealgeek · 2020-05-18T02:49:09Z

This PR is adding support for Deployment k8s Kind in addition to the already supported Pod kind and is in response to #4093

Deployment kinds generally just spit out one or more Pods from a template spec. Hence I have reused most of the existing code for Pod and just created multiple of these dictated by the replica count specified in the Deployment YAML.

The only breaking change in this PR is that container names from play kube now have the name of the pod prefixed to them since podman needs unique container names overall and not unique container names per-pod as is the expectation from Kubernetes.

openshift-ci-robot · 2020-05-18T02:49:18Z

Hi @theunrealgeek. Thanks for your PR.

I'm waiting for a containers member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

rhatdan · 2020-05-18T12:42:33Z

Thanks @theunrealgeek
This looks like it breaks a bunch of tests?
@haircommander @baude @mheon @giuseppe @umohnani8 @mrunalp PTAL

haircommander · 2020-05-18T13:32:35Z

/ok-to-test

mheon · 2020-05-18T13:35:13Z

cmd/podman/play/kube.go

-	fmt.Printf("Pod:\n%s\n", report.Pod)
+	switch len(report.Pods) {
+	case 0:
+		return nil


Hm. Are there cases where containers could be made, but not pods? We might want to make 0 print nothing but not return yet

I actually disagree, there really shouldn't be a case containers are created and pods are not. I think return early here if no printing is needed

haircommander · 2020-05-18T13:39:58Z

this looks awesome @theunrealgeek ! can you add some tests in the style of the tests in test/e2e/play_kube_test.go?

cmd/podman/play/kube.go

haircommander · 2020-05-18T13:47:57Z

pkg/domain/infra/abi/play.go

@@ -359,6 +414,11 @@ func kubeContainerToCreateConfig(ctx context.Context, containerYAML v1.Container
 	containerConfig.Image = containerYAML.Image
 	containerConfig.ImageID = newImage.ID()
 	containerConfig.Name = containerYAML.Name
+
+	if podName != "" {


maybe fail here if podName is not specified?

pkg/domain/infra/abi/play.go

theunrealgeek · 2020-05-19T03:10:38Z

this looks awesome @theunrealgeek ! can you add some tests in the style of the tests in test/e2e/play_kube_test.go?

Didn't realise there were tests for this, I'll add some new tests for the new stuff and fix any existing ones that break.

rhatdan · 2020-05-23T10:07:33Z

@theunrealgeek Please rebase this PR

theunrealgeek · 2020-05-25T07:14:20Z

I'm done fixing the existing tests and addressing most of the review comments. I need to add a few more tests with a Deployment YAML and will hopefully update the PR with all of this tomorrow

theunrealgeek · 2020-05-31T00:20:30Z

@haircommander Can you please take a look at this again ? I've addressed most of the comments. There are still some failing CI tests which don't seem related to anything I have touched, but if they are, please let me know and I'll take a look at them.

haircommander · 2020-06-01T13:17:58Z

test/e2e/play_kube_test.go

+spec:
+  hostname: unknown
+`
+
 var yamlTemplate = `


nit: I would probably change this to podYAMLTemplate now

haircommander · 2020-06-01T13:18:18Z

test/e2e/play_kube_test.go

+
+	return nil
+}
+
 func generateKubeYaml(pod *Pod, fileName string) error {


similarly, this should probably generatePodKubeYaml

also, if there's any redundancy in this function with generateDeploymentKubeYaml, It'd be ideal if the similarities could be put in a function. I see you've made writeYAML, but haven't used it in generateKubeYaml

haircommander · 2020-06-01T13:19:35Z

test/e2e/play_kube_test.go

+
+	return nil
+}
+
 func generateKubeYaml(pod *Pod, fileName string) error {


second nit for this line: we should stick to either YAML or Yaml. I don't care which, but if you'd prefer to use YAML, can you change all of them?

haircommander · 2020-06-01T13:20:24Z

@theunrealgeek excellent thank you. a couple more nits but all together looks great to me 👍 😃

haircommander · 2020-06-01T13:20:50Z

(also, I agree, test failures look like registry issues. I've tried retriggering them)

rhatdan · 2020-06-02T12:53:39Z

@theunrealgeek Any progress?

mheon · 2020-06-02T13:27:58Z

Tests are fixed on master, FYI - a rebase should pick up the fix

Signed-off-by: Aditya Kamath <theunrealgeek@gmail.com>

theunrealgeek · 2020-06-03T03:51:52Z

@rhatdan Sorry about the delay, got involved in a prod incident on my day job.

@haircommander I've addressed your comments. Thank you for your review :)

rhatdan · 2020-06-03T09:58:50Z

@theunrealgeek No problem, thanks for the contribution.
@haircommander @umohnani8 @baude @mrunalp PTAL

haircommander · 2020-06-03T13:17:59Z

pkg/domain/infra/abi/play.go

@@ -382,7 +445,10 @@ func kubeContainerToCreateConfig(ctx context.Context, containerYAML v1.Container

 	setupSecurityContext(&securityConfig, &userConfig, containerYAML)

-	securityConfig.SeccompProfilePath = seccompPaths.findForContainer(containerConfig.Name)
+	// Since we prefix the container name with pod name to work-around the uniqueness requirement,
+	// seccom stuff should reference the actual container name from the YAML


nit: s/seccom stuff/the seccomp profile

Done 👍 @haircommander

Signed-off-by: Aditya Kamath <theunrealgeek@gmail.com>

vrothberg

Just two non-blocking nits: other than that LGTM

pkg/domain/entities/play.go

Signed-off-by: Aditya Kamath <theunrealgeek@gmail.com>

vrothberg

LGTM, thanks!
@haircommander PTAL

rhatdan · 2020-06-05T08:49:28Z

/approve

openshift-ci-robot · 2020-06-05T08:49:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, theunrealgeek

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~~ [rhatdan]

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

haircommander · 2020-06-05T13:17:52Z

LGTM 😃

mheon · 2020-06-05T13:26:34Z

cmd/podman/play/kube.go

+
+		switch len(pod.Containers) {
+		case 0:
+			return nil


Should this be a continue so we print all pods in the case that there are more than 1?

@mheon Good catch, copy paste error on my part from the previous version. I've fixed it, thanks

mheon · 2020-06-05T13:27:55Z

One last nit, otherwise LGTM. Clear to merge once fixed.

Signed-off-by: Aditya Kamath <theunrealgeek@gmail.com>

rhatdan · 2020-06-05T17:08:25Z

/lgtm
/hold

rhatdan · 2020-06-11T12:50:31Z

/hold cancel

openshift-ci-robot requested review from mrunalp and rhatdan May 18, 2020 02:49

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 18, 2020

rhatdan changed the title ~~Support k8s Deploymnt in play kube~~ Support k8s Deployment in play kube May 18, 2020

openshift-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 18, 2020

mheon reviewed May 18, 2020

View reviewed changes

haircommander reviewed May 18, 2020

View reviewed changes

cmd/podman/play/kube.go Show resolved Hide resolved

haircommander reviewed May 18, 2020

View reviewed changes

TomSweeneyRedHat reviewed May 18, 2020

View reviewed changes

pkg/domain/infra/abi/play.go Show resolved Hide resolved

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2020

theunrealgeek force-pushed the play_kube_deployment branch from 1322ac2 to 56094b7 Compare May 30, 2020 08:01

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2020

theunrealgeek force-pushed the play_kube_deployment branch from 56094b7 to b264998 Compare May 30, 2020 08:26

haircommander reviewed Jun 1, 2020

View reviewed changes

supporting k8s Deployment objects

ce7a9f0

Signed-off-by: Aditya Kamath <theunrealgeek@gmail.com>

theunrealgeek added 5 commits June 2, 2020 20:33

Modify PlayKubeReport to preserve pod->container mapping

478f296

Signed-off-by: Aditya Kamath <theunrealgeek@gmail.com>

Fix existing tests

103c922

Signed-off-by: Aditya Kamath <theunrealgeek@gmail.com>

Add tests for Deployment Kind and minor fix for play kube output

c739b58

Signed-off-by: Aditya Kamath <theunrealgeek@gmail.com>

Fix podman generate tests that relied on play kube

5330ce9

Signed-off-by: Aditya Kamath <theunrealgeek@gmail.com>

Consistent Yaml convention througout play kube tests

10ebb74

Signed-off-by: Aditya Kamath <theunrealgeek@gmail.com>

theunrealgeek force-pushed the play_kube_deployment branch from 9c77473 to 10ebb74 Compare June 3, 2020 03:50

haircommander reviewed Jun 3, 2020

View reviewed changes

Update comment related to seccomp profiles in play kube

a338cd4

Signed-off-by: Aditya Kamath <theunrealgeek@gmail.com>

vrothberg reviewed Jun 4, 2020

View reviewed changes

pkg/domain/entities/play.go Show resolved Hide resolved

pkg/domain/entities/play.go Outdated Show resolved Hide resolved

Fix missing doc for field in PlayKubePod

62b7a77

Signed-off-by: Aditya Kamath <theunrealgeek@gmail.com>

vrothberg reviewed Jun 5, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2020

rhatdan added the Needed for V2 label Jun 5, 2020

mheon reviewed Jun 5, 2020

View reviewed changes

Fix play kube report printing when no containers are created

162c1d8

Signed-off-by: Aditya Kamath <theunrealgeek@gmail.com>

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2020

openshift-ci-robot assigned rhatdan Jun 5, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 11, 2020

openshift-merge-robot merged commit b62e50f into containers:master Jun 11, 2020

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support k8s Deployment in play kube #6256

Support k8s Deployment in play kube #6256

theunrealgeek commented May 18, 2020

openshift-ci-robot commented May 18, 2020

rhatdan commented May 18, 2020

haircommander commented May 18, 2020

mheon May 18, 2020

haircommander May 18, 2020

haircommander commented May 18, 2020 •

edited

Loading

haircommander May 18, 2020

theunrealgeek commented May 19, 2020

rhatdan commented May 23, 2020

theunrealgeek commented May 25, 2020

theunrealgeek commented May 31, 2020

haircommander Jun 1, 2020

haircommander Jun 1, 2020

haircommander Jun 1, 2020

haircommander Jun 1, 2020

haircommander commented Jun 1, 2020

haircommander commented Jun 1, 2020

rhatdan commented Jun 2, 2020

mheon commented Jun 2, 2020

theunrealgeek commented Jun 3, 2020

rhatdan commented Jun 3, 2020

haircommander Jun 3, 2020

theunrealgeek Jun 3, 2020 •

edited

Loading

vrothberg left a comment

vrothberg left a comment

rhatdan commented Jun 5, 2020

openshift-ci-robot commented Jun 5, 2020

haircommander commented Jun 5, 2020

mheon Jun 5, 2020

theunrealgeek Jun 5, 2020

mheon commented Jun 5, 2020

rhatdan commented Jun 5, 2020

rhatdan commented Jun 11, 2020

Support k8s Deployment in play kube #6256

Support k8s Deployment in play kube #6256

Conversation

theunrealgeek commented May 18, 2020

openshift-ci-robot commented May 18, 2020

rhatdan commented May 18, 2020

haircommander commented May 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haircommander commented May 18, 2020 • edited Loading

Choose a reason for hiding this comment

theunrealgeek commented May 19, 2020

rhatdan commented May 23, 2020

theunrealgeek commented May 25, 2020

theunrealgeek commented May 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haircommander commented Jun 1, 2020

haircommander commented Jun 1, 2020

rhatdan commented Jun 2, 2020

mheon commented Jun 2, 2020

theunrealgeek commented Jun 3, 2020

rhatdan commented Jun 3, 2020

Choose a reason for hiding this comment

theunrealgeek Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

rhatdan commented Jun 5, 2020

openshift-ci-robot commented Jun 5, 2020

haircommander commented Jun 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Jun 5, 2020

rhatdan commented Jun 5, 2020

rhatdan commented Jun 11, 2020

haircommander commented May 18, 2020 •

edited

Loading

theunrealgeek Jun 3, 2020 •

edited

Loading