Add Container Storage Interface (CSI) #13435

Binyang2014 · 2021-05-18T03:42:40Z

What changes are proposed in this pull request?

This PR integrates Alluxio CSI from community maintained repo (https://github.com/Alluxio/alluxio-csi) to main repo, with further improvement

Why are the changes needed?

Original CSI repo is not as well curated and managed as the main repo.
Given we have more

Does this PR introduce any user facing changes?

Yes, see the following example

Deploy

Go to deploy folder, Run following commands:

kubectl apply -f csi-alluxio-driver.yml
kubectl apply -f csi-alluxio-daemon.yml

Example Nginx application

The /examples folder contains PersistentVolume, PersistentVolumeClaim and an nginx Pod mounting the alluxio volume under /data.

You will need to update the alluxio MASTER_HOST_NAME and the share information under volumeAttributes in alluxio-pv.yaml file to match your alluxio master configuration.

Run following commands to create nginx pod mounted with alluxio volume:

kubectl apply -f alluxio-pv.yml
kubectl apply -f alluxio-pvc.yml
kubectl apply -f nginx.yml

alluxio-bot · 2021-05-18T03:42:45Z

Automated checks report:

Commits associated with Github account: PASS
PR title follows the conventions: FAIL
- The title of the PR does not pass all the checks. Please fix the following issues:
  - First word of title ("Integral") is not an imperative verb. Please use one of the valid words

Some checks failed. Please fix the reported issues and reply 'alluxio-bot, check this please' to re-run checks.

codecov-commenter · 2021-05-18T03:57:11Z

Codecov Report

Merging #13435 (1002c6a) into master (624d846) will increase coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #13435      +/-   ##
============================================
+ Coverage     43.66%   43.70%   +0.04%     
- Complexity     9061     9071      +10     
============================================
  Files          1384     1384              
  Lines         80308    80308              
  Branches       9768     9768              
============================================
+ Hits          35063    35097      +34     
+ Misses        42309    42276      -33     
+ Partials       2936     2935       -1

Impacted Files	Coverage Δ
...uxio/master/FaultTolerantAlluxioMasterProcess.java	`48.95% <0.00%> (-8.34%)`	⬇️
...on/src/main/java/alluxio/util/JvmPauseMonitor.java	`76.92% <0.00%> (-2.20%)`	⬇️
...main/java/alluxio/concurrent/jsr/ForkJoinPool.java	`30.62% <0.00%> (-1.85%)`	⬇️
...a/alluxio/master/file/DefaultFileSystemMaster.java	`64.70% <0.00%> (+0.04%)`	⬆️
...main/java/alluxio/concurrent/jsr/ForkJoinTask.java	`5.92% <0.00%> (+0.24%)`	⬆️
...ava/alluxio/multi/process/MultiProcessCluster.java	`60.16% <0.00%> (+0.26%)`	⬆️
...alluxio/master/journal/raft/RaftJournalSystem.java	`64.33% <0.00%> (+0.48%)`	⬆️
...e/common/src/main/java/alluxio/metrics/Metric.java	`59.16% <0.00%> (+0.83%)`	⬆️
...io/master/file/meta/InodeTreeBufferedIterator.java	`76.63% <0.00%> (+0.93%)`	⬆️
...luxio/exception/status/AlluxioStatusException.java	`25.00% <0.00%> (+1.08%)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 624d846...1002c6a. Read the comment docs.

Binyang2014 · 2021-05-18T04:36:30Z

List the requirements here

Move alluxio-csi as a module of alluxio repository
Implements the dynamic provisioning and generate csi related yaml files when using helm-generate.sh
Using an all-in-one image in whole alluxio. @maobaolong Will not implement in this PR, @apc999 For this, need to merge fuse image and alluxio image first

alluxio-bot · 2021-05-18T04:42:35Z

Automated checks report:

Commits associated with Github account: PASS
PR title follows the conventions: PASS

All checks passed!

Binyang2014 · 2021-05-19T06:07:17Z

@maobaolong Please help to review.
I am not familiar with helm chart part, may missing some file changes

maobaolong

Thank you for bring alluxio-csi to alluxio repo, left some comments, please take a look.

integration/csi/Dockerfile

maobaolong · 2021-06-01T01:08:28Z

integration/csi/README.md

@@ -0,0 +1,80 @@
+# Alluxio CSI
+
+This project implement contienr storage interface(https://github.com/container-storage-interface/spec) for Alluxio.


This project -> This module

integration/csi/alluxio/controllerserver.go

maobaolong · 2021-06-01T01:14:14Z

integration/csi/alluxio/driver.go

+)
+
+const (
+	driverName = "alluxio"


Is this a hardcode?

Sure, driver name is hardcode. k8s will use this name to find correct csi-controller and node-plugin. And storage and driver should have one to one relationship.

maobaolong · 2021-06-01T01:17:30Z

integration/csi/alluxio/nodeserver.go

+	command := exec.Command("/opt/alluxio/integration/fuse/bin/alluxio-fuse", args...)
+
+	masterConfig := ""
+	if masterHost, ok := req.GetVolumeContext()["alluxio.master.hostname"]; ok {


How to specify a alluxio.master.hostname?

Under parameters filed of storage class
such as:

apiVersion: storage.k8s.io/v1 kind: StorageClass metadata: name: alluxio provisioner: alluxio parameters: alluxio.master.hostname: master_ip volumeBindingMode: Immediate

But this can only be set in javaOptions filed. Add this option here just for backward compatibility

maobaolong · 2021-06-01T01:22:56Z

integration/csi/alluxio/nodeserver.go

+	   https://github.com/Alluxio/alluxio/blob/master/integration/fuse/bin/alluxio-fuse
+	*/
+
+	alluxioPath := req.GetVolumeContext()["alluxioPath"]


I would be better to have the following logic

Create an under storage space, for example ozone, s3, ceph bucket, or hdfs, cephfs folder.

Create an alluxio mount point to the new under storage space.

When NodeUnpublishVolume, clear the new under storage space and umount the mount point.

I think ozone, s3, ceph is the under file system, only admin need to aware about this. So admin can create such folder in alluxio filesystem namespace, and mount different UFS. And provide correct storageClass such as: alluxio-ozon, alluxio-s3, alluxio-ceph.
For end user, if they want to use the storage, he/she can decide to use which storageClass or use many of them. And mount to different container path according to POD config

integration/csi/go.mod

maobaolong · 2021-06-01T01:24:12Z

integration/csi/go.sum

@@ -0,0 +1,481 @@
+cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=


Is this file necessary?

It's auto generated, we should not change this file

integration/kubernetes/helm-chart/alluxio/templates/csi/driver.yaml

maobaolong · 2021-06-01T01:29:13Z

integration/kubernetes/helm-chart/alluxio/templates/csi/client-sc-nginx.yaml

+{{ if .Values.csi.clientEnabled -}}
+---
+apiVersion: storage.k8s.io/v1
+kind: StorageClass


It would be better to abstract the storageClass to an individual file

Binyang2014 · 2021-06-01T02:12:04Z

@maobaolong For end of line issue, if there no "\n" at the end of file, github will show:

maobaolong · 2021-06-02T01:33:47Z

@Binyang2014 Sorry, got it, thanks for share this tip.

LuQQiu

Left some minor comments, LGTM

LuQQiu · 2021-06-15T00:56:12Z

integration/csi/README.md

@@ -0,0 +1,79 @@
+# Alluxio CSI
+
+This module implement contienr storage interface(https://github.com/container-storage-interface/spec) for Alluxio.


contienr -> container

LuQQiu · 2021-06-15T00:57:49Z

integration/csi/README.md

+
+## Requirements
+
+Kubernetes 1.14 or higher, RBAC enbaled in API server.


Add the whole name and link for RBAC since it's a name quite unfamiliar for users include me

LuQQiu · 2021-06-15T00:58:11Z

integration/csi/README.md

+## Usage
+
+### Deploy
+Please use `helm-generate.sh` to generate related templates. All CSI related templates should under `integration/kubernetes/<deploy-mode>/csi` folder.


Add line between Deploy and Please

LuQQiu · 2021-06-15T00:59:12Z

integration/csi/README.md

+
+You can custmomize alluxio volumes via serveral configurations.
+
+The options you can customized:


LuQQiu · 2021-06-15T00:59:40Z

integration/csi/README.md

+| Options | Description |
+| --- | --- |
+| `alluxioPath` | The path in alluxio |
+| `javaOptions` | The customized options which pass to fuse daemon |


which pass -> which will be passed

LuQQiu · 2021-06-15T01:07:21Z

integration/csi/main.go

+}
+
+/*
+Based on https://github.com/openshift/origin/blob/master/pkg/util/proc/reaper.go


this link cannot be opened, try using a permanent link instead of the master branch link

Binyang2014 · 2021-06-15T05:39:05Z

@LuQQiu @apc999.
Still has some items left.

Currently, we don't build the CSI image and push it to docker-hub. So customers need to build the CSI image by themselves. We need to provide CSI image or integral csi into fuse image
For helm chart part. May need more tests, and I want to make CSI as a deployment option. I am not sure if current implementation is correct

maobaolong · 2021-06-15T07:13:35Z

@Binyang2014 If we can, I hope we can maintain an all-in-one alluxio image, it include alluxio main services, fuse and csi in the future. For this PR, it looks good now.

LuQQiu · 2021-06-17T03:06:22Z

@Binyang2014 @maobaolong We plan to deprecate the alluxio-fuse image since the current alluxio docker image contains the fuse logics as well.
@Binyang2014 what's needed for alluxio docker image to include the CSI logics? will it have a large impact on the docker image size? Agreed with @maobaolong that we would be better to maintain a All-in-one image if the impact is acceptable.

@jiacheliu3 can you help review the helm-chart related code in this PR? and give @Binyang2014 some suggestions for how to test them out?

Binyang2014 · 2021-06-17T06:31:19Z

@LuQQiu We need to put CSI binary into image, it just about 13MB.

LuQQiu · 2021-06-17T21:17:50Z

@Binyang2014 then it's totally acceptable to put it in the docker image. Are the CSI binaries included in this PR, or how can we build the CSI binaries?

apc999 · 2021-06-17T21:35:58Z

@jiacheliu3 can you take a look at the helmchart part?

Binyang2014 · 2021-06-18T02:52:23Z

@LuQQiu Please refer this dockerfile https://github.com/Binyang2014/alluxio/blob/binyli%2Fcsi/integration/csi/Dockerfile

LuQQiu · 2021-06-18T16:34:54Z

@Binyang2014 can you help move the CSI DockerFile logics into Alluxio DockerFile in this PR or the future PRs and update the related documentations?

Binyang2014 · 2021-06-22T03:00:18Z

@Binyang2014 can you help move the CSI DockerFile logics into Alluxio DockerFile in this PR or the future PRs and update the related documentations?

Doc updated. Will create an other PR to put CSi bit into alluxio Dockerfile.

jiacheliu3

Thanks @Binyang2014 ! Sorry about the late review. My comments are majorly about parameterizing in the helm chart you added. I think it makes sense to be able to configure a few of them. I'm open to discussions :)

integration/csi/Dockerfile

integration/csi/README.md

jiacheliu3 · 2021-06-25T03:04:16Z

integration/kubernetes/helm-chart/alluxio/templates/csi/controller.yaml

+        heritage: {{ .Release.Service }}
+        role: alluxio-csi-controller
+    spec:
+      hostNetwork: true


I'm not sure if there's a need to enable hostNetwork=false?

integration/kubernetes/helm-chart/alluxio/templates/csi/controller.yaml

jiacheliu3 · 2021-06-25T03:05:34Z

integration/kubernetes/helm-chart/alluxio/templates/csi/driver.yaml

+  attachRequired: false
+  podInfoOnMount: true


Not sure if these should be parameterized?

These parameters used by developer. Admin/users don't need to take care

integration/kubernetes/helm-chart/alluxio/templates/csi/pv.yaml

integration/kubernetes/helm-chart/alluxio/templates/csi/pvc-static.yaml

integration/kubernetes/helm-chart/alluxio/templates/csi/pvc.yaml

integration/kubernetes/helm-chart/alluxio/templates/csi/pv.yaml

integration/kubernetes/helm-chart/alluxio/templates/csi/pvc.yaml

Co-authored-by: Jiacheng Liu <jiacheliu3@gmail.com>

Binyang2014 · 2021-07-01T03:43:05Z

@jiacheliu3 Thanks for review, please take a look again.

jiacheliu3

Thanks @Binyang2014 ! Left a minor comment, LGTM. If you could resolve that minor comment that'd be even better :)

integration/kubernetes/helm-chart/alluxio/templates/csi/nodeplugin.yaml

Binyang2014 · 2021-07-01T07:22:32Z

Thanks @Binyang2014 ! Left a minor comment, LGTM. If you could resolve that minor comment that'd be even better :)

Add comments for these fields. Thx

LuQQiu · 2021-07-01T18:59:06Z

Error: 9.429 [ERROR] Errors:
Error: 9.429 [ERROR] alluxio.master.journal.raft.RaftJournalTest.gainPrimacyAfterCatchup
Error: 9.430 [ERROR] Run 1: RaftJournalTest.gainPrimacyAfterCatchup:390->promoteFollower:404 » Timeout Tim...
Error: 9.431 [ERROR] Run 2: RaftJournalTest.gainPrimacyAfterCatchup:390->promoteFollower:404 » Timeout Tim...
07:44:09.434 [INFO]
Error: 9.435 [ERROR] alluxio.master.journal.raft.RaftJournalTest.gainPrimacyAfterSuspend
Error: 9.435 [ERROR] Run 1: RaftJournalTest.gainPrimacyAfterSuspend:350->promoteFollower:404 » Timeout Tim...
Error: 9.435 [ERROR] Run 2: RaftJournalTest.gainPrimacyAfterSuspend:350->promoteFollower:404 » Timeout Tim...

LuQQiu · 2021-07-01T19:00:33Z

@Binyang2014 Can you help merge master into your csi branch and update this PR? I doubt that the RaftJournalTest is already fixed in the master branch.

apc999 · 2021-07-03T01:52:12Z

alluxio-bot, merge this please

## What changes are proposed in this pull request? This PR integrates Alluxio CSI from community maintained repo (https://github.com/Alluxio/alluxio-csi) to main repo, with further improvement ## Why are the changes needed? Original CSI repo is not as well curated and managed as the main repo. Given we have more ## Does this PR introduce any user facing changes? Yes, see the following example ### Deploy Go to `deploy` folder, Run following commands: ```bash kubectl apply -f csi-alluxio-driver.yml kubectl apply -f csi-alluxio-daemon.yml ``` ### Example Nginx application The `/examples` folder contains `PersistentVolume`, `PersistentVolumeClaim` and an nginx `Pod` mounting the alluxio volume under `/data`. You will need to update the alluxio `MASTER_HOST_NAME` and the share information under `volumeAttributes` in `alluxio-pv.yaml` file to match your alluxio master configuration. Run following commands to create nginx pod mounted with alluxio volume: ```bash kubectl apply -f alluxio-pv.yml kubectl apply -f alluxio-pvc.yml kubectl apply -f nginx.yml ``` pr-link: Alluxio#13435 change-id: cid-9f2d2c1b6723421861822388c73fb41e88a10e94

Binyang2014 mentioned this pull request May 18, 2021

Supply Alluxio csi implementation #13363

Closed

Binyang2014 changed the title ~~Integral CSI into alluxio repo~~ integrate CSI into alluxio repo May 18, 2021

Binyang2014 changed the title ~~integrate CSI into alluxio repo~~ Integrate CSI into alluxio repo May 18, 2021

alluxio-bot assigned jiacheliu3 May 18, 2021

integrate csi

bca65a5

Binyang2014 force-pushed the binyli/csi branch from 65a6bee to bca65a5 Compare May 18, 2021 15:12

add readme

17497c2

Binyang2014 marked this pull request as ready for review May 19, 2021 06:06

Binyang2014 mentioned this pull request May 26, 2021

add storage class support Alluxio/alluxio-csi#6

Draft

maobaolong reviewed Jun 1, 2021

View reviewed changes

Binyang2014 added 3 commits June 4, 2021 02:37

mergemaster

d0306d0

Fix review comments

1c898b1

upgrade go vversion to 1.15

7ac673e

Binyang2014 force-pushed the binyli/csi branch from 871db2d to 7ac673e Compare June 4, 2021 03:15

apc999 approved these changes Jun 15, 2021

View reviewed changes

LuQQiu approved these changes Jun 15, 2021

View reviewed changes

fix comments

c4dcb51

update doc

01e2e11

jiacheliu3 requested changes Jun 25, 2021

View reviewed changes

Binyang2014 and others added 2 commits June 30, 2021 18:27

Update integration/csi/README.md

917c5ca

Co-authored-by: Jiacheng Liu <jiacheliu3@gmail.com>

fix comments

d3f0647

jiacheliu3 approved these changes Jul 1, 2021

View reviewed changes

integration/kubernetes/helm-chart/alluxio/templates/csi/nodeplugin.yaml Show resolved Hide resolved

fix comments

9cd6d49

Binyang2014 force-pushed the binyli/csi branch from 360e86b to 9cd6d49 Compare July 1, 2021 07:20

Merge branch 'master' into binyli/csi

1002c6a

apc999 changed the title ~~Integrate CSI into alluxio repo~~ Add Container Storage Interface (CSI) Jul 3, 2021

alluxio-bot merged commit 3ec7ef9 into Alluxio:master Jul 3, 2021

		@@ -0,0 +1,80 @@
		# Alluxio CSI

		This project implement contienr storage interface(https://github.com/container-storage-interface/spec) for Alluxio.

		@@ -0,0 +1,481 @@
		cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=

		@@ -0,0 +1,79 @@
		# Alluxio CSI

		This module implement contienr storage interface(https://github.com/container-storage-interface/spec) for Alluxio.


		## Requirements

		Kubernetes 1.14 or higher, RBAC enbaled in API server.


		You can custmomize alluxio volumes via serveral configurations.

		The options you can customized:

Add Container Storage Interface (CSI) #13435

Add Container Storage Interface (CSI) #13435

Conversation

Binyang2014 commented May 18, 2021 • edited by apc999 Loading

What changes are proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing changes?

Deploy

Example Nginx application

alluxio-bot commented May 18, 2021

codecov-commenter commented May 18, 2021 • edited Loading

Codecov Report

Binyang2014 commented May 18, 2021 • edited Loading

alluxio-bot commented May 18, 2021

Binyang2014 commented May 19, 2021

maobaolong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Binyang2014 Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Binyang2014 commented Jun 1, 2021

maobaolong commented Jun 2, 2021

LuQQiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Binyang2014 commented Jun 15, 2021

maobaolong commented Jun 15, 2021

LuQQiu commented Jun 17, 2021

Binyang2014 commented Jun 17, 2021

LuQQiu commented Jun 17, 2021

apc999 commented Jun 17, 2021

Binyang2014 commented Jun 18, 2021

LuQQiu commented Jun 18, 2021

Binyang2014 commented Jun 22, 2021

jiacheliu3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Binyang2014 commented Jul 1, 2021

jiacheliu3 left a comment

Choose a reason for hiding this comment

Binyang2014 commented Jul 1, 2021

LuQQiu commented Jul 1, 2021

LuQQiu commented Jul 1, 2021

apc999 commented Jul 3, 2021

Binyang2014 commented May 18, 2021 •

edited by apc999

Loading

codecov-commenter commented May 18, 2021 •

edited

Loading

Binyang2014 commented May 18, 2021 •

edited

Loading

Binyang2014 Jun 1, 2021 •

edited

Loading