feat: Make importer datasource communication explicit #3101

0xFelix · 2024-02-14T16:29:26Z

What this PR does / why we need it:

Make the communication of datasources in the importer explicit by adding
a GetTerminationMessage method to the DataSourceInterface.
Then use this method to communicate additional information to the import
controller once the importer pod has terminated, instead of writing
additional data to the termination message in the Close method of
datasources.

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:

Enhancement: Make importer datasource communication explicit

0xFelix · 2024-02-14T16:32:24Z

/cc @awels Does this approach look good to you?

0xFelix · 2024-02-16T16:08:31Z

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

0xFelix · 2024-02-16T17:56:42Z

/test pull-containerized-data-importer-non-csi-hpp

0xFelix · 2024-02-19T08:23:23Z

/retest-required

awels · 2024-02-20T16:11:02Z

Sorry haven't had a chance to really look at this. I will try to do so later today.

awels

Looks pretty good a few questions.

awels · 2024-02-21T20:45:07Z

pkg/common/common_test.go

+)
+
+var _ = Describe("TerminationMessage", func() {
+	It("Should fail if serialized data is longer than 4096 bytes", func() {


Testing any of the happy paths?

Added a test for the happy path.

awels · 2024-02-21T20:46:58Z

pkg/controller/import-controller.go

+
+	termMsg, err := parseTerminationMessage(pod)
+	if err != nil {
+		log.V(3).Info("Ignoring failure to parse termination message")


Maybe at least print the actual reason the parsing failed?

0xFelix · 2024-02-22T08:29:38Z

@awels Addressed your comments.

0xFelix · 2024-02-28T15:54:43Z

@awels Ping

alromeros

I think this is a great idea, thanks for opening the PR. It looks good to me but as discussed in #3116 I think we should first merge the importer bugfix and adapt the behavior here.

alromeros · 2024-03-08T15:08:24Z

pkg/common/common.go

+}
+
+// TerminationMessage contains data to be serialized and used as the termination message of the importer.
+type TerminationMessage struct {


This is a great idea, it could come in handy to transfer unexpected but non-error import info such as some odd content types. We had an attempt to improve the import behavior when importing text/html but the solution was a bit lackluster: #2909. Just a comment though, that would be out of scope for the PR.

Yes, exactly what I'm doing in #3103

akalenyu · 2024-03-10T21:53:18Z

tests/datavolume_test.go

@@ -1013,7 +1013,7 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests",
 				runningCondition: &cdiv1.DataVolumeCondition{
 					Type:    cdiv1.DataVolumeRunning,
 					Status:  v1.ConditionFalse,
-					Message: "Import Complete; VDDK: {\"Version\":\"1.2.3\",\"Host\":\"esx.test\"}",


Probably missing something trivial, why are we dropping this from the expected output, shouldn't the version and host get propagated correctly?

After importing the importer Pod writes the extended termination message to its termination log. Once the import controller sees it and was sucessfully able to parse the message, the value written to the message annotation will be just Import Complete instead of the full serialized JSON string. The import controller will still set the appropriate annotations for VDDK version and host.

I see. @mrnold hey! do you happen to know if anyone was treating this message as an API on the CDI consuming side?

Make the communication of datasources in the importer explicit by adding a GetTerminationMessage method to the DataSourceInterface. Then use this method to communicate additional information to the import controller once the importer pod has terminated, instead of writing additional data to the termination message in the Close method of datasources. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

0xFelix · 2024-03-12T15:01:51Z

/test pull-containerized-data-importer-e2e-hpp-previous

pkg/controller/import-controller.go

akalenyu

Looks good! thank you

0xFelix · 2024-03-12T17:58:56Z

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

mhenriks · 2024-03-13T21:07:53Z

Very cool

/approve

kubevirt-bot · 2024-03-13T21:08:04Z

[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

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

kubevirt-bot requested review from akalenyu and alromeros February 14, 2024 16:29

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

0xFelix force-pushed the termination-message branch 4 times, most recently from 569d089 to b51995a Compare February 15, 2024 15:33

0xFelix changed the title ~~[WIP] feat: Add extended termination message to importer~~ [WIP] feat: Make importer datasource communication explicit Feb 15, 2024

0xFelix force-pushed the termination-message branch 4 times, most recently from 1c8370d to 5ead439 Compare February 15, 2024 16:13

0xFelix changed the title ~~[WIP] feat: Make importer datasource communication explicit~~ feat: Make importer datasource communication explicit Feb 15, 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 Feb 15, 2024

0xFelix force-pushed the termination-message branch 2 times, most recently from ba2f480 to 0fa3b25 Compare February 16, 2024 14:11

0xFelix force-pushed the termination-message branch 3 times, most recently from ad4d1cf to b793dbe Compare February 20, 2024 15:26

0xFelix mentioned this pull request Feb 21, 2024

feat(import): Set PVC labels derived from env vars on containerdisks #3103

Merged

awels reviewed Feb 21, 2024

View reviewed changes

0xFelix force-pushed the termination-message branch from b793dbe to be52686 Compare February 22, 2024 08:29

0xFelix force-pushed the termination-message branch from be52686 to 8462345 Compare February 22, 2024 08:43

0xFelix mentioned this pull request Mar 7, 2024

Avoid race condition during importer termination #3116

Merged

alromeros reviewed Mar 8, 2024

View reviewed changes

akalenyu reviewed Mar 10, 2024

View reviewed changes

0xFelix force-pushed the termination-message branch from 8462345 to c88d3ba Compare March 12, 2024 14:02

akalenyu reviewed Mar 12, 2024

View reviewed changes

pkg/controller/import-controller.go Show resolved Hide resolved

akalenyu approved these changes Mar 12, 2024

View reviewed changes

kubevirt-bot assigned akalenyu Mar 12, 2024

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

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

kubevirt-bot merged commit cd7ee9e into kubevirt:main Mar 13, 2024
18 checks passed

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Make importer datasource communication explicit #3101

feat: Make importer datasource communication explicit #3101

0xFelix commented Feb 14, 2024 •

edited by akalenyu

Loading

0xFelix commented Feb 14, 2024

0xFelix commented Feb 16, 2024

0xFelix commented Feb 16, 2024

0xFelix commented Feb 19, 2024

awels commented Feb 20, 2024

awels left a comment

awels Feb 21, 2024

0xFelix Feb 22, 2024

awels Feb 21, 2024

0xFelix Feb 22, 2024

0xFelix commented Feb 22, 2024

0xFelix commented Feb 28, 2024

alromeros left a comment

alromeros Mar 8, 2024 •

edited

Loading

0xFelix Mar 11, 2024

akalenyu Mar 10, 2024

0xFelix Mar 11, 2024

akalenyu Mar 11, 2024

0xFelix commented Mar 12, 2024

akalenyu left a comment

0xFelix commented Mar 12, 2024

mhenriks commented Mar 13, 2024

kubevirt-bot commented Mar 13, 2024

feat: Make importer datasource communication explicit #3101

feat: Make importer datasource communication explicit #3101

Conversation

0xFelix commented Feb 14, 2024 • edited by akalenyu Loading

0xFelix commented Feb 14, 2024

0xFelix commented Feb 16, 2024

0xFelix commented Feb 16, 2024

0xFelix commented Feb 19, 2024

awels commented Feb 20, 2024

awels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xFelix commented Feb 22, 2024

0xFelix commented Feb 28, 2024

alromeros left a comment

Choose a reason for hiding this comment

alromeros Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xFelix commented Mar 12, 2024

akalenyu left a comment

Choose a reason for hiding this comment

0xFelix commented Mar 12, 2024

mhenriks commented Mar 13, 2024

kubevirt-bot commented Mar 13, 2024

0xFelix commented Feb 14, 2024 •

edited by akalenyu

Loading

alromeros Mar 8, 2024 •

edited

Loading