Add Tencent GooseFS Cache Engine to fluid backend #912

xieydd · 2021-07-09T04:17:40Z

Signed-off-by: chrisydxie chrisydxie@tencent.com

Ⅰ. Describe what this PR does

Add support for tencent goosefs engine for fluid backend.
Proposal can be found in https://docs.google.com/document/d/1t6wQNP1X7TDlWVZCq0dURK0msIAAN5XWmR-ubIWju6Q/edit#heading=h.8nkpj2y22jpd.

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

In next few pr, i will add addation test case for improving the test cover rate.

Ⅴ. Special notes for reviews

@cheyang @RongGu @TrafalgarZZZ cc

codecov · 2021-07-09T09:41:06Z

Codecov Report

Merging #912 (5db4011) into master (df7e5cf) will increase coverage by 3.18%.
The diff coverage is 43.57%.

❗ Current head 5db4011 differs from pull request most recent head 681a908. Consider uploading reports for the commit 681a908 to get more accurate results

@@            Coverage Diff             @@
##           master     #912      +/-   ##
==========================================
+ Coverage   43.57%   46.76%   +3.18%     
==========================================
  Files         122      162      +40     
  Lines        6079     8780    +2701     
==========================================
+ Hits         2649     4106    +1457     
- Misses       3179     4221    +1042     
- Partials      251      453     +202

Impacted Files	Coverage Δ
pkg/ddc/alluxio/shutdown.go	`22.59% <ø> (ø)`
pkg/ddc/goosefs/create_volume.go	`0.00% <0.00%> (ø)`
pkg/ddc/goosefs/operations/cached.go	`0.00% <0.00%> (ø)`
pkg/ddc/goosefs/operations/conf.go	`0.00% <0.00%> (ø)`
pkg/ddc/goosefs/replicas.go	`0.00% <0.00%> (ø)`
pkg/ddc/goosefs/status.go	`0.00% <0.00%> (ø)`
pkg/ddc/goosefs/transform.go	`0.00% <0.00%> (ø)`
pkg/ddc/goosefs/transform_hadoop_config.go	`0.00% <0.00%> (ø)`
pkg/ddc/goosefs/types.go	`100.00% <ø> (ø)`
pkg/ddc/goosefs/types_selector.go	`90.00% <ø> (ø)`
... and 90 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 df7e5cf...681a908. Read the comment docs.

allenhaozi · 2021-07-14T08:35:33Z

pkg/controllers/v1alpha1/databackup/implement.go

+	case common.ALLUXIO_RUNTIME:
+		imageName, imageTag = docker.GetWorkerImage(r.Client, databackup.Spec.Dataset, "alluxio", databackup.Namespace)
+		javaEnv = "-Dalluxio.master.hostname=" + ip + " -Dalluxio.master.rpc.port=" + strconv.Itoa(int(rpcPort))
+		runtimeType = "alluxio"


can we use common.ALLUXIO_RUNTIME as runtimeType ?

allenhaozi · 2021-07-14T12:31:08Z

pkg/controllers/v1alpha1/dataload/dataload_controller.go

+			ctx.Log.Error(err, "Failed to get the ddc runtime")
+			return utils.RequeueIfError(errors.Wrap(err, "Unable to get ddc runtime"))
+		}
+	}


suggest this error processing logic :

return ctrl.Result{},errors.Wrapf(utils.IgnoreNotFound(err),"failed to get ddc runtime %s",ctx.NamespacedName)

utils.RequeueIfError there is no need for encapsulation, and it is not very readable

allenhaozi · 2021-07-14T12:34:53Z

pkg/controllers/v1alpha1/goosefs/goosefs_runtime_controller.go

+	ctx.Log.V(1).Info("process the request", "request", req)
+
+	//	1.Load the Runtime
+	runtime, err := r.getRuntime(ctx)


getRuntime there is no need for encapsulation

Sorry, i am not clear your question. Can you describe more details? Thanks a lot.

What I mean is that getRuntime is a simple logic, and we don't really need to wrap a function

Though the logic is sample, but i think warp function is ok for me : ) . WDYT @cheyang

i think getRuntime is need for encapsulation because every developer need to implement their own getRuntime function

allenhaozi · 2021-07-14T12:37:25Z

pkg/databackup/constants.go

