Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURES] 允许 mounts 为空. Add 'allowEmptyMounts' field to allow 'mounts' to be empty. #4322

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

y805939188
Copy link

Ⅰ. Describe what this PR does

Dataset CRD 增加一个 allowEmptyMounts 字段,如果为 true 的话,则 Datasetspec.mounts 允许传空。
我的场景中,需要使用一个自定义的 ThinRuntime 去实现一些自定义挂载的功能,比如动态挂载/卸载数据集。
在卸载数据集场景中,用户有可能把 mounts 卸成空。但是当前的 CRD 中,虽然 kubebuilder 注释中写了 "允许为空":
image
但是目前不生效:
image
原因可能是注释中存在 +kubebuilder:validation:MinItems=1

因此增加一个新的字段 allowEmptyMounts,为 true 时才允许 mounts 为空。这样相比直接去掉 MinItems 的好处是增量修改,不影响原来其他任何地方。

不过该功能只能在 k8s 1.25 以上版本生效,不确定 fluid 是否对向下兼容这块儿有什么要求。如果有什么问题的话请指出。谢谢~

I would like to propose adding a new field allowEmptyMounts for the Dataset CRD to allow spec.mounts to be empty. In my scenario, I am using ThinRuntimeProfile to achieve features like dynamically mounting datasets. However, there are situations where a user might unmount all datasets, leaving no mountPoint. Therefore, I hope that Dataset can support passing an empty list for Dataset.spec.mounts, so it can handle the empty mounts scenario specifically in the ThinRuntimeProfile image.

Currently, while the mounts code allows it to be empty, this is invalid due to the Minitems. To solve this, I added an allowEmptyMounts == true flag, which permits mounts to be empty without affecting the existing logic. If there are any concerns regarding this change, please let me know. Thanks~

Ⅱ. Does this pull request fix one issue?

fixes #4317

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

Ⅳ. Describe how to verify it

Apply it like:

apiVersion: data.fluid.io/v1alpha1
kind: Dataset
metadata:
  name: jfs-test-1
  namespace: test
spec:
  accessModes:
    - ReadWriteMany
  sharedEncryptOptions:
    - xxx
  allowEmptyMounts: true # 不传时不允许 mounts 为空
  mounts: []

Ⅴ. Special notes for reviews

Copy link

fluid-e2e-bot bot commented Sep 19, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign cheyang for approval by writing /assign @cheyang in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

Copy link

fluid-e2e-bot bot commented Sep 19, 2024

Hi @y805939188. 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.

Copy link

sonarcloud bot commented Sep 19, 2024

@Syspretor
Copy link
Collaborator

@y805939188
First of all, thank you very much for your PR. Recently, I have also been working as a participant to continuously improve dynamic mounting capabilities. In our user scenarios, we have encountered the same issue where users choose to leave mounts empty, i.e., unload all mount points. Currently, as you mentioned, the CRD Dataset has a minimum length of 1 for the mounts array. I discussed this issue with several other participants @cheyang @TrafalgarZZZ , and since different runtimes support different mount lengths for datasets, we believe that this limitation should not be enforced by the CRD configuration but should instead be determined by the runtime's own implementation. Therefore, we think the following modifications are more reasonable:

  1. Remove the minimum quantity restriction in the CRD Dataset.
  2. Delegate the validation logic for the length and legality of the mounts array in Dataset to the runtime's implementation.
  3. Define four types of mount point restrictions for the runtime to choose from: support for 0 mount points, support for 1 mount point, support for multi-mount points with length restrictions, and support for no restrictions on mount points.
  4. To ensure compatibility, only the Thinruntime supports 0 mount points, while the requirements for mounts length for other runtimes remain unchanged.
  5. If the runtime implementation determines that the dataset mounts are invalid, it will not set the dataset to a bound state, and an event will be generated.

If you have better suggestions, please feel free to share them. I am currently working on this issue and plan to submit a PR this week, so we can review it together. I'm also very interested in your business scenario and would love to have a deeper discussion about the dynamic mounting capabilities. Therefore, I will sent you an email and hope to receive a response.

@y805939188
Copy link
Author

@y805939188 First of all, thank you very much for your PR. Recently, I have also been working as a participant to continuously improve dynamic mounting capabilities. In our user scenarios, we have encountered the same issue where users choose to leave mounts empty, i.e., unload all mount points. Currently, as you mentioned, the CRD Dataset has a minimum length of 1 for the mounts array. I discussed this issue with several other participants @cheyang @TrafalgarZZZ , and since different runtimes support different mount lengths for datasets, we believe that this limitation should not be enforced by the CRD configuration but should instead be determined by the runtime's own implementation. Therefore, we think the following modifications are more reasonable:

  1. Remove the minimum quantity restriction in the CRD Dataset.
  2. Delegate the validation logic for the length and legality of the mounts array in Dataset to the runtime's implementation.
  3. Define four types of mount point restrictions for the runtime to choose from: support for 0 mount points, support for 1 mount point, support for multi-mount points with length restrictions, and support for no restrictions on mount points.
  4. To ensure compatibility, only the Thinruntime supports 0 mount points, while the requirements for mounts length for other runtimes remain unchanged.
  5. If the runtime implementation determines that the dataset mounts are invalid, it will not set the dataset to a bound state, and an event will be generated.

If you have better suggestions, please feel free to share them. I am currently working on this issue and plan to submit a PR this week, so we can review it together. I'm also very interested in your business scenario and would love to have a deeper discussion about the dynamic mounting capabilities. Therefore, I will sent you an email and hope to receive a response.

我已经发送微信好友申请了,有劳通过下,谢谢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants