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

Jump to bottom

Address possible nils in dv controller, log CSIDrivers in tests #2253

Merged

kubevirt-bot merged 2 commits into kubevirt:main from akalenyu:debug-ceph-csidriver

Oct 19, 2022

Collaborator

akalenyu commented May 1, 2022 •

edited

Loading

Signed-off-by: Alex Kalenyuk akalenyu@redhat.com

What this PR does / why we need it:

Handle some storageClassName nils
We'd expect the CSIDriver object to be there, otherwise ceph install might be struggling

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

NONE

kubevirt-bot added release-note-none do-not-merge/work-in-progress dco-signoff: yes size/M labels

kubevirt-bot requested review from awels and mhenriks

May 1, 2022 11:04

akalenyu marked this pull request as draft

May 1, 2022 11:05

Collaborator Author

akalenyu commented May 1, 2022

/test pull-containerized-data-importer-e2e-ceph

3 similar comments

Collaborator Author

akalenyu commented May 1, 2022

/test pull-containerized-data-importer-e2e-ceph

Collaborator Author

akalenyu commented May 2, 2022

/test pull-containerized-data-importer-e2e-ceph

Collaborator Author

akalenyu commented May 2, 2022

/test pull-containerized-data-importer-e2e-ceph

akalenyu force-pushed the debug-ceph-csidriver branch from 0b2c44f to d182ecb Compare

May 2, 2022 14:30

Collaborator Author

akalenyu commented May 2, 2022

/test pull-containerized-data-importer-e2e-ceph

akalenyu force-pushed the debug-ceph-csidriver branch from d5901e9 to d385e1c Compare

May 2, 2022 17:33

kubevirt-bot added size/L and removed size/M labels

akalenyu force-pushed the debug-ceph-csidriver branch 2 times, most recently from 3b08e10 to e637a2b Compare

May 2, 2022 17:55

Collaborator Author

akalenyu commented May 2, 2022

/test all

2 similar comments

Collaborator Author

akalenyu commented May 2, 2022

/test all

Collaborator Author

akalenyu commented May 3, 2022

/test all

Collaborator Author

akalenyu commented May 3, 2022

/retest

akalenyu force-pushed the debug-ceph-csidriver branch from e637a2b to 5388c83 Compare

May 4, 2022 08:39

akalenyu changed the title ~~[WIP] Debug ceph csidriver not being there~~ Address possible nils in dv controller, log CSIDrivers in tests

akalenyu marked this pull request as ready for review

May 4, 2022 09:07

kubevirt-bot removed the do-not-merge/work-in-progress label

kubevirt-bot requested a review from maya-r

May 4, 2022 09:07

brybacki reviewed

View reviewed changes

pkg/controller/datavolume-controller.go Outdated

+              			if err != nil {
+              				return reconcile.Result{}, err
+              			}
+              			if storageClass == nil {

Contributor

brybacki May 4, 2022

So previously, in the case of host-assisted clone, it would go forward and create a PVC -> line 530 (even without a default storageclass, and without storage class defined), now it will return here with error.

Is this the way we want to handle this case?

Collaborator Author

akalenyu May 4, 2022

Yeah definitely want to avoid logical changes. I think I took this too far, trying to reuse the storage class that gets passed around.

I'll have another go with more direct approach

brybacki reviewed

View reviewed changes

pkg/controller/datavolume-controller.go Outdated

               			if selectedCloneStrategy == SmartClone {
-              				snapshotClassName, _ := r.getSnapshotClassForSmartClone(datavolume, pvcSpec)
+              				snapshotClassName, _ := r.getSnapshotClassForSmartClone(datavolume, storageClass)
               				return r.reconcileSmartClonePvc(log, datavolume, pvcSpec, transferName, snapshotClassName)
               			}
               			if selectedCloneStrategy == CsiClone {

Contributor

brybacki May 4, 2022 •

edited

Loading

In theory we should not be able to get SmartClone if there is no storageClass/defaultStoratgeClass, so it should not get here.
Anyway, there is an error we ignore here. Maybe we should not?

Contributor

brybacki May 4, 2022

Also, maybe we should look into the selectCloneStrategy and revise what is returned in case there is no StorageClass.

Collaborator Author

akalenyu May 4, 2022

Yeah I think we shouldn't have ignored the error.
I minimized the scope of this commit to handle the nils without altering logic too much

Collaborator Author

akalenyu Jun 15, 2022

@brybacki Should be strictly about avoiding nils now. let me know what you think

akalenyu force-pushed the debug-ceph-csidriver branch from 5388c83 to a7a48ee Compare

May 4, 2022 11:14

akalenyu force-pushed the debug-ceph-csidriver branch from a7a48ee to 0c1d2fd Compare

May 4, 2022 11:18

akalenyu mentioned this pull request

Increment versions of k8s used in CI #2278

Merged

arnongilboa reviewed

View reviewed changes

pkg/controller/datavolume-controller.go Outdated

+              						return reconcile.Result{}, err
+              					}
+              					if storageClass == nil {
+              						log.Info("Could not find a default storage class in the cluster")

Collaborator

arnongilboa May 18, 2022

It might be better to remove the word "default" and add the SC name

Collaborator Author

akalenyu May 18, 2022

yep, changed it to adjust the event message according to if we have the sc name or not

arnongilboa reviewed

View reviewed changes

pkg/controller/datavolume-controller_test.go Outdated

Comment on lines 1542 to 1543

		Expect(err).To(Not(HaveOccurred()))
		Expect(result).To(Not(BeNil()))

Collaborator

arnongilboa May 18, 2022

Use ToNot() instead

arnongilboa reviewed

View reviewed changes

pkg/controller/datavolume-controller_test.go Outdated

+              			reconciler.extClientSet = extfake.NewSimpleClientset(createVolumeSnapshotContentCrd(), createVolumeSnapshotClassCrd(), createVolumeSnapshotCrd())
+              			By("Reconcile")
+              			result, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}})

