-
Notifications
You must be signed in to change notification settings - Fork 547
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
journal: store CreationTime for VolumeGroups #4753
journal: store CreationTime for VolumeGroups #4753
Conversation
@@ -118,6 +121,16 @@ func (rbdSnap *rbdSnapshot) toVolume() *rbdVolume { | |||
} | |||
} | |||
|
|||
func (rbdSnap *rbdSnapshot) ToCSI(ctx context.Context) (*csi.Snapshot, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make use of it here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, sure, this function actually was not required to be included, it is part of my VolumeGroupSnapshot work. I'll include it here so that it all gets a little cleaner.
func timestampToString(st *timestamppb.Timestamp) string { | ||
return fmt.Sprintf("seconds:%d nanos:%d", st.GetSeconds(), st.GetNanos()) | ||
func timestampToString(st *time.Time) string { | ||
return fmt.Sprintf("seconds:%d nanos:%d", st.Unix(), st.Nanosecond()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we check on this one, Does Unix is equivalent to GetSeconds()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test ci/centos/k8s-e2e-external-storage/1.28 |
/test ci/centos/mini-e2e-helm/k8s-1.29 |
@iPraveenParihar this adds the creation time to the journal, maybe you need to add that to #4750 as well? |
The PR description contains the unsupported |
@Mergifyio rebase |
Do not use protobuf types when there is no need. Just use the standard time.Time format instead. Signed-off-by: Niels de Vos <[email protected]>
Signed-off-by: Niels de Vos <[email protected]>
After cloning the RBD snapshot, an rbdVolume is returned for the CSI.Snapshot object. In order to use the rbdSnapshot.ToCSI() function, the rbdVolume needs to be converted (back) to an rbdSnaphot. Signed-off-by: Niels de Vos <[email protected]>
✅ Branch has been successfully rebased |
31de303
to
a82b742
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 869aace |
/test ci/centos/upgrade-tests-cephfs |
/test ci/centos/upgrade-tests-rbd |
/test ci/centos/k8s-e2e-external-storage/1.28 |
/test ci/centos/k8s-e2e-external-storage/1.27 |
/test ci/centos/mini-e2e-helm/k8s-1.28 |
/test ci/centos/mini-e2e-helm/k8s-1.27 |
/test ci/centos/mini-e2e/k8s-1.28 |
/test ci/centos/k8s-e2e-external-storage/1.29 |
/test ci/centos/mini-e2e/k8s-1.27 |
/test ci/centos/k8s-e2e-external-storage/1.30 |
/test ci/centos/mini-e2e-helm/k8s-1.29 |
/test ci/centos/mini-e2e-helm/k8s-1.30 |
/test ci/centos/mini-e2e/k8s-1.29 |
/test ci/centos/mini-e2e/k8s-1.30 |
VolumeGroupSnapshots expect to have a CreationTime attribute. This can not be obtained by any (go-ceph) API calls, so it need to be stored in the journal.