+	FINALIZER               = "fluid-databackup-controller-finalizer"
+	ALLUXIO_BACPUP_PATH_POD = "/alluxio_backups"
+	GOOSEFS_BACPUP_PATH_POD = "/goosefs_backups"
+	DATABACKUP_CHART        = "fluid-databackup"


CamelCase ? CodeReviewComments

I think this is a folder name, we can follow the naming habits of folders. : )

I mean the name of the variable, not the value

ok, clear, lgtm

allenhaozi · 2021-07-14T12:39:24Z

pkg/ddc/base/runtime.go

@@ -297,6 +297,17 @@ func GetRuntimeInfo(client client.Client, name, namespace string) (RuntimeInfoIn
 		}
 		runtimeInfo.SetupFuseDeployMode(jindoRuntime.Spec.Fuse.Global, jindoRuntime.Spec.Fuse.NodeSelector)
 		return runtimeInfo, nil
+	case "goosefs":


Unifies a constant called common.GooseFS ?

cheyang · 2021-07-15T03:27:13Z

Makefile

@@ -8,6 +8,7 @@ CRD_OPTIONS ?= "crd:trivialVersions=true"
 DATASET_CONTROLLER_IMG ?= registry.aliyuncs.com/fluid/dataset-controller
 ALLUXIORUNTIME_CONTROLLER_IMG ?= registry.aliyuncs.com/fluid/alluxioruntime-controller
 JINDORUNTIME_CONTROLLER_IMG ?= registry.aliyuncs.com/fluid/jindoruntime-controller
+GOOSEFSRUNTIME_CONTROLLER_IMG ?= registry.aliyuncs.com/fluid/goosefsruntime-controller


thanks for updating it.

TrafalgarZZZ · 2021-07-15T03:48:52Z

charts/fluid-dataloader/goosefs/templates/configmap.yaml

+    release: {{ .Release.Name }}
+    role: dataload-job
+data:
+  dataloader.init: |


better to change to dataloader.goosefs.init to be in parallel with the other two runtimes.

TrafalgarZZZ · 2021-07-15T04:32:42Z

pkg/ddc/alluxio/utils.go

