Add OVA provider and inventory #419

bkhizgiy · 2023-07-03T14:28:45Z

Adding new OVA type provider, once the provider is added a new OVA-server deployment will be applied, the controller will query the new server pod and build the provider inventory based on the info that will be provided by it.

nyoxi

Preliminary comments.

Makefile

nyoxi · 2023-07-04T16:01:19Z

Makefile

@@ -279,7 +288,8 @@ push-all-images:  push-api-image \
                  push-operator-index-image \
                  push-populator-controller-image \
                  push-ovirt-populator-image \
-                  push-openstack-populator-image
+                  push-openstack-populator-image\
+				  push-ova-parses-image

 .PHONY: check_container_runtmime


Not related to the patch but I see we have a typo in the word "runtime" here.

nyoxi · 2023-07-04T16:12:02Z

cmd/ova-parser/ova-parser.go

+		if err != nil {
+			fmt.Println("Error extracting OVF from OVA:", err)
+			continue
+		}


I suggest using defer os.RemoveAll(tmpDir) here instead calling it separately below. Either way you may want to log the error from os.RemoveAll.

yeah and we should also check if we really need to extract the file to a temporary directory, it should be possible to read the content of the OVF directly from the OVA (found this)

Using a library is probably preferable if it can be used easily. But you can also tell tar to dump the file to stdout with -O/--to-stdout. But you also need to call tar multiple times -- first to list the files and then get the output. (Just saying.)

But you also need to call tar multiple times -- first to list the files and then get the output. (Just saying.)

according to the OVF specification, the OVF descriptor should always be the first entry ("The ordering restriction ensures that it is possible to extract the OVF descriptor from a compressed OVF package without scanning the entire archive. ")

In theory yes. In reality there are plenty of hand crafted OVAs not following the specs. Some of which are not even TAR files (are e.g. ZIP files). And it's fine to discard those as not supported. But you don't want such files to cause you troubles.

Specifically in this context, if you just run tar xOf somethig.ova --wildcards '*.ovf' and the archive contains multiple OVF files you will end up with their content concatenated together which will confuse the XML parser and produce bogus errors. That's what I meant.

cmd/ova-parser/ova-parser.go

nyoxi · 2023-07-04T16:45:58Z

hack/ova-parser/Containerfile

@@ -0,0 +1,10 @@
+FROM registry.access.redhat.com/ubi8/go-toolset:1.18.9-8 as builder


It would be cleaner if we created a separate directory for the Container files naming them e.g. ova-parser.Containerfile and leave the hack directory only for support scripts.

pkg/controller/provider/controller.go

ahadas

looks like a good starting point

Makefile

ahadas · 2023-07-10T13:56:21Z

cmd/ova-parser/ova-parser.go

+	http.HandleFunc("/networks", networkHandler)
+	http.HandleFunc("/watch", watchdHandler)
+
+	http.ListenAndServe(":8080", nil)


how about separating out the other parts to a different file and renaming this one to ova-data-server.go?

ahadas · 2023-07-10T14:00:18Z

cmd/ova-parser/ova-parser.go

+		if err != nil {
+			fmt.Println("Error extracting OVF from OVA:", err)
+			continue
+		}


yeah and we should also check if we really need to extract the file to a temporary directory, it should be possible to read the content of the OVF directly from the OVA (found this)

cmd/ova-parser/ova-parser.go

ahadas · 2023-07-10T15:18:56Z

hack/ova-parser/Containerfile

we used Container files for populators that were hard to build with bazel, I guess that was the reference for this, right? it should be easy to build this with bazel instead

pkg/controller/provider/controller.go

pkg/controller/provider/model/ova/doc.go

bkhizgiy · 2023-07-16T20:22:57Z

pkg/controller/provider/web/ova/tree.go

+
+// Tree.
+func (h TreeHandler) Tree(ctx *gin.Context) {
+	// status := h.Prepare(ctx)


need to be modified for the UI.

ahadas

partial review

pkg/controller/map/storage/handler/ova/handler.go

pkg/controller/provider/container/ova/client.go

pkg/controller/provider/container/ova/watch.go

pkg/controller/provider/controller.go

pkg/controller/provider/model/ova/model.go

pkg/controller/provider/validation.go

pkg/controller/provider/container/ova/client.go

pkg/controller/provider/controller.go

pkg/controller/provider/validation.go

ahadas

complete review

operator/roles/forkliftcontroller/defaults/main.yml

operator/roles/forkliftcontroller/templates/controller/ova-scc.yml.j2

operator/roles/forkliftcontroller/tasks/main.yml

pkg/controller/provider/validation.go

pkg/controller/provider/controller.go

pkg/controller/provider/model/ova/model.go

operator/roles/forkliftcontroller/defaults/main.yml

operator/roles/forkliftcontroller/tasks/main.yml

operator/roles/forkliftcontroller/templates/controller/ova-scc.yml.j2

ahadas

looks good, just two minor comments
@bkhizgiy was it verified with #439 ?

pkg/controller/provider/controller.go

ahadas

adding comments from sonarcloud

pkg/controller/provider/container/ova/model.go

pkg/controller/provider/web/ova/workload.go

Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>

1. Remove unused empty function WorkloadHandler#List 2. Reduce Cognitive Complexity of ProviderHandler#ListContent Signed-off-by: Arik Hadas <ahadas@redhat.com>

sonarcloud · 2023-07-23T13:23:39Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
4.5% Duplication

bkhizgiy requested a review from liranr23 July 3, 2023 14:28

bkhizgiy requested a review from ahadas as a code owner July 3, 2023 14:28

bkhizgiy force-pushed the ova branch 3 times, most recently from da5e719 to c8622d3 Compare July 4, 2023 12:42

bkhizgiy requested a review from eslutsky as a code owner July 4, 2023 12:42

bkhizgiy force-pushed the ova branch from c8622d3 to 2ecfbbe Compare July 4, 2023 13:37

nyoxi requested changes Jul 4, 2023

View reviewed changes

bkhizgiy force-pushed the ova branch from 2ecfbbe to 86b1b99 Compare July 10, 2023 08:08

ahadas reviewed Jul 10, 2023

View reviewed changes

bkhizgiy force-pushed the ova branch 4 times, most recently from 5d20cfc to fed2fc7 Compare July 16, 2023 20:15

bkhizgiy requested a review from mnecas as a code owner July 16, 2023 20:15

bkhizgiy commented Jul 16, 2023

View reviewed changes

bkhizgiy changed the title ~~WIP: adding initial support for OVA migration~~ Add OVA provider and inventory Jul 16, 2023

ahadas force-pushed the ova branch from fed2fc7 to f9b2897 Compare July 17, 2023 11:48

ahadas reviewed Jul 17, 2023

View reviewed changes

pkg/controller/provider/model/ova/model.go Outdated Show resolved Hide resolved

pkg/controller/provider/model/ova/model.go Outdated Show resolved Hide resolved

pkg/controller/provider/validation.go Show resolved Hide resolved

bkhizgiy force-pushed the ova branch 2 times, most recently from 8ee60e6 to e3e36bf Compare July 17, 2023 22:24

ahadas reviewed Jul 18, 2023

View reviewed changes

pkg/controller/provider/controller.go Outdated Show resolved Hide resolved

pkg/controller/provider/validation.go Outdated Show resolved Hide resolved

ahadas reviewed Jul 18, 2023

View reviewed changes

bkhizgiy force-pushed the ova branch from e3e36bf to 764dd10 Compare July 18, 2023 10:33

This was referenced Jul 18, 2023

Remove DefaultVirtV2VImage #452

Closed

Add the details of the OVA server to the status of the OVA provider #454

Open

ahadas reviewed Jul 19, 2023

View reviewed changes

bkhizgiy force-pushed the ova branch from 764dd10 to 37bc552 Compare July 19, 2023 12:33

ahadas reviewed Jul 19, 2023

View reviewed changes

operator/roles/forkliftcontroller/defaults/main.yml Outdated Show resolved Hide resolved

operator/roles/forkliftcontroller/tasks/main.yml Outdated Show resolved Hide resolved

operator/roles/forkliftcontroller/templates/controller/ova-scc.yml.j2 Outdated Show resolved Hide resolved

bkhizgiy force-pushed the ova branch from 37bc552 to 526df4b Compare July 19, 2023 13:22

ahadas reviewed Jul 19, 2023

View reviewed changes

pkg/controller/provider/controller.go Outdated Show resolved Hide resolved

pkg/controller/provider/controller.go Outdated Show resolved Hide resolved

ahadas requested review from nyoxi and bennyz July 19, 2023 14:10

ahadas reviewed Jul 19, 2023

View reviewed changes

pkg/controller/provider/container/ova/model.go Outdated Show resolved Hide resolved

pkg/controller/provider/web/ova/workload.go Outdated Show resolved Hide resolved

bkhizgiy force-pushed the ova branch 2 times, most recently from 7153340 to b94a5d5 Compare July 20, 2023 08:13

bkhizgiy and others added 2 commits July 23, 2023 16:23

WIP: Adding initiall support for OVA migration

ea9e399

Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>

Fix code smell issues

116f637

1. Remove unused empty function WorkloadHandler#List 2. Reduce Cognitive Complexity of ProviderHandler#ListContent Signed-off-by: Arik Hadas <ahadas@redhat.com>

ahadas force-pushed the ova branch from 4704f70 to 116f637 Compare July 23, 2023 13:23

ahadas approved these changes Jul 23, 2023

View reviewed changes

ahadas merged commit 3f2bcd5 into kubev2v:main Jul 23, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OVA provider and inventory #419

Add OVA provider and inventory #419

bkhizgiy commented Jul 3, 2023 •

edited

Loading

nyoxi left a comment

nyoxi Jul 4, 2023

nyoxi Jul 4, 2023

ahadas Jul 10, 2023 •

edited

Loading

nyoxi Jul 11, 2023

ahadas Jul 13, 2023

nyoxi Jul 13, 2023

nyoxi Jul 4, 2023

ahadas left a comment

ahadas Jul 10, 2023

ahadas Jul 10, 2023 •

edited

Loading

ahadas Jul 10, 2023

bkhizgiy Jul 16, 2023

ahadas left a comment

ahadas left a comment

ahadas left a comment

ahadas left a comment

sonarcloud bot commented Jul 23, 2023

		@@ -0,0 +1,10 @@
		FROM registry.access.redhat.com/ubi8/go-toolset:1.18.9-8 as builder

Add OVA provider and inventory #419

Add OVA provider and inventory #419

Conversation

bkhizgiy commented Jul 3, 2023 • edited Loading

nyoxi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahadas Jul 10, 2023 • 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

ahadas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahadas Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahadas left a comment

Choose a reason for hiding this comment

ahadas left a comment

Choose a reason for hiding this comment

ahadas left a comment

Choose a reason for hiding this comment

ahadas left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jul 23, 2023

bkhizgiy commented Jul 3, 2023 •

edited

Loading

ahadas Jul 10, 2023 •

edited

Loading

ahadas Jul 10, 2023 •

edited

Loading