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

Add API for communicating with Docker volume plugins #8357

Merged

Conversation

mheon
Copy link
Member

@mheon mheon commented Nov 16, 2020

Docker provides extensibility through a plugin system, of which several types are available. This provides an initial library API for communicating with one type of plugins, volume plugins. Volume plugins allow for an external service to create and manage a volume on Podman's behalf.

This does not integrate the plugin system into Libpod or Podman yet; that will come in subsequent pull requests.

This replaces #4548 and will begin the continuation of that work.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2020
@mheon mheon mentioned this pull request Nov 16, 2020
@mheon
Copy link
Member Author

mheon commented Nov 16, 2020

I'm especially interested from a review from @jwhonce on the HTTP bits.

@mheon
Copy link
Member Author

mheon commented Nov 16, 2020

@rhatdan will also be interested in this.

go.mod Outdated
github.com/containernetworking/cni v0.8.0
github.com/containernetworking/plugins v0.8.7
github.com/containers/buildah v1.17.1-0.20201113135631-d0c958d65eb2
github.com/containers/common v0.27.0
github.com/containers/conmon v2.0.20+incompatible
github.com/containers/image/v5 v5.8.0
github.com/containers/libpod v1.9.3
Copy link
Member Author

Choose a reason for hiding this comment

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

Eeek. I don't know how this got in here...

@mheon mheon force-pushed the add_volume_interface_package branch 2 times, most recently from 959226d to 41636d0 Compare November 16, 2020 19:43
Copy link
Member

@jwhonce jwhonce left a comment

Choose a reason for hiding this comment

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

This is using a seperate HTTP Server from current API server. That is not "bad" but it does imply as written:

  1. people looking at this code need to know this server configuration is not impacted by pkg/api/server,
  2. this is hard-wiring a unix domain socket on the server while the imported library does allow tcp connections

I also didn't catch where the socket is being created/cleaned up

@mheon
Copy link
Member Author

mheon commented Nov 16, 2020

This is not a server, it is a client. The servers are run by individual plugins; we connect to the plugins via Unix socket and speak their REST API when we want them to perform services for us. The lifecycle of their sockets is external to Podman and is the business of the plugin creator.

libpod/plugin/volume_api.go Show resolved Hide resolved
libpod/plugin/volume_api.go Show resolved Hide resolved
@mheon mheon force-pushed the add_volume_interface_package branch from 41636d0 to f16c5ed Compare November 16, 2020 22:41
@umohnani8
Copy link
Member

code LGTM once the suggestions are addressed.

@mheon mheon force-pushed the add_volume_interface_package branch 3 times, most recently from b89eda6 to c5dc849 Compare November 18, 2020 18:24
@mheon
Copy link
Member Author

mheon commented Nov 18, 2020

All comments addressed

@TomSweeneyRedHat
Copy link
Member

@mheon looks like a gofmt issue with /libpod/plugin/volume_api.go

@mheon mheon force-pushed the add_volume_interface_package branch from c5dc849 to 5696d35 Compare November 19, 2020 16:04
@mheon
Copy link
Member Author

mheon commented Nov 19, 2020

Force-pushed with fix

@mheon
Copy link
Member Author

mheon commented Nov 19, 2020

@containers/podman-maintainers This is ready

@rhatdan
Copy link
Member

rhatdan commented Nov 19, 2020

@mheon I don't think this should go in until we do 3.0.

@mheon
Copy link
Member Author

mheon commented Nov 19, 2020

Sure, probably a good call.

Docker provides extensibility through a plugin system, of which
several types are available. This provides an initial library API
for communicating with one type of plugins, volume plugins.
Volume plugins allow for an external service to create and manage
a volume on Podman's behalf.

This does not integrate the plugin system into Libpod or Podman
yet; that will come in subsequent pull requests.

Signed-off-by: Matthew Heon <[email protected]>
@mheon mheon force-pushed the add_volume_interface_package branch from 5696d35 to 594ac4a Compare December 1, 2020 17:57
@mheon
Copy link
Member Author

mheon commented Dec 1, 2020

3.0 is landed, this is ready now @containers/podman-maintainers

@umohnani8
Copy link
Member

LGTM @baude @rhatdan PTAL

@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 7f084a8 into containers:master Dec 1, 2020
@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.
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
Development

Successfully merging this pull request may close these issues.

8 participants