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-image-scp: Load images without the use of a temporary file. #21420

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

gordonmessmer
Copy link
Contributor

This PR fixes #21239

This change requires changes to the containers-common repo, and I will send that PR shortly. To support CI, those changes are included in a separate commit which I will remove before this PR is merged.

Does this PR introduce a user-facing change?

None

@gordonmessmer
Copy link
Contributor Author

I've opened containers/common#1831 for the ssh library changes that this PR requires.

@rhatdan
Copy link
Member

rhatdan commented Jan 30, 2024

@cdoern PTAL

@cdoern
Copy link
Contributor

cdoern commented Jan 30, 2024

@gordonmessmer can you squash into one commit?

Copy link
Contributor

@cdoern cdoern left a comment

Choose a reason for hiding this comment

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

looks good once you squash into one commit. I think this is a requirement for this repo. I do wonder if there is a limit to the stdin size podman image load can read. If not, then this is a good solution.

@gordonmessmer
Copy link
Contributor Author

I can squash if that's the right thing to do, but I notice that some of the CI builds fail and the error suggests that those builds aren't using the files in vendor/github.com/containers/common/pkg/ssh from my PR.

I'm not sure where that workflow is defined... Is it replacing those files with http://github.com/containers/common/pkg/ssh ? My PR against that repo seems like it's acceptable, but the CI builds there are also failing for unclear reasons (the unmodified "main" branch doesn't pass, and the error doesn't seem to be related to my changes.)

If you want these squashed, and can merge them despite the CI failures, just say so and I'll squash them. Otherwise, I'll wait on getting things sorted out in the containers/common repo.

@rhatdan
Copy link
Member

rhatdan commented Feb 1, 2024

No the changes to common have to be merged their first and then we can vendor them into this PR. Keeping the PR for the common/vendor separate from the podman changes is correct.

@gordonmessmer
Copy link
Contributor Author

I'll remove 9232edca218e043fa52568930f54166a660ea3d9 later today, now that the supporting changes have been merged in containers/common. Does the main branch require work to import those changes, before we can test this one in CI?

@gordonmessmer
Copy link
Contributor Author

Manual tests of this change are successful. I've dropped the commit that modified vendor/ (which didn't work anyway), and rebased to the current main branch.

Copy link

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

@rhatdan
Copy link
Member

rhatdan commented Feb 8, 2024

Did you forget to add a file to your PR?

@gordonmessmer
Copy link
Contributor Author

You said that you needed to "vendor them into this PR" after the supporting changes were merged into https://github.com/containers/common/

@gordonmessmer
Copy link
Contributor Author

Just to check that I understand the process for this PR: I think that this PR is waiting on a release of containers-common v 0.58, at some point in the future. Once that's released, it can be vendored in to the podman main branch. And after that, this PR can be rebased, and tests should pass.

Does that sound right?

@rhatdan
Copy link
Member

rhatdan commented Feb 12, 2024

No the vendoring should have already happened, you do not need to wait for a release. Should rebase now and then re-push. If the latest code has not been vendored from common yet, then we can vendor it in now.

@edsantiago
Copy link
Member

edsantiago commented Feb 12, 2024

@gordonmessmer yes, exactly. The only thing missing from your synopsis is a notification mechanism to let you know when the vendoring has happened. We have no automated mechanism for this, and most of the developers on our team are fallible humans. We will try to remember to ping you when c-c is vendored, but it would help us greatly if you check in periodically -- perhaps with a github web search, or by doing periodic git pulls on your repo and grep -F 'containers/common v0.58' go.mod. Thank you for understanding.

Oops, never mind. Listen to Dan, not me.

The default location for temporary files created by mktemp may not
have enough space for an image.  Use the new SSH functions which
support an input reader to make the code simpler, more reliable,
and more efficient.

[NO NEW TESTS NEEDED]

Signed-off-by: Gordon Messmer <[email protected]>
@gordonmessmer
Copy link
Contributor Author

OK, rebased. "Build for fedora-39" completes successfully, and I can locally build podman without patching the vendor dir.

@gordonmessmer
Copy link
Contributor Author

Tests look good, and I recalled that @cdoern had asked:

I do wonder if there is a limit to the stdin size podman image load can read. If not, then this is a good solution.

So I used the modified podman to copy a large image:

$ ./bin/podman --ssh=native image scp freeipa-server:almalinux-9 local::freeipa-server
Copying ...
Writing manifest to image destination
Loaded image: docker.io/freeipa/freeipa-server:almalinux-9
# podman image ls
REPOSITORY                        TAG          IMAGE ID      CREATED       SIZE
localhost/freeipa-server          latest       a4dde2e0aa50  15 hours ago  1.03 GB
docker.io/freeipa/freeipa-server  almalinux-9  a4dde2e0aa50  15 hours ago  1.03 GB

@rhatdan
Copy link
Member

rhatdan commented Feb 12, 2024

Thanks @gordonmessmer
/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2024
Copy link
Contributor

openshift-ci bot commented Feb 12, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, gordonmessmer, rhatdan

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 271a519 into containers:main Feb 12, 2024
86 checks passed
@gordonmessmer
Copy link
Contributor Author

Thanks, @rhatdan (and everyone who helped review both PRs.)

@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 May 13, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

podman image scp fails due to ENOSPC on the remote server below /tmp without any error msg
4 participants