Skip to content
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

Add lastsyncbytes and lastsyncduration to vol rep status #366

Merged
merged 4 commits into from
Jun 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions apis/replication.storage/v1alpha1/volumereplication_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,12 @@ type VolumeReplicationStatus struct {
Conditions []metav1.Condition `json:"conditions,omitempty"`
// observedGeneration is the last generation change the operator has dealt with
// +optional
ObservedGeneration int64 `json:"observedGeneration,omitempty"`
LastStartTime *metav1.Time `json:"lastStartTime,omitempty"`
LastCompletionTime *metav1.Time `json:"lastCompletionTime,omitempty"`
LastSyncTime *metav1.Time `json:"lastSyncTime,omitempty"`
ObservedGeneration int64 `json:"observedGeneration,omitempty"`
LastStartTime *metav1.Time `json:"lastStartTime,omitempty"`
LastCompletionTime *metav1.Time `json:"lastCompletionTime,omitempty"`
LastSyncTime *metav1.Time `json:"lastSyncTime,omitempty"`
LastSyncBytes *int64 `json:"lastSyncBytes,omitempty"`
LastSyncDuration *metav1.Duration `json:"lastSyncDuration,omitempty"`
}

// +kubebuilder:object:root=true
Expand Down
10 changes: 10 additions & 0 deletions apis/replication.storage/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,11 @@ spec:
lastStartTime:
format: date-time
type: string
lastSyncBytes:
format: int64
type: integer
lastSyncDuration:
type: string
lastSyncTime:
format: date-time
type: string
Expand Down
11 changes: 11 additions & 0 deletions controllers/replication.storage/volumereplication_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,17 @@ func (r *VolumeReplicationReconciler) Reconcile(ctx context.Context, req ctrl.Re
if ts != nil {
lastSyncTime := metav1.NewTime(ts.AsTime())
instance.Status.LastSyncTime = &lastSyncTime
instance.Status.LastSyncDuration = nil
instance.Status.LastSyncBytes = nil
Comment on lines +383 to +384
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, just read the code, LGTM

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
}
Comment on lines +385 to +393
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
requeueForInfo = true
} else if !util.IsUnimplementedError(err) {
Expand Down
5 changes: 5 additions & 0 deletions deploy/controller/crds.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -937,6 +937,11 @@ spec:
lastStartTime:
format: date-time
type: string
lastSyncBytes:
format: int64
type: integer
lastSyncDuration:
type: string
lastSyncTime:
format: date-time
type: string
Expand Down
5 changes: 5 additions & 0 deletions deploy/controller/install-all-in-one.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -944,6 +944,11 @@ spec:
lastStartTime:
format: date-time
type: string
lastSyncBytes:
format: int64
type: integer
lastSyncDuration:
type: string
lastSyncTime:
format: date-time
type: string
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ go 1.20

require (
github.com/container-storage-interface/spec v1.8.0
github.com/csi-addons/spec v0.2.0
github.com/csi-addons/spec v0.2.1-0.20230606140122-d20966d2e444
github.com/go-logr/logr v1.2.4
github.com/kubernetes-csi/csi-lib-utils v0.13.0
github.com/onsi/ginkgo v1.16.5
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -333,8 +333,8 @@ github.com/creack/pty v1.1.7/go.mod h1:lj5s0c3V2DBrqTV7llrYr5NG6My20zk30Fl46Y7Do
github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
github.com/creack/pty v1.1.11 h1:07n33Z8lZxZ2qwegKbObQohDhXDQxiMMz1NOUGYlesw=
github.com/creack/pty v1.1.11/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
github.com/csi-addons/spec v0.2.0 h1:Ews7bxpN9P6nFxl1XvMg87cR1wLROdH1FzSfLfb4VfI=
github.com/csi-addons/spec v0.2.0/go.mod h1:Mwq4iLiUV4s+K1bszcWU6aMsR5KPsbIYzzszJ6+56vI=
github.com/csi-addons/spec v0.2.1-0.20230606140122-d20966d2e444 h1:hWVCrZWVHctpWt6cQxV1I6dW3wpBDMg3Vrvu9uAuUxw=
github.com/csi-addons/spec v0.2.1-0.20230606140122-d20966d2e444/go.mod h1:Mwq4iLiUV4s+K1bszcWU6aMsR5KPsbIYzzszJ6+56vI=
github.com/cyphar/filepath-securejoin v0.2.2/go.mod h1:FpkQEhXnPnOthhzymB7CGsFk2G9VLXONKD9G7QGMM+4=
github.com/cyphar/filepath-securejoin v0.2.3 h1:YX6ebbZCZP7VkM3scTTokDgBL2TY741X51MTk3ycuNI=
github.com/cyphar/filepath-securejoin v0.2.3/go.mod h1:aPGpWjXOXUn2NCNjFvBE6aRxGGx79pTxQpKOJNYHHl4=
Expand Down
115 changes: 115 additions & 0 deletions internal/proto/google/protobuf/duration.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
// Protocol Buffers - Google's data interchange format
// Copyright 2008 Google Inc. All rights reserved.
// https://developers.google.com/protocol-buffers/
//
// Redistribution and use in source and binary forms, with or without
// modification, are permitted provided that the following conditions are
// met:
//
// * Redistributions of source code must retain the above copyright
// notice, this list of conditions and the following disclaimer.
// * Redistributions in binary form must reproduce the above
// copyright notice, this list of conditions and the following disclaimer
// in the documentation and/or other materials provided with the
// distribution.
// * Neither the name of Google Inc. nor the names of its
// contributors may be used to endorse or promote products derived from
// this software without specific prior written permission.
//
// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

syntax = "proto3";

package google.protobuf;

option cc_enable_arenas = true;
option go_package = "google.golang.org/protobuf/types/known/durationpb";
option java_package = "com.google.protobuf";
option java_outer_classname = "DurationProto";
option java_multiple_files = true;
option objc_class_prefix = "GPB";
option csharp_namespace = "Google.Protobuf.WellKnownTypes";

// A Duration represents a signed, fixed-length span of time represented
// as a count of seconds and fractions of seconds at nanosecond
// resolution. It is independent of any calendar and concepts like "day"
// or "month". It is related to Timestamp in that the difference between
// two Timestamp values is a Duration and it can be added or subtracted
// from a Timestamp. Range is approximately +-10,000 years.
//
// # Examples
//
// Example 1: Compute Duration from two Timestamps in pseudo code.
//
// Timestamp start = ...;
// Timestamp end = ...;
// Duration duration = ...;
//
// duration.seconds = end.seconds - start.seconds;
// duration.nanos = end.nanos - start.nanos;
//
// if (duration.seconds < 0 && duration.nanos > 0) {
// duration.seconds += 1;
// duration.nanos -= 1000000000;
// } else if (duration.seconds > 0 && duration.nanos < 0) {
// duration.seconds -= 1;
// duration.nanos += 1000000000;
// }
//
// Example 2: Compute Timestamp from Timestamp + Duration in pseudo code.
//
// Timestamp start = ...;
// Duration duration = ...;
// Timestamp end = ...;
//
// end.seconds = start.seconds + duration.seconds;
// end.nanos = start.nanos + duration.nanos;
//
// if (end.nanos < 0) {
// end.seconds -= 1;
// end.nanos += 1000000000;
// } else if (end.nanos >= 1000000000) {
// end.seconds += 1;
// end.nanos -= 1000000000;
// }
//
// Example 3: Compute Duration from datetime.timedelta in Python.
//
// td = datetime.timedelta(days=3, minutes=10)
// duration = Duration()
// duration.FromTimedelta(td)
//
// # JSON Mapping
//
// In JSON format, the Duration type is encoded as a string rather than an
// object, where the string ends in the suffix "s" (indicating seconds) and
// is preceded by the number of seconds, with nanoseconds expressed as
// fractional seconds. For example, 3 seconds with 0 nanoseconds should be
// encoded in JSON format as "3s", while 3 seconds and 1 nanosecond should
// be expressed in JSON format as "3.000000001s", and 3 seconds and 1
// microsecond should be expressed in JSON format as "3.000001s".
//
message Duration {
// Signed seconds of the span of time. Must be from -315,576,000,000
// to +315,576,000,000 inclusive. Note: these bounds are computed from:
// 60 sec/min * 60 min/hr * 24 hr/day * 365.25 days/year * 10000 years
int64 seconds = 1;

// Signed fractions of a second at nanosecond resolution of the span
// of time. Durations less than one second are represented with a 0
// `seconds` field and a positive or negative `nanos` field. For durations
// of one second or more, a non-zero value for the `nanos` field must be
// of the same sign as the `seconds` field. Must be from -999,999,999
// to +999,999,999 inclusive.
int32 nanos = 2;
}
Loading