Update how template disks are created #1400

sjd78 · 2021-04-01T21:17:00Z

Changes have been made to following how Admin Portal works when creating VMs from a template. How template disks are created depends on (1) the VM's optimized for selection, and (2) if a new storage domain needs to be selected.

The VM's "Optimized For" selection drives the default disk type for the template disks. In Admin Portal, selecting Desktop resets the "Storage Allocation" selection to Thin. Selecting Server or High Performance resets "Storage Allocation" to Clone.

If VM's optimized for selection is Desktop:

behavior matches when Admin Portal Storage Allocation is Thin
all of the template disks' disk types are forced to "Thin Provisioned"
the VM create is sent:
- clone=true if AT LEAST ONE of the disks has a storage domain that differs from the template's disk
- clone=false as long as all of the template disk's storage domains remain unchanged

If VM's optimized for selection is Server:

behavior matches when Admin Portal's Storage Allocation is Clone
the template disks use the disk type as defined by the template
the VM create is sent clone=true

Fixes: #1371

rszwajko

Can we expose clone parameter in the summary view? We could name it independent virtual machine following naming in the API.
What happens if those rules are violated? Will the backend return an error?

rszwajko · 2021-04-08T08:50:11Z

src/sagas/vmChanges.js

@@ -200,6 +200,7 @@ function* composeProvisionSourceTemplate ({ vm, basic, disks }) {

      // did the storage domain change?
      if (disk.storageDomainId !== templateDisk.get('storageDomainId')) {
+        vmRequiresClone = true


Can we unit test this saga? Just the case we are modifying.

Probably but the infrastructure isn't in place to do it. Maybe a followup PR? I would prefer to not spend that time at the moment.

sjd78 · 2021-04-08T19:02:17Z

Can we expose clone parameter in the summary view? We could name it independent virtual machine following naming in the API.

What would the purpose of that be? It wasn't important to display an auto-calculated value previously. Since there is no way for the user to control that setting my initial thought is that it would be more confusing than helpful.

What happens if those rules are violated? Will the backend return an error?

Yes, the backend AddVmCommand or related will throw an error and the rest call will just fail if the combination of params on the target cluster etc are not valid. The error is just routed back to an error box on the create screen. Unfortunately when there are errors, there is very little a user can do to fix it themselves.

rszwajko · 2021-04-09T08:57:35Z

Can we expose clone parameter in the summary view? We could name it independent virtual machine following naming in the API.

What would the purpose of that be? It wasn't important to display an auto-calculated value previously. Since there is no way for the user to control that setting my initial thought is that it would be more confusing than helpful.

linked-clone vs full-clone functionality seems important important enough to be part of the summary. IMHO eventually it should be an editable property.
testing/preventing regressions (it's easier to notice invalid value)
educating users - choosing different storage domain makes the VM server-like although it's marked as desktop (it would be better then to explicitly switch to "optimized for server" mode)

What happens if those rules are violated? Will the backend return an error?

Yes

Great!

hstastna

I've created a couple of VMs optimized for Server, Desktop, template based and also user selected, played with the disks, it works for me as expected. The code changes LGTM.

Regarding Radek's suggestions, I suggest to implement them later, in a followup PR. Let's move this forward! :)

sjd78 · 2021-04-09T16:15:30Z

@rszwajko, @hstastna - I created an issue for the clone value in summary. See #1402.

bamsalem

LGTM

sgratch · 2021-04-22T14:44:34Z

@sjd78

If VM's optimized for selection is Desktop:

* behavior matches when Admin Portal **Storage Allocation** is **Thin**

* all of the template disks' disk types are forced to "Thin Provisioned"

* the VM create is sent:
  
  * `clone=true` if AT LEAST ONE of the disks has a storage domain that differs from the template's disk

This clone=true scenario is not working as expected (comparing to webadmin) with the following use case:

Create a template "temp" with a disk type set to Raw == "Preallocated
Create a VM via web-ui optimized for "Desktop" and based on template "temp" while changing the target SD since there is no permission for creating the template's disk on original template's SD:
Expected behaviour: the template's disk will be cloned to new targeted SD with disk type set to Thin, regardless to original storage allocation.
Actual behaviour: the template's disk is cloned to new targeted SD with disk type policy set to Preallocated

  * `clone=false` as long as all of the template disk's storage domains remain unchanged

If VM's optimized for selection is Server:

* behavior matches when Admin Portal's Storage Allocation** is **Clone**

* the template disks use the disk type as defined by the template

* the VM create is sent `clone=true`

When trying to create a VM via web-ui optimized for "Server" and based on a template with 2 disks (one is preallocated and one is thin), the VM creation failed on backend with the following error:

"ERROR [org.ovirt.engine.core.dal.dbbroker.auditloghandling.AuditLogDirector] (EE-ManagedScheduledExecutorService-engineScheduledThreadPool-Thread-
100) [] EVENT_ID: USER_ADD_VM_FINISHED_FAILURE(60), Failed to complete VM eee-server-vp1 creation."

Trying to create a VM based on the same template but optimized to "Desktop" was succeeded. Need to debug the backend for understaning why it failed.

Changes have been made to following how Admin Portal works when creating VMs from a template. How template disks are created depends on (1) the Template and VM's optimized for selection, and (2) if a new storage domain needs to be selected. The VM's "Optimized For" selection drives the default disk type for the template disks. In Admin Portal, selecting **Desktop** resets the "Storage Allocation" selection to **Thin**. Selecting **Server** or **High Performance** resets "Storage Allocation" to **Clone**. If VM's optimized for selection is **Desktop**: - behavior matches when Admin Portal **Storage Allocation** is **Thin** - all of the template disks' disk types are forced to "Thin Provisioned" - the VM create is sent: - `clone=true` if AT LEAST ONE of the disks has a storage domain that differs from the template's disk - `clone=false` as long as all of the template disk's storage domains remain unchanged If VM's optimized for selection is **Server**: - behavior matches when Admin Portal's Storage Allocation** is **Clone** - the template disks use the disk type as defined by the template - the VM create is sent `clone=true` Fixes: oVirt#1371

sjd78 · 2021-04-22T20:38:30Z

If VM's optimized for selection is Desktop:
* behavior matches when Admin Portal **Storage Allocation** is **Thin**

* all of the template disks' disk types are forced to "Thin Provisioned"

* the VM create is sent:
  
  * `clone=true` if AT LEAST ONE of the disks has a storage domain that differs from the template's disk
This clone=true scenario is not working as expected (comparing to webadmin) with the following use case:

Create a template "temp" with a disk type set to Raw == "Preallocated

Create a VM via web-ui optimized for "Desktop" and based on template "temp" while changing the target SD since there is no permission for creating the template's disk on original template's SD:

Expected behaviour: the template's disk will be cloned to new targeted SD with disk type set to Thin, regardless to original storage allocation.
Actual behaviour: the template's disk is cloned to new targeted SD with disk type policy set to Preallocated

I was forcing the disk format to 'raw' when changing the storage domain. Looks like that isn't necessary.

When creating a disk from a template, it is not necessary, and sometime wrong, to force a disk's format to 'raw' when changing/cloning to a different storage domain.

sgratch · 2021-04-23T11:52:44Z

If VM's optimized for selection is Desktop:
* behavior matches when Admin Portal **Storage Allocation** is **Thin**

* all of the template disks' disk types are forced to "Thin Provisioned"

* the VM create is sent:
  
  * `clone=true` if AT LEAST ONE of the disks has a storage domain that differs from the template's disk
This clone=true scenario is not working as expected (comparing to webadmin) with the following use case:

Create a template "temp" with a disk type set to Raw == "Preallocated

Create a VM via web-ui optimized for "Desktop" and based on template "temp" while changing the target SD since there is no permission for creating the template's disk on original template's SD:

Expected behaviour: the template's disk will be cloned to new targeted SD with disk type set to Thin, regardless to original storage allocation.
Actual behaviour: the template's disk is cloned to new targeted SD with disk type policy set to Preallocated
I was forcing the disk format to 'raw' when changing the storage domain. Looks like that isn't necessary.

This is still not working for me, I'm getting the following error:

I think that the problem is that the preallocated/raw disk is treated by web-ui as preallocated/cow while it should be converted to thin/cow(default on webadmin) or preallocated/raw.

sjd78 · 2021-04-23T17:37:37Z

I was forcing the disk format to 'raw' when changing the storage domain. Looks like that isn't necessary.

This is still not working for me, I'm getting the following error:

I think that the problem is that the preallocated/raw disk is treated by web-ui as preallocated/cow while it should be converted to thin/cow(default on webadmin) or preallocated/raw.

I saw the same error message when testing things out with a non-admin user against a 4.4.5.10 engine.

Then I tried the same thing with the non-admin user but with patch 114092 applied my master branch dev engine (version 4.4.6.6_master). The patch adding business logic around disk volumeType and volumeFormat pulled from the webadmin code. With the patch applied, the creations from web-ui work as expected.

The specific case:

Non-admin user that has permissions:
- roles on the target cluster: UserRole and VmCreator
- roles on the target storage domain (which is a different storage domain from the template disks): DiskProfileUser and DiskCreator
Template:
- optimized for Server
- 2 disks, both in storage domain A, 1 is Preallocated, 1 is Thin Provisioned
Creations (NOTE: in web-ui changing SD for a template disk should ONLY be allowed for non-admin user who do not have disk create permissions on the template disk's SD):
- no changes (if possible)
- change optimized for to desktop only (if possible)
- change storage domains for disks
- change optimized for to desktop and change storage domains for disks

Results from creating from webadmin with an admin user:

without changes ... the VM disk's have the same allocation policy as the template and live in the same storage domain
Storage Allocation defaults to "Clone", choose a different SD for each disk ... the VM's disks allocation policy shows same as the template but they are in the new storage domain
change optimied for to "Desktop", need to change Storage Allocation to "Clone" to be able to change disk SD, choose a different SD for each disk ... the VM's disks allocation policy shows same as the template but they are in the new storage domain

Hard details

template info:

> curl -k -u"admin@internal:admin" -H"Accept: application/json" -H"Filter:false" \
  "https://engine-dev.ovirt:8443/ovirt-engine/api/templates/?follow=disk_attachments.disk&search=name=TemplateServer" \
  | jq '.template[] | { name: .name, desc: .description, optimizedFor: .type, disks: [.disk_attachments.disk_attachment[].disk | { alias:.alias, format:.format, sparse:.sparse, sd:.storage_domains.storage_domain[0].id  }]|sort_by(.alias) }'

{
  "name": "TemplateServer",
  "desc": "",
  "optimizedFor": "server",
  "disks": [
    {
      "alias": "MSD1_Pre",
      "format": "raw",
      "sparse": "false",
      "sd": "d67501e7-572e-4683-a71e-a11697c98d16"
    },
    {
      "alias": "MSD1_Thin",
      "format": "raw",
      "sparse": "true",
      "sd": "d67501e7-572e-4683-a71e-a11697c98d16"
    }
  ]
}

webadmin created VMs (admin user):

> curl -k -u"admin@internal:password" -H"Accept: application/json" -H"Filter:false" \
  "https://engine-dev.ovirt:8443/ovirt-engine/api/vms/?follow=disk_attachments.disk,template,original_template&search=name=TS*%20sortby%20name%20asc" \
  | jq '.vm[] | { name: .name, desc: .description, optimizedFor: .type, template: .template.name, originalTemplate: .original_template.name, disks: [.disk_attachments.disk_attachment[].disk | { alias:.alias, format:.format, sparse:.sparse, sd:.storage_domains.storage_domain[0].id }]|sort_by(.alias) }'

{
  "name": "TS-webadmin-A",
  "desc": "no changes",
  "optimizedFor": "server",
  "template": "Blank",
  "originalTemplate": "TemplateServer",
  "disks": [
    {
      "alias": "MSD1_Pre",
      "format": "raw",
      "sparse": "false",
      "sd": "d67501e7-572e-4683-a71e-a11697c98d16"
    },
    {
      "alias": "MSD1_Thin",
      "format": "raw",
      "sparse": "true",
      "sd": "d67501e7-572e-4683-a71e-a11697c98d16"
    }
  ]
}
{
  "name": "TS-webadmin-B",
  "desc": "optimized for desktop",
  "optimizedFor": "desktop",
  "template": "TemplateServer",
  "originalTemplate": "TemplateServer",
  "disks": [
    {
      "alias": "MSD1_Pre",
      "format": "cow",
      "sparse": "true",
      "sd": "d67501e7-572e-4683-a71e-a11697c98d16"
    },
    {
      "alias": "MSD1_Thin",
      "format": "cow",
      "sparse": "true",
      "sd": "d67501e7-572e-4683-a71e-a11697c98d16"
    }
  ]
}
{
  "name": "TS-webadmin-diskSD-1",
  "desc": "change disk storage domains",
  "optimizedFor": "server",
  "template": "Blank",
  "originalTemplate": "TemplateServer",
  "disks": [
    {
      "alias": "MSD1_Pre",
      "format": "raw",
      "sparse": "false",
      "sd": "96f05ec8-6bea-4ed2-b59c-965e583e5428"
    },
    {
      "alias": "MSD1_Thin",
      "format": "raw",
      "sparse": "true",
      "sd": "96f05ec8-6bea-4ed2-b59c-965e583e5428"
    }
  ]
}
{
  "name": "TS-webadmin-diskSD-2",
  "desc": "optimized for desktop, change disk storage domains (required changing storage allocation to clone)",
  "optimizedFor": "desktop",
  "template": "Blank",
  "originalTemplate": "TemplateServer",
  "disks": [
    {
      "alias": "MSD1_Pre",
      "format": "cow",
      "sparse": "true",
      "sd": "96f05ec8-6bea-4ed2-b59c-965e583e5428"
    },
    {
      "alias": "MSD1_Thin",
      "format": "cow",
      "sparse": "true",
      "sd": "96f05ec8-6bea-4ed2-b59c-965e583e5428"
    }
  ]
}

web-ui created VMs (admin user):

with no changes
change optimized for to Desktop

> curl -k -u"admin@internal:password" -H"Accept: application/json" -H"Filter:false" \
  "https://engine-dev.ovirt:8443/ovirt-engine/api/vms/?follow=disk_attachments.disk,template,original_template&search=name=TS-vmportalA*%20sortby%20name%20asc" \
  | jq '.vm[] | { name: .name, desc: .description, optimizedFor: .type, template: .template.name, originalTemplate: .original_template.name, disks: [.disk_attachments.disk_attachment[].disk | { alias:.alias, format:.format, sparse:.sparse, sd:.storage_domains.storage_domain[0].id }]|sort_by(.alias) }'

{
  "name": "TS-vmportalA-1",
  "desc": "no changes",
  "optimizedFor": "server",
  "template": "Blank",
  "originalTemplate": "TemplateServer",
  "disks": [
    {
      "alias": "MSD1_Pre",
      "format": "raw",
      "sparse": "false",
      "sd": "d67501e7-572e-4683-a71e-a11697c98d16"
    },
    {
      "alias": "MSD1_Thin",
      "format": "raw",
      "sparse": "true",
      "sd": "d67501e7-572e-4683-a71e-a11697c98d16"
    }
  ]
}
{
  "name": "TS-vmportalA-2",
  "desc": "optimized for Desktop",
  "optimizedFor": "desktop",
  "template": "TemplateServer",
  "originalTemplate": "TemplateServer",
  "disks": [
    {
      "alias": "MSD1_Pre",
      "format": "cow",
      "sparse": "true",
      "sd": "d67501e7-572e-4683-a71e-a11697c98d16"
    },
    {
      "alias": "MSD1_Thin",
      "format": "cow",
      "sparse": "true",
      "sd": "d67501e7-572e-4683-a71e-a11697c98d16"
    }
  ]
}

web-ui created VMs (non-admin user with permissions defined above):

change storage domains for disks
change optimized for to desktop and change storage domains for disks

> curl -k -u"admin@internal:admin" -H"Accept: application/json" -H"Filter:false" \
  "https://engine-dev.ovirt:8443/ovirt-engine/api/vms/?follow=disk_attachments.disk,template,original_template&search=name=TS-vmportalU*%20sortby%20name%20asc" \
  | jq '.vm[] | { name: .name, desc: .description, optimizedFor: .type, template: .template.name, originalTemplate: .original_template.name, disks: [.disk_attachments.disk_attachment[].disk | { alias:.alias, format:.format, sparse:.sparse, sd:.storage_domains.storage_domain[0].id }]|sort_by(.alias) }'

{
  "name": "TS-vmportalU-1",
  "desc": "no changes, forced to choose new disk SDs",
  "optimizedFor": "server",
  "template": "Blank",
  "originalTemplate": "TemplateServer",
  "disks": [
    {
      "alias": "MSD1_Pre",
      "format": "raw",
      "sparse": "false",
      "sd": "96f05ec8-6bea-4ed2-b59c-965e583e5428"
    },
    {
      "alias": "MSD1_Thin",
      "format": "raw",
      "sparse": "true",
      "sd": "96f05ec8-6bea-4ed2-b59c-965e583e5428"
    }
  ]
}
{
  "name": "TS-vmportalU-2",
  "desc": "optimized for Desktop, forced to choose new disk SDs",
  "optimizedFor": "desktop",
  "template": "Blank",
  "originalTemplate": "TemplateServer",
  "disks": [
    {
      "alias": "MSD1_Pre",
      "format": "cow",
      "sparse": "true",
      "sd": "96f05ec8-6bea-4ed2-b59c-965e583e5428"
    },
    {
      "alias": "MSD1_Thin",
      "format": "cow",
      "sparse": "true",
      "sd": "96f05ec8-6bea-4ed2-b59c-965e583e5428"
    }
  ]
}

sjd78 · 2021-04-23T19:38:02Z

NOTE: Without patch 114092, the non-admin user test 2 fails with the error:

CREATE_VM failed [Cannot add VM. Disk configuration (COW Preallocated backup-None) is incompatible with the storage domain type.]

sgratch · 2021-04-27T12:04:21Z

If VM's optimized for selection is Desktop:
* behavior matches when Admin Portal **Storage Allocation** is **Thin**

* all of the template disks' disk types are forced to "Thin Provisioned"

* the VM create is sent:
  
  * `clone=true` if AT LEAST ONE of the disks has a storage domain that differs from the template's disk
This clone=true scenario is not working as expected (comparing to webadmin) with the following use case:

Create a template "temp" with a disk type set to Raw == "Preallocated

Create a VM via web-ui optimized for "Desktop" and based on template "temp" while changing the target SD since there is no permission for creating the template's disk on original template's SD:

Expected behaviour: the template's disk will be cloned to new targeted SD with disk type set to Thin, regardless to original storage allocation.
Actual behaviour: the template's disk is cloned to new targeted SD with disk type policy set to Preallocated
I was forcing the disk format to 'raw' when changing the storage domain. Looks like that isn't necessary.
This is still not working for me, I'm getting the following error:

I think that the problem is that the preallocated/raw disk is treated by web-ui as preallocated/cow while it should be converted to thin/cow(default on webadmin) or preallocated/raw.

@sjd78

I verified and can confirm that patch 114092 solves this mentioned issue.

I found another issue that when creating a VM optimized to "Server" and based on a template with disk's format set to QCOW2 (for both clone/not clone senarios):
Actual result is: the disk's format are created with Raw format.
Expected result is: the disk's format should be created with QCOW2 (same as the original template's disk).
This issue is not solved by patch 114092 so I suggest openning a dedicated issue for this and merge this one.

sgratch

LGTM except 2 found issues:

will be solved by patch 114092
A separate issue will be opened for it: Create VM Wizard- disk type of new disks based on a 'server/HP' template should be based on the template's disk format #1416

I prefer to merge this fix since it did fix part of the already known issues.

isaranova · 2021-06-01T11:51:00Z

LGTM

sjd78 requested review from hstastna, sgratch, bamsalem and rszwajko April 1, 2021 21:17

rszwajko reviewed Apr 8, 2021

View reviewed changes

sjd78 force-pushed the cretate-template-disks branch from aea1a37 to d4bd67a Compare April 8, 2021 18:48

sjd78 force-pushed the cretate-template-disks branch from d4bd67a to 765b37c Compare April 9, 2021 02:55

sjd78 requested a review from rszwajko April 9, 2021 02:55

hstastna approved these changes Apr 9, 2021

View reviewed changes

sjd78 mentioned this pull request Apr 9, 2021

[RFE] For create vm from a template, expose clone parameter in the summary view #1402

Open

rszwajko approved these changes Apr 9, 2021

View reviewed changes

bamsalem approved these changes Apr 12, 2021

View reviewed changes

sjd78 added the Status: ON_QA status ON_QA (currently being tested) label Apr 17, 2021

sjd78 mentioned this pull request Apr 20, 2021

Create VM Wizard- create disks storage domain issues #1366

Closed

sgratch removed the Status: ON_QA status ON_QA (currently being tested) label Apr 22, 2021

sjd78 force-pushed the cretate-template-disks branch from 765b37c to 3089abd Compare April 22, 2021 20:09

Don't force disks to raw when changing storage domains

6b87550

When creating a disk from a template, it is not necessary, and sometime wrong, to force a disk's format to 'raw' when changing/cloning to a different storage domain.

sjd78 self-assigned this Apr 23, 2021

sjd78 mentioned this pull request Apr 23, 2021

Create VM Wizard- disk type of new disks based on a 'server/HP' template should be based on the template's disk allocation policy #1371

Closed

sgratch approved these changes Apr 27, 2021

View reviewed changes

sgratch merged commit e9f3403 into oVirt:master Apr 27, 2021

sgratch mentioned this pull request Apr 27, 2021

Create VM Wizard- disk type of new disks based on a 'server/HP' template should be based on the template's disk format #1416

Closed

sjd78 deleted the cretate-template-disks branch June 4, 2021 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update how template disks are created #1400

Update how template disks are created #1400

sjd78 commented Apr 1, 2021 •

edited

Loading

rszwajko left a comment

rszwajko Apr 8, 2021

sjd78 Apr 8, 2021

sjd78 commented Apr 8, 2021 •

edited

Loading

rszwajko commented Apr 9, 2021

hstastna left a comment

sjd78 commented Apr 9, 2021

bamsalem left a comment

sgratch commented Apr 22, 2021

sjd78 commented Apr 22, 2021

sgratch commented Apr 23, 2021

sjd78 commented Apr 23, 2021 •

edited

Loading

sjd78 commented Apr 23, 2021

sgratch commented Apr 27, 2021 •

edited

Loading

sgratch left a comment •

edited

Loading

isaranova commented Jun 1, 2021

Update how template disks are created #1400

Update how template disks are created #1400

Conversation

sjd78 commented Apr 1, 2021 • edited Loading

rszwajko left a comment

Choose a reason for hiding this comment

rszwajko Apr 8, 2021

Choose a reason for hiding this comment

sjd78 Apr 8, 2021

Choose a reason for hiding this comment

sjd78 commented Apr 8, 2021 • edited Loading

rszwajko commented Apr 9, 2021

hstastna left a comment

Choose a reason for hiding this comment

sjd78 commented Apr 9, 2021

bamsalem left a comment

Choose a reason for hiding this comment

sgratch commented Apr 22, 2021

sjd78 commented Apr 22, 2021

sgratch commented Apr 23, 2021

sjd78 commented Apr 23, 2021 • edited Loading

The specific case:

Results from creating from webadmin with an admin user:

Hard details

sjd78 commented Apr 23, 2021

sgratch commented Apr 27, 2021 • edited Loading

sgratch left a comment • edited Loading

Choose a reason for hiding this comment

isaranova commented Jun 1, 2021

sjd78 commented Apr 1, 2021 •

edited

Loading

sjd78 commented Apr 8, 2021 •

edited

Loading

sjd78 commented Apr 23, 2021 •

edited

Loading

sgratch commented Apr 27, 2021 •

edited

Loading

sgratch left a comment •

edited

Loading