Add new DV reason: ImagePullFailed #3194

nunnatsa · 2024-04-14T05:47:38Z

What this PR does / why we need it

When the image pull fails, the DataVolume Running condition, will have the Reason field of ImagePullFailed, to allow better error handling in code that uses it.

Also, moved the StreamDataToFile function from the util package to the importer package, and made to non-exported. This is done to allow calling the new NewErrImagePullFailed function from within streamDataToFile, and to avoid import cycle. In addition, the streamDataToFile is only used in the importer package and so it's better to define it there.

Release note

When the image pull fails, the DataVolume Running condition, will have the Reason field of "ImagePullFailed"

https://issues.redhat.com/browse/CNV-39603

nunnatsa · 2024-04-14T10:30:11Z

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

awels

Looks like most of this PR is not actually related to returning the error message, but in moving the StreamDataToFile function. Other than that some minor comments.

awels · 2024-04-15T16:03:23Z

cmd/cdi-importer/importer.go

+
+	ipfErr := &importer.ErrImagePullFailed{}
+	isImagePullFailure := errors.As(err, &ipfErr)
+	if isImagePullFailure {


Why not just do

if errors.As(err, &ipfErr) { err = ipfErr }

Also if the As is true, isn't err already the right type? I am not entirely I understand what is going on here.

We don't need the boolean here. leftover from another implementation during the development. Will fix.

The As function does not change err but only populate ipfErr

In 2nd thought, I don't need this code at all. I was trying to write a new field in the TerminationMessage object, but this is not working anyway for other reasons.

I'm now relaying on the error text, that is still there.

removed this code

awels · 2024-04-15T16:21:03Z

tests/import_test.go

+
+	},
+		Entry("insecure registry", "registry:5000/myimage:latest"),
+		Entry("non exists image", "quay.io/capk/ubuntu-2004-container-disk:wrong"),


Should not be a real issue, but maybe reference the internal registry here with an invalid image. This now assumes you have access to the internet, which may not always be valid.

Any other idea?

akalenyu

Thanks!
Had some minor comments but what's more interesting to me is why we can't use the termination message API in case of failures.
Is there something missing in it's implementation? I won't ofcourse block over this

tests/import_test.go

nunnatsa · 2024-04-16T05:57:36Z

Thanks! Had some minor comments but what's more interesting to me is why we can't use the termination message API in case of failures. Is there something missing in it's implementation? I won't ofcourse block over this

the termination message API (object) is only available for pod that was terminated with no error. For error cases, the pod write unstructed text, and I didn't want to change this behavior because it's way out of the scope of this PR.

nunnatsa · 2024-04-16T06:04:42Z

e, but in moving the StreamDataToFile function. Other than that some minor comments.

Eventually, yes. But this is not the "why". I moved the function because I wanted to call the NewErrImagePullFailed function from it, and this function is defined in the importer package. Then I found out that the StreamDataToFile function only called from the importer package. So to avoid import cycle, I moved the function, but this is not the reason for this change.

Updated PR description to include this info.

nunnatsa · 2024-04-16T19:04:46Z

/retest

akalenyu

Looks good! thanks

Correct me if I'm wrong but this PR does not handle pullMethod: node (this is fine),
which I thought is quite common on OpenShift. Are you okay with that?

When the image pull fails, the DataVolume Running condition, will have the Reason field of `ImagePullFailed`, to allow better error handling in code taht uses it. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

nunnatsa · 2024-04-18T08:01:54Z

^^^ fixed new linter warning

nunnatsa · 2024-04-18T08:19:30Z

/retest

akalenyu

/approve

akalenyu · 2024-04-18T10:03:00Z

tests/import_test.go

+
+	},
+		Entry("pull method = pod", "myregistry/myorg/myimage:wrongtag", cdiv1.RegistryPullPod),
+		Entry("pull method = node", "myregistry/myorg/myimage:wrongtag", cdiv1.RegistryPullNode),


nunnatsa · 2024-04-18T11:33:02Z

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

awels · 2024-04-18T12:44:29Z

/lgtm
/approve
/cherrypick release-v1.59

kubevirt-bot · 2024-04-18T12:44:32Z

@awels: once the present PR merges, I will cherry-pick it on top of release-v1.59 in a new PR and assign it to you.

In response to this:

/lgtm
/approve
/cherrypick release-v1.59

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.

kubevirt-bot · 2024-04-18T12:44:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akalenyu, 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~~ [akalenyu,awels]

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

nunnatsa · 2024-04-18T12:55:14Z

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

kubevirt-bot · 2024-04-18T17:36:54Z

@awels: new pull request created: #3210

In response to this:

/lgtm
/approve
/cherrypick release-v1.59

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.

kubevirt-bot requested review from akalenyu and ShellyKa13 April 14, 2024 05:47

kubevirt-bot added the size/L label Apr 14, 2024

nunnatsa force-pushed the image-pull-failed branch 2 times, most recently from 0174c9a to 82814e2 Compare April 14, 2024 10:25

nunnatsa force-pushed the image-pull-failed branch from 82814e2 to 4151089 Compare April 14, 2024 13:31

nunnatsa changed the title ~~WIP: Add new DV reason: ImagePullFailed~~ Add new DV reason: ImagePullFailed Apr 14, 2024

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 Apr 14, 2024

nunnatsa force-pushed the image-pull-failed branch from 4151089 to b66b34d Compare April 15, 2024 05:41

awels reviewed Apr 15, 2024

View reviewed changes

nunnatsa force-pushed the image-pull-failed branch from b66b34d to 62789f5 Compare April 15, 2024 16:45

akalenyu reviewed Apr 15, 2024

View reviewed changes

tests/import_test.go Outdated Show resolved Hide resolved

tests/import_test.go Outdated Show resolved Hide resolved

nunnatsa force-pushed the image-pull-failed branch 2 times, most recently from e2eac1e to 83f3144 Compare April 16, 2024 09:36

nunnatsa mentioned this pull request Apr 16, 2024

Add new reason for failed vm kubernetes-sigs/cluster-api-provider-kubevirt#280

Merged

akalenyu reviewed Apr 17, 2024

View reviewed changes

nunnatsa force-pushed the image-pull-failed branch from 83f3144 to 1560113 Compare April 18, 2024 07:13

Add new DV reason: ImagePullFailed

243eb0b

When the image pull fails, the DataVolume Running condition, will have the Reason field of `ImagePullFailed`, to allow better error handling in code taht uses it. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

nunnatsa force-pushed the image-pull-failed branch from 1560113 to 243eb0b Compare April 18, 2024 08:01

akalenyu reviewed Apr 18, 2024

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 18, 2024

kubevirt-bot assigned awels Apr 18, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2024

kubevirt-bot merged commit 475a70c into kubevirt:main Apr 18, 2024
17 of 18 checks passed

kubevirt-bot mentioned this pull request Apr 18, 2024

[release-v1.59] Add new DV reason: ImagePullFailed #3210

Merged

nunnatsa deleted the image-pull-failed branch April 18, 2024 19:20

akalenyu mentioned this pull request Jun 16, 2024

Suppress CDIDefaultStorageClassDegraded on SNO #3310

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new DV reason: ImagePullFailed #3194

Add new DV reason: ImagePullFailed #3194

nunnatsa commented Apr 14, 2024 •

edited

Loading

nunnatsa commented Apr 14, 2024

awels left a comment

awels Apr 15, 2024

nunnatsa Apr 15, 2024

nunnatsa Apr 15, 2024

nunnatsa Apr 15, 2024

awels Apr 15, 2024

nunnatsa Apr 15, 2024

akalenyu left a comment

nunnatsa commented Apr 16, 2024

nunnatsa commented Apr 16, 2024 •

edited

Loading

nunnatsa commented Apr 16, 2024

akalenyu left a comment

nunnatsa commented Apr 18, 2024

nunnatsa commented Apr 18, 2024

akalenyu left a comment

akalenyu Apr 18, 2024

nunnatsa commented Apr 18, 2024

awels commented Apr 18, 2024

kubevirt-bot commented Apr 18, 2024

kubevirt-bot commented Apr 18, 2024

nunnatsa commented Apr 18, 2024

kubevirt-bot commented Apr 18, 2024

Add new DV reason: ImagePullFailed #3194

Add new DV reason: ImagePullFailed #3194

Conversation

nunnatsa commented Apr 14, 2024 • edited Loading

What this PR does / why we need it

Release note

nunnatsa commented Apr 14, 2024

awels left a comment

Choose a reason for hiding this comment

awels Apr 15, 2024

Choose a reason for hiding this comment

nunnatsa Apr 15, 2024

Choose a reason for hiding this comment

nunnatsa Apr 15, 2024

Choose a reason for hiding this comment

nunnatsa Apr 15, 2024

Choose a reason for hiding this comment

awels Apr 15, 2024

Choose a reason for hiding this comment

nunnatsa Apr 15, 2024

Choose a reason for hiding this comment

akalenyu left a comment

Choose a reason for hiding this comment

nunnatsa commented Apr 16, 2024

nunnatsa commented Apr 16, 2024 • edited Loading

nunnatsa commented Apr 16, 2024

akalenyu left a comment

Choose a reason for hiding this comment

nunnatsa commented Apr 18, 2024

nunnatsa commented Apr 18, 2024

akalenyu left a comment

Choose a reason for hiding this comment

akalenyu Apr 18, 2024

Choose a reason for hiding this comment

nunnatsa commented Apr 18, 2024

awels commented Apr 18, 2024

kubevirt-bot commented Apr 18, 2024

kubevirt-bot commented Apr 18, 2024

nunnatsa commented Apr 18, 2024

kubevirt-bot commented Apr 18, 2024

nunnatsa commented Apr 14, 2024 •

edited

Loading

nunnatsa commented Apr 16, 2024 •

edited

Loading