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

Deploy Cinder CSI driver to provision volumes over OpenStack #5184

Merged

Conversation

alijahnas
Copy link
Contributor

@alijahnas alijahnas commented Sep 18, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
Deploys Cinder CSI driver in order to provision volumes from OpenStack Cinder.

Which issue(s) this PR fixes:
Fixes #5183

Special notes for your reviewer:
I reused parts of the code from OpenStack internal provisioner and renamed variables to from openstack_ to cinder_ in order to have backward compatibility and the possibility to use both in-tree OpenStack provisioner and Cinder CSI driver.
I used the info from https://github.com/kubernetes/cloud-provider-openstack/blob/master/docs/using-cinder-csi-plugin.md to implement and test the driver.

roles/kubernetes-apps/persistent_volumes/cinder is coming in another PR.
Documentation is coming if needed.

Does this PR introduce a user-facing change?:
None

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 18, 2019
@alijahnas
Copy link
Contributor Author

@holmsten @mirwan
Hi! The failure in Kubespray CI - tf-ovh_ubuntu18-calico doesn't seem to be linked to what I propose, am I wrong? Thanks for the review!

@alijahnas alijahnas removed their assignment Sep 24, 2019
@Miouge1
Copy link
Contributor

Miouge1 commented Sep 25, 2019

Thank you for the contribution. I'll retrigger that CI job, the error might be related to an outside cause.

Regarding the content of the PR, is that the external openstack provider?

@alijahnas
Copy link
Contributor Author

Hi @Miouge1 thanks for the reply 👍

It's kind of the new external openstack provider for volumes.

Cloud controllers (like the OpenStack one) should run outside Kubernetes now, but even though they are, the volume provisioning isn't done by the external cloud controller, as specified here: https://kubernetes.io/docs/tasks/administer-cluster/running-cloud-controller/#support-for-volumes

The right way to implement cloud volume provisioning in Kubernetes now is through CSI drivers: https://kubernetes-csi.github.io/docs/introduction.html

The OpenStack Cinder CSI driver is part of the openstack external cloud controller project: https://github.com/kubernetes/cloud-provider-openstack/blob/master/docs/using-cinder-csi-plugin.md

I can also implement the OpenStack external cloud controller in Kubespray if you would like, it shouldn't be much effort. But it would only be used for Load Balancing. The Volume provisioning is in the CSI driver now.

Ask me if you want more details about it.
Thanks

@Miouge1
Copy link
Contributor

Miouge1 commented Sep 25, 2019

Great! I was looking into https://github.com/kubernetes/cloud-provider-openstack last week.

I'm definitely interested in getting this in and deprecating the in-tree cloud provider.

Will there be a migration path from the in-tree provider to the out of tree provider?

@alijahnas
Copy link
Contributor Author

Using CSI driver doesn't conflict with the old in-tree cloud provider as the provisioner has another name. By the way the code for the csi provisioner is ready on my kubespray fork. I am just waiting for this PR to be in.

For the in-tree to out-tree cloud provider OpenStack, the configuration during KubeADM should be changed so that the right flags are set on kube-controller and kube-apiserver. These flags will also be deprecated and removed from kubernetes once everybody is sure that they have all their calls out of tree.

@alijahnas
Copy link
Contributor Author

@Miouge1 @mirwan @holmsten
Hi ! Any updates on the review of this PR please ? Thanks

Copy link
Contributor

@Miouge1 Miouge1 left a comment

Choose a reason for hiding this comment

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

I have tested this and it works well. To be noted that the kind: StorageClass needs to be created with provisioner: cinder.csi.openstack.org.

I suggest to do something like in roles/kubernetes-apps/persistent_volumes/openstack/templates/openstack-storage-class.yml.j2 to make it easier to consumers.

Also I wonder how people who are currently using the in-tree provider will be able to transition. Can we add a note somewhere in a .md file for that?

name: csi-cinder-controllerplugin
namespace: kube-system
spec:
replicas: 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make replicas configurable with a group_var?

inventory/sample/group_vars/all/all.yml Outdated Show resolved Hide resolved
@alijahnas
Copy link
Contributor Author

I have tested this and it works well. To be noted that the kind: StorageClass needs to be created with provisioner: cinder.csi.openstack.org.

I suggest to do something like in roles/kubernetes-apps/persistent_volumes/openstack/templates/openstack-storage-class.yml.j2 to make it easier to consumers.

Also I wonder how people who are currently using the in-tree provider will be able to transition. Can we add a note somewhere in a .md file for that?

Hi,
I have a branch in my fork that implements the provisioner as you suggest: https://github.com/alijahnas/kubespray/tree/feature/cinder-csi-provisioner
I was just waiting for this PR to merge so that I can rebase and make a new PR for the provisioner to separate things.

I can write some doc to ease the usage and transition.

@Miouge1
Copy link
Contributor

Miouge1 commented Oct 11, 2019

Let's try to get some basic doc and the StorageClass thing in this PR if you don't mind.

@alijahnas
Copy link
Contributor Author

@Miouge1 Ok no worries, I'll try to do it as fast as I can cause I will be out of office for a week from tomorrow with limited access to electronics, other than what is found on a sailing ship :D

@alijahnas
Copy link
Contributor Author

alijahnas commented Oct 11, 2019

@Miouge1
I added the provisioner and written the documentation.

@alijahnas
Copy link
Contributor Author

@Miouge1
Hi!
I don't think the errors are related to the code I uploaded.

@Miouge1
Copy link
Contributor

Miouge1 commented Oct 22, 2019

@alijahnas OK I'm re-running that job 🤞

@alijahnas
Copy link
Contributor Author

Hi @Miouge1
The CI platform doesn't want to show me love with beautiful green checks. The red crosses are not my fault >_<

@holmsten
Copy link
Contributor

@alijahnas can you rebase from master?

@alijahnas
Copy link
Contributor Author

@alijahnas can you rebase from master?

Done, thanks!

@holmsten
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2019
@riverzhang
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alijahnas, riverzhang

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 1, 2019
@k8s-ci-robot k8s-ci-robot merged commit b0ee1f6 into kubernetes-sigs:master Nov 1, 2019
@alijahnas alijahnas deleted the feature/cinder-csi-plugin branch November 3, 2019 22:02
erulabs added a commit to kubesail/kubespray that referenced this pull request Nov 18, 2019
* 'master' of https://github.com/kubernetes-sigs/kubespray: (28 commits)
  Do not run kubeadm upgrade on first deploy (kubernetes-sigs#5339)
  Added Amazon Linux 2 support for deploying with docker (kubernetes-sigs#5301)
  Set certs and files with kubeadm token to mode 0640 (kubernetes-sigs#5325)
  Fix calico name resolution (kubernetes-sigs#5291)
  library files added to setup.cfg (kubernetes-sigs#5274)
  Fix helm for Kubernetes 1.16.2 (kubernetes-sigs#5332)
  Pass `ingress_nginx_extra_args` when deploying the nginx-ingress addon (kubernetes-sigs#5321)
  Respect cri-tool supported version matrix (kubernetes-sigs#5241)
  fix useradd etcd (kubernetes-sigs#5281)
  Fix indentation in cilium-ds.yml template (kubernetes-sigs#5305)
  Remove default docker_options from sample (kubernetes-sigs#5287)
  Add cilium_tunnel_mode variable to the cilium config (kubernetes-sigs#5295)
  Fixes issue kubernetes-sigs#5299 (kubernetes-sigs#5300)
  containerd installation on Debian (kubernetes-sigs#5326)
  Skip coredns image when evaluating kubeadm images (kubernetes-sigs#5327)
  [FIX] fix incorrect link to downloads documentation (kubernetes-sigs#5319)
  Remove unnecessary and risky reload network for resolvconf propagation (kubernetes-sigs#5322)
  Enable systemd-resolved DNS resolution mode (kubernetes-sigs#5318)
  Scale down coredns created by kubeadm upgrade to 0 replicas (kubernetes-sigs#5308)
  Deploy Cinder CSI driver to provision volumes over OpenStack (kubernetes-sigs#5184)
  ...
@Miouge1 Miouge1 mentioned this pull request Dec 5, 2019
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Dec 9, 2019
…tes-sigs#5184)

* Deploy Cinder CSI driver to provision volumes over OpenStack

* Deploy Cinder CSI StorageClass

* Cinder CSI doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for OpenStack Cinder CSI driver
5 participants