@@ -243,7 +243,7 @@ func (e *AlluxioEngine) parseFuseImage(image string, tag string, imagePullPolicy
 }

 func (e *AlluxioEngine) GetMetadataInfoFile() string {
-	return cdatabackup.BACPUP_PATH_POD + "/" + e.GetMetadataInfoFileName()
+	return cdatabackup.ALLUXIO_BACPUP_PATH_POD + "/" + e.GetMetadataInfoFileName()


typo here. Should be ALLUXIO_BACKUP_PATH_POD and GOOSEFS_BACKUP_PATH_POD

tks, i will fix it.

TrafalgarZZZ · 2021-07-15T04:46:13Z

charts/goosefs/docker/init-users/Dockerfile

@@ -0,0 +1,7 @@
+FROM centos:centos8.2.2004


Do we really need runtime-specific scripts to do initialization like init-users.sh or chmod_fuse_mountpoint.sh?

In terms of maintainability and extensibility, I feel like runtime-specific will be better. WDYT @cheyang

yangyuliufeng

Very good code.
1、In the future, we can refactor the logic of backup to logic of engine like dataload.
2、After discuss, we decide to change the name of the constant to CamelCase
3. In the past, there were many irregularities in naming when developing AlluxioRuntime. During your developing, it’s not necessary to copy the original naming convention(such as the name is same as package)

yangyuliufeng · 2021-07-15T06:42:30Z

cmd/goosefs/main.go

+func main() {
+	if err := cmd.Execute(); err != nil {
+		fmt.Fprintf(os.Stderr, "%s", err.Error())
+		os.Exit(1)
+	}
+}
+


After the discuss, we decide to split main to multi files.
You can refer to main of webhook

it may be not in this PR. we can do this in future. Thanks.

yangyuliufeng · 2021-07-15T06:46:44Z

pkg/common/goosefs.go

+	GOOSEFS_RUNTIME = "goosefs"
+
+	GOOSEFS_MOUNT_TYPE = "fuse.goosefs-fuse"
+
+	GOOSEFS_NAMESPACE = "goosefs-system"
+
+	GOOSEFS_CHART = GOOSEFS_RUNTIME
+
+	GOOSEFS_RUNTIME_IMAGE_ENV = "GOOSEFS_RUNTIME_IMAGE_ENV"
+
+	GOOSEFS_FUSE_IMAGE_ENV = "GOOSEFS_FUSE_IMAGE_ENV"
+
+	DEFAULT_GOOSEFS_RUNTIME_IMAGE = "registry.aliyuncs.com/fluid/goosefs:v1.0.1"
+
+	DEFAULT_GOOSEFS_FUSE_IMAGE = "registry.aliyuncs.com/fluid/goosefs-fuse:v1.0.1"


After the discuss, we decide to use CamelCase in the future

yangyuliufeng · 2021-07-15T06:53:06Z

api/v1alpha1/goosefsruntime_types.go

+	RunAs *User `json:"runAs,omitempty"`
+
+	// Disable monitoring for GooseFS Runtime
+	// Promethous is enabled by default


tjs, i will fix it.

yangyuliufeng · 2021-07-15T07:03:04Z

pkg/controllers/v1alpha1/goosefs/goosefs_runtime_controller.go

+	ctx.Log.V(1).Info("process the request", "request", req)
+
+	//	1.Load the Runtime
+	runtime, err := r.getRuntime(ctx)


i think getRuntime is need for encapsulation because every developer need to implement their own getRuntime function

yangyuliufeng · 2021-07-15T07:09:46Z

pkg/ddc/goosefs/worker.go

+	}
+
+	e.Log.Info("check the desired and current replicas",
+		"desriedReplicas", replicas,


tks , i will fix it.

yangyuliufeng · 2021-07-15T07:12:39Z

pkg/ddc/goosefs/transform_init_users.go

+
+// transform dataset which has ufsPaths and ufsVolumes
+func (e *GooseFSEngine) transformInitUsers(runtime *datav1alpha1.GooseFSRuntime, value *GooseFS) {
+


I think every independent function need to check nil pointer at the beginning, not only transform.go

cheyang · 2021-07-15T07:52:02Z

pkg/ddc/alluxio/metadata.go

@@ -260,7 +260,7 @@ func (e *AlluxioEngine) syncMetadataInternal() (err error) {
 			// sync local dir if necessary
 			for _, mount := range dataset.Spec.Mounts {
 				if common.IsFluidNativeScheme(mount.MountPoint) {
-					localDirPath := utils.UFSPathBuilder{}.GenLocalStoragePath(mount)
+					localDirPath := utils.UFSPathBuilder{}.GenAlluxioLocalStoragePath(mount)


How about GenAlluxioLocalStoragePath -> GenLocalStoragePathForAlluxio ?

cheyang · 2021-07-15T08:23:55Z

pkg/utils/dataset.go

@@ -73,7 +73,8 @@ func GetAccessModesOfDataset(client client.Client, name, namespace string) (acce
 func IsTargetPathUnderFluidNativeMounts(targetPath string, dataset datav1alpha1.Dataset) bool {
 	for _, mount := range dataset.Spec.Mounts {

-		mPath := UFSPathBuilder{}.GenAlluxioMountPath(mount, dataset.Spec.Mounts)
+		mAlluxioPath := UFSPathBuilder{}.GenAlluxioMountPath(mount, dataset.Spec.Mounts)
+		mGooseFSPath := UFSPathBuilder{}.GenGooseFSMountPath(mount, dataset.Spec.Mounts)


I'm wonder what does the mGooseFSPath look like? Do we need duplicate GenGooseFSUFSRootPath ? What's your opinion? @allenhaozi

Can a DataSet be used by two Runtime simultaneously ?
Or whether GooseFS can follow the same rules as Aluxio,

So we don't need a duplicate GenXXXMountPath ?

Firstly, Dataset should can be used for different type runtime.
Secondly, at present, GooseFS can follow ths same rules as Alluxio, but the future may be different.

cheyang · 2021-07-15T09:08:43Z

pkg/utils/ufs_path_builder.go

@@ -41,6 +41,22 @@ func (u UFSPathBuilder) GenAlluxioMountPath(curMount datav1alpha1.Mount, mounts
 	return fmt.Sprintf(common.AlluxioMountPathFormat, curMount.Name)
 }

+// dataset.spec.mounts mount to goosefs instance strategy:


I have concern on this package. Seems it's the utils for alluxio or Goose instead of all the runtime. If we really think it's not common. Maybe the common part can be placed in UFSPathBuilder. How about provide AlluxioUFSPathBuilder and GooseUFSPathBuilder?

type GooseUFSPathBuilder struct{ *UFSPathBuilder }

LGTM， @allenhaozi WDYT

I want to know the difference ?
Whether they can all follow one rule ?

At present, it seems that it can be consistent, but we want to maintain expansibility and isolation.

Is it fine to be consistent now? We can support GooseUFSPathBuilder in future.

I will seperate it in the future. @cheyang

allenhaozi · 2021-07-15T13:13:22Z

pkg/controllers/v1alpha1/databackup/implement.go

+			v1.EventTypeNormal,
+			common.RuntimeNotReady,
+			"Bounded accelerate runtime not supported")
+	}

 	if len(imageName) == 0 {


Can we wrap docker.GetImageTagFromEnv and docker.GetImageRepoFromEnv in docker.GetWorkerImage function ?

imageName, imageTag := docker.GetWorkerImage(r.Client, databackup.Spec.Dataset, runtimeType, databackup.Namespace) image := fmt.Sprintf("%s:%s", imageName, imageTag)

Do we have the case where the image name and the image tag come from different places ?
Or can it be avoided ?

I think we get image info from configmap and host env, will double check. But i think it is too strict.

allenhaozi · 2021-07-15T13:14:55Z

pkg/controllers/v1alpha1/databackup/implement.go

-			defaultImageInfo := strings.Split(common.DEFAULT_ALLUXIO_RUNTIME_IMAGE, ":")
-			if len(defaultImageInfo) < 2 {
+			defaultImageInfo := strings.Split(defaultImage, ":")
+			if len(defaultImageInfo) < 1 {
 				panic("invalid default databackup image!")


Does it have to be a panic?

tks, i think it is necessary be panic.

Here if it's panic and what we see ? pod restart ?

allenhaozi · 2021-07-15T13:22:25Z

pkg/ddc/goosefs/api_gateway.go

+		return endpoint, err
+	}
+
+	if svc == nil {


we have a situation where err and svc are both nil ?

you mean you have right endpoint in kubernetes cluster but err and svc is still nil?

allenhaozi · 2021-07-17T03:03:14Z

pkg/controllers/v1alpha1/goosefs/implement.go

+)
+
+const (
+	runtimeType                  = "goosefs"


common.GooseFSRuntime.String() ?

common.GooseFSRuntime, lgtm

allenhaozi · 2021-07-17T03:26:56Z

pkg/utils/databackup.go

 func GetAddressOfMaster(pod *v1.Pod) (nodeName string, ip string, rpcPort int32) {
 	// TODO: Get address of master by calling runtime controller interface instead of reading pod object
 	nodeName = pod.Spec.NodeName
 	ip = pod.Status.HostIP
 	for _, container := range pod.Spec.Containers {
-		if container.Name == "alluxio-master" {
+		if container.Name == "alluxio-master" || container.Name == "goosefs-master" {


suggest:
wrap a function with ut

allenhaozi · 2021-07-17T03:43:15Z

pkg/utils/quantity.go

+	value = q.String()
+
+	if strings.HasSuffix(value, "i") {
+		value = strings.ReplaceAll(value, "i", "B")


this logic strings.ReplaceAll means that 1Ki == 1KB is true ?

right, Ki is legal for kubernetes and KB is legal for Alluxio and GooseFS.

xieydd · 2021-07-17T16:34:57Z

@cheyang @TrafalgarZZZ @allenhaozi @yangyuliufeng Thanks for taking time for reviewing, i have fix some comment. Can we check the pr again. If any problem, ping me is welcome : )

Signed-off-by: xieydd <xieydd@gmail.com> resolve ci bug Signed-off-by: xieydd <xieydd@gmail.com> Sync test Signed-off-by: xieydd <xieydd@gmail.com>

…engine type Signed-off-by: xieydd <xieydd@gmail.com>

Signed-off-by: xieydd <xieydd@gmail.com>

cheyang · 2021-07-20T01:55:05Z

Thanks, @xieydd . Great work!

RongGu · 2021-07-20T12:23:37Z

Thank your for contributing the Tecent GooseFS Cache Engine as a Fuild backend. @xieydd !

xieydd force-pushed the add-goosefs-runtime branch from 523a03b to ed12216 Compare July 9, 2021 04:18

xieydd closed this Jul 9, 2021

xieydd reopened this Jul 9, 2021

xieydd force-pushed the add-goosefs-runtime branch 2 times, most recently from c0209fe to ec806f5 Compare July 9, 2021 09:35

xieydd force-pushed the add-goosefs-runtime branch from ec806f5 to 4eec434 Compare July 9, 2021 09:43

cheyang requested a review from yangyuliufeng July 14, 2021 07:02

xieydd force-pushed the add-goosefs-runtime branch from 6dab0ae to 614a6b4 Compare July 14, 2021 08:02

allenhaozi reviewed Jul 14, 2021

View reviewed changes

cheyang requested a review from TrafalgarZZZ July 14, 2021 10:53

allenhaozi reviewed Jul 14, 2021

View reviewed changes

cheyang reviewed Jul 15, 2021

View reviewed changes

TrafalgarZZZ requested changes Jul 15, 2021

View reviewed changes

TrafalgarZZZ reviewed Jul 15, 2021

View reviewed changes

yangyuliufeng requested changes Jul 15, 2021

View reviewed changes

cheyang reviewed Jul 15, 2021

View reviewed changes

cheyang requested changes Jul 15, 2021

View reviewed changes

allenhaozi reviewed Jul 15, 2021

View reviewed changes

allenhaozi reviewed Jul 17, 2021

View reviewed changes

xieydd force-pushed the add-goosefs-runtime branch from 046ffef to 3f04dff Compare July 17, 2021 16:31

xieydd force-pushed the add-goosefs-runtime branch 3 times, most recently from 43cf762 to 3febac9 Compare July 19, 2021 04:31

xieydd added 8 commits July 20, 2021 00:09

Add Tencent GooseFS Cache Engine to fluid backend

9b999a4

Signed-off-by: xieydd <xieydd@gmail.com> resolve ci bug Signed-off-by: xieydd <xieydd@gmail.com> Sync test Signed-off-by: xieydd <xieydd@gmail.com>

1. Delete Tencent image repo && 2. Seperate databackup helm repo via …

daa8a61

…engine type Signed-off-by: xieydd <xieydd@gmail.com>

Fix error in comment

14689a4

Signed-off-by: xieydd <xieydd@gmail.com>

Delete GooseFS ufs_path_build part

a2ff17d

Signed-off-by: xieydd <xieydd@gmail.com>

Add ut

3cafaa8

Signed-off-by: xieydd <xieydd@gmail.com>

Add ut

458f358

Signed-off-by: xieydd <xieydd@gmail.com>

Add ut for utils.go

c23428e

Signed-off-by: xieydd <xieydd@gmail.com>

Add ut for ufs.go

681a908

Signed-off-by: xieydd <xieydd@gmail.com>

xieydd force-pushed the add-goosefs-runtime branch from da35497 to 681a908 Compare July 19, 2021 16:09

cheyang approved these changes Jul 20, 2021

View reviewed changes

cheyang merged commit 9a965ec into fluid-cloudnative:master Jul 20, 2021

RongGu mentioned this pull request Mar 8, 2022

[Community] Nominating Yuandong Xie (GithubID: xieydd) for Fluid committer fluid-cloudnative/community#40

Closed


		// transform dataset which has ufsPaths and ufsVolumes
		func (e GooseFSEngine) transformInitUsers(runtime datav1alpha1.GooseFSRuntime, value *GooseFS) {

Add Tencent GooseFS Cache Engine to fluid backend #912

Add Tencent GooseFS Cache Engine to fluid backend #912

Conversation

xieydd commented Jul 9, 2021

Ⅰ. Describe what this PR does

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

Ⅴ. Special notes for reviews

codecov bot commented Jul 9, 2021 • edited Loading

Codecov Report

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

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

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

yangyuliufeng 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

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

allenhaozi Jul 15, 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

allenhaozi Jul 15, 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

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

allenhaozi Jul 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xieydd commented Jul 17, 2021

cheyang commented Jul 20, 2021

RongGu commented Jul 20, 2021

codecov bot commented Jul 9, 2021 •

edited

Loading

allenhaozi Jul 15, 2021 •

edited

Loading

allenhaozi Jul 15, 2021 •

edited

Loading

allenhaozi Jul 15, 2021 •

edited

Loading