add feature with JindoRuntime dataloader #764

frankleaf · 2021-05-06T03:42:07Z

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-commenter · 2021-05-06T03:48:05Z

Codecov Report

Merging #764 (bbbfccc) into master (76e4509) will decrease coverage by 0.12%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #764      +/-   ##
==========================================
- Coverage   12.65%   12.52%   -0.13%     
==========================================
  Files          93       93              
  Lines        4236     4279      +43     
==========================================
  Hits          536      536              
- Misses       3640     3683      +43     
  Partials       60       60

Impacted Files	Coverage Δ
pkg/ddc/jindo/cache.go	`0.00% <ø> (ø)`
pkg/ddc/jindo/master_internal.go	`0.00% <0.00%> (ø)`
pkg/ddc/jindo/runtime_info.go	`0.00% <ø> (ø)`
pkg/ddc/jindo/shutdown.go	`0.00% <0.00%> (ø)`
pkg/ddc/jindo/transform.go	`0.00% <0.00%> (ø)`
pkg/ddc/jindo/utils.go	`0.00% <0.00%> (ø)`
pkg/utils/docker/image.go	`14.54% <0.00%> (-1.79%)`	⬇️
pkg/utils/runtimes.go	`29.62% <0.00%> (-10.38%)`	⬇️
pkg/ddc/alluxio/master.go	`0.00% <0.00%> (ø)`
... and 1 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 76e4509...bbbfccc. Read the comment docs.

allenhaozi · 2021-05-06T04:57:12Z

pkg/controllers/v1alpha1/dataload/implement.go

+		fileUtils := alluxioOperations.NewAlluxioFileUtils(podName, containerName, targetDataset.Namespace, ctx.Log)
+		ready = fileUtils.Ready()
+	case common.JINDO_RUNTIME:
+		podName := fmt.Sprintf("%s-jindofs-master-0", targetDataset.Name)


suggest put %s-jindofs-master-0 in a unified place

thx,same situation as alluxio , will resolve next pr

allenhaozi · 2021-05-06T05:01:35Z

pkg/controllers/v1alpha1/dataload/implement.go

 		if err != nil {
 			log.Error(err, "failed to generate dataload chart's value file")
 			return utils.RequeueIfError(err)
 		}
-		chartName := utils.GetChartsDirectory() + "/" + cdataload.DATALOAD_CHART
+		chartName := ""
+		if boundedRuntimeType == "alluxio" {


avoid hardcode alluxio

if boundedRuntimeType == common.DataRuntimeAlluxio.String() {}

thx, replace by common constant

allenhaozi · 2021-05-06T05:11:04Z

pkg/dataload/constants.go

@@ -21,4 +21,6 @@ const (
 	DATALOAD_DEFAULT_IMAGE = "registry.cn-hangzhou.aliyuncs.com/fluid/fluid-dataloader"
 	DATALOAD_SUFFIX_LENGTH = 5
 	ENV_DATALOADER_IMG     = "DATALOADER_IMG"
+	DATALOAD_ALLUXIO_CHART = "alluxio"


https://github.com/golang/go/wiki/CodeReviewComments#mixed-caps

alluxio appear many time, can it be unified into one place ?

thx, removed and replaced by common constant

allenhaozi · 2021-05-06T05:15:45Z

pkg/ddc/jindo/operations/base.go

+// Check if the Alluxio is ready by running `alluxio fsadmin report` command
+func (a JindoFileUtils) Ready() (ready bool) {
+	var (
+		command = []string{"/sdk/bin/jindo", "jfs", "-report"}


thx,this is jindofs's fixed shell command, so be hard code

You can define constant in common pkg.

just same as with other many command , will replace "/sdk/bin/jindo" by "jindo"

allenhaozi · 2021-05-06T05:17:36Z

pkg/ddc/jindo/transform.go

 	for _, mount := range dataset.Spec.Mounts {

-		jfsNamespace = jfsNamespace + mount.Name + ","
+		//jfsNamespace = jfsNamespace + mount.Name + ","

 		if !strings.HasSuffix(mount.MountPoint, "/") {
 			mount.MountPoint = mount.MountPoint + "/"
 		}
 		// transform mountpoint for oss or hdfs format
 		if strings.HasPrefix(mount.MountPoint, "hdfs://") {


Which protocol can be encapsulated as a util function

cheyang · 2021-05-06T08:06:45Z

api/v1alpha1/dataload_types.go

+	LoadMemoryData bool `json:"loadMemoryData,omitempty"`
+
+	// add HdfsConfig for JindoRuntime
+	HdfsConfig string `json:"hdfsConfig,omitempty"`


Should user define this HDFSConfig?

cheyang · 2021-05-06T08:09:59Z

charts/fluid/fluid/values.yaml

@@ -33,8 +33,8 @@ runtime:
    portRange: 18000-19999
    enabled: false
    smartdata:
-      image: registry.cn-shanghai.aliyuncs.com/jindofs/smartdata:3.5.2
+      image: registry.cn-shanghai.aliyuncs.com/jindofs/smartdata:3.5.0


3.5.0 is older than 3.5.2? Is it your expectation?

thx，The version of the mirror will be same as JindoFS version in the future，so 3.5.0 is expected

cheyang · 2021-05-06T08:11:05Z

api/v1alpha1/dataload_types.go

@@ -53,6 +53,12 @@ type DataLoadSpec struct {

 	// Target defines target paths that needs to be loaded
 	Target []TargetPath `json:"target,omitempty"`
+
+	// LoadMemoryData specifies if the dataload job should load memory or not
+	LoadMemoryData bool `json:"loadMemoryData,omitempty"`


What does loadMemoryData mean?

already removed

TrafalgarZZZ · 2021-05-06T08:14:06Z

pkg/ddc/jindo/types.go

+	OssKey           string            `yaml:"osskey,omitempty"`
+	OssSecret        string            `yaml:"osssecret,omitempty"`


I think it's better to use ossKey and ossSecret for consistency.

thx, Capitalization is for external reference

TrafalgarZZZ · 2021-05-06T08:15:32Z

charts/fluid-dataloader/jindo/README.md

@@ -0,0 +1,86 @@
+# fluid-dataloader


READMEs in both fluid-dataloader/alluxio and fluid-dataloader/jindo is deprecated. So I suggest we should delete them to avoid misleading info.

this is two different dataload type I think it's better to be separated

TrafalgarZZZ · 2021-05-06T08:16:57Z

charts/fluid-dataloader/jindo/values.yaml

+  # Required
+  # Description: the dataset that this DataLoad targets
+  targetDataset: #imagenet
+


Add default values and comments for hdfsConfig and loadMemoryData

thx，already removed this two unnecessary parameter

cheyang · 2021-05-08T07:16:26Z

pkg/utils/docker/image.go

@@ -68,10 +68,20 @@ func GetWorkerImage(client client.Client, datasetName string, runtimeType string

 	}
 	if imageName == "" {
-		imageName = "registry.cn-huhehaote.aliyuncs.com/alluxio/alluxio"
+		if runtimeType == common.ALLUXIO_RUNTIME {


I suggest to use switch here.

I and @XIAO-HOU are trying to unify get image methods.
perhaps you need to agree with us in the future.

cheyang · 2021-05-08T07:18:14Z

pkg/dataload/value.go

@@ -21,6 +21,12 @@ type DataLoadInfo struct {

 	// Image specifies the image that the DataLoad job uses
 	Image string `yaml:"image,omitempty"`
+
+	// LoadMemoryData specifies if the dataload job should load memory or not
+	LoadMemoryData bool `yaml:"loadMemoryData,omitempty"`


Do we need define this in the struct for the comon dataloadInfo Object?

thx, so define JindoOptions to include these two parameter, not in the common dataloadInfo

cheyang · 2021-05-08T07:19:51Z

pkg/controllers/v1alpha1/dataload/implement.go

 		if err != nil {
 			log.Error(err, "failed to generate dataload chart's value file")
 			return utils.RequeueIfError(err)
 		}
-		chartName := utils.GetChartsDirectory() + "/" + cdataload.DATALOAD_CHART
+		chartName := ""
+		if boundedRuntimeType == common.ALLUXIO_RUNTIME {


Suggest to change to switch, thanks.

cheyang · 2021-05-08T07:23:38Z

pkg/controllers/v1alpha1/dataload/implement.go

 	image := fmt.Sprintf("%s:%s", imageName, imageTag)

+	runtime, err := utils.GetJindoRuntime(r.Client, boundedRuntime.Name, boundedRuntime.Namespace)


Will this change break dataload logic for alluxio?

thx，to avoid break the logic for alluixo, make generateJindoDataLoadValueFile function to separate each other

cheyang · 2021-05-08T15:00:25Z

pkg/dataload/value.go

@@ -21,6 +21,17 @@ type DataLoadInfo struct {

 	// Image specifies the image that the DataLoad job uses
 	Image string `yaml:"image,omitempty"`
+
+	// JindoOptions specifies the options that jindoruntime uses
+	JindoOptions JindoOptions `yaml:"jindoOptions,omitempty"`


I think JindoOptions shouldn't be part of dataLoadInfo. Just like the carOption is not part of the vehicle.

Agreed. Perhaps better to use two different chart value structs for each ddc engine and shared properties like Image and TargetDataset can be put together in a struct.

We can build a field to pass options, which option can be effective is decided by runtime type, just like csi options.

Maybe you can provide csi sample for reference.

yangyuliufeng · 2021-05-10T02:07:37Z

pkg/utils/docker/image.go

@@ -68,10 +68,20 @@ func GetWorkerImage(client client.Client, datasetName string, runtimeType string

 	}
 	if imageName == "" {
-		imageName = "registry.cn-huhehaote.aliyuncs.com/alluxio/alluxio"
+		if runtimeType == common.ALLUXIO_RUNTIME {


I and @XIAO-HOU are trying to unify get image methods.
perhaps you need to agree with us in the future.

yangyuliufeng · 2021-05-10T02:14:52Z

pkg/controllers/v1alpha1/dataload/implement.go

+				log.Error(err, "failed to generate dataload chart's value file")
+				return utils.RequeueIfError(err)
+			}
+			chartName = utils.GetChartsDirectory() + "/" + cdataload.DATALOAD_CHART + "/" + common.ALLUXIO_RUNTIME


how about make it a function which return chartName according to runtimeName?

thx, will refeactor next PR

yangyuliufeng · 2021-05-10T02:18:35Z

pkg/dataload/value.go

@@ -21,6 +21,17 @@ type DataLoadInfo struct {

 	// Image specifies the image that the DataLoad job uses
 	Image string `yaml:"image,omitempty"`
+
+	// JindoOptions specifies the options that jindoruntime uses
+	JindoOptions JindoOptions `yaml:"jindoOptions,omitempty"`


We can build a field to pass options, which option can be effective is decided by runtime type, just like csi options.

* add ns with the cache path * fix err unused * fix err unused * fix directory dir * add dataloader crd for JindoRuntime * fix dataloader runtime * fix constant ns to jindo * add jindofs client conf yaml * fix configmap yaml * add support with s3 and global fuse * upgrade jindo version to 3.5.0 * refine dataloader config * change default nsname * add core-site dataloader * add jindo dataset status judge * add load data to memory type * change default ns to jindo * fix ufs mode * change dataload * change dataload to alluxio and jindo * refine dataload * refine dataload * refine dataload * refine dataload * refine dataload function * delete readme file * add generateJindoDataLoadValueFile to separate from alluxioRuntime * dataload options define * refine Signed-off-by: xieydd <[email protected]>

frankleaf added 24 commits April 16, 2021 15:13

add ns with the cache path

e345b6d

fix err unused

eff63f5

fix err unused

d59031c

fix directory dir

357069e

add dataloader crd for JindoRuntime

98e19bf

fix dataloader runtime

c2d7db2

fix constant ns to jindo

8d13208

add jindofs client conf yaml

aabe26b

fix configmap yaml

0b6ad0f

add support with s3 and global fuse

72a197d

upgrade jindo version to 3.5.0

8f53279

refine dataloader config

54840b2

change default nsname

96f9adc

merge master

8bb2cfe

add core-site dataloader

657bc6c

add jindo dataset status judge

50d2f88

add load data to memory type

3899554

change default ns to jindo

20a775d

fix ufs mode

591b9b8

change dataload

ae00201

change dataload to alluxio and jindo

dcc1d82

refine dataload

8bc019f

refine dataload

34ec6e3

refine dataload

1a5b5a4

allenhaozi reviewed May 6, 2021

View reviewed changes

cheyang requested a review from TrafalgarZZZ May 6, 2021 06:10

refine dataload

6f122af

cheyang approved these changes May 6, 2021

View reviewed changes

cheyang requested changes May 6, 2021

View reviewed changes

TrafalgarZZZ requested changes May 6, 2021

View reviewed changes

refine dataload function

d1d94fd

frankleaf requested review from cheyang and TrafalgarZZZ May 6, 2021 13:30

delete readme file

2cb2243

cheyang requested changes May 8, 2021

View reviewed changes

add generateJindoDataLoadValueFile to separate from alluxioRuntime

bbbfccc

cheyang requested changes May 8, 2021

View reviewed changes

yangyuliufeng requested changes May 10, 2021

View reviewed changes

frankleaf added 2 commits May 11, 2021 15:30

dataload options define

c83a693

refine

233916b

cheyang approved these changes May 11, 2021

View reviewed changes

cheyang merged commit db75dcf into fluid-cloudnative:master May 11, 2021

RongGu mentioned this pull request Mar 8, 2022

[Community] Nominating Tao Wang(GithubID: frankleaf) for Fluid committer fluid-cloudnative/community#41

Closed

		OssKey string `yaml:"osskey,omitempty"`
		OssSecret string `yaml:"osssecret,omitempty"`

		image := fmt.Sprintf("%s:%s", imageName, imageTag)

		runtime, err := utils.GetJindoRuntime(r.Client, boundedRuntime.Name, boundedRuntime.Namespace)

add feature with JindoRuntime dataloader #764

add feature with JindoRuntime dataloader #764

Conversation

frankleaf commented May 6, 2021

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-commenter commented May 6, 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

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

codecov-commenter commented May 6, 2021 •

edited

Loading