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

rbd-mirror: add information about the last snapshot sync to image status #49299

Merged
merged 1 commit into from Feb 26, 2023

Conversation

weirdwiz
Copy link
Contributor

@weirdwiz weirdwiz commented Dec 7, 2022

Currently, the rbd mirror status command does not include information about the time taken to sync and bytes in the last snapshot, making it difficult to monitor and troubleshoot the mirroring process. This feature request is to add "last_snapshot_bytes" and "last_snapshot_seconds" fields to the rbd mirror status command, providing administrators with valuable information about the performance of the mirroring process.

Use case:
In snapshot-based mirroring, it's important to know how long it took to sync the last snapshot and how many bytes were transferred during the sync process. This information is useful for monitoring the performance of the mirroring process and identifying any issues that may arise. By adding these fields to the rbd mirror status command, administrators will be able to quickly and easily retrieve this information and use it to optimize the mirroring process.

Fixes: https://tracker.ceph.com/issues/58755

@weirdwiz weirdwiz requested a review from a team as a code owner December 7, 2022 10:28
@github-actions github-actions bot added the rbd label Dec 7, 2022
@weirdwiz weirdwiz marked this pull request as draft December 7, 2022 10:28
Copy link
Contributor

@idryomov idryomov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd propose to name the new fields last_snapshot_bytes and last_snapshot_seconds to be closer to existing bytes_per_snapshot and seconds_until_synced fields (and make the units clear -- "time" is is too generic).

@weirdwiz weirdwiz marked this pull request as ready for review December 16, 2022 05:38
Copy link
Contributor

@ajarr ajarr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need a tracker ticket for this so that it's easier to backport? And that can have more info about the use case.

src/tools/rbd_mirror/image_replayer/snapshot/Replayer.cc Outdated Show resolved Hide resolved
@idryomov
Copy link
Contributor

Maybe we need a tracker ticket for this so that it's easier to backport?

@weirdwiz Is this done? Please add a link to the commit message as per https://github.com/ceph/ceph/blob/main/SubmittingPatches.rst#id8.

@weirdwiz
Copy link
Contributor Author

sorry for the delay, there was an issue in my build environment.
@idryomov i tried to make the tracker, but for some reason my account doesn't seem to work correctly.

@idryomov
Copy link
Contributor

sorry for the delay, there was an issue in my build environment.

How was this tested? Can you link some pastes/screenshots show-casing the new fields? last_snapshot_bytes in particular should be equal to the size of the delta that is generated.

@idryomov i tried to make the tracker, but for some reason my account doesn't seem to work correctly.

Please try again now.

@weirdwiz weirdwiz requested review from ajarr and idryomov and removed request for ajarr February 17, 2023 06:14
@weirdwiz
Copy link
Contributor Author

I tested it out by writing data to the snapshots using the bench command,

for an empty image,

bin/rbd --cluster site-a bench --io-type write --io-size 64K --io-threads 2 --io-total 4G --io-pattern seq data/image2

status:

{"bytes_per_second":0.0,"bytes_per_snapshot":4294967296.0,"last_sna
pshot_bytes":4294967296,"last_snapshot_seconds":38,"local_snapshot_timestamp":1676612875,"remot
e_snapshot_timestamp":1676612875,"replay_state":"idle"}

wrote 2 gigabytes again to the same image

status:

 {"bytes_per_second":0.0,"bytes_per_snapshot":3221225472.0,"last_snapshot_bytes":2147483648,"last_snapshot_seconds":20,"local_snapshot_timestamp":1676613141,"remot
e_snapshot_timestamp":1676613141,"replay_state":"idle"}

@idryomov
Copy link
Contributor

Fixes: https://tracker.ceph.com/issues/58755

This needs to be added to the commit message too, not just to the description of the PR.

@weirdwiz weirdwiz requested review from ajarr and idryomov and removed request for idryomov and ajarr February 17, 2023 09:56
idryomov
idryomov previously approved these changes Feb 17, 2023
@idryomov idryomov dismissed their stale review February 17, 2023 10:26

A couple more tweaks are needed

this commit adds fields for time taken to sync and bytes in the last
snapshot to the mirror image status command.

Fixes: https://tracker.ceph.com/issues/58755

Signed-off-by: Divyansh Kamboj <dkamboj@redhat.com>
@idryomov
Copy link
Contributor

jenkins test make check

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