rbd: get lastsyncbytes and lastsycduration for volrep #3894

yati1998 · 2023-06-09T10:47:28Z

This commit get more information from the description like lastsyncbytes and lastsyncduration and send them as a response of getvolumereplicationinfo request.

internal/csi-addons/rbd/replication.go

go.mod

yati1998 · 2023-06-12T12:18:16Z

cc @ShyamsundarR

internal/csi-addons/rbd/replication.go

Rakshith-R

Have you tested this end to end in a live cluster ?

Can you please provide the new CR yaml with status and logs of each container?

Please test it also with ceph cluster version that does not have this change(if these variables were added recently)?

Rakshith-R · 2023-06-13T04:47:31Z

internal/csi-addons/rbd/replication.go

+	// description = `replaying, {"bytes_per_second":0.0,"bytes_per_snapshot":81920.0,
+	// "last_snapshot_bytes":81920,"last_snapshot_sync_seconds":0,
+	// "local_snapshot_timestamp":1684675261,
+	// "remote_snapshot_timestamp":1684675261,"replay_state":"idle"}`
+	// In case there is no last snapshot bytes returns 0 as the
+	// LastSyncBytes is optional.
+	// In case there is no last snapshot sync seconds, it returns nil as the
+	// LastSyncDuration is optional.


Was lastSync[Byte|duration] added recently to ceph ?
if so, please add ceph tracker/ version from which this was added for reference in comment.

have you tested this call on a ceph cluster version that does not provide these information ?

The above query was not answered.

The localStatus struct which is used for unmarshalling will give default 0 value when the field is not present ?
Check this with unit test cases.

We will need to handle missing values.
The current code workflow does not do that.

Yeah, the 0 value of lastSyncBytes states undefined files, which has been mentioned in csi-addons. and if the value is 0 the lastsyncbytes in volumereplication operator will not be updated.

Similar logic needs to be applied for lastSyncDuration below too?

yes, thats done.

My very first question in this thread is still unanswered.

Still does not handle the case where duration does not exist in the description.

Current workflow will return 0s which is misleading IMO.

That's what is mentioned in the comments, o/nil indicates the value of this field is undefined. We can't return any error as it's optional, and in case duration is not there it will be set to nil, 0 in case of lastsyncbytes. These values are sent by ceph-csi and in csi-addons we are keeping it nil only, which will be updates to the status.

check.
https://github.com/ceph/ceph-csi/pull/3894/files#r1238240556

internal/csi-addons/rbd/replication.go

nixpanic · 2023-06-20T08:11:03Z

