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.

Sign up for GitHub

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

Jump to bottom

Implement containers/{id or name}/archive api #8126

Merged
merged 10 commits into from
Dec 1, 2020
Merged

Implement containers/{id or name}/archive api #8126

merged 10 commits into from
Dec 1, 2020

Conversation

matejvasek
Copy link
Contributor

This is heavily in progress, but I could use any good advice.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 24, 2020
@matejvasek
Copy link
Contributor Author

I duplicate some code form cp.go.

Maybe I could create some shared package that would be used by both api and cli.

At first I was thinking of calling

func (ic *ContainerEngine) ContainerCp(ctx context.Context, source, dest string, options entities.ContainerCpOptions) (*entities.ContainerCpReport, error)

, but that would require creating unnecessary temporary files.

@matejvasek
Copy link
Contributor Author

matejvasek commented Oct 24, 2020 •
edited
Loading

relates to #6050

@mheon I are you already working on this or planning to?

@matejvasek
Copy link
Contributor Author

matejvasek commented Oct 25, 2020 •
edited
Loading

I noticed that podman-cp does evaluate symlinks (I assume this is a feature).
Meaning

# alink -> a
podman cp xyz:/adir/alink .

Will copy file that the link points to, i.e user ends up with the a file in cwd.
However docker (and the API v2) doesn't evaluate symlinks.
This may affect may ability to reuse code from cp.go used by CLI 😞

@mheon
Copy link
Member

mheon commented Oct 25, 2020

@matejvasek When I was looking into implementing this, I was using https://github.com/containers/buildah/tree/master/copier - it's a new package designed for secure copy mimicking Docker behavior. The intention was to implement the archive endpoints using it, then swap podman cp over to use the same code.

@mheon
Copy link
Member

mheon commented Oct 25, 2020

I was using https://github.com/containers/buildah/blob/master/copier/copier.go#L248 for copying out of, and https://github.com/containers/buildah/blob/master/copier/copier.go#L290 for copying into, to be specific.

@matejvasek
Copy link
Contributor Author

@mheon how much have you done implementing it? Does it make sense for me to continue on this PR?

@matejvasek
Copy link
Contributor Author

That Buildah pkg looks nice, it also has Stats for HEAD method.

@mheon
Copy link
Member

mheon commented Oct 26, 2020

@matejvasek Feel free to continue, I didn't get very far - got pulled off quickly in favor of other work.

@matejvasek
Copy link
Contributor Author

CLI podman-cp allows to optionally pause a container. What APIv2 endpoint should do, pause or not pause?

@matejvasek
Copy link
Contributor Author

I noticed that docker cp doesn't create non-existent directories. e.g docker cp afile xyz:/some/non/existent/dir will fail. However copier.Put() is creating them, is that OK? Another thing I noticed: podman-cp also creates missing dirs on path, but the created directories are owned by root, whereas copier.Put() uses correct user which is nice.

@matejvasek
Copy link
Contributor Author

also with regard to symlinks: when getting a sole symlink directly by copier.Get() the method writes the file that the symlink points to into io.Writer (not the symlink itself). Docker doesn't do that, however getting a sole symlink doesn't really make a sense, so I think that's OK. When downloading a directory the symliks are downloaded as expected (as symlinks).

@mheon
Copy link
Member

mheon commented Oct 26, 2020

Pause was an attempt at fixing a CVE that turned out to not be strictly required; I don't believe it's necessary here.

@matejvasek matejvasek mentioned this pull request Oct 27, 2020
Closed
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2020
@mheon
Copy link
Member

mheon commented Nov 4, 2020

@matejvasek FYI, we're fine with merging this without support for copying into volumes, at least initially - we can resolve that later, once the basic functionality is in place.

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2020
@matejvasek matejvasek marked this pull request as ready for review November 4, 2020 20:09
@matejvasek
Copy link
Contributor Author

@mheon @baude I rebased and added support for volumes.

@matejvasek
Copy link
Contributor Author

It still has one issue, when calling PUT the file is not visible immediately in running container, it's required to stop and start the container again to see the file.
/cc @nalind

@matejvasek matejvasek changed the title [WIP] Implement containers/{id or name}/archive api Implement containers/{id or name}/archive api Nov 4, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2020
@matejvasek matejvasek force-pushed the impl-apiv2-archive branch 2 times, most recently from f666bf9 to be06ac1 Compare November 4, 2020 20:36
@matejvasek
Copy link
Contributor Author

It still has one issue, when calling PUT the file is not visible immediately in running container, it's required to stop and start the container again to see the file.

I noticed that buildah copy has the same issue. Maybe I won't be able to use copier pkg, and I will have to use raw archive pkg.

@mheon
Copy link
Member

mheon commented Nov 4, 2020

That sounds very strange... @nalind Any thoughts?

@matejvasek
Copy link
Contributor Author

@mheon is copier.Put() working for you?

@matejvasek
Copy link
Contributor Author

I don't know if it changes anything, but I am running cgroups v1, not v2.

@matejvasek
style: wsl
7da4083 
Signed-off-by: Matej Vasek <[email protected]>
@matejvasek
fix: unmount container without force
6ad2f1d 
Signed-off-by: Matej Vasek <[email protected]>
@matejvasek
add comment
be7e9f6 
Signed-off-by: Matej Vasek <[email protected]>
@matejvasek
add test
d5cabc3 
Signed-off-by: Matej Vasek <[email protected]>
@matejvasek
few more tests
0dcdb3c 
Signed-off-by: Matej Vasek <[email protected]>
@matejvasek
not forcing unmount
4d0346c 
Signed-off-by: Matej Vasek <[email protected]>
@matejvasek
more tests
24ff6f4 
Signed-off-by: Matej Vasek <[email protected]>
@matejvasek
test resource cleanup
34931ac 
Signed-off-by: Matej Vasek <[email protected]>
vrothberg
vrothberg reviewed Nov 20, 2020
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM
@baude @mheon PTAL

Thanks a lot for the great work, @matejvasek !

pkg/api/handlers/compat/containers_archive.go
return
}

w.Header().Add("X-Docker-Container-Path-Stat", statHeader)
Copy link
Member

Choose a reason for hiding this comment

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

I agree. Thanks for clarifying.

test/apiv2/23-containersArchive.at Show resolved Hide resolved
@TomSweeneyRedHat
Copy link
Member

LGTM
thanks @matejvasek !

@baude
Copy link
Member

baude commented Nov 20, 2020

@matejvasek thanks!

@baude
Copy link
Member

baude commented Nov 20, 2020

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 20, 2020
@baude
Copy link
Member

baude commented Nov 20, 2020

Do not merge until after 2.2

@abitrolly
Copy link
Contributor

I wonder if that could speed up https://github.com/wagoodman/dive when using podman?

@baude
Copy link
Member

baude commented Dec 1, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 1, 2020
@baude
Copy link
Member

baude commented Dec 1, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2020
@openshift-merge-robot openshift-merge-robot merged commit ce45b71 into containers:master Dec 1, 2020
@vrothberg vrothberg mentioned this pull request Dec 2, 2020
Closed
@matejvasek matejvasek mentioned this pull request Dec 4, 2020
Closed
@BarDweller BarDweller mentioned this pull request Dec 10, 2020
Closed
@edsantiago edsantiago mentioned this pull request Dec 18, 2020
Closed
@carwyn carwyn mentioned this pull request Feb 8, 2021
Closed
@github-actions 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 github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vrothberg vrothberg vrothberg left review comments

@nalind nalind Awaiting requested review from nalind

Assignees

@baude baude

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@matejvasek @mheon @vrothberg @rhatdan @openshift-ci-robot @TomSweeneyRedHat @baude @abitrolly @openshift-merge-robot