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

Standalone cinder #368

Merged
merged 19 commits into from
Sep 19, 2017
Merged

Conversation

aglitke
Copy link
Contributor

@aglitke aglitke commented Sep 13, 2017

This PR adds a new provisioner called standalone-cinder. The provisioner works by creating a cinder volume and mapping that volume's connection details to a PV using a natively supported VolumeSource. Currently iscsi and rbd volumes are supported. See README.md for more details on the architecture and flow.

The status of the code is a working prototype and I plan to actively collaborate with the community on further improvements.

Please see Issue 317 for the initial discussions about adding this provisioner.
#317

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 13, 2017
@aglitke
Copy link
Contributor Author

aglitke commented Sep 13, 2017

@j-griffith @humblec @dims FYI

Also, I tried to follow convention when updating the vendor code by using 'glide up -v ; glide-vc --use-lock-file'. Please advise if I should do this differently. Thanks!

@rootfs
Copy link
Contributor

rootfs commented Sep 13, 2017

@aglitke need to resolve the conflict

@rootfs
Copy link
Contributor

rootfs commented Sep 13, 2017

gofmt and golint

Verifying /home/travis/gopath/src/github.com/kubernetes-incubator/external-storage/repo-infra/verify/go-tools/verify-gofmt.sh
!!! 'gofmt -s' needs to be run on the following files: 
./openstack/standalone-cinder/mapper.go
./openstack/standalone-cinder/provisioner.go
./openstack/standalone-cinder/mapper-rbd.go
./openstack/standalone-cinder/mapper-iscsi.go
./openstack/standalone-cinder/openstack.go
FAILED   /home/travis/gopath/src/github.com/kubernetes-incubator/external-storage/repo-infra/verify/go-tools/verify-gofmt.sh	0s
Verifying /home/travis/gopath/src/github.com/kubernetes-incubator/external-storage/repo-infra/verify/go-tools/verify-golint.sh
openstack/standalone-cinder/mapper-iscsi.go:26:7:warning: don't use ALL_CAPS in Go names; use CamelCase (golint)
openstack/standalone-cinder/mapper-iscsi.go:26:7:warning: exported const ISCSI_TYPE should have comment or be unexported (golint)
openstack/standalone-cinder/mapper-iscsi.go:27:7:warning: don't use ALL_CAPS in Go names; use CamelCase (golint)
openstack/standalone-cinder/mapper-iscsi.go:27:7:warning: exported const INITIATOR_NAME should have comment or be unexported (golint)
openstack/standalone-cinder/mapper-iscsi.go:38:9:warning: if block ends with a return statement, so drop this else and outdent its block (golint)
openstack/standalone-cinder/mapper-iscsi.go:103:8:warning: if block ends with a return statement, so drop this else and outdent its block (golint)
openstack/standalone-cinder/mapper-rbd.go:28:7:warning: don't use ALL_CAPS in Go names; use CamelCase (golint)
openstack/standalone-cinder/mapper-rbd.go:28:7:warning: exported const RBD_TYPE should have comment or be unexported (golint)
openstack/standalone-cinder/mapper.go:36:2:warning: struct field VolumeId should be VolumeID (golint)
openstack/standalone-cinder/mapper.go:105:49:warning: func parameter volumeId should be volumeID (golint)
openstack/standalone-cinder/mapper.go:134:51:warning: func parameter volumeId should be volumeID (golint)
openstack/standalone-cinder/mapper.go:152:47:warning: func parameter volumeId should be volumeID (golint)
openstack/standalone-cinder/mapper.go:170:2:warning: struct field cinderVolumeId should be cinderVolumeID (golint)
openstack/standalone-cinder/mapper.go:199:1:warning: exported function BuildPV should have comment or be unexported (golint)
openstack/standalone-cinder/mapper.go:199:48:warning: func parameter volumeId should be volumeID (golint)
openstack/standalone-cinder/openstack.go:38:6:warning: exported type Config should have comment or be unexported (golint)
openstack/standalone-cinder/openstack.go:41:3:warning: struct field AuthUrl should be AuthURL (golint)
openstack/standalone-cinder/openstack.go:43:3:warning: struct field UserId should be UserID (golint)
openstack/standalone-cinder/openstack.go:45:3:warning: struct field TenantId should be TenantID (golint)
openstack/standalone-cinder/openstack.go:47:3:warning: struct field TrustId should be TrustID (golint)
openstack/standalone-cinder/openstack.go:48:3:warning: struct field DomainId should be DomainID (golint)
openstack/standalone-cinder/openstack.go:106:9:warning: if block ends with a return statement, so drop this else and outdent its block (golint)
openstack/standalone-cinder/openstack.go:187:9:warning: if block ends with a return statement, so drop this else and outdent its block (golint)
openstack/standalone-cinder/provisioner.go:39:2:warning: const cinderVolumeId should be cinderVolumeID (golint)
openstack/standalone-cinder/provisioner.go:88:2:warning: var volumeId should be volumeID (golint)
openstack/standalone-cinder/provisioner.go:139:2:warning: var volumeId should be volumeID (golint)
openstack/standalone-cinder/provisioner.go:188:2:warning: var prId should be prID (golint)
FAILED   /home/travis/gopath/src/github.com/kubernetes-incubator/external-storage/repo-infra/verify/go-tools/verify-golint.sh	0s
Verifying /home/travis/gopath/src/github.com/kubernetes-incubator/external-storage/repo-infra/verify/go-tools/verify-gometalinter.sh
openstack/standalone-cinder/mapper.go:169:1:warning: mapperContext is unused (deadcode)
openstack/standalone-cinder/openstack.go:144::warning: declaration of "err" shadows declaration at openstack/standalone-cinder/openstack.go:139 (vetshadow)
FAILED   /home/travis/gopath/src/github.com/kubernetes-incubator/external-storage/repo-infra/verify/go-tools/verify-gometalinter.sh	6s

