Add the helm chart for vineyard runtime. #3624

dashanji · 2023-12-07T12:57:01Z

Ⅰ. Describe what this PR does

Add the helm chart for vineyard runtime including the master, worker and fuse.

Ⅱ. Does this pull request fix one issue?

A follow-up of #3555 and fix parts of #3528

fluid-e2e-bot · 2023-12-07T12:57:19Z

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.

cheyang · 2023-12-07T13:04:27Z

charts/alluxio/values.yaml

@@ -130,11 +130,6 @@ master:
  #   mountPath: /data/secrets/mySecretVolume/
  # - name: myConfigMapVolume
  #   mountPath: /data/configmap/myConfigMapVolume/
-  # volumeMounts:


Why do you need to change alluxio chart?

I find it's a dummy part here.

codecov · 2023-12-07T13:14:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (69539a3) 64.26% compared to head (bd96909) 64.26%.
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3624   +/-   ##
=======================================
  Coverage   64.26%   64.26%           
=======================================
  Files         443      443           
  Lines       26755    26755           
=======================================
  Hits        17194    17194           
  Misses       7546     7546           
  Partials     2015     2015

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

cheyang · 2023-12-08T02:10:54Z

charts/vineyard/templates/master/service.yaml

+{{- $fullName := include "vineyard.fullname" . }}
+{{- $chart := include "vineyard.chart" . }}
+
+{{- if not .Values.master.externalEndpoint }}


Could you provide the reason of providing both service and headless service reason for master?

The headless service is used for internal communication between master pods, and the service provides external access entry for worker.

Is it imperative to use different ways to access master pods from masters and workers? According to my knowledge, Kubernetes API Server connects multiple endpoints.

Thanks, I will try to use the headless service only.

cheyang · 2023-12-08T02:22:24Z

charts/vineyard/templates/fuse/daemonset.yaml

@@ -0,0 +1,91 @@
+# Copyright 2020-2023 Alibaba Group Holding Limited.


Could you use fluid copyright once contributing this to fluid project.

cheyang · 2023-12-08T02:25:33Z

charts/vineyard/templates/master/statefulset.yaml

@@ -0,0 +1,124 @@
+# Copyright 2020-2023 Alibaba Group Holding Limited.


cheyang · 2023-12-08T02:27:01Z

charts/vineyard/templates/master/statefulset.yaml

+          image: {{ .Values.master.image }}:{{ .Values.master.imageTag }}
+          imagePullPolicy: {{ .Values.master.imagePullPolicy }}
+          securityContext:
+            privileged: true


What's the reason of using privileged.

Thanks for pointing out. It should be false via https://github.com/bitnami/charts/blob/main/bitnami/etcd/values.yaml#L317.

dashanji · 2023-12-08T07:04:26Z

@cheyang, could you please take a look at this?

* Add an option for worker to wait for the master ready. * Delete the option 'etcd.prefix' in ExternalEndpoint as user can set it in the worker's options. * Add the kubebuilder markers to the vineyard runtime API. Signed-off-by: Ye Cao <[email protected]>

TrafalgarZZZ · 2023-12-21T02:12:29Z

charts/vineyard/templates/worker/statefulset.yaml

+          requiredDuringSchedulingIgnoredDuringExecution:
+            - labelSelector:
+                matchExpressions:
+                  - key: app.kubernetes.io/instance


The label app.kubernetes.io/instance seems missing in Vineyard worker's Pod metadata?

That's my fault. Thanks for pointing out.

TrafalgarZZZ · 2023-12-21T02:13:13Z

charts/vineyard/templates/worker/statefulset.yaml

+                  - key: app.kubernetes.io/instance
+                    operator: In
+                    values:
+                     - {{ .Release.Namespace -}} - {{- .Release.Name }}


How about using

Suggested change

- {{ .Release.Namespace -}} - {{- .Release.Name }}

- {{ .Release.Namespace }}-{{ .Release.Name }}

TrafalgarZZZ · 2023-12-21T02:14:57Z

charts/vineyard/templates/worker/statefulset.yaml

+          - /bin/bash
+          - -c
+          - >
+            /usr/local/bin/vineyardd
+            --socket /var/run/vineyard/vineyard.sock
+            --size {{ include "vineyard.worker.size" . }}
+            {{- if eq (toString (index .Values.worker.options "vineyardd.reserve.memory")) "true" }}
+            --reserve_memory
+            {{- end }}
+            --meta_timeout {{ default "120" (index .Values.worker.options "wait.etcd.timeout") }}
+            --etcd_prefix {{ default "/vineyard" (index .Values.worker.options "etcd.prefix") }}
+            --etcd_endpoint {{ include "vineyard.master.endpoint" . }}
+            {{- if eq (include "vineyard.checkTieredStore" .) "true" }}
+            --spill_path {{ include "vineyard.spill.path" . }}
+            --spill_lower_rate {{ include "vineyard.spill.lowerRate" . }}
+            --spill_upper_rate {{ include "vineyard.spill.upperRate" . }}
+            {{- end }}


I think better to directly use /usr/local/bin/vineyard as the entrypoint command. Seems that bash -c is not necessary here.

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

cheyang · 2023-12-21T04:07:15Z

charts/fluid/fluid/crds/data.fluid.io_vineyardruntimes.yaml

    kind: VineyardRuntime
    listKind: VineyardRuntimeList
    plural: vineyardruntimes
+    shortNames:
+    - vineyard


How about using v6d for shortname?

Just kidding. You can decide by yourself.

How about using v6d for shortname?

Make sense.

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

sonarcloud · 2023-12-21T05:43:33Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.5% Duplication on New Code

See analysis details on SonarCloud

cheyang

/lgtm
/approve

fluid-e2e-bot · 2023-12-22T02:41:56Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [TrafalgarZZZ,cheyang]

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

* * Add the helm chart for Vineyard Runtime. * Add an option for worker to wait for the master ready. * Delete the option 'etcd.prefix' in ExternalEndpoint as user can set it in the worker's options. * Add the kubebuilder markers to the vineyard runtime API. Signed-off-by: Ye Cao <[email protected]> * Add the app.kubernetes.io/instance label to the vineyard worker. Signed-off-by: Ye Cao <[email protected]> * Change the short name of vineyard to v6d. Signed-off-by: Ye Cao <[email protected]> --------- Signed-off-by: Ye Cao <[email protected]>

TrafalgarZZZ · 2024-01-04T04:55:25Z

charts/vineyard/templates/master/statefulset.yaml

+            privileged: false
+          command:
+            - etcd
+            - --name=$(POD_NAME)


Do we have to give a full url like <pod_name>.<svc_name>.namespace.svc.cluster.local, I'm not sure if users may have customized domain intead of svc.cluster.local?

Do you mean --name=<pod_name>.<svc_name>.namespace.svc.cluster.local? Actually, the name is just used in the internal etcd cluster.

Do you mean --name=<pod_name>.<svc_name>.namespace.svc.cluster.local?

Yes, willcluster.local be configured globally in coreDNS config? Refering to https://kubernetes.io/docs/tasks/administer-cluster/dns-custom-nameservers/#coredns-configmap-options. I'm not sure if it can be modified.

TrafalgarZZZ · 2024-01-04T04:56:04Z

charts/vineyard/templates/master/statefulset.yaml

+            - --listen-peer-urls=http://0.0.0.0:{{ $peerPort }}
+            - --advertise-client-urls=http://$(POD_NAME).{{ $etcdServiceName }}.{{ .Release.Namespace }}.svc.cluster.local:{{ $clientPort }}
+            - --listen-client-urls=http://0.0.0.0:{{ $clientPort }}
+            - --initial-cluster-token=my-etcd-cluster


Replacing it with a helm release related name looks better to me.

Make sense to me.

TrafalgarZZZ · 2024-01-04T04:56:37Z

charts/vineyard/templates/master/statefulset.yaml

+            {{- range $e, $i := until $replicas }}
+            {{- $initialCluster = append $initialCluster (printf "%s-%d=http://%s-%d.%s.%s.svc.cluster.local:%d" $etcdFullname $i $etcdFullname $i $etcdServiceName $releaseNamespace $peerPort) }}
+            {{- end }}
+            - --initial-cluster={{ join "," $initialCluster }}


