Migrate VMs with direct LUNs from ovirt #371

liranr23 · 2023-05-30T15:52:30Z

This patch will allow importing direct LUNs from oVirt provider to
kubeVirt. The attached direct LUNs will be created in the destination
cluster. It will be connected to the VM as PVC and a PV which contains
the relevant direct LUN details.

Only on late oVirt version the API provides the relevant details.
Therefore, older version can not perform this flow.

The storage should be exposed to the destination environment.

ahadas · 2023-05-31T07:21:24Z

@liranr23 I took a look and it doesn't seem that the ovirt go-sdk was built since @bennyz added those fields that should be consumed here so that might explain why forklift doesn't get them

liranr23 · 2023-05-31T07:23:14Z

@liranr23 I took a look and it doesn't seem that the ovirt go-sdk was built since @bennyz added those fields that should be consumed here so that might explain why forklift doesn't get them

that doesn't matter as we get the data using raw http client not ovirt-go-sdk

ahadas · 2023-05-31T13:39:19Z

\

@liranr23 I took a look and it doesn't seem that the ovirt go-sdk was built since @bennyz added those fields that should be consumed here so that might explain why forklift doesn't get them

that doesn't matter as we get the data using raw http client not ovirt-go-sdk

not sure I'm following why the raw client is relevant here but looking at this, it looks like the code we use is supposed to set the address..

liranr23 · 2023-05-31T14:28:53Z

\

@liranr23 I took a look and it doesn't seem that the ovirt go-sdk was built since @bennyz added those fields that should be consumed here so that might explain why forklift doesn't get them

that doesn't matter as we get the data using raw http client not ovirt-go-sdk

not sure I'm following why the raw client is relevant here but looking at this, it looks like the code we use is supposed to set the address..

it does. thanks to miguel the problem was on ovirt (in my opinion). some details are not showed when querying the list of disks and when checking out the specific disk it has all the details.

codecov · 2023-05-31T15:22:14Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (3533ffb) 0.00% compared to head (9b556c5) 0.00%.

❗ Current head 9b556c5 differs from pull request most recent head c87dd17. Consider uploading reports for the commit c87dd17 to get more accurate results

Additional details and impacted files

@@     Coverage Diff     @@
##   main   #371   +/-   ##
===========================
===========================

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

ahadas · 2023-05-31T15:51:20Z

\

@liranr23 I took a look and it doesn't seem that the ovirt go-sdk was built since @bennyz added those fields that should be consumed here so that might explain why forklift doesn't get them

that doesn't matter as we get the data using raw http client not ovirt-go-sdk

not sure I'm following why the raw client is relevant here but looking at this, it looks like the code we use is supposed to set the address..

it does. thanks to miguel the problem was on ovirt (in my opinion). some details are not showed when querying the list of disks and when checking out the specific disk it has all the details.

ah yeah, I remember this behavior - makes sense

liranr23 · 2023-06-12T10:04:55Z

A gap exist when the VM is set only with direct lun. then we must have storage map (ui and backend), but any map will work (without really doing anything).

ahadas · 2023-06-27T15:06:13Z

pkg/controller/plan/adapter/ovirt/validator.go

+				return
+			}
+		} else if len(da.Disk.Lun.LogicalUnits.LogicalUnit) > 0 && da.Disk.Lun.LogicalUnits.LogicalUnit[0].Address == "" {
+			// have LUN but without the relevant data


that's the information we get from oVirt, unrelated to the storage mapping, right? in that case, why putting it here?

because we fill up the inventory and in older ovirt versions we won't get the whole details. if so - we can't proceed with the migration

yeah, it makes sense to do that but the question is why here - can't we filter out this information before saving it to the inventory then?

filtering before saving - so the vm will be without the disk?

oh that's even better - then we can produce a validation error!

how so? empty map is allowed...

right but the storage map is set later on - I'm referring to an earlier phase where we examine the vm from the source, if we see a diskless vm (e.g., a vm with a lun disk that we cannot migrate) then we can produce a validation error (not necessarily an 'error', maybe a 'warning in the general case and an 'error' when the vm is set with a bootable disk that we cannot migrate)

I still think it would be better to filter out the disk so we won't see invalid disks in the storage mapping and ideally also have a validation warning about this

pkg/controller/plan/kubevirt.go

pkg/controller/plan/adapter/ovirt/builder.go

pkg/controller/plan/adapter/ovirt/client.go

validation/policies/io/konveyor/forklift/ovirt/disk_storage_type.rego

pkg/controller/plan/migration.go

ahadas · 2023-06-27T16:11:10Z

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

+			if err != nil {
+				return
+			}
+		}


this code is not that clear - what does it do?

when we get disks from ovirt it gives us the list. only when querying the disk we will get full details (in case of LUN we need them).

so shouldn't it be 'get'?

i'm getting an error when it's get.

{"level":"info","ts":1688392887.263588,"logger":"collector|ovirt","msg":"Failed.","provider":"konveyor-forklift/ovirt","url":"engineurl/disks/c467d491-2646-44f2-bd5c-f4bff1c3186b","phase":"load","error":"not found.","stacktrace":"\ngithub.com/konveyor/forklift-controller/pkg/controller/provider/container/ovirt.(*Client).get.func1()\n\tpkg/controller/provider/container/ovirt/client.go:173\ngithub.com/konveyor/forklift-controller/pkg/controller/provider/container/ovirt.(*Client).get()\n\tpkg/controller/provider/container/ovirt/client.go:188\ngithub.com/konveyor/forklift-controller/pkg/controller/provider/container/ovirt.(*DiskAdapter).List()\n\tpkg/controller/provider/container/ovirt/model.go:1014\ngithub.com/konveyor/forklift-controller/pkg/controller/provider/container/ovirt.(*Collector).create()\n\tpkg/controller/provider/container/ovirt/collector.go:279\ngithub.com/konveyor/forklift-controller/pkg/controller/provider/container/ovirt.(*Collector).load()\n\tpkg/controller/prov...

@liranr23 the response of the 'list' call should specify the URL of each disk AFAIR, what's the URL of the disk c467d491-2646-44f2-bd5c-f4bff1c3186b there?

ahadas · 2023-06-27T18:22:39Z

@liranr23 it looks good overall!

pkg/controller/plan/adapter/ovirt/builder.go

pkg/controller/plan/kubevirt.go

ahadas

@liranr23 I thought you said the changes to the 'rego' files don't seem to work - so this PR is not up-to-date in this area, right?

pkg/controller/plan/adapter/ovirt/client.go

pkg/controller/plan/migration.go

This patch will collect the data from oVirt provider regarding direct LUNs and populate the inventory. Signed-off-by: Liran Rotenberg <lrotenbe@redhat.com>

This patch will allow importing direct LUNs from oVirt provider to kubeVirt. The attached direct LUNs will be created in the destination cluster. It will be connected to the VM as PVC and a PV which contains the relevant direct LUN details. Only on late oVirt version the API provides the relevant details. Therefore, older version can not perform this flow. The storage should be exposed to the destination environment. Signed-off-by: Liran Rotenberg <lrotenbe@redhat.com>

Signed-off-by: Liran Rotenberg <lrotenbe@redhat.com>

When failing to get the VM from the inventory to detach its LUN disks. Signed-off-by: Arik Hadas <ahadas@redhat.com>

And improve the comment that describes it. Signed-off-by: Arik Hadas <ahadas@redhat.com>

Signed-off-by: Arik Hadas <ahadas@redhat.com>

sonarcloud · 2023-07-16T21:59:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
5.7% Duplication

ahadas

the open threads can be resolved separately

liranr23 force-pushed the direct_luns branch from 1b62581 to f87c160 Compare May 31, 2023 15:14

liranr23 force-pushed the direct_luns branch 2 times, most recently from 9fb4c19 to 605a8ea Compare June 12, 2023 08:59

liranr23 marked this pull request as ready for review June 12, 2023 08:59

liranr23 requested review from ahadas, bennyz and mmartinv as code owners June 12, 2023 08:59

liranr23 force-pushed the direct_luns branch 2 times, most recently from 381c85d to 49f04dc Compare June 15, 2023 08:43

ahadas changed the title ~~Introduce direct LUNs from ovirt~~ Migrate VMs with direct LUNs from ovirt Jun 27, 2023

ahadas reviewed Jun 27, 2023

View reviewed changes

bennyz reviewed Jun 28, 2023

View reviewed changes

pkg/controller/plan/adapter/ovirt/builder.go Show resolved Hide resolved

liranr23 force-pushed the direct_luns branch from 49f04dc to 3bbcb39 Compare June 29, 2023 08:59

liranr23 requested a review from ahadas June 29, 2023 11:38

ahadas reviewed Jun 29, 2023

View reviewed changes

pkg/controller/plan/kubevirt.go Outdated Show resolved Hide resolved

pkg/controller/plan/kubevirt.go Outdated Show resolved Hide resolved

ahadas mentioned this pull request Jul 10, 2023

Check that errors for LUN disks propagate to the user #426

Closed

ahadas reviewed Jul 10, 2023

View reviewed changes

pkg/controller/plan/adapter/ovirt/client.go Show resolved Hide resolved

pkg/controller/plan/migration.go Outdated Show resolved Hide resolved

liranr23 force-pushed the direct_luns branch 4 times, most recently from 8086926 to 9b556c5 Compare July 10, 2023 14:52

ahadas mentioned this pull request Jul 11, 2023

Multiple storage domains are always mapped to a single storage class? #427

Closed

liranr23 and others added 7 commits July 17, 2023 00:02

Introduce LUN to ovirt inventory

f329f25

This patch will collect the data from oVirt provider regarding direct LUNs and populate the inventory. Signed-off-by: Liran Rotenberg <lrotenbe@redhat.com>

validation: allow lun storage type

e218ecd

Signed-off-by: Liran Rotenberg <lrotenbe@redhat.com>

Add missing 'return'

22f9bd9

When failing to get the VM from the inventory to detach its LUN disks. Signed-off-by: Arik Hadas <ahadas@redhat.com>

Rename Client:DetachDisk to DetachDisks

900b67a

And improve the comment that describes it. Signed-off-by: Arik Hadas <ahadas@redhat.com>

Improve error message when failing to detach LUNs

ea9afc0

Signed-off-by: Arik Hadas <ahadas@redhat.com>

Fix check of PVCs for LUNs

e1628b5

Signed-off-by: Arik Hadas <ahadas@redhat.com>

ahadas force-pushed the direct_luns branch from 9b556c5 to e1628b5 Compare July 16, 2023 21:27

Reduce Cognitive Complexity in kubevirt.go

c87dd17

Signed-off-by: Arik Hadas <ahadas@redhat.com>

ahadas approved these changes Jul 16, 2023

View reviewed changes

ahadas requested a review from bennyz July 16, 2023 22:04

ahadas merged commit 1557a58 into kubev2v:main Jul 17, 2023
7 checks passed

liranr23 deleted the direct_luns branch November 20, 2023 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate VMs with direct LUNs from ovirt #371

Migrate VMs with direct LUNs from ovirt #371

liranr23 commented May 30, 2023

ahadas commented May 31, 2023

liranr23 commented May 31, 2023

ahadas commented May 31, 2023

liranr23 commented May 31, 2023

codecov bot commented May 31, 2023 •

edited

Loading

ahadas commented May 31, 2023

liranr23 commented Jun 12, 2023

ahadas Jun 27, 2023

liranr23 Jun 28, 2023

ahadas Jun 29, 2023

liranr23 Jun 29, 2023

ahadas Jun 29, 2023

liranr23 Jul 2, 2023

ahadas Jul 2, 2023 •

edited

Loading

ahadas Jul 10, 2023

ahadas Jun 27, 2023

liranr23 Jun 28, 2023

ahadas Jun 29, 2023

liranr23 Jul 3, 2023

ahadas Jul 11, 2023

ahadas commented Jun 27, 2023

ahadas left a comment

sonarcloud bot commented Jul 16, 2023

ahadas left a comment

Migrate VMs with direct LUNs from ovirt #371

Migrate VMs with direct LUNs from ovirt #371

Conversation

liranr23 commented May 30, 2023

ahadas commented May 31, 2023

liranr23 commented May 31, 2023

ahadas commented May 31, 2023

liranr23 commented May 31, 2023

codecov bot commented May 31, 2023 • edited Loading

Codecov Report

ahadas commented May 31, 2023

liranr23 commented Jun 12, 2023

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

Choose a reason for hiding this comment

ahadas Jul 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahadas commented Jun 27, 2023

ahadas left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jul 16, 2023

ahadas left a comment

Choose a reason for hiding this comment

codecov bot commented May 31, 2023 •

edited

Loading

ahadas Jul 2, 2023 •

edited

Loading