@wongma7
Copy link
Contributor

wongma7 commented Sep 13, 2017

feel free to add directories to .golintignore.... imo menial task of fixing golint should not block merging of the PR, it is something we can fix later as we did with snapshots.

config, err = rest.InClusterConfig()
}
prId := string(uuid.NewUUID())
if *id != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I'll change it to use the provisionerName

return ret, nil
}

func (m *iscsiMapper) AuthSetup(ctx provisionCtx) error {
Copy link
Contributor

@rootfs rootfs Sep 13, 2017

Choose a reason for hiding this comment

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

iSCSI CHAP is only in 1.7+, check server git version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should I handle the kubernetes version dependencies for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

glog.Errorf("Failed to connect volume: %v", err)
return nil, err
}

Copy link
Contributor

Choose a reason for hiding this comment

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

reserve the volume

Copy link
Contributor Author

@aglitke aglitke Sep 13, 2017

Choose a reason for hiding this comment

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

I would like to add TODOs in the code for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

sgtm

}

mapper.AuthTeardown(ctx)

Copy link
Contributor

Choose a reason for hiding this comment

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

unreserve the volume

@rootfs
Copy link
Contributor

rootfs commented Sep 13, 2017

also need to import blockstorage extensions github.com/gophercloud/gophercloud/openstack/extensions/volumeactions

return nil, errors.New("No monitors could be parsed from connection info")
}
splitName := strings.SplitN(ctx.connection.Data.Name, "/", 2)

Copy link
Contributor

Choose a reason for hiding this comment

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

validate len(splitName)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

@rootfs
Copy link
Contributor

rootfs commented Sep 13, 2017

please also provide a deployment and docker file

@rootfs
Copy link
Contributor

rootfs commented Sep 13, 2017

/approve

@rootfs rootfs self-assigned this Sep 13, 2017
@aglitke
Copy link
Contributor Author

aglitke commented Sep 13, 2017

Sorry guys. A bit new to the github PR process and the code validation tools for this project.

@rootfs
Copy link
Contributor

rootfs commented Sep 13, 2017

build error

make[1]: Entering directory `/home/travis/gopath/src/github.com/kubernetes-incubator/external-storage/ceph/cephfs'
CGO_ENABLED=0 GOOS=linux go build -a -ldflags '-extldflags "-static"' -o cephfs-provisioner cephfs-provisioner.go
# github.com/kubernetes-incubator/external-storage/vendor/golang.org/x/sys/unix
../../vendor/golang.org/x/sys/unix/zsyscall_linux_amd64.go:1491: Msync redeclared in this block
	previous declaration at ../../vendor/golang.org/x/sys/unix/zsyscall_linux_amd64.go:1459
make[1]: *** [build] Error 2

@aglitke
Copy link
Contributor Author

aglitke commented Sep 13, 2017

To resolve that build error I think I am going to have to close this PR and start over. I resolved some vendor dependencies using the github merge ui and I think that broke things.

@k8s-ci-robot k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 15, 2017
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 15, 2017
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 15, 2017
@rootfs
Copy link
Contributor

rootfs commented Sep 15, 2017

@aglitke add a Makefile in openstack/standalone-cinder and update top level Makefile (when vendor is updated)

@aglitke
Copy link
Contributor Author

aglitke commented Sep 18, 2017

I think this is ready now?

.PHONY: all provisoner

all: provisioner

Copy link
Contributor

Choose a reason for hiding this comment

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

also add a container target

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 19, 2017
rootfs and others added 19 commits September 19, 2017 10:40
In this patch, we introduce a basic cinder provisioner that will
dynamically provision cinder volumes and expose them as PVs with native
k8s support (such as iscsi).  This initial commit introduces the basic
structure but does not yet populate iscsi connection parameters into the
PV.

Signed-off-by: Huamin Chen <hchen@redhat.com>
Signed-off-by: Adam Litke <alitke@redhat.com>
Gather connection information by calling cinder InitializeConnection and
populate a PV so that it will work with k8s native iscsi PV support.
Also support deleting volumes from cinder when requested.

Signed-off-by: Adam Litke <alitke@redhat.com>
In order to prepare to support other volume types such as rbd, separate
the type-specific logic into an interface.  Now, new types can be added
by creating a new implementation of the interface as now exists in
iscsi.go.

Signed-off-by: Adam Litke <alitke@redhat.com>
Interfaces cannot contain fields and the code previously attempted to
approximate inheritance from other languages by using a base struct with
an embedded interface and then embedding that base struct into the
actual volume types.  This does not work.  Do it in a more idiomatic way
by making the volume types pure interfaces and then passing a context
struct into methods that need inputs.

Signed-off-by: Adam Litke <alitke@redhat.com>
Create a new rbdMapper that implements the volumeMapper interface.  We
currently assume that the cluster uses cephx authentication.  We
configure the PV to look for a secret named
<storage class name>-cephx-secret of type "kubernetes.io/rbd" that
contains the appropriate key.

Signed-off-by: Adam Litke <alitke@redhat.com>
Since this provisioner is designed to work outside of the cloudprovider
context we should not be using that infrastructure.  Instead, use the
gophercloud openstack SDK directly.  This also allows us to support
noauth authentication strategy in a to be delivered gophercloud
enhancement[1].

[1] https://github.com/tchughesiv/gophercloud/commits/cinder-noauth

Signed-off-by: Adam Litke <alitke@redhat.com>
Signed-off-by: Adam Litke <alitke@redhat.com>
When we receive connection information without CHAP authentication, we
do not need a chap secret and chap auth should be disabled in the
created PV.

Signed-off-by: Adam Litke <alitke@redhat.com>
VolumeId is not returned by all cinder volume drivers when connecting
the volume.  Pass the Id received when we created the volume to BuildPV.

Signed-off-by: Adam Litke <alitke@redhat.com>
Document the design of the cinder provisioner
Kubernetes already has a cinder provisioner which is designed to work
with the openstack cloud provider for clusters which are deployed on
openstack instances.  To avoid confusion, rename this provisioner to
standalone-cinder.

Signed-off-by: Adam Litke <alitke@redhat.com>
Signed-off-by: Adam Litke <alitke@redhat.com>
By default, external provisioners are using their name as the
provisioner ID.  Adopt this convention.

Signed-off-by: Adam Litke <alitke@redhat.com>
We split the name field in the connection info to get the pool and
image.  Make sure splitting on '/' yields two components.

Signed-off-by: Adam Litke <alitke@redhat.com>
In order to prevent other cinder users from clobbering volumes that we
have created for kubernetes we reserve volumes before connecting them
and unreserve them after disconnect (before deletion).

Signed-off-by: Adam Litke <alitke@redhat.com>
Signed-off-by: Adam Litke <alitke@redhat.com>
- Add make targets for standalone-cinder
- Update the standalone-cinder provisioner to support podified
  deployments:
  - Update the build to produce a docker image
  - Build the provisioner with static flags
  - Allow openstack parameters to be specified in the environment
  - Provide sample deployment.yaml

TODO: Acquire OS_PASSWORD from secret

Signed-off-by: Adam Litke <alitke@redhat.com>
Signed-off-by: Adam Litke <alitke@redhat.com>
Signed-off-by: Adam Litke <alitke@redhat.com>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 19, 2017
@aglitke
Copy link
Contributor Author

aglitke commented Sep 19, 2017

I fixed the gophercloud/snapshot vendor issue by specifying today's gophercloud HEAD as the requested version in glide.yaml. For some reason glide was choosing an older version implicitly.

The provisioner works by mapping cinder volume connections
(iscsi, rbd, fc, etc) to the corresponding native/raw kubernetes
volume types. New cinder types can be supported in the provisioner
by creating a new implementation of the volunmeMapper interface. The
Copy link
Contributor

Choose a reason for hiding this comment

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

s/volunmeMapper/volumeMapper

providing the service catalog, and a standalone configuration where
cinder is accessed directly.

Conventional cinder deployments can be used by supplying a clound
Copy link
Contributor

Choose a reason for hiding this comment

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

s/clound/cloud

@wongma7
Copy link
Contributor

wongma7 commented Sep 19, 2017

merging: please fix typos in a follow-up! thanks all!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants