Add lastsyncbytes and lastsyncduration to vol rep status by yati1998 · Pull Request #366 · csi-addons/kubernetes-csi-addons

yati1998 · 2023-06-12T11:46:42Z

This PR adds lastsyncbytes and lastsyncduration to volume replication status and generates updated crds.
Also it adds the logic to update the status in the volume replication object.

Rakshith-R

Can you please add controller and sidecar changes too (in same pr) and (keep it in draft) / open it for review when completed ?

Rakshith-R · 2023-06-12T11:49:52Z

+	LastStartTime      *metav1.Time     `json:"lastStartTime,omitempty"`
+	LastCompletionTime *metav1.Time     `json:"lastCompletionTime,omitempty"`
+	LastSyncTime       *metav1.Time     `json:"lastSyncTime,omitempty"`
+	LastSyncBytes      int64            `json:"lastSyncBytes,omitempty"`


Suggested change

LastSyncBytes int64 `json:"lastSyncBytes,omitempty"`

LastSyncBytes *int64 `json:"lastSyncBytes,omitempty"`

This should be a pointer since it is a optional parameter,

nixpanic

the duration.proto file should probably be moved to the commit where it is imported.

nixpanic · 2023-06-14T08:29:42Z

+                format: int64
+                type: integer
+              lastSyncDuration:
+                format: date-time


is this really date-time? is there not a duration format, there is no date in this, I think?

nixpanic · 2023-06-16T07:54:36Z

@yati1998 make sure to install protoc v3.19.6, or do not commit the changes in the version of the .proto files.

There is also a crd generation issue, it seems that the duration can not have a format: date-time.

yati1998 · 2023-06-16T09:36:13Z

@yati1998 make sure to install protoc v3.19.6, or do not commit the changes in the version of the .proto files.

There is also a crd generation issue, it seems that the duration can not have a format: date-time.

yeah sure, I am testing with changes, once it is successful, I will update all.

nixpanic · 2023-06-20T08:20:15Z

 // Code generated by protoc-gen-go. DO NOT EDIT.
 // versions:
 // 	protoc-gen-go v1.30.0
-// 	protoc        v3.19.6


You are using an old version to re-generate these files, please use protoc v3.19.6

yati1998 · 2023-06-21T11:34:27Z

Working on to revert the protobuf version issue, meanwhile please do review the PR.
cc @nixpanic @Rakshith-R @Madhu-1

Madhu-1 · 2023-06-21T12:40:38Z

+		LastSyncDuration: lastsyncduration,
+		LastSyncBytes:    lastsyncbytes,


instead of new variables you can directly use resp.GetLastSyncDuration() and resp.GetLastSyncBytes() here

Pull request has been modified.

yati1998 · 2023-06-21T16:12:13Z

@nixpanic and @Madhu-1 can you please revisit it again.

Rakshith-R · 2023-06-22T06:04:29Z

+  // This field is OPTIONAL.
+  .google.protobuf.Duration last_sync_duration = 2;
+  // This field is OPTIONAL.
+  // A value of 0 is equal to an unspecified field value.
+  // The value of this field MUST NOT be negative.
+  int64 last_sync_bytes = 3;


Make sure to match the comments with csi-addons spec or close to it.

This has to be edited in spec then, cause it's required to clear it here, will send another PR to update spec comments

I meant to update it here.

Rakshith-R · 2023-06-22T06:04:32Z

+				td := info.GetLastSyncDuration()
+				if td != nil {
+					lastSyncDuration := metav1.Duration{Duration: td.AsDuration()}
+					instance.Status.LastSyncDuration = &lastSyncDuration
+				}
+				tb := info.GetLastSyncBytes()
+				if tb != 0 {
+					instance.Status.LastSyncBytes = &tb
+				}


Fields should be explicitly set to nil to indicate missing value.
otherwise, they may inherit values that were previously set.

I am not sure about this, considering the corner case where lastsynctime retains its value and on requeue other values will be nil. Will verify this once and update if it's fine.

On line 91 (wow, what a horrible long function this is?!) instance is fetched, so it will have a .Status.LastSyncBytes from the previous reconcile. If there were 0 bytes in the last sync, the information would not be correct in the object. So yes, setting it explicitly to an initial value is probably a good thing.

@Rakshith-R @nixpanic IMO we should not set this to nil explicitly #366 (comment)

This commit adds lastsyncbytes and lastsyncduration to volume replication status and generates updated crds. Signed-off-by: yati1998 <ypadia@redhat.com>

yati1998 · 2023-06-22T08:35:33Z

Addressed the changes, @Rakshith-R @nixpanic

Rakshith-R

a small nit

Rakshith-R · 2023-06-22T09:04:37Z

+  // last_sync_duration states the time taken to sync
+  // to execute the last sync operation.


Suggested change

// last_sync_duration states the time taken to sync

// to execute the last sync operation.

// last_sync_duration represents the time taken

// to execute the last sync operation.

This commit updates the getvolumereplicationinfo rpc to include lastsyncbytes and lastsyncduration. Signed-off-by: yati1998 <ypadia@redhat.com>

This commit updates the vendor directory to include lastest spec. Signed-off-by: yati1998 <ypadia@redhat.com>

This commit updates the reconcile logic to get more info and update the volrep status. Signed-off-by: yati1998 <ypadia@redhat.com>

Rakshith-R

Thanks !

Madhu-1 · 2023-06-22T09:28:01Z

+				instance.Status.LastSyncDuration = nil
+				instance.Status.LastSyncBytes = nil


i dont think you should make this nil. You should preserve the last data because you already have lastSyncTime, Duration, Byte to give to complete sync information.

i dont think you should make this nil. You should preserve the last data because you already have lastSyncTime, Duration, Byte to give to complete sync information.

@Madhu-1 ,
We'll reach this point only when csi driver gives us a valid new lastSyncTime.

It'll be incorrect to keep previous duration and bytes data with new lastSyncTime according to me.

okay, just read the code, LGTM

Syncing latest changes from upstream main for kubernetes-csi-addons

mergify Bot requested review from Rakshith-R and nixpanic June 12, 2023 11:47

mergify Bot added the vendor Pull requests that update vendored dependencies label Jun 12, 2023

Rakshith-R reviewed Jun 12, 2023

View reviewed changes

nixpanic reviewed Jun 14, 2023

View reviewed changes

yati1998 force-pushed the volrepstatus branch from 4b1fea3 to 8dba67a Compare June 19, 2023 18:45

nixpanic reviewed Jun 20, 2023

View reviewed changes

yati1998 force-pushed the volrepstatus branch 4 times, most recently from 37b2062 to 5f45cec Compare June 21, 2023 11:22

yati1998 requested review from Rakshith-R and nixpanic June 21, 2023 11:33

nixpanic force-pushed the volrepstatus branch from 5f45cec to 05b4a79 Compare June 21, 2023 11:40

nixpanic previously approved these changes Jun 21, 2023

View reviewed changes

Madhu-1 reviewed Jun 21, 2023

View reviewed changes

yati1998 requested review from Madhu-1 and nixpanic June 21, 2023 15:50

yati1998 force-pushed the volrepstatus branch from 05b4a79 to 8ccb906 Compare June 21, 2023 15:54

Rakshith-R requested changes Jun 22, 2023

View reviewed changes

crds: add lastsyncduration and lastsyncbytes to volrep

51ddfd2

This commit adds lastsyncbytes and lastsyncduration to volume replication status and generates updated crds. Signed-off-by: yati1998 <ypadia@redhat.com>

yati1998 force-pushed the volrepstatus branch from 8ccb906 to 9711c1f Compare June 22, 2023 07:39

yati1998 requested a review from Rakshith-R June 22, 2023 07:39

yati1998 force-pushed the volrepstatus branch from 9711c1f to ae8904f Compare June 22, 2023 08:28

Rakshith-R reviewed Jun 22, 2023

View reviewed changes

yati1998 added 3 commits June 22, 2023 14:45

proto: update rpc to include other volrep info

974614c

This commit updates the getvolumereplicationinfo rpc to include lastsyncbytes and lastsyncduration. Signed-off-by: yati1998 <ypadia@redhat.com>

vendor: updates the vendor directory

dbd9fb1

This commit updates the vendor directory to include lastest spec. Signed-off-by: yati1998 <ypadia@redhat.com>

replication: update reconcile logic

929d92b

This commit updates the reconcile logic to get more info and update the volrep status. Signed-off-by: yati1998 <ypadia@redhat.com>

yati1998 force-pushed the volrepstatus branch from ae8904f to 929d92b Compare June 22, 2023 09:15

yati1998 requested a review from Rakshith-R June 22, 2023 09:15

Rakshith-R approved these changes Jun 22, 2023

View reviewed changes

Madhu-1 reviewed Jun 22, 2023

View reviewed changes

Madhu-1 approved these changes Jun 22, 2023

View reviewed changes

mergify Bot merged commit ee9403c into csi-addons:main Jun 22, 2023

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

replication: add lastsyncbytes and lastsyncduration to volumereplication object #361

Closed

Nikhil-Ladha pushed a commit to Nikhil-Ladha/kubernetes-csi-addons that referenced this pull request Jan 6, 2026

Merge pull request csi-addons#366 from red-hat-storage/sync_us--main

219e9fe

Syncing latest changes from upstream main for kubernetes-csi-addons

	LastSyncBytes int64 `json:"lastSyncBytes,omitempty"`
	LastSyncBytes *int64 `json:"lastSyncBytes,omitempty"`

		LastSyncDuration: lastsyncduration,
		LastSyncBytes: lastsyncbytes,

		// last_sync_duration states the time taken to sync
		// to execute the last sync operation.

		instance.Status.LastSyncDuration = nil
		instance.Status.LastSyncBytes = nil

Conversation

yati1998 commented Jun 12, 2023

Uh oh!

Rakshith-R left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nixpanic commented Jun 16, 2023

Uh oh!

yati1998 commented Jun 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yati1998 commented Jun 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yati1998 commented Jun 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yati1998 commented Jun 22, 2023

Uh oh!

Rakshith-R left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rakshith-R left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants