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.

Sign up for GitHub

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

Update VolumeGroupSnapshot to v1beta1 #1150

Merged
merged 5 commits into from
Dec 2, 2024
Merged

Update VolumeGroupSnapshot to v1beta1 #1150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
57560c1
Update VolumeGroupSnapshot to v1beta1
leonardoce Jul 1, 2024
58bae80
Remove v1alpha1 API
leonardoce Nov 21, 2024
341ac4c
Remove unused structures
leonardoce Nov 23, 2024
42b9deb
Improve VolumeGroupSnapshotStatus comments
leonardoce Nov 26, 2024
2c63f95
Improve VolumeSnapshotPairList comments
leonardoce Nov 26, 2024
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
2 changes: 1 addition & 1 deletion .../apis/volumegroupsnapshot/v1alpha1/doc.go → ...t/apis/volumegroupsnapshot/v1beta1/doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,4 @@ limitations under the License.
// +k8s:deepcopy-gen=package
// +groupName=groupsnapshot.storage.k8s.io

package v1alpha1
package v1beta1
4 changes: 2 additions & 2 deletions .../volumegroupsnapshot/v1alpha1/register.go → ...s/volumegroupsnapshot/v1beta1/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package v1alpha1
package v1beta1

import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -28,7 +28,7 @@ var (
// AddToScheme adds to scheme
AddToScheme = SchemeBuilder.AddToScheme
// SchemeGroupVersion is the group version used to register these objects.
SchemeGroupVersion = schema.GroupVersion{Group: GroupName, Version: "v1alpha1"}
SchemeGroupVersion = schema.GroupVersion{Group: GroupName, Version: "v1beta1"}
)

