server: fix diskoffering details in vm response by shwstppr · Pull Request #8135 · apache/cloudstack

shwstppr · 2023-10-24T13:40:35Z

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

Screenshots (if appropriate):

How Has This Been Tested?

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

Fixesapache#8120 Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2023-10-24T13:42:36Z

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 · 2023-10-24T14:33:59Z

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 · 2023-10-24T14:52:38Z

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 · 2023-10-24T14:58:21Z

@blueorangutan package

blueorangutan · 2023-10-24T15:02:04Z

@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 · 2023-10-24T16:02:49Z

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

yadvr

LGTM - didn't test it

shwstppr · 2023-10-26T05:13:50Z

@blueorangutan test

blueorangutan · 2023-10-26T05:14:07Z

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

blueorangutan · 2023-10-26T16:58:54Z

[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 · 2023-11-01T10:01:22Z

@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 · 2023-11-02T07:25:11Z

What if there are multiple data disks ?

harikrishna-patnala · 2023-11-03T06:41:16Z

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 · 2023-11-03T07:42:28Z

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 · 2023-11-08T13:42:59Z

@shwstppr is this not ready for review yet?

DaanHoogland · 2023-11-09T15:38:10Z

@blueorangutan package

blueorangutan · 2023-11-09T15:50:03Z

@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 · 2023-11-09T16:52:00Z

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

DaanHoogland · 2023-11-09T16:59:53Z

@blueorangutan test keepEnv

blueorangutan · 2023-11-09T17:02:04Z

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

shwstppr · 2023-11-09T18:31:06Z

@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 · 2023-11-10T07:39:15Z

[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 · 2023-11-10T08:08:59Z

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 · 2023-11-29T09:55:44Z

@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 · 2023-11-29T10:03:31Z

@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 · 2023-11-29T12:54:12Z

@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 · 2023-11-29T14:03:40Z

@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 · 2023-11-29T14:33:42Z

@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 · 2023-11-29T14:49:18Z

@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 · 2023-11-29T15:21:14Z

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 · 2023-11-29T15:40:58Z

@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 · 2023-12-05T21:56:42Z

@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 · 2023-12-06T08:39:42Z

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

agreed 🤝

…fering

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2023-12-07T13:01:02Z

@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 · 2023-12-07T13:01:15Z

@blueorangutan package

blueorangutan · 2023-12-07T13:02:03Z

@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 · 2023-12-07T14:04:32Z

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

shwstppr · 2023-12-08T06:11:42Z

@blueorangutan test

blueorangutan · 2023-12-08T06:12:03Z

@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 · 2023-12-08T18:23:30Z

[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

Fixesapache#8120

server: fix diskoffering details in vm response
67f3b9f
Fixesapache#8120 Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

yadvr approved these changes Oct 25, 2023
View reviewed changes

yadvr added this to the 4.18.2.0 milestone Oct 25, 2023

yadvr requested review from DaanHoogland, harikrishna-patnala and weizhouapache October 27, 2023 07:32

DaanHoogland approved these changes Oct 27, 2023
View reviewed changes

harikrishna-patnala approved these changes Nov 2, 2023
View reviewed changes

DaanHoogland self-assigned this Nov 9, 2023

DaanHoogland linked an issue Nov 15, 2023 that may be closed by this pull request
ACS 4.18 diskofferingid missing from deployVirtualMachine response #8120
Closed

weizhouapache reviewed Nov 29, 2023
View reviewed changes

Comment threadserver/src/main/java/com/cloud/api/query/dao/UserVmJoinDaoImpl.java

shwstppr added 2 commits December 7, 2023 18:20

Merge remote-tracking branch 'apache/4.18' into fix-vmresponse-diskof…
b0a56a6
…fering

update param description
a519e0c
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

boring-cyborgBot added the component:api label Dec 7, 2023

weizhouapache approved these changes Dec 8, 2023
View reviewed changes

shwstppr marked this pull request as ready for review December 9, 2023 06:13

shwstppr merged commit a11fc43 into apache:4.18Dec 9, 2023

shwstppr deleted the fix-vmresponse-diskoffering branch December 9, 2023 06:14

shwstppr mentioned this pull request Dec 9, 2023
ACS 4.18 diskofferingid missing from deployVirtualMachine response #8120
Closed

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Dec 11, 2023
server: fix diskoffering details in vm response (apache#8135)
23f536a
Fixesapache#8120

DaanHoogland removed their assignment Dec 24, 2025

Uh oh!

Conversation

shwstppr commented Oct 24, 2023

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

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

Uh oh!

shwstppr commented Oct 24, 2023

Uh oh!

weizhouapache commented Oct 24, 2023

Uh oh!

codecovBot commented Oct 24, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shwstppr commented Oct 24, 2023

Uh oh!

blueorangutan commented Oct 24, 2023

Uh oh!

blueorangutan commented Oct 24, 2023

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

shwstppr commented Oct 26, 2023

Uh oh!

blueorangutan commented Oct 26, 2023

Uh oh!

blueorangutan commented Oct 26, 2023

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

shwstppr commented Nov 1, 2023

Uh oh!

harikrishna-patnala left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Nov 2, 2023

Uh oh!

harikrishna-patnala commented Nov 3, 2023

Uh oh!

weizhouapache commented Nov 3, 2023

Uh oh!

DaanHoogland commented Nov 8, 2023

Uh oh!

DaanHoogland commented Nov 9, 2023

Uh oh!

blueorangutan commented Nov 9, 2023

Uh oh!

blueorangutan commented Nov 9, 2023

Uh oh!

DaanHoogland commented Nov 9, 2023

Uh oh!

blueorangutan commented Nov 9, 2023

Uh oh!

shwstppr commented Nov 9, 2023

Uh oh!

blueorangutan commented Nov 10, 2023

Uh oh!

DaanHoogland commented Nov 10, 2023

Uh oh!

DaanHoogland commented Nov 29, 2023

Uh oh!

weizhouapache commented Nov 29, 2023

Uh oh!

DaanHoogland commented Nov 29, 2023

Uh oh!

weizhouapache commented Nov 29, 2023

Uh oh!

DaanHoogland commented Nov 29, 2023

Uh oh!

shwstppr commented Nov 29, 2023

Uh oh!

weizhouapache commented Nov 29, 2023

codecovBot commented Oct 24, 2023•
edited
Loading