Add the vineyard runtime CRD definitions. #3555

dashanji · 2023-11-17T13:00:56Z

Ⅰ. Describe what this PR does

Add the vineyard runtime CRD definitions.

Ⅱ. Does this pull request fix one issue?

Fixes parts of #3528

fluid-e2e-bot · 2023-11-17T13:01:13Z

Hi @dashanji. Thanks for your PR.

I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

codecov · 2023-11-18T07:56:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6e500f9) 64.13% compared to head (46bb89b) 64.14%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3555      +/-   ##
==========================================
+ Coverage   64.13%   64.14%   +0.01%     
==========================================
  Files         441      442       +1     
  Lines       26572    26574       +2     
==========================================
+ Hits        17042    17046       +4     
+ Misses       7525     7524       -1     
+ Partials     2005     2004       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cheyang · 2023-11-18T08:03:49Z

api/v1alpha1/vineyardruntime_types.go

+	// Resources contains the resource requirements and limits for the Vineyard Master.
+	// Default is not set.
+	// +optional
+	Resources corev1.ResourceRequirements `json:"resources,omitempty"`


I think master, worker and fuse needs configuring Resources.

One consideration that does not allow the master to configure resources is that users need to consider the relationship between shared memory and resources memory. If only keep the shared memory configuration, it will be more transparent to users.

I think the meaning of transparent is that the users don't need to configure it by default, but they can do it when they really need it.

Okay, it looks good to me.

cheyang · 2023-11-18T08:07:38Z

api/v1alpha1/vineyardruntime_types.go

+	//   vineyardd.reserve.memory: "true"
+	//
+	// +optional
+	Properties map[string]string `json:"properties,omitempty"`


How about renaming properties to options? And I think master spec also need options which can provides flexibility for future.

Make sense to me.

cheyang · 2023-11-18T08:10:11Z

@dashanji please fix github action/lint by running make update-crd.

GO111MODULE=off /home/runner/work/fluid/fluid/bin/controller-gen "crd" rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases
cp config/crd/bases/* charts/fluid/fluid/crds
 M config/crd/bases/data.fluid.io_vineyardruntimes.yaml
?? charts/fluid/fluid/crds/data.fluid.io_vineyardruntimes.yaml
CRD validation failed. Please use 'make update-crd' to keep CRDs latest

dashanji · 2023-11-20T03:00:25Z

@cheyang Could you please take another look at this? Thanks.

cheyang · 2023-11-20T06:27:05Z

api/v1alpha1/vineyardruntime_types.go

+	// If set to false, the Etcd component will not be deployed,
+	// which means the Vineyard Worker component will use an external Etcd cluster.
+	// +optional
+	Enabled bool `json:"enabled,omitempty"`


What will happen if enabled is false？ In other words, how to handle external etcd for example?

xliuqq · 2023-11-20T07:30:11Z

api/v1alpha1/vineyardruntime_types.go

+	// Default is not set.
+	// +optional
+	Resources corev1.ResourceRequirements `json:"resources,omitempty"`
+}


Does Vineyard Fuse need NodeSelector and CleanPolicy field like other runtimes? These fields are used at GetRuntimeInfo method in the runtime.go file.

I think the answer is no, as the vineyard fuse only mounts the vineyard socket from hostpath to the application pod's PVC, so it must be co-located with application Pod. Adding a new nodeselector may mix the deployment of Vineyard Fuse. As for CleanPolicy, the only strategy is OnDemand. When the application pod is deleted, the fuse pod will be deleted after that. What do you think? @xliuqq

@dashanji As far as I know，fuse is always co-located with application pod because the CSI plugin sets the node label fluid.io/f-{namespace}-{name} (fuse daemonset node selector expression), the nodeselector defines their(fuse/app) placement requirements.
The proposal says it supports RPC between app pod and vineyard pod, so we can use nodeselector to place the fuse/app pod with requirements ?
As for CleanPolicy, do you mean vineyard fuse not support OnRuntimeDeletedCleanPolicy ?

The #3528 says it supports RPC between app pod and vineyard pod. So we can use nodeselector to place the fuse/app pod with requirements?

I think the original nodeselector fluid.io/f-{namespace}-{name} is enough. If the app pod uses the RPC, we don't need the vineyard fuse (which mainly mounts vineyard socket from the hostpath) at all.

As for CleanPolicy, do you mean vineyard fuse not support OnRuntimeDeletedCleanPolicy?

Sorry for the mistake. I have checked the NodeStageVolume. Both OnDemand and OnRuntimeDeleted are supported, and the difference is whether the fuse pod still exists after the app pod is deleted. Maybe OnRuntimeDeletedCleanPolicy is more better.

@xliuqq Thanks for the advice. I have added the OnRuntimeDeletedCleanPolicy.
Maybe we could add nodeselector after the users make a request.

cheyang · 2023-11-20T07:32:44Z

api/v1alpha1/vineyardruntime_types.go

+type MasterSpec struct {
+	// The component configurations for Vineyard Master
+	// +optional
+	VineyardCompTemplateSpec `json:",inline"`


How about using endpoint for external uri?

endpoint: uri: "etcd-svc.etcd-namespace.svc.cluster.local:2379" encryptOptions: - name: access-key valueFrom: secretKeyRef: name: jfs-secret key: accesskey - name: secret-key valueFrom: secretKeyRef: name: jfs-secret key: secretkey

Good point. In this way, we can check the status of the external etcd cluster.

cheyang · 2023-11-20T12:52:49Z

api/v1alpha1/vineyardruntime_types.go

+	// E,g. "etcd-svc.etcd-namespace.svc.cluster.local:2379"
+	// Default is not set and use http protocol to connect to external etcd cluster
+	// +optional
+	URI string `json:"uri"`


Do we also need add options?

Thanks for reminding, we have an option /prefix to define the prefix of etcd key for vineyard object.

cheyang · 2023-11-20T12:53:41Z

api/v1alpha1/vineyardruntime_types.go

+	// 		   valueFrom:
+	//           secretKeyRef:
+	//             name: etcd-secret
+	//			   key: accesskey
 	//
 	// +optional
 	Enabled bool `json:"enabled,omitempty"`


I think there is no need for Enabled?Right?

dashanji · 2023-11-22T03:13:38Z

Hi @cheyang @xliuqq @TrafalgarZZZ Could you please approve the workflow? Thanks.

TrafalgarZZZ · 2023-11-22T12:19:45Z

api/v1alpha1/vineyardruntime_types.go

+
+	// Fuse holds the configurations for Vineyard Fuse
+	// +optional
+	Fuse VineyardFuseSpec `json:"fuse,omitempty"`


@dashanji Would it be better to use something like VineyardSockSpec or VineyardClientSpec instead of VineyardFuseSpec? From my understanding, we are not going to use the real vineyard fuse but using the vineyard socket to mimic a fake fuse.

So what I'm thinking is we can keep the VineyardRuntime.spec.fuse for API consistency with other runtimes. But in Fluid's codebase, we can avoid using Fuse as the golang struct's name. For example, something like:

// Fuse holds the configurations for Vineyard client socket. // Note that the "Fuse" here is kept just for API consistency, VineyardRuntime mount a socket file instead of a FUSE filesystem to make data cache available. // <Maybe some Vineyard arch document here> Fuse VineyardSockSpec `json:"fuse,omitempty"`

Make sense to me. Thanks @TrafalgarZZZ

Hi @TrafalgarZZZ. Could you please take another look?

@dashanji Sorry for responding late. Thanks! The PR LGTM.

Signed-off-by: Ye Cao <[email protected]>

sonarcloud · 2023-11-23T12:10:17Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
14 Code Smells

No Coverage information
6.9% Duplication

TrafalgarZZZ

/lgtm

cheyang

/lgtm
/approve

fluid-e2e-bot · 2023-11-24T03:38:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheyang, xliuqq

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:

~~OWNERS~~ [cheyang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fluid-e2e-bot bot added the needs-ok-to-test label Nov 17, 2023

cheyang reviewed Nov 18, 2023

View reviewed changes

cheyang requested a review from TrafalgarZZZ November 18, 2023 08:07

cheyang requested review from xliuqq and cheyang November 20, 2023 06:13

cheyang reviewed Nov 20, 2023

View reviewed changes

xliuqq reviewed Nov 20, 2023

View reviewed changes

cheyang reviewed Nov 20, 2023

View reviewed changes

dashanji force-pushed the add-vineyard-runtime-api branch from dc48f04 to 2321579 Compare November 22, 2023 03:11

xliuqq approved these changes Nov 22, 2023

View reviewed changes

TrafalgarZZZ reviewed Nov 22, 2023

View reviewed changes

Add the vineyard runtime CRD definitions.

46bb89b

Signed-off-by: Ye Cao <[email protected]>

dashanji force-pushed the add-vineyard-runtime-api branch from 5148cc6 to 46bb89b Compare November 23, 2023 12:09

TrafalgarZZZ reviewed Nov 24, 2023

View reviewed changes

fluid-e2e-bot bot assigned TrafalgarZZZ Nov 24, 2023

fluid-e2e-bot bot added the lgtm label Nov 24, 2023

cheyang approved these changes Nov 24, 2023

View reviewed changes

fluid-e2e-bot bot assigned cheyang Nov 24, 2023

fluid-e2e-bot bot added the approved label Nov 24, 2023

fluid-e2e-bot bot merged commit 1e547fe into fluid-cloudnative:master Nov 24, 2023
11 checks passed

dashanji deleted the add-vineyard-runtime-api branch November 28, 2023 06:32

dashanji mentioned this pull request Dec 7, 2023

Add the helm chart for vineyard runtime. #3624

Merged

dashanji mentioned this pull request Dec 18, 2023

Enhance Observability and Unit Testing for Vineyard Runtime Integration with Fluid v6d-io/v6d#1672

Closed

3 tasks

RongGu mentioned this pull request Aug 18, 2024

[Community] Nominating Ye Cao (GithubID: dashanji) for Fluid committer fluid-cloudnative/community#64

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the vineyard runtime CRD definitions. #3555

Add the vineyard runtime CRD definitions. #3555

dashanji commented Nov 17, 2023

fluid-e2e-bot bot commented Nov 17, 2023

codecov bot commented Nov 18, 2023 •

edited

Loading

cheyang Nov 18, 2023

dashanji Nov 19, 2023

cheyang Nov 20, 2023

dashanji Nov 20, 2023

cheyang Nov 18, 2023

dashanji Nov 19, 2023

cheyang commented Nov 18, 2023

dashanji commented Nov 20, 2023

cheyang Nov 20, 2023

dashanji Nov 20, 2023

xliuqq Nov 20, 2023 •

edited

Loading

dashanji Nov 20, 2023

xliuqq Nov 21, 2023

dashanji Nov 21, 2023

dashanji Nov 21, 2023

cheyang Nov 20, 2023

dashanji Nov 20, 2023

cheyang Nov 20, 2023

dashanji Nov 20, 2023

cheyang Nov 20, 2023

dashanji commented Nov 22, 2023

TrafalgarZZZ Nov 22, 2023

dashanji Nov 23, 2023

dashanji Nov 23, 2023

TrafalgarZZZ Nov 24, 2023

sonarcloud bot commented Nov 23, 2023

TrafalgarZZZ left a comment

cheyang left a comment

fluid-e2e-bot bot commented Nov 24, 2023

Add the vineyard runtime CRD definitions. #3555

Add the vineyard runtime CRD definitions. #3555

Conversation

dashanji commented Nov 17, 2023

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

fluid-e2e-bot bot commented Nov 17, 2023

codecov bot commented Nov 18, 2023 • 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

cheyang commented Nov 18, 2023

dashanji commented Nov 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xliuqq Nov 20, 2023 • 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

Choose a reason for hiding this comment

dashanji commented Nov 22, 2023

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

sonarcloud bot commented Nov 23, 2023

TrafalgarZZZ left a comment

Choose a reason for hiding this comment

cheyang left a comment

Choose a reason for hiding this comment

fluid-e2e-bot bot commented Nov 24, 2023

codecov bot commented Nov 18, 2023 •

edited

Loading

xliuqq Nov 20, 2023 •

edited

Loading