Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Podman CLI support kube play remote build #23894

Closed
wants to merge 1 commit into from

Conversation

fixomatic-ctrl
Copy link
Contributor

@fixomatic-ctrl fixomatic-ctrl commented Sep 7, 2024

Closes #14527

Does this PR introduce a user-facing change?

Allow to build flag when using kube play on remote machine

Logic

When the remote client receives a kube play command with the --build=true flag, it parses the YAML file, attempts to build the image(s) referenced, and replaces the image field with the corresponding image ID before sending the request.

🔎 Example

apiVersion: v1
kind: Pod
metadata:
  name: demo-build-remote
spec:
  containers:
    - name: container
      image: foobar

The client will build the foobar image and replace image: foobar with the image ID, then proceed with sending the request.

Internal change

  • To reduce code duplication, YAML parsing logic has been moved to pkg/util/kube.go

Testing

First build the binaries 🏗️

Let's create the same example as the one from the documentation

.
└── example/
    ├── example.yaml
    └── foobar/
        └── Containerfile

On windows

./bin/windows/podman.exe kube play ./example/example.yaml --build 
./bin/windows/podman.exe pod ls

@openshift-ci openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Sep 7, 2024
Copy link

We were not able to find or create Copr project packit/containers-podman-23894 specified in the config with the following error:

Packit received HTTP 500 Internal Server Error from Copr Service. Check the Copr status page: https://copr.fedorainfracloud.org/status/stats/, or ask for help in Fedora Build System matrix channel: https://matrix.to/#/#buildsys:fedoraproject.org.

Unless the HTTP status code above is >= 500, please check your configuration for:

  1. typos in owner and project name (groups need to be prefixed with @)
  2. whether the project name doesn't contain not allowed characters (only letters, digits, underscores, dashes and dots must be used)
  3. whether the project itself exists (Packit creates projects only in its own namespace)
  4. whether Packit is allowed to build in your Copr project
  5. whether your Copr project/group is not private

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@openshift-ci openshift-ci bot added release-note and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Sep 7, 2024
@fixomatic-ctrl fixomatic-ctrl force-pushed the main branch 2 times, most recently from 0d722f8 to a88581f Compare September 7, 2024 18:45
@fixomatic-ctrl
Copy link
Contributor Author

Go the error bin/podman-remote grew by 422016 bytes; max allowed is 51200. in the github workflows

Copy link

Cockpit tests failed for commit a88581f. @martinpitt, @jelly, @mvollmer please check.

@lsm5
Copy link
Member

lsm5 commented Sep 10, 2024

@l0rd PTAL

@lsm5 lsm5 added the bloat_approved Approve a PR in which binary file size grows by over 50k label Sep 10, 2024
@@ -18,3 +35,139 @@ const (
// Kube annotation for podman volume image.
VolumeImageAnnotation = "volume.podman.io/image"
)

// move it here ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is my concern as well. It's a good idea to move them away from domain/infra/abi/play.go because the file had grown too much but you should probably keep it under domain/infra/abi/kube.go.

The other concern is that you have added the logic to build the image, in the case of a remote, in cmd/podman/kube/. If possible we should keep the remote/local build closer considered that the logic is very similar.

Also it would be great if you could add a podman machine e2e test (see /home/mariolet/github/podman/pkg/machine/e2e/README.md).

Anyway thank you for this PR @fixomatic-ctrl, this is a great feature to add.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but you should probably keep it under domain/infra/abi/kube.go.

I am not sure to follow, as the content mention here cannot built for the remote target given my understanding (might be wrong, as I am not familiar with go).

//go:build !remote

Moreover, would it make sense to import functions in domain/infra/abi/kube.go from cmd/podman/kube/ ?

If possible we should keep the remote/local build closer considered that the logic is very similar.

Sorry, I am not sure to understand what you are expecting here.

Before putting the parsing logic under pkg/util/kube.go I tried to add it inside cmd/podman/kube/. However I faced a circular issue when I was trying to import functions in cmd/podman/kube/ from domain/infra/abi/kube.go.

I used the pkg/util/kube.go to share the code between the remote doing the build and the native doing the build, to keep a single source of truth for the parsing logic.

Also it would be great if you could add a podman machine e2e test (see /home/mariolet/github/podman/pkg/machine/e2e/README.md).

I tried to write one, I will try to add more when I will have some time available.

Anyway thank you for this PR @fixomatic-ctrl, this is a great feature to add.

Thanks you for your time and comments!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image builds happen server side. A remote client cannot build an image without a running podman server.

That said there are 2 possible approaches:

  1. as for podman build, podman kube play sends the build context to the server. So that the podman server has access to the Dockerfile and to the files required by the build.
  2. podman kube play runs podman build as a preparation step (something like what you have done in this PR)

I tend to prefer the first approach as it seams more consistent with how Podman works today.

Copy link
Contributor Author

@fixomatic-ctrl fixomatic-ctrl Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must admit I have some doubt about it being fully on the server side. I read the discussion on the ticket ( #14527) related to sending the all context directory to the server as a tar, just like this is done for the podman build command, but some people commented the following

Sending a (possible) large directory by default is very bad.1

I do not have a problem sending the full context, and I would be willing to try making the change.

It seems that according to the documentation, the libpod endpoint already have some kind of support for a tar

image

I would probably have to experiment with it, as I am not familiar with it.

Footnotes

  1. https://github.com/containers/podman/issues/14527#issuecomment-2180869202

@fixomatic-ctrl
Copy link
Contributor Author

I don't understand what just happen, I rebased my branch and got my PR closed :(

Copy link
Contributor

openshift-ci bot commented Sep 11, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fixomatic-ctrl
Once this PR has been reviewed and has the lgtm label, please assign n1hility for approval. For more information see the Kubernetes Code Review Process.

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

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

1 similar comment
Copy link
Contributor

openshift-ci bot commented Sep 11, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fixomatic-ctrl
Once this PR has been reviewed and has the lgtm label, please assign n1hility for approval. For more information see the Kubernetes Code Review Process.

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

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

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2024
@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

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-sigs/prow repository.

@fixomatic-ctrl
Copy link
Contributor Author

Closing (ref #14527 (comment))

@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Dec 19, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Dec 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bloat_approved Approve a PR in which binary file size grows by over 50k locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. machine needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add podman play kube --build support for podman remotes
4 participants