Is it possible to reuse the helm func {{ include "vineyard.master.endpoint" . }} here?

Yes, "vineyard.master.endpoint" use the client port, but we need the peer port here. We could make the helm func suitable for the master port and the client port.

TrafalgarZZZ · 2024-01-04T04:57:56Z

charts/vineyard/templates/_helpers.tpl

+          claimName: {{ .name }}
+      {{- else }}
+      - name: {{ $mediumName }}
+        emptyDir:


I think we should consider mediumtype for emptyDir? when mediumtype=="MEM", emptyDir.meidum should be "Memory"

TrafalgarZZZ · 2024-01-04T05:27:11Z

charts/vineyard/Chart.yaml

@@ -0,0 +1,17 @@
+apiVersion: v1
+appVersion: 0.18.2
+version: 0.18.2


Better not relate version to appVersion according to Helm‘s doc .

Make sense. Thanks.

fluid-e2e-bot bot added the needs-ok-to-test label Dec 7, 2023

cheyang reviewed Dec 7, 2023

View reviewed changes

cheyang reviewed Dec 8, 2023

View reviewed changes

cheyang requested a review from TrafalgarZZZ December 10, 2023 07:10

dashanji force-pushed the add-vineyard-runtime-charts branch 4 times, most recently from a16c94b to b841f50 Compare December 14, 2023 12:26

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

dashanji force-pushed the add-vineyard-runtime-charts branch 3 times, most recently from aea1520 to 39b22bf Compare December 20, 2023 11:29

dashanji mentioned this pull request Dec 20, 2023

Add the controller and rbac yaml for vineyard runtime. #3659

Merged

dashanji force-pushed the add-vineyard-runtime-charts branch from 39b22bf to 604f676 Compare December 20, 2023 13:20

TrafalgarZZZ reviewed Dec 21, 2023

View reviewed changes

Add the app.kubernetes.io/instance label to the vineyard worker.

1a252b6

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

cheyang reviewed Dec 21, 2023

View reviewed changes

Change the short name of vineyard to v6d.

bd96909

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

dashanji force-pushed the add-vineyard-runtime-charts branch from e3a773e to bd96909 Compare December 21, 2023 05:42

TrafalgarZZZ approved these changes Dec 21, 2023

View reviewed changes

fluid-e2e-bot bot added the approved label Dec 21, 2023

cheyang self-requested a review December 22, 2023 02:41

cheyang approved these changes Dec 22, 2023

View reviewed changes

fluid-e2e-bot bot assigned cheyang Dec 22, 2023

fluid-e2e-bot bot added the lgtm label Dec 22, 2023

fluid-e2e-bot bot merged commit 7221cf3 into fluid-cloudnative:master Dec 22, 2023
10 checks passed

dashanji deleted the add-vineyard-runtime-charts branch December 22, 2023 02:55

TrafalgarZZZ reviewed Jan 4, 2024

View reviewed changes

TrafalgarZZZ mentioned this pull request Jan 4, 2024

Implement vineyard runtime engine and controller #3649

Merged

TrafalgarZZZ reviewed Jan 4, 2024

View reviewed changes

RongGu mentioned this pull request Aug 18, 2024

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

Open

		@@ -0,0 +1,91 @@
		# Copyright 2020-2023 Alibaba Group Holding Limited.

		@@ -0,0 +1,124 @@
		# Copyright 2020-2023 Alibaba Group Holding Limited.

	- {{ .Release.Namespace -}} - {{- .Release.Name }}
	- {{ .Release.Namespace }}-{{ .Release.Name }}

Add the helm chart for vineyard runtime. #3624

Add the helm chart for vineyard runtime. #3624

Conversation

dashanji commented Dec 7, 2023

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

fluid-e2e-bot bot commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashanji commented Dec 8, 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

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 Dec 21, 2023

Quality Gate passed

cheyang left a comment

Choose a reason for hiding this comment

fluid-e2e-bot bot commented Dec 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

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 bot commented Dec 7, 2023 •

edited

Loading