Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Add photon volumes to volume plugin #811

Merged
merged 3 commits into from
Dec 23, 2016
Merged

Add photon volumes to volume plugin #811

merged 3 commits into from
Dec 23, 2016

Conversation

govint
Copy link
Contributor

@govint govint commented Dec 12, 2016

This PR is a new version of #798. The old PR had to be closed as the branch used had to be deleted and remade.

@kerneltime
Copy link
Contributor

This PR should also includes updates for demos that are online.

@govint govint force-pushed the add-photon branch 2 times, most recently from fe6adb0 to 450c168 Compare December 12, 2016 19:42
Copy link
Contributor

@msterin msterin left a comment

Choose a reason for hiding this comment

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

Since it's a replica of #798 I've only skimmed the code. One nit, and one request - would it be possible to have a distinct commit for "adding photon files to vendor folder" so it does not interfere with content of the actual code change ?

sleepBeforeMount = 1 * time.Second
watchPath = "/dev/disk/by-path"
version = "VMDK Volume Driver v0.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

version is long overdue for clicking up. I suggest 1.3 :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

The value checked in should be irrelevant.
We need to set this from the make file similar to how we patch the vib xml.
This is important for tagged releases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kerneltime, is it a make generated go file with the version in it and it gets compiled along with the driver?

Copy link
Contributor

Choose a reason for hiding this comment

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

To do it properly would be more involved, but looking at the code, why do we need this field? This is not the same as a RPC version which can be something checked in and tracked in code. This field can be dropped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assuming we don't have a problem with having a version string logged and returned by the driver, we can do it via the build vs. hardcoding it here.

Actually, the driver version is pretty much the plugin version itself, which is why its there (may not be displayed).

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I follow. We install a plugin on the client, the plugin binary itself could be tagged with a prefix and symlinked to the binary name that is used. I think we should remove this const here and move to tracking version of the binary via the packaging scheme. This is how i t works for vib as well. For the RPC between the ESX server and client we can hard code the version and change it when needed.
If we do decide to keep it we should leverage https://golang.org/cmd/link/

@govint
Copy link
Contributor Author

govint commented Dec 21, 2016

PR #833 was created to run the CI tests of the changes in this PR. Build vis #833 is ok.

Testing on Photon if without authentication and authentication will be added up later.

Copy link
Contributor

@pdhamdhere pdhamdhere left a comment

Choose a reason for hiding this comment

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

LGTM

[TOC]
The Docker volume plugin supports the below platforms and the corresponding drivers for those. The plugin supports all volume provisioning and managenment operations, defined by the Docker Volume plugin interface, on both platforms.

1. Vmdk
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we going to add/replace driver name to `vSphere'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've raised a separate issue to rename to "vsphere".

@govint govint merged commit 8e2c3e1 into master Dec 23, 2016
@govint govint deleted the add-photon branch December 23, 2016 07:46
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants