Add New VRCs & VSCs, FixDuplicate SC-ID Issue #1823
Conversation
rakeshgm
force-pushed
the
drenv-multisc-cont
branch
from
48ebadd to
538dc1d
Compare
Can you separate the fix so we can merge it quickly? If the previous changes introduced an issue, we should fix it first before adding more changes. |
|
Also it is not clear to me what is the purpose of this change. What are the changes and why they are needed? The pr message shows list of resources but it is not clear what we had before and what is new. |
|
results of |
rakeshgm
force-pushed
the
drenv-multisc-cont
branch
from
538dc1d to
8221ac3
Compare
test/addons/rbd-mirror/start
Outdated
| @@ -108,11 +108,11 @@ def configure_rbd_mirroring(cluster, peer_info): | |||
|
|
|||
| print("Creating VolumeReplicationClass") | |||
| template = drenv.template("start-data/vrc-sample.yaml") | |||
| yaml = template.substitute(cluster=cluster) | |||
| yaml = template.substitute(cluster=cluster, scname="rook-ceph-block") | |||
There was a problem hiding this comment.
scname is cryptic. How about storage_class="rook-ceph-block"?
There was a problem hiding this comment.
I agree, I would like to take this enhancement in the next PR. the important parts are completed now.
There was a problem hiding this comment.
I think it is better to fix this now, this is a simple rename. But I'm also ok with fixing this later if you don't have time now.
| @@ -13,7 +13,7 @@ kind: VolumeSnapshotClass | |||
| metadata: | |||
| name: csi-cephfsplugin-snapclass | |||
| labels: | |||
| ramendr.openshift.io/storageid: rook-cephfs-$cluster-1 | |||
| ramendr.openshift.io/storageid: rook-cephfs-$fsname-$cluster-1 | |||
There was a problem hiding this comment.
why we don't use the storage class name instead of hardcoding "rook-cephfs"?
| @@ -7,7 +7,7 @@ kind: StorageClass | |||
| metadata: | |||
| name: $name | |||
| labels: | |||
| ramendr.openshift.io/storageid: rook-ceph-$cluster-1 | |||
| ramendr.openshift.io/storageid: $name-$cluster-1 | |||
| @@ -11,7 +11,7 @@ kind: StorageClass | |||
| metadata: | |||
| name: rook-cephfs-$fsname | |||
| labels: | |||
| ramendr.openshift.io/storageid: rook-cephfs-$cluster-1 | |||
| ramendr.openshift.io/storageid: rook-cephfs-$fsname-$cluster-1 | |||
There was a problem hiding this comment.
I wonder why we don't use the same method - the storage class name. Using the same way to customize will make the code simpler and more uniform.
rakeshgm
force-pushed
the
drenv-multisc-cont
branch
3 times, most recently
from
a6cd233 to
9a817f1
Compare
test/addons/rbd-mirror/start
Outdated
| @@ -108,11 +108,11 @@ def configure_rbd_mirroring(cluster, peer_info): | |||
|
|
|||
| print("Creating VolumeReplicationClass") | |||
| template = drenv.template("start-data/vrc-sample.yaml") | |||
| yaml = template.substitute(cluster=cluster) | |||
| yaml = template.substitute(cluster=cluster, scname="rook-ceph-block") | |||
There was a problem hiding this comment.
I think it is better to fix this now, this is a simple rename. But I'm also ok with fixing this later if you don't have time now.
| @@ -9,9 +9,9 @@ | |||
| apiVersion: storage.k8s.io/v1 | |||
| kind: StorageClass | |||
| metadata: | |||
| name: rook-cephfs-$fsname | |||
| name: $scname | |||
There was a problem hiding this comment.
This is called "name" in the rbd storage class - lets be consistent.
| VOLUME_REPLICATION_CLASSES, | ||
| VOLUME_GROUP_REPLICATION_CLASSES, | ||
| ) | ||
| ] |
There was a problem hiding this comment.
STORAGE_CLASSES_MAP is very confusing since this is not a map - it is a list.
This is also very confusing code, I find it very hard to understand.
We can simplify this by creating static list of dicts like this:
CONFIG = [
{
"pool": "replicapool",
"sc": "rook-ceph-block",
"vrc": "vrc-sample",
"vgrc": "vgrc-sample",
},
{
"pool": "replicapool-2",
"sc": "rook-ceph-block-2",
"vrc": "vrc-sample-2",
"vgrc": "vgrc-sample-2",
},
]The code can can accept a config dict and use the values from the dict.
This structure can also be extracted to external yaml files later, to separate the code from the configuration. Same script will be able to create different configurations.
| @@ -33,7 +52,7 @@ def fetch_secret_info(cluster): | |||
|
|
|||
| print(f"Getting mirroring info site name for cluster '{cluster}'") | |||
| info["name"] = drenv.wait_for( | |||
| f"cephblockpools.ceph.rook.io/{POOL_NAME}", | |||
| f"cephblockpools.ceph.rook.io/{POOL_NAMES[0]}", | |||
There was a problem hiding this comment.
With the simplified configuration I suggested, this will become:
CONFIG[0]["pool"]
| vgrcname=storage_class["vgrc"], | ||
| scname=storage_class["name"], | ||
| ) | ||
| kubectl.apply("--filename=-", input=yaml, context=cluster) |
There was a problem hiding this comment.
I don't want to complicate the code like this. Lets keep the code simple, creating and setting up mirroring on for single pool.
To create multiple vrc/vgrc/sc and setup mirroring on all pools, we can run the entire code in parallel with 2 configurations. Each thread will create resources and set up mirroring for single pool.
| STORAGE_CLASSES_MAP = [ | ||
| {"fs": fs, "name": f"{STORAGE_CLASS_NAME_PREFIX}{fs}", "vsc": vsc} | ||
| for fs, vsc in zip(FS_NAMES, CEPHFS_VOLUME_SNAPSHOT_CLASSES) | ||
| ] |
There was a problem hiding this comment.
This is not a map but a list. Should be the same static list with all the info as in rbd-mirror/start.
|
I will make this PR a draft now and open a separate PR for the duplicate SID fix since that changes are mostly reviewed and we want them soon. |
rakeshgm
force-pushed
the
drenv-multisc-cont
branch
from
9a817f1 to
56762b3
Compare
adding new VRCs and VSCs based on the newly created SCs Signed-off-by: rakeshgm <[email protected]>
rakeshgm
force-pushed
the
drenv-multisc-cont
branch
from
56762b3 to
3827c60
Compare
This PR continues the improvements introduced in [1] and [2] by adding new VRCs and VSCs based on the newly created SCs. Additionally, it includes fixes to prevent duplicate SC-ID generation.
References:
[1] #1756
[2] #1756
Here is the tree of all the resources. I have not made any changes to the existing names. I will try to fix them in the next updates. However, I believe this is a necessary change with multiple SCs in the cluster.
This is the status of
PeerClassesfrom DRPolicy