Skip to content
This repository was archived by the owner on Jun 23, 2020. It is now read-only.

Support using symlinks to find correct driver #127

Closed
wants to merge 38 commits into from

Conversation

harveylowndes
Copy link
Contributor

Addresses oracle/oci-volume-provisioner#131

The volume provisioner has been patched to now not use names to dispatch custom logic. A storage class can now use it's provisioner property to determine what driver should be used. At the moment only block volume storage is supported provisioner: oracle.com/oci-bvs with aim that file storage will soon be supported also oracle/oci-volume-provisioner#90. Backwards compatible so that existing storage classes using provisioner: oracle.com/oci will default to using the block volume storage however this should be marked as deprecated and removed in a future release.

@harveylowndes harveylowndes force-pushed the symlink-flexvolume-driver-provisioner branch from 61a875b to 28094a3 Compare June 14, 2018 15:30
@harveylowndes harveylowndes force-pushed the symlink-flexvolume-driver-provisioner branch from 5caac59 to 67e0bd5 Compare June 14, 2018 16:34
@harveylowndes harveylowndes force-pushed the symlink-flexvolume-driver-provisioner branch from 7377b02 to 26e9c2c Compare June 14, 2018 16:39
Copy link
Contributor

@prydie prydie left a comment

Choose a reason for hiding this comment

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

A few changes but generally looking good 👍

cmd/oci/main.go Outdated
flexvolume.ExecDriver(&driver.OCIFlexvolumeDriver{}, os.Args)

drivers := map[string]flexvolume.Driver{
"oci-bvs": &driver.OCIFlexvolumeDriver{},
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we use a registration mechanism for drivers like in the CCM's RegisterCloudProvider().

@@ -82,26 +86,6 @@ type Driver interface {
Unmount(mountDir string) DriverStatus
}

// ExitWithResult outputs the given Result and exits with the appropriate exit
// code.
func ExitWithResult(result DriverStatus) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Whilst hoisting ExitWithResult out of ExecDriver is a good idea, I don't see where you've re-implemented outputting the JSON to stdout. I'd suggest keeping this code and calling it from main.go.

bak := out
out = new(bytes.Buffer)
defer func() { out = bak }()
status := ExecDriver(map[string]Driver{"oci-bvs": mockFlexvolumeDriver{}},
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: personally find the following more readable:

	status := ExecDriver(
		map[string]Driver{"oci-bvs": mockFlexvolumeDriver{}},
		[]string{"oci", "getvolumename", defaultTestOps},
	)

@@ -143,127 +144,143 @@ func processOpts(optsStr string) (Options, error) {

// ExecDriver executes the appropriate FlexvolumeDriver command based on
// recieved call-out.
func ExecDriver(driver Driver, args []string) {
func ExecDriver(drivers map[string]Driver, args []string) DriverStatus {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we implement the driver selection logic elsewhere and then pass it into ExecDriver as before rather than overloading ExecDriver.

@harveylowndes harveylowndes force-pushed the symlink-flexvolume-driver-provisioner branch 3 times, most recently from 1ce1c73 to fbf6928 Compare June 20, 2018 10:04
@harveylowndes harveylowndes force-pushed the symlink-flexvolume-driver-provisioner branch from fbf6928 to c696bfd Compare June 20, 2018 10:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants