-
Notifications
You must be signed in to change notification settings - Fork 59
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
Set up mirroring for multiple cephblockpools #1829
base: main
Are you sure you want to change the base?
Conversation
48871ee
to
28732b9
Compare
I am not sure how much aligned this change would be wrt to the drenv tool, but me as a user found this inconvenience while using the tool for my testing and thought of sending a fix for it :) |
/cc @nirs |
Test results:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
One problem in this approach is that we have to change lot of code to loop over pools, and it may not efficient since we need to wait for one pool and then wait for the other.
In test/addons/volsync/test we use another approach - we run the same test twice using concurrent.futures.ThreadPoolExecutor
. This keep the code simple and run everything in parallel without modifying the code. I think this is the right approach for new code since it is much easier to work with the code this way, and it is likely more efficient.
Regardless, we need to include the pool names in logs so we can understand whats going on and debug this.
test/addons/rbd-mirror/start
Outdated
@@ -32,33 +32,35 @@ def fetch_secret_info(cluster): | |||
info = {} | |||
|
|||
print(f"Getting mirroring info site name for cluster '{cluster}'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would help to move the log into the loop, so it also include the pool name.
Same for other logs in this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, will update this.
test/addons/rbd-mirror/start
Outdated
@@ -86,33 +88,37 @@ def disable_rbd_mirror_debug_logs(cluster): | |||
def configure_rbd_mirroring(cluster, peer_info): | |||
print(f"Applying rbd mirror secret in cluster '{cluster}'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Samme for this log, we want to have one log per pool.
test/addons/rbd-mirror/start
Outdated
|
||
print("Creating VolumeReplicationClass") | ||
template = drenv.template("start-data/vrc-sample.yaml") | ||
yaml = template.substitute(cluster=cluster) | ||
kubectl.apply("--filename=-", input=yaml, context=cluster) | ||
|
||
template = drenv.template("start-data/vgrc-sample.yaml") | ||
yaml = template.substitute(cluster=cluster, pool=POOL_NAME) | ||
yaml = template.substitute(cluster=cluster, pool="replicapool") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we configure mirroring on multiple pools, why not create multiple vrc/vgrc?
If we don't need it now, please add a comment explaining why we create vrc/vgrc only for replicapool.
But in any case, use POOL_NAMES[0] so if we change the name of the pools this code will remain correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not sure if we need multiple vrc/vgrc, hence I omitted their creation.
They might just be idle and not be required at all, hence I beleive it might not be worth creating them at the first place and if anybody needs it, they can create it manually.
It is easier to create than a blockpool with mirroring enabled ;)
But in any case, use POOL_NAMES[0] so if we change the name of the pools this code will remain correct.
Sure, will update that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we call this in parallel the vrc/vgrc will be created automatically. I don't see reason not to create them if we create a pool and set up mirroring, this is the easy part. It will enable interesting tests that we don't do now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if we go ahead with creating multiple vrc/vgrc then how are planning to name them? If we just strip
the number from replicapool-2
even then are we sure that is how we are going to name always?
test/addons/rbd-mirror/start
Outdated
"--namespace=rook-ceph", | ||
context=cluster, | ||
) | ||
info = {f"Cluster '{cluster}' ceph block pool status": json.loads(status)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the name of the pool to make the log lines unique.
test/addons/rbd-mirror/start
Outdated
print( | ||
f"Cluster '{cluster}' mirroring healthy in {elapsed:.2f} seconds" | ||
) | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will wait for one pool status, and then wait for the other. It would be better to wait for both in parallel but it should be good enough. When the first pool is ready the wait for the second should be very short or zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, from my testing while the loop reaches for the second pool it hardly takes a couple of secs to complete through as the mirroring already got setup when it was iterating through the first loop. So, probably we can skip the trouble of parallelizing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parallelizing is easy if we do this in the top level, see test/addons/volsync/test.
Can you open an issue explaining the problem you experience? Our e2e tests works fine with current code since we don't use the second replica pool. |
I won't describe it as an issue exactly, but more of an incovenience. |
Ok, than no issue needed. |
@Nikhil-Ladha can you update the commit message and pr message to reflect the actual change? (we have multiple pool but configure mirroring only one one, why you want to configure mirroring on the second, etc). |
28732b9
to
91fd243
Compare
Updated the code to parallelize the setup of mirroring 2 pools. Also, added pool names in the log wherever applicable. |
update setup of rbd mirroring and peer secrets addition, when more than one cephblockpool is configured in ramen tests Signed-off-by: Nikhil-Ladha <[email protected]>
91fd243
to
1cb97a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comments wrt to logging
context=cluster, | ||
) | ||
|
||
print("Waiting for replica pool peer token") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pool name needs to be logged here
kubectl.apply("--filename=-", input=yaml, context=cluster) | ||
|
||
print("Creating VolumeGroupReplicationClass") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since planning to have multiple vrc and vrgc, logging their names would make it clearer and easier to debug.
cluster1_info = fetch_secret_info(cluster1, pool) | ||
cluster2_info = fetch_secret_info(cluster2, pool) | ||
|
||
print(f"Setting up mirroring from '{cluster2}' to '{cluster1}'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log "Setting up mirroring for '{pool}' from...."
print(f"Setting up mirroring from '{cluster2}' to '{cluster1}'") | ||
configure_rbd_mirroring(cluster1, pool, cluster2_info) | ||
|
||
print(f"Setting up mirroring from '{cluster1}' to '{cluster2}'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
"--namespace=rook-ceph", | ||
context=cluster, | ||
) | ||
info = {"ceph pool status": json.loads(out)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log ceph pool status with pool name
Seems that this change does a lot of changes already in #1823. Lets wait until that PR is finished. |
vrc_name = "vrc-sample" | ||
vgrc_name = "vgrc-sample" | ||
if pool != 'replicapool': | ||
num = pool.split("-")[1] | ||
vrc_name = f"{vrc_name}-{num}" | ||
vgrc_name = f"{vgrc_name}-{num}" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though the logic looks nice, I would not check if the name is replicapool
and then proceed to create secondary vrc and vgrc. Also the storageIDs would be same for both VRC and VGRC in this case, replicationID should also change once storageClass is different. I have a PR opened with similar changes. Once that PR gets merged, you probably need to rebase and work on top of it.
fix rbd mirroring and peer secrets addition, when more than one cephblockpool is configured in ramen tests