Implement vineyard runtime engine and controller #3649
Conversation
|
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 Once the patch is verified, the new status will be reflected by the 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. |
dashanji
changed the title
dashanji
force-pushed
the
implement-vineyard-runtime-dev
branch
from
65d7f32
to
8dd8590
Compare
dashanji
changed the title
dashanji
force-pushed
the
implement-vineyard-runtime-dev
branch
from
8dd8590
to
72a8699
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3649 +/- ##
==========================================
+ Coverage 64.40% 64.47% +0.06%
==========================================
Files 444 471 +27
Lines 26797 28140 +1343
==========================================
+ Hits 17258 18142 +884
- Misses 7524 7844 +320
- Partials 2015 2154 +139 ☔ View full report in Codecov by Sentry. |
dashanji
force-pushed
the
implement-vineyard-runtime-dev
branch
2 times, most recently
from
88e3956
to
7f51863
Compare
|
@dashanji Please fix the conflict. Thanks. |
Makefile
Outdated
| CSI_IMG ?= ${IMG_REPO}/fluid-csi | ||
| LOADER_IMG ?= ${IMG_REPO}/fluid-dataloader | ||
| INIT_USERS_IMG ?= ${IMG_REPO}/init-users | ||
| WEBHOOK_IMG ?= ${IMG_REPO}/fluid-webhook | ||
| CRD_UPGRADER_IMG?= ${IMG_REPO}/fluid-crd-upgrader | ||
| GO_MODULE ?= off |
There was a problem hiding this comment.
I suggest keeping it as off for compatible with other runtime controllers.
ok. |
dashanji
force-pushed
the
implement-vineyard-runtime-dev
branch
from
7f51863
to
9337d8b
Compare
| @@ -0,0 +1,120 @@ | |||
| /* | |||
There was a problem hiding this comment.
Could you help keep the same format with the followings:
/*
Copyright 2023 The Fluid Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/There was a problem hiding this comment.
Thanks for the advice, I have updated the lincense from 2023 to 2024.
dashanji
force-pushed
the
implement-vineyard-runtime-dev
branch
from
e48a60c
to
bd81941
Compare
|
/test fluid-e2e |
| @@ -161,6 +176,8 @@ spec: | |||
| hostPath: | |||
| path: /runtime-mnt/vineyard/{{ .Release.Namespace }}/{{ $fullName }} | |||
There was a problem hiding this comment.
I think /runtime-mnt is configurable. Try to avoid hard code this directory.
dashanji
force-pushed
the
implement-vineyard-runtime-dev
branch
from
4311a72
to
a5908fa
Compare
| @@ -91,6 +91,9 @@ spec: | |||
| - --spill_upper_rate | |||
| - "{{ include "vineyard.spill.upperRate" . }}" | |||
| {{- end }} | |||
| - --metrics | |||
There was a problem hiding this comment.
Does it mean that vineyard reports metrics via exporter? But how vineyardd manage to find out which is the allocated exporter port?
There was a problem hiding this comment.
Does it mean that vineyard reports metrics via exporter?
By vineyardd itself. The --metrics field means vineyardd will print internal metrics info (only about vineyard object) to the stdout and the log_dir field means vineyard will store these metrics info under a directory.
But how vineyardd manage to find out which is the allocated exporter port?
vineyardd doesn't need to know the port, we have an external exporter to convert these logs (/var/log/vineyard) to metrics via https://github.com/fluid-cloudnative/fluid/pull/3649/files#diff-6109bb7b99b63ccc2d90ea9d7c1ca8dece4d3873aabfc77280196643e7f6593cR162.
There was a problem hiding this comment.
Thanks! Got it now. One more question is that, does Vineyardd automatically rotate its log messages? And what will happen if the external exporter fail over? Will the external exporter recover all the metrics from the all the logs?
There was a problem hiding this comment.
does Vineyardd automatically rotate its log messages?
No, vineyardd will preserve all log messages.
And what will happen if the external exporter fail over?
It will convert all logs to metrics again.
Will the external exporter recover all the metrics from the all the logs?
Yes, from the log file.
There was a problem hiding this comment.
I've also found some minor issue in the Vineyard helm chart, and I left some comments in the previous PR
| return utils.RequeueIfError(errors.Wrap(err, "Unable to get ddc runtime")) | ||
| } | ||
| } | ||
| ctx.Runtime = runtime |
There was a problem hiding this comment.
Please also assign ctx.EngineImpl like the code here. Recently, we added a new field named ctx.EngineImpl to build ddc engine. You can see the PR description for more details.
pkg/ddc/vineyard/master_internal.go
Outdated
| } | ||
|
|
||
| //2. Get the template value file | ||
| valueFile, err := os.CreateTemp(os.TempDir(), fmt.Sprintf("%s-%s-values.yaml", e.name, e.runtimeType)) |
There was a problem hiding this comment.
Ditto. Recently change the temp file name using <e.name>-<e.engineImpl>-values.yaml. For vineyard, engineImpl should be the same with runtimeType, I think.
pkg/ddc/vineyard/master_internal.go
Outdated
| } | ||
|
|
||
| func (e *VineyardEngine) getConfigmapName() string { | ||
| return e.name + "-" + e.runtimeType + "-values" |
There was a problem hiding this comment.
Ditto. Better use e.engineImpl instead
pkg/ddc/vineyard/transform.go
Outdated
| } | ||
|
|
||
| func (e *VineyardEngine) transformTieredStore(runtime *datav1alpha1.VineyardRuntime) TieredStore { | ||
| quota := resource.MustParse("4Gi") |
There was a problem hiding this comment.
For vineyard users, How to set Vineyard's memory quota? And How to set spill quota and path in VineyardRuntime.spec.tieredstore?
There was a problem hiding this comment.
How to set Vineyard's memory quota?
tieredstore:
levels:
- mediumtype: MEM
quota: 4GiHow to set spill quota and path in VineyardRuntime.spec.tieredstore?
tieredstore:
levels:
- level: 0
mediumtype: MEM
quota: 4Gi
- level: 1
mediumtype: SSD
quota: 10Gi
volumeType: Hostpath
path: /var/spill-pathThere was a problem hiding this comment.
Thx. I wonder why there is a constant MEM level with 4Gi in transformTieredStore?
There was a problem hiding this comment.
If users doesn't set the any tieredstore fields in the VineyardRuntime.spec, the 4Gi will be used as the default one.
There was a problem hiding this comment.
OK, but here's something tricky. Now the code firstly value.TieredStore = e.transformTieredStore(runtime), and then calculates memory request of Vineyard workers according to runtimeInfo's GetTieredStoreInfo() in e.transformResourcesForWorker(). That makes an inconsistency when a default 4Gi MEM level is used. However, I do think it's a common problem for all the runtimes, so I think the code LGTM now. I will try to refactor the runtimeInfo code to make it clearer and more consistent. cc @cheyang
There was a problem hiding this comment.
Make sense to me. I have fixed it in the latest commit and it will raise an error if the tieredStore of vineyard runtime is empty.
Signed-off-by: Ye Cao <[email protected]>
Signed-off-by: Ye Cao <[email protected]>
Signed-off-by: Ye Cao <[email protected]>
Signed-off-by: Ye Cao <[email protected]>
Signed-off-by: Ye Cao <[email protected]>
Signed-off-by: Ye Cao <[email protected]>
Signed-off-by: Ye Cao <[email protected]>
Signed-off-by: Ye Cao <[email protected]>
Signed-off-by: Ye Cao <[email protected]>
Signed-off-by: Ye Cao <[email protected]>
dashanji
force-pushed
the
implement-vineyard-runtime-dev
branch
from
a5908fa
to
e04d56c
Compare
Signed-off-by: Ye Cao <[email protected]>
|
Hi @cheyang @TrafalgarZZZ, could you please take a look at this? Thanks. |
…ocket to vineyard-fluid-mount. Signed-off-by: Ye Cao <[email protected]>
charts/vineyard/values.yaml
Outdated
|
|
||
| fuse: | ||
| image: vineyardcloudnative/mount-vineyard-socket | ||
| image: vineyardcloudnative/vineyard-fluid-mount |
There was a problem hiding this comment.
How about naming it to vineyard-fluid-fuse?
…fluid-fuse. Signed-off-by: Ye Cao <[email protected]>
Signed-off-by: Ye Cao <[email protected]>
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheyang 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:
Approvers can indicate their approval by writing |
) * Expose metric for vineyard runtime. Signed-off-by: Ye Cao <[email protected]> * Add the rbac role and dockerfile for vineyard runtime. Signed-off-by: Ye Cao <[email protected]> * Add the vineyard runtime command for lanuching vineyard controller. Signed-off-by: Ye Cao <[email protected]> * Implement the vineyard engine and controller. Signed-off-by: Ye Cao <[email protected]> * Add the unit test for vineyard engine and controller. Signed-off-by: Ye Cao <[email protected]> * Fix the lint issues. Signed-off-by: Ye Cao <[email protected]> * Fix unit test errors. Signed-off-by: Ye Cao <[email protected]> * Turn off the golang module. Signed-off-by: Ye Cao <[email protected]> * Resolve conflicts. Signed-off-by: Ye Cao <[email protected]> * Format the licenses. Signed-off-by: Ye Cao <[email protected]> * Make the fuse dir configurable. Signed-off-by: Ye Cao <[email protected]> * Improve the helm chart of vineyard runtime and the vineyard engine. Signed-off-by: Ye Cao <[email protected]> * Delete the svc domain in the vineyard helm chart. Signed-off-by: Ye Cao <[email protected]> * Disable the vineyard metrics and change the image of vineyard-mount-socket to vineyard-fluid-mount. Signed-off-by: Ye Cao <[email protected]> * Change the vineyard fuse image from vineyard-fluid-mount to vineyard-fluid-fuse. Signed-off-by: Ye Cao <[email protected]> * Raise an error when the tieredStore of vineyard runtime is empty. Signed-off-by: Ye Cao <[email protected]> --------- Signed-off-by: Ye Cao <[email protected]>
…native#3649)" This reverts commit 45b9c92.
Fixes #3528