Containerized Ubiquity-Docker-Plugin

aswarke commented 7 years ago

Containerize ubiquity-docker-plugin as per Docker's managed plugin system ( plugin v2 architecture)

Pull request supporting 1) Issue #52 2) Issue #51 3) IBM/ubiquity#126 4) IBM/ubiquity#127 (awaiting SCBE changes if any) 5) IBM/ubiquity#125

TODO: Will change glide.yaml ubiquity dependency to dev branch once capabilities branch has been checked into ubiquity/dev branch

This change is

shay-berman commented 7 years ago

Review status: 0 of 12 files reviewed at latest revision, 19 unresolved discussions.

config.json, line 3 at r3 (raw file):

{
  "description": "Ubiquity Docker Volume Plugin",
  "documentation": "https://github.com/IBM/ubiquity-docker-plugin",

This is problematic The documentation for GA is going to be in SCBE user guide, so the right location here should be change according for the GA

config.json, line 76 at r3 (raw file):

    "socket": "ubiquity.sock",
    "types": [
      "docker.volumedriver/1.0"

r u sure about this version 1.0?

config.json, line 95 at r3 (raw file):

    },
    {
      "source": "/root/.ssh",

what is this for? if its for Spectrum Scale please add docstring here to mention it

config.json, line 113 at r3 (raw file):

    },
    {
      "source": "/etc/passwd",

r u sure we need this bind? why? the contain run as root so why we need it?

Dockerfile, line 10 at r3 (raw file):


FROM alpine:latest
RUN apk --update add ca-certificates multipath-tools nfs-utils open-iscsi openssh sg3_utils \

you should mention package=VERSION, so it will take a specific version and not the latest. so please checkout what are the latest one that it install it with, and just put the vesion inside. (we want to have full control on the version that comes to GA version)

glide.yaml, line 6 at r3 (raw file):

  version: ^1.3.0
- package: github.com/IBM/ubiquity
  version: capabilities

change it to dev before the marj

main_suite_test.go, line 80 at r3 (raw file):

  backend = "spectrum-scale"

  confFileName := fmt.Sprintf("/tmp/ubiquity-plugin%s.conf", "Sock")

what is this name Sock?

ubiquity-client.conf, line 1 at r3 (raw file): Are all these params still relevant?? I think you put them all inside the json file, don't you? please clarify.

= "/tmp" backends = ["spectrum-scale"] logLevel = "info" # debug / info / error

scripts/plugin_v2.sh, line 3 at r3 (raw file):

#!/bin/bash

set -e

please rename the file to build_docker_plugin.sh

scripts/plugin_v2.sh, line 5 at r3 (raw file):

set -e

PLUGIN_NAME="ubiquity/ubiquity-docker-plugin"

check if this param already exported and if so use it, if not use the default you mentioned here. and i think the name PLUGIN_NAME is wrong, because it actually the plugin docker repository, right?

scripts/plugin_v2.sh, line 6 at r3 (raw file):


PLUGIN_NAME="ubiquity/ubiquity-docker-plugin"
TAG="v1"

the same here please check if TAG export already exist, if so use this TAG if not use v1.0.0 as the default. also print at the begin of the script the plugin name and the tag

web_server/handler.go, line 82 at r3 (raw file):

      return
  }
  removeVolumeRequest.ContainerOrchestrator = "docker"

please add this "docker" string as const inside the ubiquity repository and use it here instead of duplicate the "docker" here and in the plan you going to use it for validation.

web_server/handler.go, line 98 at r3 (raw file):

  }
  attachRequest.Host = c.hostname
  attachRequest.ContainerOrchestrator = "docker"

same here - use const

web_server/server.go, line 60 at r3 (raw file):

  router.HandleFunc("/VolumeDriver.Path", s.handler.Path).Methods("POST")
  router.HandleFunc("/VolumeDriver.List", s.handler.List).Methods("POST")
  router.HandleFunc("/VolumeDriver.Capabilities", s.handler.GetCapabilities).Methods("POST")

why capability is post and not GET?

ahhh its because we sending the request - i must say its very confusing.

web_server/server.go, line 70 at r3 (raw file):

  err = s.serveUnixSocket(pluginsPath, router)
  if err != nil {
      panic("Error starting web server: " + err.Error())

why the error message is "error starting web server"?? isn't should be fail to open unix socket?

web_server/server.go, line 77 at r3 (raw file):

  if _,err := os.Stat(resources.DockerPropagatedMount); err != nil {
      if os.IsNotExist(err) {
          if err := os.MkdirAll(resources.DockerPropagatedMount, 0755); err != nil {

why 755?
do you thing other should be able to cd to this directory?

do we expect this path to be already created?

web_server/server.go, line 93 at r3 (raw file):

  if _,err := os.Stat(pluginsPath); err != nil {
      if os.IsNotExist(err) {
          if err := os.MkdirAll(pluginsPath, 0755); err != nil {

755 is ok?

web_server/server.go, line 103 at r3 (raw file):

  }

  ubiquitySocketAddress := path.Join(pluginsPath, fmt.Sprintf("%s.sock", "ubiquity"))

please put this string : fmt.Sprintf("%s.sock", "ubiquity") as const

web_server/server.go, line 105 at r3 (raw file):

  ubiquitySocketAddress := path.Join(pluginsPath, fmt.Sprintf("%s.sock", "ubiquity"))

  err := syscall.Unlink(ubiquitySocketAddress)

why syscall?

anyway please move this operation inside the ubiquity Executor interface so you will be able to add unit testing for this part.

did you done unit testing for this code? not sure you can until u put it inside the xecutor

Comments from Reviewable

shay-berman commented 7 years ago

Hi @amit Thanks for the effort of this task.

I conducted code review on this PR, please review and apply them. In addition - is this the only PR for the new docker plugin v2?

Thanks

aswarke commented 7 years ago

Review status: 0 of 13 files reviewed at latest revision, 19 unresolved discussions.

config.json, line 3 at r3 (raw file):