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

APIv2: Add docker compatible volume endpoints #6736

Merged
merged 2 commits into from
Jul 2, 2020
Merged

APIv2: Add docker compatible volume endpoints #6736

merged 2 commits into from
Jul 2, 2020

Conversation

maybe-sybr
Copy link
Contributor

This is a WIP for a simple docker compatibility endpoint for volumes in APIv2. Happy to take any feedback on the approach or any admin I'd need to do (CLA?) since this is my first contrib to libpod.

@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 Jun 24, 2020
@openshift-ci-robot
Copy link
Collaborator

Hi @maybe-sybr. 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.

@openshift-ci-robot 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 Jun 24, 2020
@maybe-sybr
Copy link
Contributor Author

Related to my question about compat endpoints for volumes at #6720

maybe-sybr
maybe-sybr commented Jun 24, 2020
View reviewed changes
pkg/api/server/register_volumes.go Show resolved Hide resolved
pkg/api/handlers/compat/volumes.go Outdated Show resolved Hide resolved
@mheon
Copy link
Member

mheon commented Jun 24, 2020

No CLA required, but all commits do need a Signoff.

maybe-sybr
maybe-sybr commented Jun 24, 2020
View reviewed changes
pkg/api/handlers/compat/volumes.go Outdated Show resolved Hide resolved
pkg/api/handlers/compat/volumes.go Outdated Show resolved Hide resolved
@maybe-sybr
Copy link
Contributor Author

This changeset implements all of the v1.40 endpoints with a few support caveats noted in the commit message. The endpoints work fine when I hit them in a project I'm working on, but I'm not testing many of the edge cases. I'm leaving this as is for now, pending some input from y'all. I'll keep an eye on the PR for any feedback.

@maybe-sybr maybe-sybr changed the title WIP: APIv2: Add docker compatible volume endpoints APIv2: Add docker compatible volume endpoints Jun 24, 2020
@maybe-sybr maybe-sybr marked this pull request as ready for review June 24, 2020 05:34
@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 Jun 24, 2020
@maybe-sybr maybe-sybr mentioned this pull request Jun 24, 2020
Closed
@rhatdan rhatdan changed the title APIv2: Add docker compatible volume endpoints [WIP] APIv2: Add docker compatible volume endpoints Jun 24, 2020
@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 Jun 24, 2020
@mheon
Copy link
Member

mheon commented Jun 24, 2020

@maybe-sybr Let me get a quick handler for the dangling filter written, should be pretty easy

@mheon
Copy link
Member

mheon commented Jun 24, 2020

#6756

@maybe-sybr
Copy link
Contributor Author

@mheon I have a new changeset which uses your dangling filter and also excludes the libpod specific filters. I'll push a HEAD which merges in the current HEAD from #6756 and has that fixup commit for my change. This will obviously need a rebase/autosquash once your merge goes in, or a rebase/autosquash/removal of the merge commit if we want to merge this first and wait for dangling support to land independently.

@maybe-sybr maybe-sybr force-pushed the maybe/apiv2/volumes-compat branch 3 times, most recently from 6e060b8 to 9161a4c Compare June 28, 2020 22:49
@maybe-sybr maybe-sybr changed the title [WIP] APIv2: Add docker compatible volume endpoints APIv2: Add docker compatible volume endpoints Jun 28, 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 Jun 28, 2020
@maybe-sybr
Copy link
Contributor Author

Rebased on master, squashed down and I fixed up those swagger model types so they're capital cased/public. I think this will be good to go now.

@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 Jul 1, 2020
@maybe-sybr
Copy link
Contributor Author

The swagger check in gating is complaining at me for changing some of the endpoint lines to match a change it suggested for one of them in a previous run. https:/containers/libpod/pull/6736/checks?check_run_id=824743139

I would have assumed all of these should be either compat (and end up being volumes (compat)in the API docs) or justvolumes` (and some magic takes place?). Any ideas on how to make it happy and have the docs render sensibly?

@vrothberg
Copy link
Member

I reads like Swagger wants volumes to be at the end of the lines it complains about

@maybe-sybr
Copy link
Contributor Author

@vrothberg , check out these two checks and the diff between them:

The way I read that complaint was that initially I needed to change the word volumes to compat on L111, which I did as well as making the same change to lines following it. Now it's complaining about the other lines I changed and wants them to be volumes again.

I think I understand what's happening through - swagger likely wants that word to be the parent of the endpoint being described on that line. So for the top level /volumes endpoint, that's / which is probably assigned to compat somewhere. Then all of the other endpoints live under /volumes so that's their parent and swagger is right to complain when I lie about them being children of the compat "group".

I'm going to make that change now that I have a handle on why it's telling me to do things. Will push a squashed and rebased change momentarily.

@maybe-sybr maybe-sybr changed the title [WIP] APIv2: Add docker compatible volume endpoints APIv2: Add docker compatible volume endpoints Jul 2, 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 Jul 2, 2020
@maybe-sybr maybe-sybr changed the title APIv2: Add docker compatible volume endpoints [WIP] APIv2: Add docker compatible volume endpoints Jul 2, 2020
@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 Jul 2, 2020
@maybe-sybr
APIv2: Add docker compatible volume endpoints
cb61a2d 
This change implements docker compatibile endpoint for interacting with
volumes. The code is mostly lifted from the `libpod` API handlers but
decodes and constructs data using types defined in the docker API
package.

Some notable support caveats with the current implementation:
  * we don't return the nullable `Status` or `UsageData` keys when
    returning volume information for inspect and create endpoints
  * we don't support filters when pruning
  * we return a fixed `0` for the `SpaceReclaimed` key when pruning
    since we have no insight into how much space was freed from runtime

Signed-off-by: Matt Brindley <[email protected]>
@maybe-sybr
APIv2:fix: Handle docker volume force as expected
3aa22cb 
In response to input regarding the semantic difference for the `force`
parameter for volume removal between Docker and us, this change ensures
that we emulate the Dockr behaviour correctly when this parameter is
specified.

Signed-off-by: Matt Brindley <[email protected]>
@maybe-sybr maybe-sybr changed the title [WIP] APIv2: Add docker compatible volume endpoints APIv2: Add docker compatible volume endpoints Jul 2, 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 Jul 2, 2020
@maybe-sybr
Copy link
Contributor Author

CI looks much happier on the unsquashed version of the diff I just re-pushed. LMK if any other changes need to be made prior to merge.

vrothberg
vrothberg reviewed Jul 2, 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, thanks a lot for the great work, @maybe-sybr

@vrothberg
Copy link
Member

/lgtm
/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 Jul 2, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2020
@vrothberg
Copy link
Member

/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 Jul 2, 2020
@openshift-merge-robot openshift-merge-robot merged commit 9fb0b56 into containers:master Jul 2, 2020
@maybe-sybr maybe-sybr deleted the maybe/apiv2/volumes-compat branch July 3, 2020 00:55
@edsantiago edsantiago mentioned this pull request Jul 9, 2020
Closed
@edsantiago edsantiago mentioned this pull request Jul 28, 2020
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

@mheon mheon mheon left review comments

@vrothberg vrothberg vrothberg left review comments

@rhatdan rhatdan rhatdan approved these changes

@giuseppe giuseppe Awaiting requested review from giuseppe

@jwhonce jwhonce Awaiting requested review from jwhonce

Assignees

@vrothberg vrothberg

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

Successfully merging this pull request may close these issues.
7 participants
@maybe-sybr @openshift-ci-robot @mheon @rhatdan @TomSweeneyRedHat @vrothberg @openshift-merge-robot