From 25d1f1a41386aaa29ed9b8986b037a4a7fb07f07 Mon Sep 17 00:00:00 2001 From: Pawan Date: Mon, 18 May 2020 16:30:20 +0530 Subject: [PATCH] feat(zfspv): pvc should be bound only if volume has been created. The controller does not check whether the volume has been created or not and return successful. Which in turn binds the pvc to the pv. The PVC should not bound until corresponding zfs volume has been created. Now controller will check the ZFSVolume CR state to be "Ready" before returning successful. The CSI will retry the CreateVolume request when it will get a error reply and when the ZFS node agent creates the ZFS volume and sets the ZFSVolume CR state to be "Ready", the controller will return success for the CreateVolume Request and then PVC will be bound. Signed-off-by: Pawan --- README.md | 6 +++- changelogs/unreleased/121-pawanpraka1 | 1 + deploy/sample/zfspvcr.yaml | 4 ++- deploy/yamls/zfsvolume-crd.yaml | 16 ++++++++++ deploy/zfs-operator.yaml | 16 ++++++++++ docs/import-existing-volume.md | 2 ++ pkg/apis/openebs.io/zfs/v1alpha1/zfsvolume.go | 13 ++++++++- .../zfs/v1alpha1/zz_generated.deepcopy.go | 17 +++++++++++ pkg/builder/volbuilder/build.go | 6 ++++ pkg/driver/controller.go | 29 +++++++++++++++++-- .../typed/zfs/v1alpha1/fake/fake_zfsvolume.go | 12 ++++++++ .../typed/zfs/v1alpha1/zfsvolume.go | 17 +++++++++++ pkg/zfs/volume.go | 16 ++++++++++ 13 files changed, 150 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/121-pawanpraka1 diff --git a/README.md b/README.md index 197ce7a..c25402b 100644 --- a/README.md +++ b/README.md @@ -314,6 +314,8 @@ Spec: Pool Name: zfspv-pool Recordsize: 4k Volume Type: DATASET +Status: + State: Ready Events: ``` @@ -488,7 +490,7 @@ Name: pvc-e1230d2c-b32a-48f7-8b76-ca335b253dcd Namespace: openebs Labels: kubernetes.io/nodename=zfspv-node1 Annotations: -API Version: openebs.io/v1alpha1 +API Version: zfs.openebs.io/v1alpha1 Kind: ZFSVolume Metadata: Creation Timestamp: 2019-11-22T09:49:29Z @@ -505,6 +507,8 @@ Spec: Pool Name: zfspv-pool Snap Name: pvc-34133838-0d0d-11ea-96e3-42010a800114@snapshot-3cbd5e59-4c6f-4bd6-95ba-7f72c9f12fcd Volume Type: DATASET +Status: + State: Ready Events: Here you can note that this resource has Snapname field which tells that this volume is created from that snapshot. diff --git a/changelogs/unreleased/121-pawanpraka1 b/changelogs/unreleased/121-pawanpraka1 new file mode 100644 index 0000000..343a0b1 --- /dev/null +++ b/changelogs/unreleased/121-pawanpraka1 @@ -0,0 +1 @@ +Fixes an issue where PVC was bound to unusable PV created using incorrect values provided in PVC/Storageclass diff --git a/deploy/sample/zfspvcr.yaml b/deploy/sample/zfspvcr.yaml index 85e47f1..8acae36 100644 --- a/deploy/sample/zfspvcr.yaml +++ b/deploy/sample/zfspvcr.yaml @@ -1,4 +1,4 @@ -apiVersion: openebs.io/v1alpha1 +apiVersion: zfs.openebs.io/v1alpha1 kind: ZFSVolume metadata: name: pvc-34133838-0d0d-11ea-96e3-42010a800114 @@ -16,3 +16,5 @@ spec: recordsize: 8k thinProvision: "off" volumeType: DATASET +status: + state: Ready diff --git a/deploy/yamls/zfsvolume-crd.yaml b/deploy/yamls/zfsvolume-crd.yaml index 2f29b19..0822e60 100644 --- a/deploy/yamls/zfsvolume-crd.yaml +++ b/deploy/yamls/zfsvolume-crd.yaml @@ -32,6 +32,10 @@ spec: description: Size of the volume name: Size type: string + - JSONPath: .status.state + description: Status of the volume + name: Status + type: string - JSONPath: .spec.volblocksize description: volblocksize of volume name: volblocksize @@ -216,6 +220,18 @@ spec: - poolName - volumeType type: object + status: + properties: + state: + description: State specifies the current state of the volume provisioning + request. The state "Pending" means that the volume creation request + has not processed yet. The state "Ready" means that the volume has + been created and it is ready for the use. + enum: + - Pending + - Ready + type: string + type: object required: - spec type: object diff --git a/deploy/zfs-operator.yaml b/deploy/zfs-operator.yaml index b202f3f..a16f531 100644 --- a/deploy/zfs-operator.yaml +++ b/deploy/zfs-operator.yaml @@ -53,6 +53,10 @@ spec: description: Size of the volume name: Size type: string + - JSONPath: .status.state + description: Status of the volume + name: Status + type: string - JSONPath: .spec.volblocksize description: volblocksize of volume name: volblocksize @@ -237,6 +241,18 @@ spec: - poolName - volumeType type: object + status: + properties: + state: + description: State specifies the current state of the volume provisioning + request. The state "Pending" means that the volume creation request + has not processed yet. The state "Ready" means that the volume has + been created and it is ready for the use. + enum: + - Pending + - Ready + type: string + type: object required: - spec type: object diff --git a/docs/import-existing-volume.md b/docs/import-existing-volume.md index 9210a39..34636e2 100644 --- a/docs/import-existing-volume.md +++ b/docs/import-existing-volume.md @@ -151,6 +151,8 @@ spec: ownerNodeID: pawan-3 # should be the nodename where ZPOOL is running poolName: zfspv-pool # poolname where the volume is present volumeType: DATASET # whether it is a DATASET or ZVOL +Status: + State: Ready ``` Modify the parameters :- diff --git a/pkg/apis/openebs.io/zfs/v1alpha1/zfsvolume.go b/pkg/apis/openebs.io/zfs/v1alpha1/zfsvolume.go index bc9a4e3..80df30d 100644 --- a/pkg/apis/openebs.io/zfs/v1alpha1/zfsvolume.go +++ b/pkg/apis/openebs.io/zfs/v1alpha1/zfsvolume.go @@ -30,6 +30,7 @@ import ( // +kubebuilder:printcolumn:name="ZPool",type=string,JSONPath=`.spec.poolName`,description="ZFS Pool where the volume is created" // +kubebuilder:printcolumn:name="Node",type=string,JSONPath=`.spec.ownerNodeID`,description="Node where the volume is created" // +kubebuilder:printcolumn:name="Size",type=string,JSONPath=`.spec.capacity`,description="Size of the volume" +// +kubebuilder:printcolumn:name="Status",type=string,JSONPath=`.status.state`,description="Status of the volume" // +kubebuilder:printcolumn:name="volblocksize",type=string,JSONPath=`.spec.volblocksize`,description="volblocksize of volume" // +kubebuilder:printcolumn:name="recordsize",type=string,JSONPath=`.spec.recordsize`,description="recordsize of created zfs dataset" // +kubebuilder:printcolumn:name="Filesystem",type=string,JSONPath=`.spec.fsType`,description="filesystem created on the volume" @@ -39,7 +40,8 @@ type ZFSVolume struct { metav1.TypeMeta `json:",inline"` metav1.ObjectMeta `json:"metadata,omitempty"` - Spec VolumeInfo `json:"spec"` + Spec VolumeInfo `json:"spec"` + Status VolStatus `json:"status,omitempty"` } // MountInfo contains the volume related info @@ -198,3 +200,12 @@ type VolumeInfo struct { // Default Value: ext4. FsType string `json:"fsType,omitempty"` } + +type VolStatus struct { + // State specifies the current state of the volume provisioning request. + // The state "Pending" means that the volume creation request has not + // processed yet. The state "Ready" means that the volume has been created + // and it is ready for the use. + // +kubebuilder:validation:Enum=Pending;Ready + State string `json:"state,omitempty"` +} diff --git a/pkg/apis/openebs.io/zfs/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/openebs.io/zfs/v1alpha1/zz_generated.deepcopy.go index 6f591a4..afc768c 100644 --- a/pkg/apis/openebs.io/zfs/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/openebs.io/zfs/v1alpha1/zz_generated.deepcopy.go @@ -66,6 +66,22 @@ func (in *SnapStatus) DeepCopy() *SnapStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *VolStatus) DeepCopyInto(out *VolStatus) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new VolStatus. +func (in *VolStatus) DeepCopy() *VolStatus { + if in == nil { + return nil + } + out := new(VolStatus) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *VolumeInfo) DeepCopyInto(out *VolumeInfo) { *out = *in @@ -149,6 +165,7 @@ func (in *ZFSVolume) DeepCopyInto(out *ZFSVolume) { out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) out.Spec = in.Spec + out.Status = in.Status return } diff --git a/pkg/builder/volbuilder/build.go b/pkg/builder/volbuilder/build.go index a25945f..7f10d64 100644 --- a/pkg/builder/volbuilder/build.go +++ b/pkg/builder/volbuilder/build.go @@ -160,6 +160,12 @@ func (b *Builder) WithVolumeType(vtype string) *Builder { return b } +// WithVolumeStatus sets ZFSVolume status +func (b *Builder) WithVolumeStatus(status string) *Builder { + b.volume.Object.Status.State = status + return b +} + // WithFsType sets filesystem for the ZFSVolume func (b *Builder) WithFsType(fstype string) *Builder { b.volume.Object.Spec.FsType = fstype diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index db0f222..fda1be3 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -110,6 +110,7 @@ func CreateZFSVolume(req *csi.CreateVolumeRequest) (string, error) { WithThinProv(tp). WithOwnerNode(selected). WithVolumeType(vtype). + WithVolumeStatus(zfs.ZFSStatusPending). WithFsType(fstype). WithCompression(compression).Build() @@ -161,7 +162,9 @@ func CreateZFSClone(req *csi.CreateVolumeRequest, snapshot string) (string, erro selected := snap.Spec.OwnerNodeID volObj, err := volbuilder.NewBuilder(). - WithName(volName).Build() + WithName(volName). + WithVolumeStatus(zfs.ZFSStatusPending). + Build() volObj.Spec = snap.Spec volObj.Spec.SnapName = snapshot @@ -187,12 +190,22 @@ func (cs *controller) CreateVolume( volName := req.GetName() pool := req.GetParameters()["poolname"] size := req.GetCapacityRange().RequiredBytes + contentSource := req.GetVolumeContentSource() if err = cs.validateVolumeCreateReq(req); err != nil { return nil, err } - contentSource := req.GetVolumeContentSource() + selected, state, err := zfs.GetZFSVolumeState(req.Name) + + if err == nil { + // ZFSVolume CR has been created, check if it is in Ready state + if state == zfs.ZFSStatusReady { + goto CreateVolumeResponse + } + return nil, status.Errorf(codes.Internal, "volume %s creation is Pending", volName) + } + if contentSource != nil && contentSource.GetSnapshot() != nil { snapshotID := contentSource.GetSnapshot().GetSnapshotId() @@ -205,6 +218,18 @@ func (cs *controller) CreateVolume( return nil, status.Error(codes.Internal, err.Error()) } + _, state, err = zfs.GetZFSVolumeState(req.Name) + + if err != nil { + return nil, status.Errorf(codes.Internal, "createvolume: failed to fetch the volume %v", err.Error()) + } + + if state == zfs.ZFSStatusPending { + return nil, status.Errorf(codes.Internal, "volume %s is being created", volName) + } + +CreateVolumeResponse: + sendEventOrIgnore(volName, strconv.FormatInt(int64(size), 10), "zfs-localpv", analytics.VolumeProvision) topology := map[string]string{zfs.ZFSTopologyKey: selected} diff --git a/pkg/generated/clientset/internalclientset/typed/zfs/v1alpha1/fake/fake_zfsvolume.go b/pkg/generated/clientset/internalclientset/typed/zfs/v1alpha1/fake/fake_zfsvolume.go index b74a0a1..f509989 100644 --- a/pkg/generated/clientset/internalclientset/typed/zfs/v1alpha1/fake/fake_zfsvolume.go +++ b/pkg/generated/clientset/internalclientset/typed/zfs/v1alpha1/fake/fake_zfsvolume.go @@ -100,6 +100,18 @@ func (c *FakeZFSVolumes) Update(zFSVolume *v1alpha1.ZFSVolume) (result *v1alpha1 return obj.(*v1alpha1.ZFSVolume), err } +// UpdateStatus was generated because the type contains a Status member. +// Add a +genclient:noStatus comment above the type to avoid generating UpdateStatus(). +func (c *FakeZFSVolumes) UpdateStatus(zFSVolume *v1alpha1.ZFSVolume) (*v1alpha1.ZFSVolume, error) { + obj, err := c.Fake. + Invokes(testing.NewUpdateSubresourceAction(zfsvolumesResource, "status", c.ns, zFSVolume), &v1alpha1.ZFSVolume{}) + + if obj == nil { + return nil, err + } + return obj.(*v1alpha1.ZFSVolume), err +} + // Delete takes name of the zFSVolume and deletes it. Returns an error if one occurs. func (c *FakeZFSVolumes) Delete(name string, options *v1.DeleteOptions) error { _, err := c.Fake. diff --git a/pkg/generated/clientset/internalclientset/typed/zfs/v1alpha1/zfsvolume.go b/pkg/generated/clientset/internalclientset/typed/zfs/v1alpha1/zfsvolume.go index b2b377f..d11e1ac 100644 --- a/pkg/generated/clientset/internalclientset/typed/zfs/v1alpha1/zfsvolume.go +++ b/pkg/generated/clientset/internalclientset/typed/zfs/v1alpha1/zfsvolume.go @@ -39,6 +39,7 @@ type ZFSVolumesGetter interface { type ZFSVolumeInterface interface { Create(*v1alpha1.ZFSVolume) (*v1alpha1.ZFSVolume, error) Update(*v1alpha1.ZFSVolume) (*v1alpha1.ZFSVolume, error) + UpdateStatus(*v1alpha1.ZFSVolume) (*v1alpha1.ZFSVolume, error) Delete(name string, options *v1.DeleteOptions) error DeleteCollection(options *v1.DeleteOptions, listOptions v1.ListOptions) error Get(name string, options v1.GetOptions) (*v1alpha1.ZFSVolume, error) @@ -132,6 +133,22 @@ func (c *zFSVolumes) Update(zFSVolume *v1alpha1.ZFSVolume) (result *v1alpha1.ZFS return } +// UpdateStatus was generated because the type contains a Status member. +// Add a +genclient:noStatus comment above the type to avoid generating UpdateStatus(). + +func (c *zFSVolumes) UpdateStatus(zFSVolume *v1alpha1.ZFSVolume) (result *v1alpha1.ZFSVolume, err error) { + result = &v1alpha1.ZFSVolume{} + err = c.client.Put(). + Namespace(c.ns). + Resource("zfsvolumes"). + Name(zFSVolume.Name). + SubResource("status"). + Body(zFSVolume). + Do(). + Into(result) + return +} + // Delete takes name of the zFSVolume and deletes it. Returns an error if one occurs. func (c *zFSVolumes) Delete(name string, options *v1.DeleteOptions) error { return c.client.Delete(). diff --git a/pkg/zfs/volume.go b/pkg/zfs/volume.go index d7ca48f..c637a48 100644 --- a/pkg/zfs/volume.go +++ b/pkg/zfs/volume.go @@ -157,6 +157,21 @@ func GetZFSVolume(volumeID string) (*apis.ZFSVolume, error) { return vol, err } +// GetZFSVolumeState returns ZFSVolume OwnerNode and State for +// the given volume. CreateVolume request may call it again and +// again until volume is "Ready". +func GetZFSVolumeState(volID string) (string, string, error) { + getOptions := metav1.GetOptions{} + vol, err := volbuilder.NewKubeclient(). + WithNamespace(OpenEBSNamespace).Get(volID, getOptions) + + if err != nil { + return "", "", err + } + + return vol.Spec.OwnerNodeID, vol.Status.State, nil +} + // UpdateZvolInfo updates ZFSVolume CR with node id and finalizer func UpdateZvolInfo(vol *apis.ZFSVolume) error { finalizers := []string{ZFSFinalizer} @@ -168,6 +183,7 @@ func UpdateZvolInfo(vol *apis.ZFSVolume) error { newVol, err := volbuilder.BuildFrom(vol). WithFinalizer(finalizers). + WithVolumeStatus(ZFSStatusReady). WithLabels(labels).Build() if err != nil {