Replace Hashtable with LinkedHashMap in createIsoResponse by CharlesQueiroz · Pull Request #7844 · apache/cloudstack

CharlesQueiroz · 2023-08-10T11:26:34Z

Description

This change replaces Hashtable with LinkedHashMap in the createIsoResponse method of ViewResponseHelper.
This modification is to maintain the insertion order of entries, which isn't the case with Hashtable.

This solves the problem described here:

#7278

Related to the ordering of the ISOs table in CloudStack.
Currently, the order is not maintained because, when using a Hashtable, the insertion order gets lost when it tries to create the list on the API side.

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)

Bug Severity

This change replaces the use of Hashtable with LinkedHashMap in the `createIsoResponse` method of `ViewResponseHelper`. The reason for this modification is to maintain the insertion order of entries, which isn't the case with Hashtable. This could lead to more predictable results and behaviors in calling methods.

weizhouapache · 2023-08-10T12:25:18Z

thanks @CharlesQueiroz for the fix

Hashtable is used many times in same class. can you fix the others as well ?

@kiranchavala can you please verify it ?

kiranchavala · 2023-08-10T12:35:05Z

@blueorangutan package

blueorangutan · 2023-08-10T12:36:04Z

@kiranchavala a [SF] 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.

DaanHoogland

clgtm

blueorangutan · 2023-08-10T13:31:24Z

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

weizhouapache · 2023-08-10T13:33:05Z

thanks @CharlesQueiroz for the update
it looks there is still one in the class

Hashtable<Long, UserVmResponse> vmDataList = new Hashtable<Long, UserVmResponse>();

Changed Hashtable to LinkedHashMap in various response creation methods within ViewResponseHelper class. This modification ensures an ordered iteration which is beneficial for scenarios where the insertion order of responses needs to be maintained consistently.

codecov · 2023-08-10T14:17:47Z

Codecov Report

Merging #7844 (6a14be0) into 4.18 (19aacad) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               4.18    #7844      +/-   ##
============================================
- Coverage     13.02%   13.02%   -0.01%     
+ Complexity     9040     9039       -1     
============================================
  Files          2720     2720              
  Lines        257094   257094              
  Branches      40092    40092              
============================================
- Hits          33491    33489       -2     
- Misses       219398   219401       +3     
+ Partials       4205     4204       -1

Files Changed	Coverage Δ
...n/java/com/cloud/api/query/ViewResponseHelper.java	`0.00% <0.00%> (ø)`

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

weizhouapache · 2023-08-10T14:26:08Z

@blueorangutan package

blueorangutan · 2023-08-10T14:28:03Z

@weizhouapache a [SF] 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-08-10T15:20:24Z

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

kiranchavala

LGTM , Tested manually, the order of the ISO is maintained, able to move to the top/bottom and move one row up/down

weizhouapache

code lgtm

weizhouapache · 2023-08-11T06:37:55Z

@blueorangutan test rocky8 kvm-rocky8

blueorangutan · 2023-08-11T06:40:03Z

@weizhouapache a [SF] Trillian-Jenkins test job (rocky8 mgmt + kvm-rocky8) has been kicked to run smoke tests

yadvr

LGTM, didn't test it myself but changes and description makes sense.
I'm not sure if manual testing is needed, but any integration/marvin test would be great. Also I'm not sure of any consequences/side effects of this.

blueorangutan · 2023-08-11T21:59:34Z

[SF] Trillian test result (tid-7374)
Environment: kvm-rocky8 (x2), Advanced Networking with Mgmt server r8
Total time taken: 53134 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7844-t7374-kvm-rocky8.zip
Smoke tests completed. 106 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_list_vms_metrics_admin	`Error`	3607.97	test_metrics_api.py
test_list_vms_metrics_history	`Error`	4.57	test_metrics_api.py
test_list_volumes_metrics_history	`Error`	5.50	test_metrics_api.py
test_01_deploy_vm_on_specific_host	`Error`	4.39	test_vm_deployment_planner.py
test_02_deploy_vm_on_specific_cluster	`Error`	4.36	test_vm_deployment_planner.py
test_03_deploy_vm_on_specific_pod	`Error`	4.41	test_vm_deployment_planner.py
test_04_deploy_vm_on_host_override_pod_and_cluster	`Error`	4.43	test_vm_deployment_planner.py
test_05_deploy_vm_on_cluster_override_pod	`Error`	4.42	test_vm_deployment_planner.py

