Add PVC bound check #32
Conversation
There was a problem hiding this comment.
Hi let me be a bit pedantic :) and ask that you add description for the PR and explanation in the commit message of what you did here. And also you added and changed params that are not related to PVC bound check and they are hidden in the commit and PR, such as the vmiTimeout and storageclass, it should have received separate commits with explanations.
pkg/internal/checkup/checkup.go
Outdated
| @@ -342,14 +350,18 @@ func isDataSourceReady(conditions []cdiv1.DataSourceCondition) bool { | |||
| return false | |||
| } | |||
|
|
|||
| func (c *Checkup) updategoldenImagePvc(pvc *corev1.PersistentVolumeClaim, cs *goldenImagesCheckState) { | |||
| func (c *Checkup) updateGoldenImagePvc(pvc *corev1.PersistentVolumeClaim, cs *goldenImagesCheckState) { | |||
| if pvc == nil { | |||
| return | |||
| } | |||
|
|
|||
| // Prefer golden image with default SC | |||
There was a problem hiding this comment.
Prefer golden image with **configured**/ default SC
pkg/internal/checkup/checkup.go
Outdated
| if pvc == nil { | ||
| return | ||
| } | ||
|
|
||
| // Prefer golden image with default SC | ||
| if c.goldenImagePvc != nil { | ||
| if sc := c.goldenImagePvc.Spec.StorageClassName; sc == nil || *sc == c.results.DefaultStorageClass { | ||
| sc := c.goldenImagePvc.Spec.StorageClassName | ||
| if c.checkupConfig.StorageClass != "" && sc != nil && *sc == c.checkupConfig.StorageClass { |
There was a problem hiding this comment.
nit but, how about:
if sc == nil{
return
}
if *sc == c.results.DefaultStorageClass ||
(c.checkupConfig.StorageClass != "" && *sc == c.checkupConfig.StorageClass) {
pkg/internal/checkup/checkup.go
Outdated
| AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}, | ||
| Resources: corev1.ResourceRequirements{ | ||
| Requests: corev1.ResourceList{ | ||
| corev1.ResourceStorage: resource.MustParse("10Mi"), |
There was a problem hiding this comment.
We already know that there are some provisioners that needs at least 20Mi
pkg/internal/checkup/checkup.go
Outdated
| @@ -390,6 +402,61 @@ func (c *Checkup) checkDefaultStorageClass(scs *storagev1.StorageClassList, errS | |||
| } | |||
| } | |||
|
|
|||
| func (c *Checkup) checkPVCBound(ctx context.Context, errStr *string) error { | |||
| log.Print("checkPVCCreation") | |||
There was a problem hiding this comment.
Maybe even more specific print of checkPVCCreationAndBinding
| } | ||
|
|
||
| if sc := c.checkupConfig.StorageClass; sc != "" { | ||
| log.Printf("PVC storage class %q", sc) |
There was a problem hiding this comment.
Didnt knew this %q option :) thanks
| } | ||
|
|
||
| log.Printf("Waiting for PVC %q bound", pvcName) | ||
| if err := wait.PollImmediateWithContext(ctx, pollInterval, time.Minute, conditionFn); err != nil { |
There was a problem hiding this comment.
So in case the provisioner binding mode is WFFC we are returning error? why is this the desired behavior?
There was a problem hiding this comment.
so we better create DV with cdi.kubevirt.io/storage.bind.immediate.requested: "true" instead?
There was a problem hiding this comment.
Im not sure what we want to test exactly.. so I guess is depends
There was a problem hiding this comment.
all we want to check is that a PVC of the configured/default SC can be successfully bound
There was a problem hiding this comment.
ok then I guess yes we should use the bind annotation
pkg/internal/config/config.go
Outdated
| PodUID: baseConfig.PodUID, | ||
| PodName: baseConfig.PodName, | ||
| PodUID: baseConfig.PodUID, | ||
| StorageClass: baseConfig.Params[StorageClassParamName], |
There was a problem hiding this comment.
I think since storageclass is also optional you shouldnt set it here, but you should do it in the set optional params.. I know that in the current way it will also be empty if it wasnt set but code wise its a bit confusing.
arnongilboa
force-pushed
the
pvc_bound_check
branch
from
a690e70
to
b84873b
Compare
Add a check that we can create a 20MiB PVC of the default storage class and it will be bound by the provisioner. Signed-off-by: Arnon Gilboa <[email protected]>
- spec.param.storageClass - optional storage class to be used instead of the default one. - spec.param.vmiTimeout - optional timeout for VMI operations (the default is 3min). Signed-off-by: Arnon Gilboa <[email protected]>
arnongilboa
force-pushed
the
pvc_bound_check
branch
from
b84873b
to
684479b
Compare
Add a check that we can create a 20MiB PVC of the configured/default storage class and it will be bound by the provisioner.
Also add optional checkup params:
spec.param.storageClass- optional storage class to be used instead of the default one (needed for the new check).spec.param.vmiTimeout- optional timeout for VMI operations (the default is 3min).