func Resource(resource string) schema.GroupResource {
Expand Down
25 changes: 5 additions & 20 deletions ...pis/volumegroupsnapshot/v1alpha1/types.go → ...apis/volumegroupsnapshot/v1beta1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/
// +kubebuilder:object:generate=true
package v1alpha1
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is VolumeGroupSnapshot.Status.CreationTime --> metav1.Time and why is VolumeGroupSnapshotContentStatus.CreationTime --> int64 ?

I know that change was not introduced in this PR, but since we are moving this to beta, we have to be careful about all the decisions we make.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same is happening with independent VolumeSnapshots:

external-snapshotter/client/apis/volumesnapshot/v1/types.go

Line 158 in 07425bd

CreationTime *metav1.Time `json:"creationTime,omitempty" protobuf:"bytes,2,opt,name=creationTime"`

vs:

external-snapshotter/client/apis/volumesnapshot/v1/types.go

Line 408 in 07425bd

CreationTime *int64 `json:"creationTime,omitempty" protobuf:"varint,2,opt,name=creationTime"`

The content object looks more directly related to the GRPC CSI call.

Copy link
Contributor

@gnufied gnufied Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is how it is displayed in CLI (via kubectl ) then:

  creationTime: "2024-11-22T22:14:35Z"
  creationTime: 1732313675689050917

@xing-yang can you confirm if this intentional? This doesn't seem right.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can always convert int64 to metav1.Time so as this is nicely handled by k8s tooling. I am not sure if this has to be int64...

Copy link
Collaborator

Choose a reason for hiding this comment

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

In CSI spec, the timestamp is .google.protobuf.Timestamp which represents seconds of UTC time since Unix epoch 1970-01-01T00:00:00Z. So in VolumeSnapshotContent and VolumeGroupSnapshotContent, it is a direct mapping to what is returned from the CSI driver.
VolumeSnapshot and VolumeGroupSnapshot are in user namespace, we try to show them nicely in the readable format of metav1.Time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@leonardoce can you please update the comments with the highlighted text?

type VolumeGroupSnapshotStatus struct {
..........
// CreationTime is the timestamp when the point-in-time group snapshot is taken
// by the underlying storage system.
// If not specified, it may indicate that the creation time of the group snapshot
// is unknown.
// The format of this field is a Unix nanoseconds time encoded as an int64.
// On Unix, the command date +%s%N returns the current time in nanoseconds
// since 1970-01-01 00:00:00 UTC.
// This field is updated based on the CreationTime field in VolumeGroupSnapshotContentStatus
// +optional
CreationTime *metav1.Time json:"creationTime,omitempty" protobuf:"bytes,2,opt,name=creationTime"

type VolumeGroupSnapshotContentStatus struct {
..........
// CreationTime is the timestamp when the point-in-time group snapshot is taken
// by the underlying storage system.
// If not specified, it indicates the creation time is unknown.
// If not specified, it means the readiness of a group snapshot is unknown.
// The format of this field is a Unix nanoseconds time encoded as an int64.
// On Unix, the command date +%s%N returns the current time in nanoseconds
// since 1970-01-01 00:00:00 UTC.
// This field is the source for the CreationTime field in VolumeGroupSnapshotStatus
// +optional
CreationTime *int64 json:"creationTime,omitempty" protobuf:"varint,2,opt,name=creationTime"

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 improved the comments @xing-yang.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we concluded last week to use metav1.Time for CreationTime?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Opened an issue: #1234
cc @leonardoce

Copy link
Collaborator

Choose a reason for hiding this comment

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

@msau42 PR is submitted here: #1235

package v1beta1

leonardoce marked this conversation as resolved.
Show resolved Hide resolved
import (
core_v1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -90,6 +90,7 @@ type VolumeGroupSnapshotStatus struct {
// The format of this field is a Unix nanoseconds time encoded as an int64.
// On Unix, the command date +%s%N returns the current time in nanoseconds
// since 1970-01-01 00:00:00 UTC.
// This field is updated based on the CreationTime field in VolumeGroupSnapshotContentStatus
// +optional
CreationTime *metav1.Time `json:"creationTime,omitempty" protobuf:"bytes,2,opt,name=creationTime"`

Expand All @@ -110,15 +111,6 @@ type VolumeGroupSnapshotStatus struct {
Error *snapshotv1.VolumeSnapshotError `json:"error,omitempty" protobuf:"bytes,4,opt,name=error,casttype=VolumeSnapshotError"`
}

// PVCVolumeSnapshotPair defines a pair of a PVC reference and a Volume Snapshot Reference
type PVCVolumeSnapshotPair struct {
// PersistentVolumeClaimRef is a reference to the PVC this pair is referring to
PersistentVolumeClaimRef core_v1.LocalObjectReference `json:"persistentVolumeClaimRef,omitempty" protobuf:"bytes,1,opt,name=persistentVolumeClaimRef"`

// VolumeSnapshotRef is a reference to the VolumeSnapshot this pair is referring to
VolumeSnapshotRef core_v1.LocalObjectReference `json:"volumeSnapshotRef,omitempty" protobuf:"bytes,2,opt,name=volumeSnapshotRef"`
}

//+genclient
//+k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

Expand Down Expand Up @@ -333,6 +325,7 @@ type VolumeGroupSnapshotContentStatus struct {
// The format of this field is a Unix nanoseconds time encoded as an int64.
// On Unix, the command date +%s%N returns the current time in nanoseconds
// since 1970-01-01 00:00:00 UTC.
// This field is the source for the CreationTime field in VolumeGroupSnapshotStatus
// +optional
CreationTime *int64 `json:"creationTime,omitempty" protobuf:"varint,2,opt,name=creationTime"`

Expand All @@ -354,16 +347,6 @@ type VolumeGroupSnapshotContentStatus struct {
VolumeSnapshotHandlePairList []VolumeSnapshotHandlePair `json:"volumeSnapshotHandlePairList,omitempty" protobuf:"bytes,6,opt,name=volumeSnapshotHandlePairList"`
}

// PVVolumeSnapshotContentPair represent a pair of PV names and
// VolumeSnapshotContent names
type PVVolumeSnapshotContentPair struct {
// PersistentVolumeRef is a reference to the persistent volume resource
PersistentVolumeRef core_v1.LocalObjectReference `json:"persistentVolumeRef,omitempty" protobuf:"bytes,1,opt,name=persistentVolumeRef"`

// VolumeSnapshotContentRef is a reference to the volume snapshot content resource
VolumeSnapshotContentRef core_v1.LocalObjectReference `json:"volumeSnapshotContentRef,omitempty" protobuf:"bytes,2,opt,name=volumeSnapshotContentRef"`
}

// VolumeGroupSnapshotContentSource represents the CSI source of a group snapshot.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not visible in PR - Can VolumeHandle and SnapshotHandle inside VolumeSnapshotHandlePair be empty? Or they both have to be specified. I didn't see these fields marked as required and neither see any validation kinda logic preventing saving of these fields without any values. Correct me if I missed something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

VolumeSnapshotHandlePair itself is not required, but if a VolumeSnapshotHandlePair is specified, then both VolumeHandle and SnapshotHandle should be required. Otherwise, it is not very useful.
We can add CEL for them.

Copy link
Contributor

@gnufied gnufied Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We can add CEL for them.

Yeah we should IMO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@leonardoce Can you make changes to make VolumeHandle and SnapshotHandle required in VolumeSnapshotHandlePair?

Copy link
Contributor Author

@leonardoce leonardoce Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sure! I think it's better to file a different PR for that and keep this as a giant substitution. If we use this PR to review the API, it will become really difficult to rebase.

Is that fine with you @gnufied ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The main reason I wanted all API changes in same PR is, so as we don't have to do follow up API fixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gnufied I see both fields are already marked as required (meaning they are not optional) in the CRD.
See:

external-snapshotter/client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshotcontents.yaml

Lines 290 to 312 in b79aef9

volumeSnapshotHandlePairList:
description: |-
VolumeSnapshotHandlePairList is a list of CSI "volume_id" and "snapshot_id"
pair returned by the CSI driver to identify snapshots and their source volumes
on the storage system.
items:
description: VolumeSnapshotHandlePair defines a pair of a source
volume handle and a snapshot handle
properties:
snapshotHandle:
description: |-
SnapshotHandle is a unique id returned by the CSI driver to identify a volume
snapshot on the storage system
type: string
volumeHandle:
description: |-
VolumeHandle is a unique id returned by the CSI driver to identify a volume
on the storage system
type: string
required:
- snapshotHandle
- volumeHandle
type: object

Do you still need me to add some CEL rules for them? Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

That should be sufficient. The API server will reject it if VolumeHandle or SnapshotHandle is not set in VolumeSnapshotHandlePair.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add "// Required." as comments for both VolumeHandle and SnapshotHandle in types.go?

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 did it and rebased the PR too.

// Exactly one of its members must be set.
// Members in VolumeGroupSnapshotContentSource are immutable.
Expand Down Expand Up @@ -408,9 +391,11 @@ type GroupSnapshotHandles struct {
type VolumeSnapshotHandlePair struct {
// VolumeHandle is a unique id returned by the CSI driver to identify a volume
// on the storage system
// Required.
VolumeHandle string `json:"volumeHandle" protobuf:"bytes,1,opt,name=volumeHandle"`

// SnapshotHandle is a unique id returned by the CSI driver to identify a volume
// snapshot on the storage system
// Required.
SnapshotHandle string `json:"snapshotHandle" protobuf:"bytes,2,opt,name=snapshotHandle"`
}
38 changes: 1 addition & 37 deletions ...napshot/v1alpha1/zz_generated.deepcopy.go → ...snapshot/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

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

18 changes: 9 additions & 9 deletions client/clientset/versioned/clientset.go
View file
Open in desktop

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

10 changes: 5 additions & 5 deletions client/clientset/versioned/fake/clientset_generated.go
View file
Open in desktop

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

4 changes: 2 additions & 2 deletions client/clientset/versioned/fake/register.go
View file
Open in desktop

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

4 changes: 2 additions & 2 deletions client/clientset/versioned/scheme/register.go
View file
Open in desktop

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

2 changes: 1 addition & 1 deletion ...typed/volumegroupsnapshot/v1alpha1/doc.go → .../typed/volumegroupsnapshot/v1beta1/doc.go
View file
Open in desktop

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

0 .../volumegroupsnapshot/v1alpha1/fake/doc.go → ...d/volumegroupsnapshot/v1beta1/fake/doc.go
View file
Open in desktop
File renamed without changes.
50 changes: 25 additions & 25 deletions ...v1alpha1/fake/fake_volumegroupsnapshot.go → .../v1beta1/fake/fake_volumegroupsnapshot.go
View file
Open in desktop

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

Loading
Loading