DaanHoogland · 2023-08-14T06:40:04Z

@blueorangutan test matrix

blueorangutan · 2023-08-14T06:42:03Z

@DaanHoogland a [SF] Trillian-Jenkins matrix job (centos7 mgmt + xenserver71, rocky8 mgmt + vmware67u3, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

blueorangutan · 2023-08-14T18:33:27Z

[SF] Trillian test result (tid-7391)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41247 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7844-t7391-xenserver-71.zip
Smoke tests completed. 106 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestUnmanageVM>:setup	`Error`	0.00	test_vm_lifecycle_unmanage_import.py
ContextSuite context=TestChangeServiceOfferingForVmWithSnapshots>:setup	`Error`	0.00	test_vm_snapshots.py

blueorangutan · 2023-08-14T19:58:19Z

[SF] Trillian test result (tid-7393)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 46159 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7844-t7393-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_01_migrate_VM_and_root_volume	`Error`	79.81	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	`Error`	51.43	test_vm_life_cycle.py

blueorangutan · 2023-08-14T23:04:32Z

[SF] Trillian test result (tid-7392)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server r8
Total time taken: 57452 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7844-t7392-vmware-67u3.zip
Smoke tests completed. 105 look OK, 3 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestVMWareStoragePolicies>:setup	`Error`	0.00	test_storage_policy.py
test_01_deploy_vm_on_specific_host	`Error`	13.66	test_vm_deployment_planner.py
test_02_deploy_vm_on_specific_cluster	`Error`	3601.88	test_vm_deployment_planner.py
test_03_deploy_vm_on_specific_pod	`Error`	1.37	test_vm_deployment_planner.py
test_04_deploy_vm_on_host_override_pod_and_cluster	`Error`	2.37	test_vm_deployment_planner.py
test_05_deploy_vm_on_cluster_override_pod	`Error`	2.33	test_vm_deployment_planner.py
test_09_expunge_vm	`Failure`	425.71	test_vm_life_cycle.py

weizhouapache · 2023-08-15T06:48:01Z

@blueorangutan test matrix

blueorangutan · 2023-08-15T06:50:03Z

@weizhouapache a [SF] Trillian-Jenkins matrix job (centos7 mgmt + xenserver71, rocky8 mgmt + vmware67u3, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

github-actions · 2023-08-15T13:42:45Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

blueorangutan · 2023-08-15T19:11:44Z

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

Test	Result	Time (s)	Test File

blueorangutan · 2023-08-15T19:59:46Z

[SF] Trillian test result (tid-7412)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 45766 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7844-t7412-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_01_migrate_VM_and_root_volume	`Error`	79.70	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	`Error`	54.43	test_vm_life_cycle.py

soreana · 2023-08-15T20:09:28Z

@blueorangutan package

blueorangutan · 2023-08-15T20:10:03Z

@soreana a [SF] 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-08-15T21:04:55Z

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

blueorangutan · 2023-08-16T01:50:08Z

[SF] Trillian test result (tid-7411)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server r8
Total time taken: 66847 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7844-t7411-vmware-67u3.zip
Smoke tests completed. 104 look OK, 4 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_02_upgrade_kubernetes_cluster	`Failure`	141.54	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	`Failure`	184.60	test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster	`Failure`	0.07	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	0.05	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	526.93	test_kubernetes_clusters.py
test_01_deploy_vm_on_specific_host	`Error`	15.68	test_vm_deployment_planner.py
test_02_deploy_vm_on_specific_cluster	`Error`	3603.26	test_vm_deployment_planner.py
test_03_deploy_vm_on_specific_pod	`Error`	28.01	test_vm_deployment_planner.py
test_04_deploy_vm_on_host_override_pod_and_cluster	`Error`	3602.64	test_vm_deployment_planner.py
test_05_deploy_vm_on_cluster_override_pod	`Error`	1.32	test_vm_deployment_planner.py
test_09_expunge_vm	`Failure`	424.68	test_vm_life_cycle.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Failure`	145.55	test_vpc_redundant.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Error`	145.56	test_vpc_redundant.py
test_02_redundant_VPC_default_routes	`Error`	429.73	test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	343.97	test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Error`	343.99	test_vpc_redundant.py

boring-cyborg · 2023-08-16T06:58:44Z

Awesome work, congrats on your first merged pull request!

CharlesQueiroz mentioned this pull request Aug 10, 2023

Unable to order the ISO/templates in the UI #7278

Closed

soreana added this to the 4.18.1.0 milestone Aug 10, 2023

soreana added component:api Severity:Minor labels Aug 10, 2023

kiranchavala self-assigned this Aug 10, 2023

DaanHoogland approved these changes Aug 10, 2023

View reviewed changes

CharlesQueiroz force-pushed the isoorderfix branch from 2e4f724 to 5400927 Compare August 10, 2023 13:54

kiranchavala approved these changes Aug 11, 2023

View reviewed changes

weizhouapache approved these changes Aug 11, 2023

View reviewed changes

weizhouapache added the status:ready-for-merge label Aug 11, 2023

weizhouapache linked an issue Aug 11, 2023 that may be closed by this pull request

Unable to order the ISO/templates in the UI #7278

Closed

yadvr approved these changes Aug 11, 2023

View reviewed changes

github-actions Bot added the status:has-conflicts label Aug 15, 2023

Merge branch '4.18' into isoorderfix

6a14be0

github-actions Bot removed the status:has-conflicts label Aug 15, 2023

weizhouapache merged commit e8b4911 into apache:4.18 Aug 16, 2023

DaanHoogland removed the status:ready-for-merge label Dec 4, 2024

Uh oh!

Conversation

CharlesQueiroz commented Aug 10, 2023

Description

Types of changes

Bug Severity

Uh oh!

weizhouapache commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiranchavala commented Aug 10, 2023

Uh oh!

blueorangutan commented Aug 10, 2023

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Aug 10, 2023

Uh oh!

weizhouapache commented Aug 10, 2023

Uh oh!

codecov Bot commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

weizhouapache commented Aug 10, 2023

Uh oh!

blueorangutan commented Aug 10, 2023

Uh oh!

blueorangutan commented Aug 10, 2023

Uh oh!

kiranchavala left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Aug 11, 2023

Uh oh!

blueorangutan commented Aug 11, 2023

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Aug 11, 2023

Uh oh!

DaanHoogland commented Aug 14, 2023

Uh oh!

blueorangutan commented Aug 14, 2023

Uh oh!

blueorangutan commented Aug 14, 2023

Uh oh!

blueorangutan commented Aug 14, 2023

Uh oh!

blueorangutan commented Aug 14, 2023

Uh oh!

weizhouapache commented Aug 15, 2023

Uh oh!

blueorangutan commented Aug 15, 2023

Uh oh!

github-actions Bot commented Aug 15, 2023

Uh oh!

blueorangutan commented Aug 15, 2023

Uh oh!

blueorangutan commented Aug 15, 2023

Uh oh!

soreana commented Aug 15, 2023

Uh oh!

blueorangutan commented Aug 15, 2023

Uh oh!

blueorangutan commented Aug 15, 2023

Uh oh!

blueorangutan commented Aug 16, 2023

Uh oh!

boring-cyborg Bot commented Aug 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

weizhouapache commented Aug 10, 2023 •

edited

Loading

codecov Bot commented Aug 10, 2023 •

edited

Loading