Add lastsyncbytes and lastsyncduration to vol rep status #366

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

apis/replication.storage/v1alpha1/volumereplication_types.go

+	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

config/crd/bases/replication.storage.openshift.io_volumereplications.yaml

+                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

internal/proto/networkfence.pb.go

@@ -1,7 +1,7 @@
 // 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

internal/sidecar/service/volumereplication.go

+		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

internal/proto/replication.proto

+  // 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

controllers/replication.storage/volumereplication_controller.go

+				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

internal/proto/replication.proto

+  // 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

controllers/replication.storage/volumereplication_controller.go

+				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

mergify bot requested review from nixpanic and Rakshith-R 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 nixpanic and Rakshith-R 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
14 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lastsyncbytes and lastsyncduration to vol rep status #366

Add lastsyncbytes and lastsyncduration to vol rep status #366

yati1998 commented Jun 12, 2023

Rakshith-R left a comment

Rakshith-R Jun 12, 2023

nixpanic left a comment

nixpanic Jun 14, 2023

nixpanic commented Jun 16, 2023

yati1998 commented Jun 16, 2023

nixpanic Jun 20, 2023

yati1998 commented Jun 21, 2023

Madhu-1 Jun 21, 2023

yati1998 commented Jun 21, 2023

Rakshith-R Jun 22, 2023

yati1998 Jun 22, 2023

Rakshith-R Jun 22, 2023

Rakshith-R Jun 22, 2023

yati1998 Jun 22, 2023

nixpanic Jun 22, 2023

yati1998 Jun 22, 2023

Madhu-1 Jun 22, 2023

yati1998 commented Jun 22, 2023

Rakshith-R left a comment

Rakshith-R Jun 22, 2023

Rakshith-R left a comment

Madhu-1 Jun 22, 2023

Rakshith-R Jun 22, 2023

Madhu-1 Jun 22, 2023

	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

Add lastsyncbytes and lastsyncduration to vol rep status #366

Add lastsyncbytes and lastsyncduration to vol rep status #366

Conversation

yati1998 commented Jun 12, 2023

Rakshith-R left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixpanic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixpanic commented Jun 16, 2023

yati1998 commented Jun 16, 2023

Choose a reason for hiding this comment

yati1998 commented Jun 21, 2023

Choose a reason for hiding this comment

yati1998 commented Jun 21, 2023

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 22, 2023

Rakshith-R left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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