Storage proposal #385

LiZhenCheng9527 · 2023-09-15T12:10:40Z

What type of PR is this?

/kind design
/kind documentation

What this PR does / why we need it:
add proposal aimed to enhance Kurator by introducing unified distributed storage capabilities.
Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

add proposal for unified distributed storage

kurator-bot · 2023-09-15T12:10:44Z

@LiZhenCheng9527: The label(s) kind/design cannot be applied, because the repository doesn't have them.

In response to this:

What type of PR is this?

/kind design
/kind documentation

What this PR does / why we need it:
add proposal aimed to enhance Kurator by introducing unified distributed storage capabilities.
Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
add proposal for unified distributed storage

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.

netlify · 2023-09-15T12:10:44Z

✅ Deploy Preview for kurator-dev ready!

Name	Link
🔨 Latest commit	`8c8ffcd`
🔍 Latest deploy log	https://app.netlify.com/sites/kurator-dev/deploys/65221542e4869f0008ea1950
😎 Deploy Preview	https://deploy-preview-385--kurator-dev.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

docs/proposals/distributedstorage/distributedstorage.md

Xieql · 2023-09-18T03:09:21Z

docs/proposals/distributedstorage/distributedstorage.md

+
+    // ClusterSpec indicates the state that the cluster wants to be in
+    // +optional
+    ClusterSpec StorageClusterSpec `json:"spec"`


what if multiple clusters use the same StorageClusterSpec?
Is there a potential for duplicate StorageClusterSpec definitions if multiple clusters use the same spec?
you can consider fleet cluster, see

kurator/pkg/fleet-manager/fleet_cluster.go

Lines 45 to 74 in 76fcb95

func buildFleetClusters(ctx context.Context, client client.Client, fleet *fleetapi.Fleet) (map[ClusterKey]*fleetCluster, error) {

log := ctrl.LoggerFrom(ctx)

res := make(map[ClusterKey]*fleetCluster, len(fleet.Spec.Clusters))

for _, cluster := range fleet.Spec.Clusters {

clusterKey := types.NamespacedName{Namespace: fleet.Namespace, Name: cluster.Name}

clusterInterface, err := getFleetClusterInterface(ctx, client, cluster.Kind, clusterKey)

// TODO: should we make it work

if err != nil {

return nil, err

}

if !clusterInterface.IsReady() {

log.V(4).Info("cluster is not ready", "cluster", clusterKey)

continue

}

kclient, err := clientForCluster(client, fleet.Namespace, clusterInterface)

if err != nil {

return nil, err

}

res[ClusterKey{Kind: cluster.Kind, Name: cluster.Name}] = &fleetCluster{

Secret: clusterInterface.GetSecretName(),

SecretKey: clusterInterface.GetSecretKey(),

client: kclient,

}

}

return res, nil

}

Previously I thought fleet was able to filter managed clusters by label, but fleet doesn't support it. It will allow users to directly specify which clusters use the same ClusterSpec to avoid having multiple identical ClusterSpecs.

hzxuzhonghu

This is not user friendly.

Keep in mind, when design an api, it is a user-faced one, we should put UX as the highest priority.

For this target, we should expose only necessary settings with explicitly readable fields.

docs/proposals/distributedstorage/distributedstorage.md

hzxuzhonghu · 2023-09-18T02:26:15Z

docs/proposals/distributedstorage/distributedstorage.md

+- **unified distributed cloud storage**
+    - Supports three types of storage: Block storage, Filesystem storage and Object storage.
+    - Support for scoping different storage types by name, label, and Annotation.
+    - Support for applying these policies to multiple clusters or a single cluster.


Not sure what policies you mean

policy means storage type policies on nodes. To make the description clearer, third bullet point will be modified to Support for applying storage type policies on nodes to multiple clusters.

And The second bullet will be modified to Support for specifying different storage types at different nodes by name, label and annotation.

docs/proposals/distributedstorage/distributedstorage.md

hzxuzhonghu · 2023-09-18T02:29:28Z

docs/proposals/distributedstorage/distributedstorage.md

+
+- **unified distributed cloud storage**
+    - Supports three types of storage: Block storage, Filesystem storage and Object storage.
+    - Support for scoping different storage types by name, label, and Annotation.


what does scoping mean

Scoping means delimit the use of a storage type.

It is not clearly expressed here and will be modified to Support for specifying different storage types at different nodes by name, label and annotation.

hzxuzhonghu · 2023-09-18T02:31:38Z

docs/proposals/distributedstorage/distributedstorage.md

+and make progress.
+-->
+
+- **support for another distributed storage system** Rook only supports ceph distributed storage system, so it does not support other distributed storage systems.


This is an implementation detail.

NON-goals should be like this, do not support XXX storage, do not support storage backup/snapshot

docs/proposals/distributedstorage/distributedstorage.md

hzxuzhonghu · 2023-09-18T03:18:33Z

docs/proposals/distributedstorage/distributedstorage.md

+    // The annotations-related configuration to add/set on each Pod related object.
+	// +nullable
+	// +optional
+	Annotations rookv1.AnnotationsSpec `json:"annotations,omitempty"`


What are these annotations for? It it a must?

hzxuzhonghu · 2023-09-18T03:19:02Z

docs/proposals/distributedstorage/distributedstorage.md

+	// +kubebuilder:pruning:PreserveUnknownFields
+	// +nullable
+	// +optional
+	Labels rookv1.LabelsSpec `json:"labels,omitempty"`


same questions as annotations

hzxuzhonghu · 2023-09-18T03:20:30Z

docs/proposals/distributedstorage/distributedstorage.md

+	Mon MonSpec `json:"mon,omitempty"`
+
+    // mgr is responsible for connect between the node and operator
+    // A spec for mgr related options
+	// +optional
+	// +nullable
+	Mgr MgrSpec `json:"mgr,omitempty"`


As a user, i cannot understand these two fields at all

hzxuzhonghu · 2023-09-18T03:21:01Z

docs/proposals/distributedstorage/distributedstorage.md

+	// +kubebuilder:pruning:PreserveUnknownFields
+	// +nullable
+	// +optional
+	Placement PlacementSpec `json:"placement,omitempty"`


place what?

hzxuzhonghu · 2023-09-22T02:44:39Z

docs/proposals/distributedstorage/distributedstorage.md

+    // e.g. /var/lib/rook
+    // +kubebuilder:validation:Pattern=`^/(\S+)`
+    // +optional
+    DataDirHostPath string `json:"dataDirHostPath,omitempty"`


which component uses this config

hzxuzhonghu

I think it is still lack of the usage info, please put it under the plugins

hzxuzhonghu · 2023-10-07T01:24:54Z

docs/proposals/distributedstorage/distributedstorage.md

+-->
+Kurator, as an open-source distributed cloud-native platform, has been pivotal in aiding users to construct their distributed cloud-native infrastructure, thereby facilitating enterprise digital transformation.
+
+In order to further enhance its functionality, this proposal introduces a unified solution for distributed storage across multiple clsuters in `Fleet` through a seamless one-click operation.


clsuters typo

hzxuzhonghu · 2023-10-07T09:32:46Z

docs/proposals/distributedstorage/distributedstorage.md

+    // +optional
+    DataDirHostPath string `json:"dataDirHostPath,omitempty"`
+
+    // Monitor is the daemon for the state of ceph cluster.


this is not a sentence, can you complete it

hzxuzhonghu · 2023-10-07T09:33:14Z

docs/proposals/distributedstorage/distributedstorage.md

+```console
+// DistributedStorageSpec is the schema for the DistributedStorage's API.
+// Note: partly copied from https://github.com/rook/rook/blob/release-1.10/pkg/apis/ceph.rook.io/v1/types.go
+type DistributedStorageSpec struct {


Please donot call xxxSpec, unless it is a separate CRD

hzxuzhonghu

Generally LGTM

hzxuzhonghu · 2023-10-08T01:58:30Z

docs/proposals/distributedstorage/distributedstorage.md

+    // +optional
+    Chart *ChartConfig `json:"chart,omitempty"`
+
+    // Storage provides details on where the backup data should be stored.


remove on

why should you mention backup?

Signed-off-by: LiZhenCheng9527 <[email protected]>

hzxuzhonghu

/lgtm

kurator-bot · 2023-10-08T03:37:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

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~~ [hzxuzhonghu]

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

kurator-bot added the kind/documentation label Sep 15, 2023

kurator-bot requested review from hzxuzhonghu and kevin-wangzefeng September 15, 2023 12:10

kurator-bot added the size/XL label Sep 15, 2023

Xieql reviewed Sep 18, 2023

View reviewed changes

hzxuzhonghu reviewed Sep 18, 2023

View reviewed changes

hzxuzhonghu reviewed Sep 22, 2023

View reviewed changes

kurator-bot added size/L and removed size/XL labels Sep 25, 2023

LiZhenCheng9527 requested a review from hzxuzhonghu October 7, 2023 09:07

hzxuzhonghu reviewed Oct 7, 2023

View reviewed changes

hzxuzhonghu approved these changes Oct 8, 2023

View reviewed changes

kurator-bot added the approved label Oct 8, 2023

give a proposal of distributed storage

8c8ffcd

Signed-off-by: LiZhenCheng9527 <[email protected]>

hzxuzhonghu approved these changes Oct 8, 2023

View reviewed changes

kurator-bot assigned hzxuzhonghu Oct 8, 2023

kurator-bot added the lgtm label Oct 8, 2023

kurator-bot merged commit c8ad399 into kurator-dev:main Oct 8, 2023

LiZhenCheng9527 mentioned this pull request Oct 19, 2023

Implementing unified distributed storage for kurator #402

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage proposal #385

Storage proposal #385

LiZhenCheng9527 commented Sep 15, 2023

kurator-bot commented Sep 15, 2023

netlify bot commented Sep 15, 2023 •

edited

Loading

Xieql Sep 18, 2023 •

edited

Loading

LiZhenCheng9527 Sep 18, 2023

hzxuzhonghu left a comment

hzxuzhonghu Sep 18, 2023

LiZhenCheng9527 Sep 18, 2023

hzxuzhonghu Sep 18, 2023

LiZhenCheng9527 Sep 18, 2023

hzxuzhonghu Sep 18, 2023

hzxuzhonghu Sep 18, 2023

hzxuzhonghu Sep 18, 2023

hzxuzhonghu Sep 18, 2023

hzxuzhonghu Sep 18, 2023

hzxuzhonghu Sep 22, 2023

hzxuzhonghu left a comment

hzxuzhonghu Oct 7, 2023

hzxuzhonghu Oct 7, 2023

hzxuzhonghu Oct 7, 2023

hzxuzhonghu left a comment

hzxuzhonghu Oct 8, 2023

hzxuzhonghu Oct 8, 2023

hzxuzhonghu left a comment

kurator-bot commented Oct 8, 2023

	func buildFleetClusters(ctx context.Context, client client.Client, fleet fleetapi.Fleet) (map[ClusterKey]fleetCluster, error) {
	log := ctrl.LoggerFrom(ctx)

	res := make(map[ClusterKey]*fleetCluster, len(fleet.Spec.Clusters))
	for _, cluster := range fleet.Spec.Clusters {
	clusterKey := types.NamespacedName{Namespace: fleet.Namespace, Name: cluster.Name}
	clusterInterface, err := getFleetClusterInterface(ctx, client, cluster.Kind, clusterKey)
	// TODO: should we make it work
	if err != nil {
	return nil, err
	}

	if !clusterInterface.IsReady() {
	log.V(4).Info("cluster is not ready", "cluster", clusterKey)
	continue
	}

	kclient, err := clientForCluster(client, fleet.Namespace, clusterInterface)
	if err != nil {
	return nil, err
	}
	res[ClusterKey{Kind: cluster.Kind, Name: cluster.Name}] = &fleetCluster{
	Secret: clusterInterface.GetSecretName(),
	SecretKey: clusterInterface.GetSecretKey(),
	client: kclient,
	}
	}

	return res, nil
	}

Storage proposal #385

Storage proposal #385

Conversation

LiZhenCheng9527 commented Sep 15, 2023

kurator-bot commented Sep 15, 2023

netlify bot commented Sep 15, 2023 • edited Loading

✅ Deploy Preview for kurator-dev ready!

Xieql Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxuzhonghu left a 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

hzxuzhonghu left a 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

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxuzhonghu left a comment

Choose a reason for hiding this comment

kurator-bot commented Oct 8, 2023

netlify bot commented Sep 15, 2023 •

edited

Loading

Xieql Sep 18, 2023 •

edited

Loading