server: fix diskoffering details in vm response

[shwstppr]

Description

Fixes #8120

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

[shwstppr]

Not sure if we should return DATA disk offering details after #5008 but this should allow fixing the issue as we have multiple entries in user_vm_view when there is a data disk @harikrishna-patnala

[weizhouapache]

Not sure if we should return DATA disk offering details after #5008 but this should allow fixing the issue as we have multiple entries in user_vm_view when there is a data disk @harikrishna-patnala

I have the same concern

[codecov]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (7ea068c) 13.11% compared to head (a519e0c) 13.12%.

Files	Patch %	Lines
...ava/com/cloud/api/query/dao/UserVmJoinDaoImpl.java	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               4.18    #8135   +/-   ##
=========================================
  Coverage     13.11%   13.12%           
- Complexity     9133     9135    +2     
=========================================
  Files          2720     2720           
  Lines        257659   257662    +3     
  Branches      40171    40172    +1     
=========================================
+ Hits          33802    33807    +5     
+ Misses       219566   219563    -3     
- Partials       4291     4292    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[shwstppr]

@blueorangutan package

[blueorangutan]

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7488

[yadvr]

LGTM - didn't test it

[shwstppr]

@blueorangutan test

[blueorangutan]

@shwstppr a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-8099)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40853 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8135-t8099-kvm-centos7.zip
Smoke tests completed. 107 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_08_arping_in_ssvm	Failure	5.15	test_diagnostics.py

[DaanHoogland]

clgtm

[shwstppr]

@harikrishna-patnala can you please have a look at this?

[harikrishna-patnala]

I've not changed this response in #5008. Not sure why is this missing.
But this PR change looks good to me.

[weizhouapache]

What if there are multiple data disks ?

[harikrishna-patnala]

What if there are multiple data disks ?

for each disk there is a separate entry in user_vm_view. "testVM" has two disks and "test" has 3 disks

[weizhouapache]

What if there are multiple data disks ?

for each disk there is a separate entry in user_vm_view. "testVM" has two disks and "test" has 3 disks

yes @harikrishna-patnala
in this case, the diskofferingid and diskofferingname in the uservm response will be the uuid and name of the first data disk, right ?
is it expected result ?

[DaanHoogland]

@shwstppr is this not ready for review yet?

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7698

[DaanHoogland]

@blueorangutan test keepEnv

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

[shwstppr]

@shwstppr is this not ready for review yet?

I'm not sure about this change. Although we seem to be returning diskofferingid of the first data disk even in the past but I think we can deprecate it or update its param description to say that it will be returned only in particular cases.
I'm okay to mark it ready if we have a consensus that the behaviour of returning first data disk offering id is fine

[blueorangutan]

[SF] Trillian test result (tid-8294)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 51074 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8135-t8294-kvm-centos7.zip
Smoke tests completed. 108 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_invalid_upgrade_kubernetes_cluster	Failure	3616.58	test_kubernetes_clusters.py
test_02_upgrade_kubernetes_cluster	Failure	3606.20	test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster	Failure	0.04	test_kubernetes_clusters.py
test_04_autoscale_kubernetes_cluster	Failure	0.04	test_kubernetes_clusters.py
test_05_basic_lifecycle_kubernetes_cluster	Failure	0.03	test_kubernetes_clusters.py
test_06_delete_kubernetes_cluster	Failure	0.03	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	Failure	0.04	test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster	Failure	0.04	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	Failure	0.04	test_kubernetes_clusters.py
test_10_vpc_tier_kubernetes_cluster	Failure	45.76	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	Error	99.01	test_kubernetes_clusters.py

[DaanHoogland]

personally I think that if we include diskinfo in the VM response it should be an array of all disks.
If we don't have consensus, shall we move this to a newer release?

[DaanHoogland]

@shwstppr we are having second thoughts on how this should be implemented but in the mean while this is a regression to the user as reported in #8120. I think we should test and merge and considder how to improve later.
agree?
cc @harikrishna-patnala @weizhouapache @rohityadavcloud

[weizhouapache]

@shwstppr we are having second thoughts on how this should be implemented but in the mean while this is a regression to the user as reported in #8120. I think we should test and merge and considder how to improve later. agree? cc @harikrishna-patnala @weizhouapache @rohityadavcloud

@DaanHoogland
I would suggest to add all volumes info in the response, and ask users to get diskofferingid/name from volumes info, if they face the issue #8120

[DaanHoogland]

@shwstppr we are having second thoughts on how this should be implemented but in the mean while this is a regression to the user as reported in #8120. I think we should test and merge and considder how to improve later. agree? cc @harikrishna-patnala @weizhouapache @rohityadavcloud

@DaanHoogland I would suggest to add all volumes info in the response, and ask users to get diskofferingid/name from volumes info, if they face the issue #8120

I agree adding all to the volume list, but this is a backwards compatibility issue.

[weizhouapache]

@shwstppr we are having second thoughts on how this should be implemented but in the mean while this is a regression to the user as reported in #8120. I think we should test and merge and considder how to improve later. agree? cc @harikrishna-patnala @weizhouapache @rohityadavcloud

@DaanHoogland I would suggest to add all volumes info in the response, and ask users to get diskofferingid/name from volumes info, if they face the issue #8120

I agree adding all to the volume list, but this is a backwards compatibility issue.

IMHO, we can give more information, rather than giving wrong info (if there are multiple volumes.
) just to keep backwards compatibility

[DaanHoogland]

@shwstppr we are having second thoughts on how this should be implemented but in the mean while this is a regression to the user as reported in #8120. I think we should test and merge and considder how to improve later. agree? cc @harikrishna-patnala @weizhouapache @rohityadavcloud

@DaanHoogland I would suggest to add all volumes info in the response, and ask users to get diskofferingid/name from volumes info, if they face the issue #8120

I agree adding all to the volume list, but this is a backwards compatibility issue.

IMHO, we can give more information, rather than giving wrong info (if there are multiple volumes. ) just to keep backwards compatibility

I dissagree that the offering id and offering name of the root volume are wrong information.

In addition after analysing the DB and code I think it is not good to add the other information of the volumes to the VM response. There is a possibility to call list volumes virtualmachineid=<uuid> separately. If any bug needs solving it is that this one returns an empty offering and Id if you didn't choose one for the root volume.

[shwstppr]

@DaanHoogland @weizhouapache I suggest we go ahead with this PR for 4.18 branch and then in main create a PR,

• updating the params description that they are returned only when VM deployed from ISO or any other valid case.
• use listVirtualMachines API details param to return volume details

[weizhouapache]

I dissagree that the offering id and offering name of the root volume are wrong information.

@DaanHoogland
the diskofferingid and diskofferingname are the info of DATA disk, not ROOT disk.

[weizhouapache]

@DaanHoogland @weizhouapache I suggest we go ahead with this PR for 4.18 branch and then in main create a PR,

• updating the params description that they are returned only when VM deployed from ISO or any other valid case.
• use listVirtualMachines API details param to return volume details

@shwstppr
I agree with the changes you mentioned.
AFAIK, diskofferingid and diskofferingname are the info of ROOT disk in 4.16.x. However, disk_offering_id has been removed from vm_instance table in #5008 , which leads to a bit complicated to set the value, but should be feasible.

that's say, this PR needs to be updated.

[weizhouapache]

@weizhouapache for ROOT disk and ISO we are already setting the diskoffering. This PR change will only affect with VM deployed from templates having multiple disks. In older version we were returning DATA disk's offering as disk offering

(localcloud) 🐱 > list managementservers 
{
  "count": 1,
  "managementserver": [
    {
      "id": "8d7b1557-cf29-44b6-8d35-876d1959d7f4",
      "name": "ghost",
      "state": "Up",
      "version": "4.16.0.0"
    }
  ]
}
(localcloud) 🐱 > list volumes virtualmachineid=8f411a8b-4fb5-4b23-9909-f6c9ccc62d0a filter=id,name,diskofferingid,type,
{
  "count": 2,
  "volume": [
    {
      "diskofferingid": "f32c64b7-22bd-4057-9456-e15773986455",
      "id": "8414beca-a378-48c5-bbaf-42bdbcff5dc7",
      "name": "DATA-3",
      "type": "DATADISK"
    },
    {
      "id": "14a335c2-ad48-4d33-abfd-ec9b45e9be20",
      "name": "ROOT-3",
      "type": "ROOT"
    }
  ]
}
(localcloud) 🐱 > list virtualmachines id=8f411a8b-4fb5-4b23-9909-f6c9ccc62d0a filter=id,name,diskofferingid,diskofferingname,
{
  "count": 1,
  "virtualmachine": [
    {
      "diskofferingid": "f32c64b7-22bd-4057-9456-e15773986455",
      "diskofferingname": "Medium",
      "id": "8f411a8b-4fb5-4b23-9909-f6c9ccc62d0a",
      "name": "VM-8f411a8b-4fb5-4b23-9909-f6c9ccc62d0a"
    }
  ]
}
(localcloud) 🐱 > list virtualmachines id=8f411a8b-4fb5-4b23-9909-f6c9ccc62d0a filter=id,name,diskofferingid,diskofferingname,templateid,templatename,
{
  "count": 1,
  "virtualmachine": [
    {
      "diskofferingid": "f32c64b7-22bd-4057-9456-e15773986455",
      "diskofferingname": "Medium",
      "id": "8f411a8b-4fb5-4b23-9909-f6c9ccc62d0a",
      "name": "VM-8f411a8b-4fb5-4b23-9909-f6c9ccc62d0a",
      "templateid": "cb8d1d7a-9370-11ee-9baf-9843fa67404e",
      "templatename": "CentOS 5.6 (64-bit) no GUI (Simulator)"
    }
  ]
}

@shwstppr
thanks
Let's move forward with your suggestion in #8135 (comment)

@DaanHoogland what do you think ?

[DaanHoogland]

@shwstppr thanks Let's move forward with your suggestion in #8135 (comment)

agreed 🤝

[shwstppr]

@weizhouapache @DaanHoogland I've updated the params description in UserVmResponse. And I've drafted a PR (needs more changes) to return volumes list in listVitualMachines API response, #8330. We can discuss it there if not needed or shouldn't be done

[shwstppr]

@blueorangutan package

[blueorangutan]

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7971

[shwstppr]

@blueorangutan test

[blueorangutan]

@shwstppr a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

[weizhouapache]

code lgtm
not tested it

[blueorangutan]

[SF] Trillian test result (tid-8513)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42330 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8135-t8513-kvm-centos7.zip
Smoke tests completed. 109 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File