internal/csi-addons/rbd/replication.go

 	}
 	type localStatus struct {
-		LocalSnapshotTime int64 `json:"local_snapshot_timestamp"`
+		LocalSnapshotTime     int64 `json:"local_snapshot_timestamp"`
+		LocalSnapshotBytes    int64 `json:"last_snapshot_bytes"`


the naming of these members are not matching the json contents. Note the difference in Local and last_. Please rename them to match the json, it is a little confusing otherwise.

yati1998 · 2023-06-20T10:50:22Z

Have you tested this end to end in a live cluster ?
testing is stuck still de to some image issue, but it shouldn't be any issue, as this follows the same flow as lastsynctime. But Still working on to get working examples and logs.
Meanwhile, feel free to review the PR if any other changes are required.

Rakshith-R · 2023-06-20T11:15:32Z

internal/csi-addons/rbd/replication.go

+	// description = `replaying, {"bytes_per_second":0.0,"bytes_per_snapshot":81920.0,
+	// "last_snapshot_bytes":81920,"last_snapshot_sync_seconds":0,
+	// "local_snapshot_timestamp":1684675261,
+	// "remote_snapshot_timestamp":1684675261,"replay_state":"idle"}`
+	// In case there is no last snapshot bytes returns 0 as the
+	// LastSyncBytes is optional.
+	// In case there is no last snapshot sync seconds, it returns nil as the
+	// LastSyncDuration is optional.


The above query was not answered.

The localStatus struct which is used for unmarshalling will give default 0 value when the field is not present ?
Check this with unit test cases.

We will need to handle missing values.
The current code workflow does not do that.

Madhu-1 · 2023-06-21T08:43:47Z

internal/csi-addons/rbd/replication.go

+// This function gets the local snapshot time, last sync snapshot seconds
+// and last sync bytes from the description of localStatus and convert
+// it into required types.
+func getLastSyncInfo(description string) (*timestamppb.Timestamp, *durationpb.Duration, int64, error) {


create a local struct and return it in response, this avoids returning many values

internal/csi-addons/rbd/replication_test.go

internal/csi-addons/rbd/replication.go

This commit get more information from the description like lastsyncbytes and lastsyncduration and send them as a response of getvolumereplicationinfo request. Signed-off-by: Yati Padia <ypadia@redhat.com>

Rakshith-R · 2023-06-23T07:30:37Z

@Mergifyio rebase

mergify · 2023-06-23T07:30:40Z

rebase

✅ Nothing to do for rebase action

ceph-csi-bot · 2023-06-23T07:37:02Z

/test ci/centos/k8s-e2e-external-storage/1.25

ceph-csi-bot · 2023-06-23T07:37:03Z

/test ci/centos/k8s-e2e-external-storage/1.26

ceph-csi-bot · 2023-06-23T07:37:04Z

/test ci/centos/k8s-e2e-external-storage/1.27

ceph-csi-bot · 2023-06-23T07:37:04Z

/test ci/centos/mini-e2e-helm/k8s-1.25

ceph-csi-bot · 2023-06-23T07:37:05Z

/test ci/centos/mini-e2e-helm/k8s-1.26

ceph-csi-bot · 2023-06-23T07:37:06Z

/test ci/centos/mini-e2e-helm/k8s-1.27

ceph-csi-bot · 2023-06-23T07:37:07Z

/test ci/centos/mini-e2e/k8s-1.25

ceph-csi-bot · 2023-06-23T07:37:08Z

/test ci/centos/mini-e2e/k8s-1.26

ceph-csi-bot · 2023-06-23T07:37:09Z

/test ci/centos/mini-e2e/k8s-1.27

ceph-csi-bot · 2023-06-23T07:37:10Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2023-06-23T07:37:10Z

/test ci/centos/upgrade-tests-rbd

yati1998 requested a review from nixpanic June 9, 2023 10:47

mergify bot added the component/rbd Issues related to RBD label Jun 9, 2023

nixpanic reviewed Jun 9, 2023

View reviewed changes

internal/csi-addons/rbd/replication.go Outdated Show resolved Hide resolved

nixpanic reviewed Jun 9, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

yati1998 force-pushed the volrepinfo branch 2 times, most recently from 05392be to 02eea42 Compare June 12, 2023 12:17

yati1998 marked this pull request as ready for review June 12, 2023 12:17

yati1998 requested a review from nixpanic June 12, 2023 12:17

nixpanic reviewed Jun 12, 2023

View reviewed changes

internal/csi-addons/rbd/replication.go Outdated Show resolved Hide resolved

Rakshith-R requested changes Jun 13, 2023

View reviewed changes

Rakshith-R linked an issue Jun 16, 2023 that may be closed by this pull request

update getVolumeReplicationInfo to reflect more metrics/info #3889

Closed

yati1998 force-pushed the volrepinfo branch 3 times, most recently from d11899d to 5f41c9d Compare June 19, 2023 19:58

nixpanic reviewed Jun 20, 2023

View reviewed changes

yati1998 force-pushed the volrepinfo branch 2 times, most recently from ea09ae9 to 0df93de Compare June 20, 2023 10:48

yati1998 requested review from Rakshith-R and nixpanic June 20, 2023 10:48

Rakshith-R requested changes Jun 20, 2023

View reviewed changes

yati1998 force-pushed the volrepinfo branch 2 times, most recently from 757ca93 to 80f64dc Compare June 21, 2023 08:39

Madhu-1 reviewed Jun 21, 2023

View reviewed changes

yati1998 force-pushed the volrepinfo branch 4 times, most recently from f41a361 to ed6af0f Compare June 21, 2023 19:23

nixpanic reviewed Jun 21, 2023

View reviewed changes

internal/csi-addons/rbd/replication_test.go Show resolved Hide resolved

yati1998 force-pushed the volrepinfo branch 2 times, most recently from 5755a46 to 55bb11f Compare June 23, 2023 04:14

Rakshith-R requested changes Jun 23, 2023

View reviewed changes

internal/csi-addons/rbd/replication.go Outdated Show resolved Hide resolved

Rakshith-R added ci/skip/e2e skip running e2e CI jobs ci/skip/multi-arch-build skip building on multiple architectures labels Jun 23, 2023

yati1998 force-pushed the volrepinfo branch 3 times, most recently from 9ef3f00 to 3c6dd32 Compare June 23, 2023 07:00

rbd: get lastsyncbytes and lastsycduration for volrep

b517e08

This commit get more information from the description like lastsyncbytes and lastsyncduration and send them as a response of getvolumereplicationinfo request. Signed-off-by: Yati Padia <ypadia@redhat.com>

yati1998 force-pushed the volrepinfo branch from 3c6dd32 to b517e08 Compare June 23, 2023 07:11

Rakshith-R approved these changes Jun 23, 2023

View reviewed changes

Rakshith-R requested review from nixpanic and a team June 23, 2023 07:30

nixpanic approved these changes Jun 23, 2023

View reviewed changes

nixpanic added the ok-to-test Label to trigger E2E tests label Jun 23, 2023

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Jun 23, 2023

mergify bot merged commit 2e2e904 into ceph:devel Jun 23, 2023
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rbd: get lastsyncbytes and lastsycduration for volrep #3894

rbd: get lastsyncbytes and lastsycduration for volrep #3894

yati1998 commented Jun 9, 2023

yati1998 commented Jun 12, 2023

Rakshith-R left a comment

Rakshith-R Jun 13, 2023

Rakshith-R Jun 20, 2023

yati1998 Jun 21, 2023

Rakshith-R Jun 21, 2023

yati1998 Jun 21, 2023

Rakshith-R Jun 22, 2023

yati1998 Jun 22, 2023

Rakshith-R Jun 22, 2023

nixpanic Jun 20, 2023

yati1998 commented Jun 20, 2023

Rakshith-R Jun 20, 2023

Madhu-1 Jun 21, 2023

Rakshith-R commented Jun 23, 2023

mergify bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

rbd: get lastsyncbytes and lastsycduration for volrep #3894

rbd: get lastsyncbytes and lastsycduration for volrep #3894

Conversation

yati1998 commented Jun 9, 2023

yati1998 commented Jun 12, 2023

Rakshith-R 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

yati1998 commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rakshith-R commented Jun 23, 2023

mergify bot commented Jun 23, 2023

✅ Nothing to do for rebase action

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023

ceph-csi-bot commented Jun 23, 2023