Add LastSyncTime to volume replication status by yati1998 · Pull Request #232 · csi-addons/kubernetes-csi-addons

yati1998 · 2022-09-07T07:41:07Z

This PR adds lastsynctime to volumereplication status and updates the reconcile logic to get the LastSynctime

nixpanic · 2022-09-07T07:48:11Z

 	// remove the prefix keys in volume replication class parameters
 	parameters := filterPrefixedParameters(replicationParameterPrefix, vrcObj.Spec.Parameters)
+	schedulingTime := parameters["schedulingInterval"]
+	scheduleTime, _ := time.ParseDuration(schedulingTime)


describe the schedulingInterval parameter somewhere

Rakshith-R · 2022-09-08T06:45:14Z

+message GetVolumeReplicationInfoResponse{
+  // Holds the last sync time.
+  // This field is REQUIRED.
+  int64 last_sync_time = 1;


https://github.com/csi-addons/spec/pull/44/files#diff-36da0b8129c7cf1c1695bf0ae42ea29185aa75ee81a8a34b7ad1a5d1eedd989f

Use timestamp here too.

This is still not implemented,
with this change, we can remove some code for conversion in other functions too.

Rakshith-R · 2022-09-08T06:47:30Z

+	schedulingTime := parameters["schedulingInterval"]
+	scheduleTime, _ := time.ParseDuration(schedulingTime)


This is optional and maybe empty ?

do we need a default schedule time ?

cc @Madhu-1

Yes it can be empty and it's optional

Madhu-1 · 2022-09-08T08:51:24Z

 	}

 	instance.Status.LastCompletionTime = getCurrentTime()
+	var last_sync_time int64


Don't use _ in variable names

Madhu-1 · 2022-09-08T08:52:11Z

 	instance.Status.LastCompletionTime = getCurrentTime()
+	var last_sync_time int64
+	var requeueForInfo bool
+	if instance.Spec.ReplicationState == replicationv1alpha1.Primary {


Is it required only for primary state?

Yes, we need to resync only when it is in primary state.

The primary end has the status, that it reads from the secondary, and so we use that to update the lastSyncTime.

In cases where we demoted a Primary and are waiting for both Secondaries to be in sync, the timestamp is still useful. Tells us how far back the sync is, before either image can be gracefully promoted.

The demoted snapshot sync time is not critical at present, it would also just show that the lastSyncTime is in the past as compared to the demoted snapshot, and ideally once the demoted snapshot is synced the timestamps would be the same. So not sure if it is useful as such.

Needs more thought on how to use that information, hence sticking to when image is primary at one end is useful.

Madhu-1 · 2022-09-08T08:53:23Z

 	// remove the prefix keys in volume replication class parameters
 	parameters := filterPrefixedParameters(replicationParameterPrefix, vrcObj.Spec.Parameters)
+	schedulingTime := parameters["schedulingInterval"]
+	scheduleTime, _ := time.ParseDuration(schedulingTime)


Whhy we are sending parameters?

We want to update the last sync time after every interval and that is, scheduling interval and hence, it's required.

from VolumeReplication CR schedulingInterval is optional

okay, thanks for notifying, will check on that and update the PR.

Madhu-1 · 2022-09-08T08:54:34Z

 	return &metav1NowTime
 }
