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

Update VolumeSnapshot CRD version to v1beta #139

Merged
merged 1 commit into from
Oct 24, 2019
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
16 changes: 0 additions & 16 deletions Gopkg.lock

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

4 changes: 0 additions & 4 deletions Gopkg.toml
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,6 @@ required = [
name = "k8s.io/code-generator"
version = "kubernetes-1.14.0"

[[constraint]]
name = "k8s.io/apiextensions-apiserver"
version = "kubernetes-1.14.0"

[[constraint]]
name = "github.com/kubernetes-csi/csi-lib-utils"
version = ">=v0.6.1"
Expand Down
100 changes: 0 additions & 100 deletions cmd/csi-snapshotter/create_crd.go

This file was deleted.

21 changes: 3 additions & 18 deletions cmd/csi-snapshotter/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ import (
clientset "github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned"
snapshotscheme "github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned/scheme"
informers "github.com/kubernetes-csi/external-snapshotter/pkg/client/informers/externalversions"
apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
coreinformers "k8s.io/client-go/informers"
)

Expand Down Expand Up @@ -119,20 +118,6 @@ func main() {
factory := informers.NewSharedInformerFactory(snapClient, *resyncPeriod)
coreFactory := coreinformers.NewSharedInformerFactory(kubeClient, *resyncPeriod)

// Create CRD resource
aeclientset, err := apiextensionsclient.NewForConfig(config)
if err != nil {
klog.Error(err.Error())
os.Exit(1)
}

// initialize CRD resource if it does not exist
err = CreateCRD(aeclientset)
if err != nil {
klog.Error(err.Error())
os.Exit(1)
}

// Add Snapshot types to the defualt Kubernetes so events can be logged for them
snapshotscheme.AddToScheme(scheme.Scheme)

Expand Down Expand Up @@ -185,9 +170,9 @@ func main() {
snapClient,
kubeClient,
*snapshotterName,
factory.Snapshot().V1alpha1().VolumeSnapshots(),
factory.Snapshot().V1alpha1().VolumeSnapshotContents(),
factory.Snapshot().V1alpha1().VolumeSnapshotClasses(),
factory.Snapshot().V1beta1().VolumeSnapshots(),
factory.Snapshot().V1beta1().VolumeSnapshotContents(),
factory.Snapshot().V1beta1().VolumeSnapshotClasses(),
coreFactory.Core().V1().PersistentVolumeClaims(),
*createSnapshotContentRetryCount,
*createSnapshotContentInterval,
Expand Down
72 changes: 72 additions & 0 deletions config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@

---
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: (devel)
api-approved.kubernetes.io: "https://github.com/kubernetes-csi/external-snapshotter/pull/139"
creationTimestamp: null
name: volumesnapshotclasses.snapshot.storage.k8s.io
spec:
group: snapshot.storage.k8s.io
names:
kind: VolumeSnapshotClass
listKind: VolumeSnapshotClassList
plural: volumesnapshotclasses
singular: volumesnapshotclass
scope: Cluster
Copy link

Choose a reason for hiding this comment

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

I would strongly recommend setting preserveUnknownFields: false and making sure these CRDs are structural (creating it and checking status doesn't show a non-structural condition). That will ensure they are well-positioned to work with the v1 CRD API

Copy link

Choose a reason for hiding this comment

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

Also, since this is an official k8s.io-suffixed API, add an api-approved.kubernetes.io annotation containing a URL to the PR/Issue where the API is approved (see https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/20190612-crd-group-protection.md#summary for details). That can be this PR if this is the canonical home for this API.

Copy link
Contributor

Choose a reason for hiding this comment

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

updated thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

there are pending issues like this to be resolved if kubebuilder is used. As the schemas in this PR are generated using controller-gen, I manually removed metadata field from validation to make crd valid for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there an automatic way to validate that this schema is structural with v1beta? Or should we test it out as v1 to verify that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct me if I am wrong @msau42, you were asking to check whether the schema is valid with v1beta?
If that's your question, I have conducted manual tests on 3 different k8s versions, 1.14/1.15/1.16, all seems to be fine. v1 CRD API is stable in k8s 1.16. I have not tested using v1 though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean to test that this v1beta1 schema is compatible with v1. Can we temporarily convert this to v1 and try it on a 1.16 cluster?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've double verified the schemas could be seamlessly converted to v1 in a 1.16 cluster with following output.
k get crd volumesnapshotclasses.snapshot.storage.k8s.io -o yaml
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
api-approved.kubernetes.io: #139
controller-gen.kubebuilder.io/version: (devel)
creationTimestamp: "2019-10-24T20:37:44Z"
generation: 1
name: volumesnapshotclasses.snapshot.storage.k8s.io
resourceVersion: "1238"
selfLink: /apis/apiextensions.k8s.io/v1/customresourcedefinitions/volumesnapshotclasses.snapshot.storage.k8s.io
uid: 524ff8f2-b783-46d4-953f-60a6fd3c32af
spec:
conversion:
strategy: None
group: snapshot.storage.k8s.io
names:
kind: VolumeSnapshotClass
listKind: VolumeSnapshotClassList
plural: volumesnapshotclasses
singular: volumesnapshotclass
scope: Cluster
versions:

  • name: v1beta1
    schema:
    openAPIV3Schema:
    description: VolumeSnapshotClass specifies parameters that a underlying storage
    system uses when creating a volume snapshot. A specific VolumeSnapshotClass
    is used by specifying its name in a VolumeSnapshot object. VolumeSnapshotClasses

Copy link
Contributor

Choose a reason for hiding this comment

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

status:
status:
acceptedNames:
kind: VolumeSnapshotContent
listKind: VolumeSnapshotContentList
plural: volumesnapshotcontents
singular: volumesnapshotcontent
conditions:

  • lastTransitionTime: "2019-10-24T20:50:02Z"
    message: no conflicts found
    reason: NoConflicts
    status: "True"
    type: NamesAccepted
  • lastTransitionTime: "2019-10-24T20:50:02Z"
    message: the initial names have been accepted
    reason: InitialNamesAccepted
    status: "True"
    type: Established
  • lastTransitionTime: "2019-10-24T20:50:02Z"
    message: approved in Update VolumeSnapshot CRD version to v1beta #139
    reason: ApprovedAnnotation
    status: "True"
    type: KubernetesAPIApprovalPolicyConformant
    storedVersions:
  • v1beta1

preserveUnknownFields: false
validation:
msau42 marked this conversation as resolved.
Show resolved Hide resolved
openAPIV3Schema:
description: VolumeSnapshotClass specifies parameters that a underlying storage
system uses when creating a volume snapshot. A specific VolumeSnapshotClass
is used by specifying its name in a VolumeSnapshot object. VolumeSnapshotClasses
are non-namespaced
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation
of an object. Servers should convert recognized schemas to the latest
internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#resources'
type: string
deletionPolicy:
description: deletionPolicy determines whether a VolumeSnapshotContent created
through the VolumeSnapshotClass should be deleted when its bound VolumeSnapshot
is deleted. Supported values are "Retain" and "Delete". "Retain" means
that the VolumeSnapshotContent and its physical snapshot on underlying
storage system are kept. "Delete" means that the VolumeSnapshotContent
and its physical snapshot on underlying storage system are deleted. Required.
enum:
- Delete
- Retain
type: string
driver:
description: driver is the name of the storage driver that handles this
VolumeSnapshotClass. Required.
type: string
msau42 marked this conversation as resolved.
Show resolved Hide resolved
kind:
description: 'Kind is a string value representing the REST resource this
object represents. Servers may infer this from the endpoint the client
submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#types-kinds'
type: string
parameters:
additionalProperties:
type: string
description: parameters is a key-value map with storage driver specific
parameters for creating snapshots. These values are opaque to Kubernetes.
type: object
msau42 marked this conversation as resolved.
Show resolved Hide resolved
required:
- deletionPolicy
- driver
type: object
version: v1beta1
versions:
- name: v1beta1
served: true
storage: true
status:
acceptedNames:
kind: ""
plural: ""
conditions: []
storedVersions: []
Loading