play: kube: support io.reader body arg and remove tempfiles #13606

paralin · 2022-03-23T07:28:57Z

The PlayKube and PlayKubeDown commands accept a "path" argument to a YAML file to play. This requires the caller to write the YAML to a file path.

The file is opened & used as the body of the HTTP request, it is possible to instead pass a io.Reader and use a fully in-memory YAML config without needing a temporary file on disk.

pkg/bindings/play/play.go

pkg/domain/entities/play.go

paralin · 2022-03-23T13:07:22Z

@Luap99 I've implemented what you requested for the bindings - removed the new Body param and now added new functions KubeWithBody and KubeDownWithBody.

paralin · 2022-03-23T14:48:29Z

@Luap99 @rhatdan

OK - I have refactored everything to use a io.Reader instead of a path.

This is significantly cleaner in the HTTP handlers, which previously were reading the body to a tempfile on disk. Now they can pass the http body directly to PlayKube and remove the tempfile altogether.

Luap99

LGTM, one small nit but not worth to repush for that.

pkg/domain/infra/tunnel/play.go

mheon · 2022-03-23T17:13:43Z

LGTM

paralin · 2022-03-23T18:09:05Z

Tests appear to be flaking :)

Luap99 · 2022-03-23T18:12:52Z

Tests appear to be flaking :)

Yeah this happens regularly, we can restart them manually if needed. Also github was down earlier so that likely confused the CI tests.

mheon · 2022-03-24T00:02:41Z

I'll re-run the tests, but most of the failures are remote play kube, which seems suspicious

paralin · 2022-03-24T06:11:20Z