+
+func getTime(time time.Time) *metav1.Time {


No need for a function for this one.

Rakshith-R · 2022-09-09T06:37:47Z

 	parameters := filterPrefixedParameters(replicationParameterPrefix, vrcObj.Spec.Parameters)
+	schedulingTime := parameters["schedulingInterval"]
+	if schedulingTime == "" {
+		schedulingTime = "1m"


define it as a constant ?
and write elaborate how it will be used.

This is still not done.

If the schedule is not set for this VR, then the pool level schedule (if available) should be used for the reconcile requeue. If that is not available then we can either NOT requeue or requeue for a duration till the last sync time is not empty (as post that there would be no further syncs).

There is a possibility that a pool level schedule is added later, once we make the above not to requeue decision, but that would either require a restart of the plugin (so that all VRs are reconciled), or reconciling based on added pool level schedules, or waiting for the periodic reconcile (24h) at which time this will be picked up.

We possibly want to track the above for the future, for now if there is no schedule we can default to a something (1m/5m), as the current DR stack comes with a schedule and should not break the functionality.

Rakshith-R · 2022-09-09T06:39:00Z

+message GetVolumeReplicationInfoResponse{
+  // Holds the last sync time.
+  // This field is REQUIRED.
+  int64 last_sync_time = 1;


This is still not implemented,
with this change, we can remove some code for conversion in other functions too.

Rakshith-R · 2022-09-09T11:14:01Z

+		return 0, resp.Error
+	}
+
+	infoResponse, ok := resp.Response.(*proto.GetVolumeReplicationInfoResponse)
+	if !ok {
+		err := fmt.Errorf("received response of unexpected type")
+		vr.logger.Error(err, "unable to parse response")
+
+		return 0, err


Suggested change

return 0, resp.Error

}

infoResponse, ok := resp.Response.(*proto.GetVolumeReplicationInfoResponse)

if !ok {

err := fmt.Errorf("received response of unexpected type")

vr.logger.Error(err, "unable to parse response")

return 0, err

return nil, resp.Error

}

infoResponse, ok := resp.Response.(*proto.GetVolumeReplicationInfoResponse)

if !ok {

err := fmt.Errorf("received response of unexpected type")

vr.logger.Error(err, "unable to parse response")

return nil, err

Rakshith-R · 2022-09-09T11:15:11Z

+		metav1Time := metav1.NewTime(lastSyncTime1)
+		instance.Status.LastSyncTime = &metav1Time
+		requeueForInfo = true
+


unnecessary newline

Rakshith-R · 2022-09-09T11:17:40Z

+	scheduleTime, err := time.ParseDuration(schedulingTime)
+	if err != nil {
+		logger.Error(err, "failed to parse time: %v", schedulingTime)
+	}


Take a look at these lines again.

Rakshith-R · 2022-09-09T11:19:12Z

+	schedulingTime := parameters["schedulingInterval"]
+	if schedulingTime == "" {
+		schedulingTime = "1m"
+	}
+	scheduleTime, err := time.ParseDuration(schedulingTime)


use rawScheduleTime instead schedulingTime,
it is confusing.

Rakshith-R · 2022-09-09T11:19:28Z

 	parameters := filterPrefixedParameters(replicationParameterPrefix, vrcObj.Spec.Parameters)
+	schedulingTime := parameters["schedulingInterval"]
+	if schedulingTime == "" {
+		schedulingTime = "1m"


This is still not done.

nixpanic · 2022-09-09T11:47:03Z

+
+	var requeueForInfo bool
+	if instance.Spec.ReplicationState == replicationv1alpha1.Primary {
+		lastSyncTime, err := r.getVolumeReplicationInfo(vr)


maybe call this info instead of lastSyncTime

nixpanic · 2022-09-09T11:48:53Z

 }

+// getVolumeReplicationInfo gets volume replication info.
+func (r *VolumeReplicationReconciler) getVolumeReplicationInfo(vr *volumeReplicationInstance) (*timestamppb.Timestamp, error) {


this does not return an info kind of object? return something like what volumeReplication.GetInfo() does, or rename the function to return a LastSyncTimestamp?

yeah for now it just returns last sync time, but this name is given just with the thought of future implementation, we may like to return other informations along with the last sync time, hence don't want it to be specific.

The current state is confusing, so you will have to decide what this function does. It can be renamed once it gets extended, no need to prepare function names for things in the future. Leave a comment with the possibility if you think it is useful.

nixpanic · 2022-09-09T11:50:09Z

+		if err != nil {
+			logger.Error(err, "Failed to get volume replication info")
+		}
+		lastSyncTime1 := lastSyncTime.AsTime()


if this can not fail, just place it in metav1Time := metav1.NewTime(lastSyncTime.AsTime()).

metav1Time sounds more like a package name, similar to metav1. This is just a variable, so why not call it time?

ShyamsundarR · 2022-09-09T12:22:06Z

 	instance.Status.LastCompletionTime = getCurrentTime()
+
+	var requeueForInfo bool
+	if instance.Spec.ReplicationState == replicationv1alpha1.Primary {


@Madhu-1 If an image is in the split-brain state, is there a TS that is usable?

AFAIK no during split-brain no sync will happen this is not useful.

In split-brain, I do not assume one of the images is marked Primary? If that is the case, the lastSyncTime should be the time of the last successful sync, maybe.

since we are updating it in every interval, I guess if we reach split brain case, it would already have that, cc @ShyamsundarR

The question really is what does RBD report, and then what should the proto report. In the case of split-brain we should report not-synced time (which IMHO is the "0" timestamp), or if available the last time it was synced...

I agree, on a sync failure (like split-brain), no LastSyncTime should be reported, but ideally an error. Either the automatic rescheduling should stop, or be done with an exponential backoff.

When the image is in replaying state with RBD we get the additional description, if stopped or split-brain the additional data is not available, and we will hence default to the "0" time stamp. We should be fine with that.

ShyamsundarR · 2022-09-09T12:27:52Z

+
+	var requeueForInfo bool
+	if instance.Spec.ReplicationState == replicationv1alpha1.Primary {
+		lastSyncTime, err := r.getVolumeReplicationInfo(vr)


Just a heads up!

The last sync time can be "0" or empty, i.e it was never synced even once, we need to determine how that value would be reflected. One option is to set it to the lower bound of UTC epoch (I am a bit confused on what Timestamp holds, UTC epoch (since 1970), or since 0001-01-01T00:00:00Z)

Rakshith-R · 2022-09-12T06:10:58Z

+	rawScheduleTime := parameters["schedulingInterval"]
+	if rawScheduleTime == "" {
+		rawScheduleTime = "1m"
+	}
+	scheduleTime, err := time.ParseDuration(rawScheduleTime)
+	if err != nil {
+		logger.Error(err, "failed to parse time: %v", rawScheduleTime)
+		return ctrl.Result{}, err
+	}


Suggested change

rawScheduleTime := parameters["schedulingInterval"]

if rawScheduleTime == "" {

rawScheduleTime = "1m"

}

scheduleTime, err := time.ParseDuration(rawScheduleTime)

if err != nil {

logger.Error(err, "failed to parse time: %v", rawScheduleTime)

return ctrl.Result{}, err

}

const defaultScheduleTime = time.Minute

func getScheduleTime(parameters map[string]string) (time.Duration) {

rawScheduleTime := parameters["schedulingInterval"]

if rawScheduleTime == "" {

return defaultScheduleTime

}

scheduleTime, err := time.ParseDuration(rawScheduleTime)

if err != nil {

logger.Error(err, "failed to parse time: %v", rawScheduleTime)

return defaultScheduleTime

}

return scheduleTime

}

Create a constant and a function like above and use it .

@yati1998 ^

Madhu-1 · 2022-09-12T14:37:40Z


+	scheduleTime := getScheduleTime(parameters, logger)
+	if requeueForInfo {
+		logger.Info("volume is primary, requeuing to get volume replication info")


logging is not much helpful. Remove it on add the next queue interval time. and also move line 388 inside the if condition

Madhu-1 · 2022-09-12T14:40:48Z

 }

+// getVolumeReplicationInfo gets volume replication info.
+func (r *VolumeReplicationReconciler) getLastSyncTime(vr *volumeReplicationInstance) (*timestamppb.Timestamp, error) {


IMO we should not be doing this. Currently, we have only one parameter in the GetInfo this looks fine, If we add more parameters we need to write a new function for it. My suggestion is to return a complete response infoResponse and use it in caller

Madhu-1 · 2022-09-12T14:41:08Z

 	ResyncVolume(volumeID, replicationID string, force bool, secretName, secretNamespace string, parameters map[string]string) (*proto.
 		ResyncVolumeResponse, error)
+	// GetVolumeReplicationInfo RPC call to get volume replication info.
+	GetVolumeReplicationInfo(volumeID, replicationID string, secretName, secretNamespace string) (*proto.GetVolumeReplicationInfoResponse, error)


Suggested change

GetVolumeReplicationInfo(volumeID, replicationID string, secretName, secretNamespace string) (*proto.GetVolumeReplicationInfoResponse, error)

GetVolumeReplicationInfo(volumeID, replicationID , secretName, secretNamespace string) (*proto.GetVolumeReplicationInfoResponse, error)```

Madhu-1 · 2022-09-12T14:41:50Z

+func (rc *replicationClient) GetVolumeReplicationInfo(volumeID, replicationID string,
+	secretName, secretNamespace string) (*proto.GetVolumeReplicationInfoResponse, error) {


Suggested change

func (rc *replicationClient) GetVolumeReplicationInfo(volumeID, replicationID string,

secretName, secretNamespace string) (*proto.GetVolumeReplicationInfoResponse, error) {

func (rc *replicationClient) GetVolumeReplicationInfo(volumeID, replicationID,

secretName, secretNamespace string) (*proto.GetVolumeReplicationInfoResponse, error) {

Madhu-1 · 2022-09-12T14:43:19Z

+	}
+
+	lastsynctime := resp.GetLastSyncTime()
+	if lastsynctime == nil {


when this can be nil?

in case there is no local snapshot timestamp and the csi driver return nil.

nixpanic · 2022-09-12T13:41:00Z

 	pvcDataSource          = "PersistentVolumeClaim"
 	volumeReplicationClass = "VolumeReplicationClass"
 	volumeReplication      = "VolumeReplication"
+	defaultScheduleTime    = time.Minute


Every minute sounds very often as a default, how about hourly? Consider that there can be many volumes, and the LastSyncTime may not be critical information. In environments with hundred (to name a random number) volume, users should not run into high resource consumption because of the constant reconciling.

For environments that require more strict updates on syncing, the parameter can be used to change it.

nixpanic · 2022-09-12T16:31:10Z

 	instance.Status.LastCompletionTime = getCurrentTime()
+
+	var requeueForInfo bool
+	if instance.Spec.ReplicationState == replicationv1alpha1.Primary {


I agree, on a sync failure (like split-brain), no LastSyncTime should be reported, but ideally an error. Either the automatic rescheduling should stop, or be done with an exponential backoff.

nixpanic · 2022-09-12T16:33:46Z

+		}
+		lastSyncTime := info.GetLastSyncTime()
+
+		lastsynctime := metav1.NewTime(lastSyncTime.AsTime())


very confusing naming, there is lastSyncTime and lastsynctime. If you really need both, make sure to call them differently (maybe ts or timestamp and lastSyncTime or just t).

This commit adds lastsynctime to volume replicationstatus and updates the crds. Signed-off-by: yati1998 <ypadia@redhat.com>

This commit creates internal grpc to get volume replication information. Signed-off-by: yati1998 <ypadia@redhat.com>

this commit updates the vendor to get latest spec. Signed-off-by: yati1998 <ypadia@redhat.com>

yati1998 · 2022-09-13T07:47:12Z

@nixpanic @ShyamsundarR @Rakshith-R @Madhu-1 done with all the changes here as well.

Rakshith-R · 2022-09-13T09:08:12Z

+	if instance.Spec.ReplicationState == replicationv1alpha1.Secondary {
+		instance.Status.LastSyncTime = nil
+	}


This should go at line 382,
otherwise there will be no update to status.

nixpanic

Just the documentation for the new parameter needs to be added. Otherwise it looks good to me.

This commit adds reconcile logic to update the last sync time. Signed-off-by: yati1998 <ypadia@redhat.com>

Madhu-1 · 2022-09-13T09:36:22Z

If the RPC is not implemented, this will be a breaking change. we need to address it later

Rakshith-R

@yati1998 ,can you open an issue to track handling NotImplemented Error ?
We can set lastSyncTime to nil if getvolrepInfo returns this error.
We'll need to fix this before another release of csi-addons operator.

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

yati1998 changed the title ~~Volrep last~~ Add LastSyncTime to volume replication status Sep 7, 2022

mergify Bot added the vendor Pull requests that update vendored dependencies label Sep 7, 2022

nixpanic reviewed Sep 7, 2022

View reviewed changes

yati1998 force-pushed the volrep-last branch from 8b345f5 to 13f6350 Compare September 7, 2022 09:07

Rakshith-R reviewed Sep 8, 2022

View reviewed changes

Madhu-1 reviewed Sep 8, 2022

View reviewed changes

Comment thread controllers/replication.storage/volumereplication_controller.go

Madhu-1 reviewed Sep 8, 2022

View reviewed changes

Madhu-1 requested a review from ShyamsundarR September 8, 2022 11:00

yati1998 force-pushed the volrep-last branch from 13f6350 to 23037e5 Compare September 9, 2022 03:57

Rakshith-R requested changes Sep 9, 2022

View reviewed changes

yati1998 force-pushed the volrep-last branch 2 times, most recently from 6478936 to 817bea4 Compare September 9, 2022 10:35

yati1998 requested review from Rakshith-R and nixpanic September 9, 2022 10:37

Rakshith-R requested changes Sep 9, 2022

View reviewed changes

nixpanic reviewed Sep 9, 2022

View reviewed changes

ShyamsundarR reviewed Sep 9, 2022

View reviewed changes

yati1998 force-pushed the volrep-last branch 2 times, most recently from e9ad9e1 to bccc4ef Compare September 12, 2022 05:05

yati1998 marked this pull request as ready for review September 12, 2022 05:23

mergify Bot requested a review from Yuggupta27 September 12, 2022 05:24

yati1998 force-pushed the volrep-last branch from bccc4ef to 7e294fc Compare September 12, 2022 05:25

Rakshith-R requested changes Sep 12, 2022

View reviewed changes

yati1998 force-pushed the volrep-last branch 2 times, most recently from ca61302 to e4bc1d9 Compare September 12, 2022 08:39

yati1998 requested a review from Rakshith-R September 12, 2022 12:51

Madhu-1 reviewed Sep 12, 2022

View reviewed changes

yati1998 force-pushed the volrep-last branch from d93d7e8 to 7418c6f Compare September 12, 2022 15:14

yati1998 requested a review from Madhu-1 September 12, 2022 15:15

nixpanic requested changes Sep 12, 2022

View reviewed changes

ShyamsundarR mentioned this pull request Sep 12, 2022

Add lastSyncTime status update to DRPC RamenDR/ramen#539

Merged

ShyamsundarR reviewed Sep 12, 2022

View reviewed changes

Comment thread controllers/replication.storage/volumereplication_controller.go

yati1998 added 3 commits September 13, 2022 12:35

crds: add last sync to vol rep status

e9f5f41

This commit adds lastsynctime to volume replicationstatus and updates the crds. Signed-off-by: yati1998 <ypadia@redhat.com>

proto: add rpc to get vol rep info

0c1026c

This commit creates internal grpc to get volume replication information. Signed-off-by: yati1998 <ypadia@redhat.com>

vendor: update vendor to include latest spec

dc04589

this commit updates the vendor to get latest spec. Signed-off-by: yati1998 <ypadia@redhat.com>

yati1998 force-pushed the volrep-last branch 2 times, most recently from 42af0cf to f0c7f15 Compare September 13, 2022 07:44

yati1998 requested review from Rakshith-R, ShyamsundarR and nixpanic September 13, 2022 07:47

Rakshith-R requested changes Sep 13, 2022

View reviewed changes

yati1998 force-pushed the volrep-last branch from f0c7f15 to 31c190b Compare September 13, 2022 09:14

yati1998 requested a review from Rakshith-R September 13, 2022 09:14

nixpanic requested changes Sep 13, 2022

View reviewed changes

replication: add reconcile logic to update last_sync

07aff99

This commit adds reconcile logic to update the last sync time. Signed-off-by: yati1998 <ypadia@redhat.com>

yati1998 force-pushed the volrep-last branch from 31c190b to 07aff99 Compare September 13, 2022 09:32

yati1998 requested a review from nixpanic September 13, 2022 09:32

nixpanic approved these changes Sep 13, 2022

View reviewed changes

Rakshith-R approved these changes Sep 13, 2022

View reviewed changes

mergify Bot merged commit 17e671e into csi-addons:main Sep 13, 2022

yati1998 mentioned this pull request Sep 13, 2022

Handle non implemented error for getVolumeReplicationInfo RPC #236

Closed

Madhu-1 mentioned this pull request Oct 17, 2022

Unwanted RPC calls due to LastSyncTime feature #250

Closed

Madhu-1 mentioned this pull request Nov 7, 2022

Issue to track handling NotImplemented Error for GetVolumeReplicationInfo #264

Closed

Madhu-1 pushed a commit to Madhu-1/kubernetes-csi-addons that referenced this pull request Nov 27, 2024

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

f1f9821

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

		schedulingTime := parameters["schedulingInterval"]
		scheduleTime, _ := time.ParseDuration(schedulingTime)

	GetVolumeReplicationInfo(volumeID, replicationID string, secretName, secretNamespace string) (*proto.GetVolumeReplicationInfoResponse, error)
	GetVolumeReplicationInfo(volumeID, replicationID , secretName, secretNamespace string) (*proto.GetVolumeReplicationInfoResponse, error)```

		func (rc *replicationClient) GetVolumeReplicationInfo(volumeID, replicationID string,
		secretName, secretNamespace string) (*proto.GetVolumeReplicationInfoResponse, error) {

Conversation

yati1998 commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

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!

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!

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 Sep 7, 2022 •

edited

Loading