-
Notifications
You must be signed in to change notification settings - Fork 547
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: (lastSyncInfo)handle last sync duration being empty case #3930
Conversation
This commit modifies code to handle last sync duration being empty & 0,returning nil & 0 on encountering it respectively. Earlier both case return 0. Test case is added too. Signed-off-by: Rakshith R <rar@redhat.com>
/test ci/centos/k8s-e2e-external-storage/1.25 |
/test ci/centos/k8s-e2e-external-storage/1.26 |
/test ci/centos/k8s-e2e-external-storage/1.27 |
/test ci/centos/mini-e2e-helm/k8s-1.25 |
/test ci/centos/mini-e2e-helm/k8s-1.26 |
/test ci/centos/mini-e2e-helm/k8s-1.27 |
/test ci/centos/mini-e2e/k8s-1.25 |
/test ci/centos/mini-e2e/k8s-1.26 |
/test ci/centos/mini-e2e/k8s-1.27 |
/test ci/centos/upgrade-tests-cephfs |
/test ci/centos/upgrade-tests-rbd |
description: `replaying, {"bytes_per_second":0.0,"bytes_per_snapshot":81920.0,"last_snapshot_sync_seconds":0, | ||
"last_snapshot_bytes":81920,"local_snapshot_timestamp":1684675261,"remote_snapshot_timestamp":1684675261,"replay_state":"idle"}`, |
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.
am not sure we can have this case from Ceph, where we will have last_snapshot_bytes but duration is 0
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.
am not sure we can have this case from Ceph, where we will have last_snapshot_bytes but duration is
0
I did not focus on other parameters.
We can ignore that parameter for this test case since parameters are not interlinked here, this test case only focuses on duration.
LastSnapshotDuration int64 `json:"last_snapshot_sync_seconds"` | ||
LocalSnapshotTime int64 `json:"local_snapshot_timestamp"` | ||
LastSnapshotBytes int64 `json:"last_snapshot_bytes"` | ||
LastSnapshotDuration *int64 `json:"last_snapshot_sync_seconds"` |
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 last_snapshot_sync_seconds
can be empty when Ceph returns this, can last_snapshot_bytes
not also be empty?
Is it known what Ceph versions return incomplete 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.
if
last_snapshot_sync_seconds
can be empty when Ceph returns this, canlast_snapshot_bytes
not also be empty?
Maybe but currently it does not matter since there's no *int64 type in protoc3 sadly.
Anyway csiaddons defines bytes 0 as empty and sets it to nil in status.
This pr just rectifies the nil case for last sync duration.
Is it known what Ceph versions return incomplete results?
I asked the same too in previous pr but didn't get an answer.
This commit modifies code to handle last sync duration being empty & 0,returning nil & 0 on encountering it respectively. Earlier both case returned 0. Test case is added for the same too.