Error: /tmp/podman_test1872415588/kube.yaml: Client not set in context
         [AfterEach] Podman play kube
           /var/tmp/go/src/github.com[/containers/podman/test/e2e/play_kube_test.go:1214](https://github.com/containers/podman/blob/e36ba5f141973620f085e69e613aa5358394fc94/test/e2e/play_kube_test.go#L1214)
         # podman-remote [options] stop -a --time 0
         # podman-remote [options] pod stop -a -t 0
         # podman-remote [options] pod rm -fa
         # podman-remote [options] rm -fa
         
         

         • Failure [1.502 seconds]
         Podman play kube
         /var/tmp/go/src/github.com[/containers/podman/test/e2e/play_kube_test.go:1194](https://github.com/containers/podman/blob/e36ba5f141973620f085e69e613aa5358394fc94/test/e2e/play_kube_test.go#L1194)
           podman play kube teardown with volume [It]
           /var/tmp/go/src/github.com[/containers/podman/test/e2e/play_kube_test.go:3077](https://github.com/containers/podman/blob/e36ba5f141973620f085e69e613aa5358394fc94/test/e2e/play_kube_test.go#L3077)
         
           Expected
               <int>: 125
           to match exit code:
               <int>: 0
         
           /var/tmp/go/src/github.com[/containers/podman/test/e2e/play_kube_test.go:3101](https://github.com/containers/podman/blob/e36ba5f141973620f085e69e613aa5358394fc94/test/e2e/play_kube_test.go#L3101)
         
           Full Stack Trace
           github.com/containers/podman/v4/test/e2e.glob..func46.89()
           	/var/tmp/go/src/github.com[/containers/podman/test/e2e/play_kube_test.go:3101](https://github.com/containers/podman/blob/e36ba5f141973620f085e69e613aa5358394fc94/test/e2e/play_kube_test.go#L3101) +0x666
           github.com/containers/podman/v4/test/e2e.TestLibpod(0x0)
           	/var/tmp/go/src/github.com[/containers/podman/test/e2e/common_test.go:100](https://github.com/containers/podman/blob/e36ba5f141973620f085e69e613aa5358394fc94/test/e2e/common_test.go#L100) +0x109
           testing.tRunner(0xc0001cd520, 0x132fdc0)
           	/usr/local/go/src/testing/testing.go:1259 +0x102
           created by testing.(*T).Run
           	/usr/local/go/src/testing/testing.go:1306 +0x35a

https://storage.googleapis.com/cirrus-ci-6707778565701632-fcae48/artifacts/containers/podman/5275768656756736/html/int-remote-ubuntu-2110-root-host.log.html#t--podman-play-kube-teardown-with-volume--1

This test passes for me when I run it locally.

... That said, I did notice that while you can construct an engine with ClientCtx:

	engine = &tunnel.ContainerEngine{ClientCtx: pmConn}

The client context is not always used, for example when the parameters of a call have a context:

	report, err := engine.PlayKube(ctx, ...)

In this case, if the client is not attached to the context:

podman/pkg/bindings/connection.go

Line 46 in 80123ca

return nil, errors.Errorf("%s not set in context", clientKey)

The error "Client is not attached to context" will be returned. Regardless of if ClientCtx had it attached.

I'll try to figure out why it's breaking this way.

paralin · 2022-03-24T07:43:39Z

The tests pass locally & there were no changes in which context.Context was used anywhere in the code, so I don't know why that one ubuntu test is failing.

.. unless ic.ClientCtx, registry.GetContext() do not have the Client set on them.

Or in the HTTP request handler, it uses the http request context - but this is no different from how it is on the main branch, so I don't see why it would suddenly stop working now.

pkg/domain/infra/tunnel/play.go

The PlayKube and PlayKubeDown commands accepted a "path" argument to a YAML file to play. This requires the caller to write the YAML to a file path. The downside of this is apparent in the HTTP handlers which have to use a temporary file on disk to store the YAML file. The file is opened & used as the body of the HTTP request. It's possible to instead pass a io.Reader and use a fully in-memory request body. Add backwards-compatible changes to bindings to allow passing either a filepath or a io.Reader body. Refactor the podman bindings to use a io.Reader instead of a filepath. Simplify the HTTP handlers for PlayKube by removing the now unneeded tempfile. [NO NEW TESTS NEEDED] Signed-off-by: Christian Stewart <christian@paral.in>

Luap99

LGTM

openshift-ci · 2022-03-24T11:40:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, paralin

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

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

Luap99 · 2022-03-24T11:40:32Z

/lgtm

openshift-ci bot assigned vrothberg Mar 23, 2022

Luap99 reviewed Mar 23, 2022

View reviewed changes

pkg/bindings/play/play.go Outdated Show resolved Hide resolved

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

paralin force-pushed the play-kube-inmem branch 2 times, most recently from bfcba6e to bf6eb16 Compare March 23, 2022 13:06

paralin force-pushed the play-kube-inmem branch 2 times, most recently from ba841aa to 7fc33b6 Compare March 23, 2022 14:46

paralin changed the title ~~play: kube: add support for in-memory kubefile~~ play: kube: support in-memory io.reader body argument Mar 23, 2022

paralin changed the title ~~play: kube: support in-memory io.reader body argument~~ play: kube: support io.reader body arg and remove tempfiles Mar 23, 2022

paralin force-pushed the play-kube-inmem branch 2 times, most recently from ecdba84 to 67cf759 Compare March 23, 2022 15:07

Luap99 reviewed Mar 23, 2022

View reviewed changes

pkg/domain/infra/tunnel/play.go Outdated Show resolved Hide resolved

paralin force-pushed the play-kube-inmem branch from 67cf759 to e36ba5f Compare March 23, 2022 18:08

paralin force-pushed the play-kube-inmem branch from e36ba5f to be55be0 Compare March 24, 2022 06:38

Luap99 reviewed Mar 24, 2022

View reviewed changes

pkg/domain/infra/tunnel/play.go Outdated Show resolved Hide resolved

paralin force-pushed the play-kube-inmem branch from be55be0 to 7526803 Compare March 24, 2022 07:57

Luap99 approved these changes Mar 24, 2022

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2022

openshift-ci bot assigned Luap99 Mar 24, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2022

openshift-merge-robot merged commit 2d46165 into containers:main Mar 24, 2022

paralin deleted the play-kube-inmem branch March 24, 2022 12:02

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 21, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

play: kube: support io.reader body arg and remove tempfiles #13606

play: kube: support io.reader body arg and remove tempfiles #13606

paralin commented Mar 23, 2022 •

edited

Loading

paralin commented Mar 23, 2022 •

edited

Loading

paralin commented Mar 23, 2022

Luap99 left a comment

mheon commented Mar 23, 2022

paralin commented Mar 23, 2022

Luap99 commented Mar 23, 2022

mheon commented Mar 24, 2022

paralin commented Mar 24, 2022

paralin commented Mar 24, 2022

Luap99 left a comment

openshift-ci bot commented Mar 24, 2022

Luap99 commented Mar 24, 2022

play: kube: support io.reader body arg and remove tempfiles #13606

play: kube: support io.reader body arg and remove tempfiles #13606

Conversation

paralin commented Mar 23, 2022 • edited Loading

paralin commented Mar 23, 2022 • edited Loading

paralin commented Mar 23, 2022

Luap99 left a comment

Choose a reason for hiding this comment

mheon commented Mar 23, 2022

paralin commented Mar 23, 2022

Luap99 commented Mar 23, 2022

mheon commented Mar 24, 2022

paralin commented Mar 24, 2022

paralin commented Mar 24, 2022

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 24, 2022

Luap99 commented Mar 24, 2022

paralin commented Mar 23, 2022 •

edited

Loading

paralin commented Mar 23, 2022 •

edited

Loading