Include a nil check for the snapshotContent #278
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @huffmanca. Thanks for your PR. I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
size/XS
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| ctrl.setAnnVolumeSnapshotBeingDeleted(content) | ||
| if content != nil { | ||
| klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent: Set VolumeSnapshotBeingDeleted annotation on the content [%s]", content.Name) | ||
| ctrl.setAnnVolumeSnapshotBeingDeleted(content) |
There was a problem hiding this comment.
This is called in multiple places. Can you add a nil check inside setAnnVolumeSnapshotBeingDeleted()?
I actually just ran into this yesterday so I added it here:
https://github.com/kubernetes-csi/external-snapshotter/pull/261/files#diff-214a2fc68717c170415a3578536d3b74R1281
I'll remove my change once your PR is merged.
There was a problem hiding this comment.
I've updated this to also include the check inside of setAnnVolumeSnapshotBeingDeleted(). I was hitting the panic on line 270 above, when we were printing out the debug statement.
I can remove the name of the content, since it should print this out within setAnnVolumeSnapshotBeingDeleted immediately after it checks for nil now.
There was a problem hiding this comment.
This nil check on line 269 can be removed now since you moved the check inside setAnnVolumeSnapshotBeingDeleted()
There was a problem hiding this comment.
I removed the nil check here, but I also updated the debug logging to not print the content name. That's where I was hitting the panic
huffmanca
force-pushed
the
check_nil_content
branch
from
fa6d56a to
703d7ac
Compare
k8s-ci-robot
added
size/S
huffmanca
force-pushed
the
check_nil_content
branch
from
703d7ac to
806ccc0
Compare
k8s-ci-robot
added
size/XS
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: huffmanca, xing-yang 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
| if !metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted) { | ||
| if content == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This check should be moved before the if block. Otherwise we'll still get a nil exception on line 1283 as it references the content. I'm going to revert this PR. Can you please submit a different PR?
74502e544 Merge pull request kubernetes-csi#278 from liangyuanpeng/migrate_k8s_testimages 533443055 Merge pull request kubernetes-csi#281 from kubernetes-csi/dependabot/github_actions/actions/checkout-5 458ce146f Bump actions/checkout from 4 to 5 5ec1a52b8 use gcr.io/k8s-staging-test-infra instead of gcr.io/k8s-testimages git-subtree-dir: release-tools git-subtree-split: 74502e544bc6a17820892c0d490e8f0b59462998
3 participants
/kind bug
What this PR does / why we need it:
It's currently possible for the snapshotController to crash when checking snapshot Finalizers if the VolumeSnapshotContent is nil. This PR introduces a simple nil check to prevent the panic.
Special notes for your reviewer:
Does this PR introduce a user-facing change?: