Handle nil ptr in dataimportcron controller #2769

akalenyu · 2023-06-22T11:46:30Z

What this PR does / why we need it:
We see a spike in failures for this test:
https://search.ci.kubevirt.io/?search=8033&maxAge=336h&context=1&type=bug%2Bissue%2Bjunit&name=containerized-data-importer&excludeName=&maxMatches=5&maxBytes=20971520&groupBy=job
CDI controller crashes because of a nil pointer

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 · 2023-06-22T11:46:32Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

akalenyu · 2023-06-22T11:48:10Z

/test all

awels · 2023-06-22T12:23:38Z

pkg/controller/dataimportcron-controller.go

@@ -336,8 +336,10 @@ func (r *DataImportCronReconciler) update(ctx context.Context, dataImportCron *c
 	}

 	handlePopulatedPvc := func() error {
-		if err := r.updateSource(ctx, dataImportCron, pvc); err != nil {
-			return err
+		if pvc != nil {


Instead of checking here, check in updateSource if the object is nil? that way other callers are also protected. If it is nil, just return nil as you can't update the object anyway.

We're still not sure exactly what is going on.. but once we know, and this fixes it, we can do that

So if we want to check for nil in updateSource (receives the client.Object interface),
we must use reflect, which I am not sure is worth it.. wdyt?
(reflect.ValueOf(i).IsNil())

I personally don't see the problem with using reflect but most of the uses of updateSource already check the object before calling the function. I think it'd be enough to add the check here.

I like this fix as-is

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

akalenyu · 2023-06-22T18:01:58Z

/test all

kubevirt-bot · 2023-06-22T18:07:52Z

@akalenyu: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cdi-apidocs	`3cfd449`	link	false	`/test pull-cdi-apidocs`

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. I understand the commands that are listed here.

akalenyu · 2023-06-22T18:40:09Z

@arnongilboa I noticed the failing test doesn't even create DVs because of the wrong config map name

So another theory is that one of the previous test namespaces gets deleted in such an order that
triggers this scenario

EDIT:
Yeah that seems to be it.. I can reproduce this by focusing a completely different test
and looping over it (I took the XXXX test case for example) so this has nothing to do with 8033

mhenriks · 2023-06-26T19:38:34Z

/lgtm
/approve

kubevirt-bot · 2023-06-26T19:38:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

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~~ [mhenriks]

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

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

* dataimportcron: Pass dynamic credential support label (#2760) * dataimportcron: code change: Use better matchers in tests Signed-off-by: Andrej Krejcir <akrejcir@redhat.com> * dataimportcron: Pass dynamic credential support label The label is passed from DataImportCron to DataVolume and DataSource. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com> --------- Signed-off-by: Andrej Krejcir <akrejcir@redhat.com> * Add DataImportCron snapshot sources docs (#2747) Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * add akalenyu as approver, some others as reviewers (#2766) Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * Run `make rpm-deps` (#2741) * Run make rpm-deps Signed-off-by: Maya Rashish <mrashish@redhat.com> * Avoid overlayfs error message by using vfs driver Signed-off-by: Maya Rashish <mrashish@redhat.com> --------- Signed-off-by: Maya Rashish <mrashish@redhat.com> * Fix Destructive test lane failure - missing pod following recreate of CDI (#2744) Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * [WIP] Handle nil ptr in dataimportcron controller (#2769) Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Revert some gomega error checking that produce confusing output (#2772) One of these tests flakes, but the error is hard to debug because gomega will yell about `Unexpected non-nil/non-zero argument at index 0` Instead of showing the error. Apparently this is intended: https://github.com/onsi/gomega/pull/480/files#diff-e696deff1a5be83ad03053b772926cb325cede3b33222fa76c2bb1fcf2efd809R186-R190 Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Run bazelisk run //robots/cmd/uploader:uploader -- -workspace /home/prow/go/src/github.com/kubevirt/project-infra/../containerized-data-importer/WORKSPACE -dry-run=false (#2770) Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com> * [CI] Add metrics name linter (#2774) Signed-off-by: Aviv Litman <alitman@redhat.com> --------- Signed-off-by: Andrej Krejcir <akrejcir@redhat.com> Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> Signed-off-by: Michael Henriksen <mhenriks@redhat.com> Signed-off-by: Maya Rashish <mrashish@redhat.com> Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com> Signed-off-by: Aviv Litman <alitman@redhat.com> Co-authored-by: Andrej Krejcir <akrejcir@gmail.com> Co-authored-by: Michael Henriksen <mhenriks@redhat.com> Co-authored-by: Maya Rashish <mrashish@redhat.com> Co-authored-by: kubevirt-bot <kubevirtbot@redhat.com> Co-authored-by: Aviv Litman <64130977+avlitman@users.noreply.github.com>

kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XS labels Jun 22, 2023

kubevirt-bot requested review from awels and mhenriks June 22, 2023 11:46

awels reviewed Jun 22, 2023

View reviewed changes

[WIP] Handle nil ptr in dataimportcron controller

3cfd449

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

akalenyu force-pushed the nil-ptr-dataimportcron-controller branch from 04b3832 to 3cfd449 Compare June 22, 2023 18:01

akalenyu changed the title ~~[WIP] Handle nil ptr in dataimportcron controller~~ Handle nil ptr in dataimportcron controller Jun 25, 2023

akalenyu marked this pull request as ready for review June 25, 2023 09:34

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 25, 2023

kubevirt-bot requested review from aglitke and awels June 25, 2023 09:34

akalenyu mentioned this pull request Jun 26, 2023

DataVolume Controller uses VolumeCloneSource Populator #2750

Merged

kubevirt-bot assigned mhenriks Jun 26, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2023

kubevirt-bot merged commit 6fd040a into kubevirt:main Jun 27, 2023
1 check passed

akalenyu added a commit to akalenyu/containerized-data-importer that referenced this pull request Jul 3, 2023

[WIP] Handle nil ptr in dataimportcron controller (kubevirt#2769)

6d9d47f

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle nil ptr in dataimportcron controller #2769

Handle nil ptr in dataimportcron controller #2769

akalenyu commented Jun 22, 2023 •

edited

Loading

kubevirt-bot commented Jun 22, 2023

akalenyu commented Jun 22, 2023

awels Jun 22, 2023

akalenyu Jun 22, 2023 •

edited

Loading

akalenyu Jun 25, 2023

alromeros Jun 26, 2023

mhenriks Jun 26, 2023

akalenyu commented Jun 22, 2023

kubevirt-bot commented Jun 22, 2023

akalenyu commented Jun 22, 2023 •

edited

Loading

mhenriks commented Jun 26, 2023

kubevirt-bot commented Jun 26, 2023

Handle nil ptr in dataimportcron controller #2769

Handle nil ptr in dataimportcron controller #2769

Conversation

akalenyu commented Jun 22, 2023 • edited Loading

kubevirt-bot commented Jun 22, 2023

akalenyu commented Jun 22, 2023

awels Jun 22, 2023

Choose a reason for hiding this comment

akalenyu Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

akalenyu Jun 25, 2023

Choose a reason for hiding this comment

alromeros Jun 26, 2023

Choose a reason for hiding this comment

mhenriks Jun 26, 2023

Choose a reason for hiding this comment

akalenyu commented Jun 22, 2023

kubevirt-bot commented Jun 22, 2023

akalenyu commented Jun 22, 2023 • edited Loading

mhenriks commented Jun 26, 2023

kubevirt-bot commented Jun 26, 2023

akalenyu commented Jun 22, 2023 •

edited

Loading

akalenyu Jun 22, 2023 •

edited

Loading

akalenyu commented Jun 22, 2023 •

edited

Loading