Collaborator

arnongilboa May 18, 2022

Use dv.Name and dv.Namespace here

arnongilboa reviewed

View reviewed changes

pkg/monitoring/BUILD.bazel Outdated

Comment on lines 1 to 9

+              load("@io_bazel_rules_go//go:def.bzl", "go_library")
+              go_library(
+                  name = "go_default_library",
+                  srcs = ["metrics.go"],
+                  importpath = "kubevirt.io/containerized-data-importer/pkg/monitoring",
+                  visibility = ["//visibility:public"],
+                  deps = ["//pkg/common:go_default_library"],
+              )

Collaborator

arnongilboa May 18, 2022

Are you sure this is needed?

Collaborator Author

akalenyu May 18, 2022

yeah but it already got added here #2226
one of the make targets re-adds this, not sure which one

Collaborator Author

akalenyu May 18, 2022 •

edited

Loading

Rebased and this is now gone from my PR

akalenyu force-pushed the debug-ceph-csidriver branch from 0c1d2fd to 80fcaeb Compare

May 18, 2022 11:15

Contributor

brybacki commented May 27, 2022

/test pull-cdi-apidocs

Member

awels commented Jul 8, 2022

Does this still need to get in or did we solve it some other way?

Collaborator Author

akalenyu commented Jul 10, 2022

Does this still need to get in or did we solve it some other way?

This NPE is still possible afaik

akalenyu force-pushed the debug-ceph-csidriver branch from 80fcaeb to 5611cf8 Compare

July 10, 2022 11:49

akalenyu force-pushed the debug-ceph-csidriver branch from 5611cf8 to edcc165 Compare

July 31, 2022 14:04

Member

awels commented Aug 3, 2022

/retest

kubevirt-bot added the needs-rebase label

akalenyu force-pushed the debug-ceph-csidriver branch from edcc165 to bd6012e Compare

August 11, 2022 09:01

kubevirt-bot removed the needs-rebase label

akalenyu force-pushed the debug-ceph-csidriver branch from bd6012e to 49eef45 Compare

August 11, 2022 09:01

akalenyu added 2 commits

October 2, 2022 14:52


          [WIP] Debug ceph csidriver not being there

cedc471

We'd expect the CSIDriver object to be there, otherwise ceph install might be struggling

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>


          Avoid possible StorageClassName nils

1ad4cea

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

akalenyu force-pushed the debug-ceph-csidriver branch from 49eef45 to 1ad4cea Compare

October 2, 2022 11:52

brybacki reviewed

View reviewed changes

tests/framework/framework.go

               		return
               	}
+              	r.logCSIDrivers(kubeCli)

Contributor

brybacki Oct 3, 2022

👍

pkg/controller/datavolume-controller.go

               	sourcePvc *corev1.PersistentVolumeClaim,
               	targetStorageSpec *corev1.PersistentVolumeClaimSpec) (bool, error) {
               	srcStorageClass := &storagev1.StorageClass{}
+              	if sourcePvc.Spec.StorageClassName == nil {

Contributor

brybacki Oct 3, 2022

is StorageClassName required for advancedClone to work (CSI and Snapshot)

Collaborator Author

akalenyu Oct 3, 2022

I was just protecting nil access in the following line

containerized-data-importer/pkg/controller/datavolume-controller.go

Line 1903 in 1ad4cea

    
           if err := r.client.Get(context.TODO(), types.NamespacedName{Name: *sourcePvc.Spec.StorageClassName}, srcStorageClass); IgnoreNotFound(err) != nil {

But this is a good question - I think that a GET from our cache should return PVCs that already have a storage class name populated

Member

awels Oct 19, 2022

Storage class is only optional for manually created PVs

Contributor

brybacki commented Oct 3, 2022

I think now it looks really good, just one question about source PVC storage class.

Collaborator Author

akalenyu commented Oct 19, 2022 •

edited

Loading

I think now it looks really good, just one question about source PVC storage class.

ping - I think this is pretty harmless and spares us from nil panics

Contributor

brybacki commented Oct 19, 2022

/lgtm

kubevirt-bot assigned brybacki

kubevirt-bot added the lgtm label

Member

awels commented Oct 19, 2022

/approve

Contributor

kubevirt-bot commented Oct 19, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [awels]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kubevirt-bot added the approved label

kubevirt-bot merged commit 4f0fa1f into kubevirt:main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

arnongilboa arnongilboa left review comments

brybacki brybacki left review comments

awels Awaiting requested review from awels

mhenriks Awaiting requested review from mhenriks

maya-r Awaiting requested review from maya-r

Labels

approved dco-signoff: yes lgtm release-note-none size/M