server: use id column as secondary sort criteria with sortKey by shwstppr · Pull Request #5320 · apache/cloudstack

shwstppr · 2021-08-16T13:36:39Z

Description

For tables that use sort_key field, incorrect results can be returned by the list* APIs while using pagination and with the same values for sort-key field. (Default value for sort_key field remains 0).
This change adds id column as secondary sort criteria in such cases.

Fixes#5316

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Created 1000 disk offerings
Listed 500 disk offerings ids using cmk at a time for 2 pages
Verified if there are any duplicates
No duplicates found

diskoffering-fix.log

shwstppr · 2021-08-16T13:45:04Z

@blueorangutan package

blueorangutan · 2021-08-16T13:46:03Z

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-08-16T14:09:12Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 884

nvazquez · 2021-08-16T14:54:17Z

@blueorangutan test

blueorangutan · 2021-08-16T14:55:03Z

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

shwstppr · 2021-08-17T05:28:04Z

@blueorangutan test

blueorangutan · 2021-08-17T05:29:02Z

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

borisstoyanov

LGTM,

I've manually checked with ~1600 records and page size of 450, divided in 4 pages I could not get duplicates.

[root@pr5320-t1674-kvm-centos7-mgmt1 ~]# grep -x -i -f page1 page4 | wc
0 0 0
[root@pr5320-t1674-kvm-centos7-mgmt1 ~]# grep -x -i -f page2 page4 | wc
0 0 0
[root@pr5320-t1674-kvm-centos7-mgmt1 ~]# grep -x -i -f page3 page4 | wc
0 0 0
[root@pr5320-t1674-kvm-centos7-mgmt1 ~]# grep -x -i -f page1 page3 | wc
0 0 0
[root@pr5320-t1674-kvm-centos7-mgmt1 ~]# grep -x -i -f page1 page2 | wc
0 0 0
[root@pr5320-t1674-kvm-centos7-mgmt1 ~]# grep -x -i -f page3 page2 | wc
0 0 0

Lets wait for results from smoketests to appear.

blueorangutan · 2021-08-17T11:01:54Z

Trillian test result (tid-1671)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 65094 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5320-t1671-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_backup_recovery_dummy.py
Intermittent failure detected: /marvin/tests/smoke/test_global_settings.py
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_supported_versions.py
Intermittent failure detected: /marvin/tests/smoke/test_network.py
Intermittent failure detected: /marvin/tests/smoke/test_outofbandmanagement.py
Intermittent failure detected: /marvin/tests/smoke/test_projects.py
Intermittent failure detected: /marvin/tests/smoke/test_public_ip_range.py
Intermittent failure detected: /marvin/tests/smoke/test_reset_vm_on_reboot.py
Intermittent failure detected: /marvin/tests/smoke/test_resource_accounting.py
Intermittent failure detected: /marvin/tests/smoke/test_router_dhcphosts.py
Intermittent failure detected: /marvin/tests/smoke/test_router_dns.py
Intermittent failure detected: /marvin/tests/smoke/test_router_dnsservice.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_iptables_default_policy.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_routers.py
Intermittent failure detected: /marvin/tests/smoke/test_secondary_storage.py
Intermittent failure detected: /marvin/tests/smoke/test_service_offerings.py
Intermittent failure detected: /marvin/tests/smoke/test_snapshots.py
Intermittent failure detected: /marvin/tests/smoke/test_ssvm.py
Intermittent failure detected: /marvin/tests/smoke/test_storage_policy.py
Intermittent failure detected: /marvin/tests/smoke/test_templates.py
Intermittent failure detected: /marvin/tests/smoke/test_usage.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_deployment_planner.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_snapshots.py
Intermittent failure detected: /marvin/tests/smoke/test_volumes.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 57 look OK, 32 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_UpdateConfigParamWithScope	`Error`	0.06	test_global_settings.py
test_03_vpc_internallb_haproxy_stats_on_all_interfaces	`Error`	3.24	test_internal_lb.py
test_04_rvpc_internallb_haproxy_stats_on_all_interfaces	`Error`	1.20	test_internal_lb.py
ContextSuite context=TestDummyBackupAndRecovery>:setup	`Error`	0.00	test_backup_recovery_dummy.py
test_delete_account	`Error`	113.31	test_network.py
ContextSuite context=TestCrossDomainAccountAdd>:setup	`Error`	0.00	test_projects.py
ContextSuite context=TestDeleteAccountWithProject>:setup	`Error`	0.00	test_projects.py
ContextSuite context=TestDeleteDomainWithProject>:setup	`Error`	0.00	test_projects.py
ContextSuite context=TestMultipleProjectCreation>:setup	`Error`	0.00	test_projects.py
ContextSuite context=TestProjectOwners>:setup	`Error`	0.00	test_projects.py
ContextSuite context=TestProjectResources>:setup	`Error`	0.00	test_projects.py
ContextSuite context=TestProjectSuspendActivate>:setup	`Error`	0.00	test_projects.py
ContextSuite context=TestKubernetesCluster>:setup	`Error`	0.00	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesSupportedVersion>:setup	`Error`	0.00	test_kubernetes_supported_versions.py
test_oobm_multiple_mgmt_server_ownership	`Error`	0.16	test_outofbandmanagement.py
ContextSuite context=TestResetVmOnReboot>:setup	`Error`	0.00	test_reset_vm_on_reboot.py
ContextSuite context=TestVPCRedundancy>:setup	`Error`	0.00	test_vpc_redundant.py
ContextSuite context=TestRAMCPUResourceAccounting>:setup	`Error`	0.00	test_resource_accounting.py
ContextSuite context=TestRouterDHCPHosts>:setup	`Error`	0.00	test_router_dhcphosts.py
ContextSuite context=TestRouterDHCPOpts>:setup	`Error`	0.00	test_router_dhcphosts.py
ContextSuite context=TestRouterDns>:setup	`Error`	0.00	test_router_dns.py
ContextSuite context=TestRouterDnsService>:setup	`Error`	0.00	test_router_dnsservice.py
ContextSuite context=TestCreateVolume>:setup	`Error`	0.00	test_volumes.py
ContextSuite context=TestVolumes>:setup	`Error`	0.00	test_volumes.py
ContextSuite context=TestRouterIpTablesPolicies>:setup	`Error`	0.00	test_routers_iptables_default_policy.py
ContextSuite context=TestVPCIpTablesPolicies>:setup	`Error`	0.00	test_routers_iptables_default_policy.py
ContextSuite context=TestIsolatedNetworks>:setup	`Error`	0.00	test_routers_network_ops.py
ContextSuite context=TestRedundantIsolateNetworks>:setup	`Error`	0.00	test_routers_network_ops.py
ContextSuite context=TestRouterServices>:setup	`Error`	0.00	test_routers.py
test_01_sys_vm_start	`Failure`	0.08	test_secondary_storage.py
ContextSuite context=TestCpuCapServiceOfferings>:setup	`Error`	0.00	test_service_offerings.py
ContextSuite context=TestServiceOfferings>:setup	`Error`	0.11	test_service_offerings.py
ContextSuite context=TestSnapshotRootDisk>:setup	`Error`	0.00	test_snapshots.py
test_01_list_sec_storage_vm	`Failure`	0.03	test_ssvm.py
test_02_list_cpvm_vm	`Failure`	0.02	test_ssvm.py
test_03_ssvm_internals	`Failure`	0.03	test_ssvm.py
test_04_cpvm_internals	`Failure`	0.02	test_ssvm.py
test_05_stop_ssvm	`Failure`	0.02	test_ssvm.py
test_06_stop_cpvm	`Failure`	0.02	test_ssvm.py
test_07_reboot_ssvm	`Failure`	0.03	test_ssvm.py
test_08_reboot_cpvm	`Failure`	0.02	test_ssvm.py
test_09_reboot_ssvm_forced	`Failure`	0.03	test_ssvm.py
test_10_reboot_cpvm_forced	`Failure`	0.03	test_ssvm.py
test_11_destroy_ssvm	`Failure`	0.02	test_ssvm.py
test_12_destroy_cpvm	`Failure`	0.02	test_ssvm.py
ContextSuite context=TestVPCNics>:setup	`Error`	0.00	test_vpc_router_nics.py
ContextSuite context=TestVMWareStoragePolicies>:setup	`Error`	0.00	test_storage_policy.py
test_02_create_template_with_checksum_sha1	`Error`	65.35	test_templates.py
test_03_create_template_with_checksum_sha256	`Error`	65.35	test_templates.py
test_04_create_template_with_checksum_md5	`Error`	65.35	test_templates.py
test_05_create_template_with_no_checksum	`Error`	65.34	test_templates.py
test_02_deploy_vm_from_direct_download_template	`Error`	1.18	test_templates.py
ContextSuite context=TestTemplates>:setup	`Error`	18.72	test_templates.py
ContextSuite context=TestRVPCSite2SiteVpn>:setup	`Error`	0.00	test_vpc_vpn.py
ContextSuite context=TestVPCSite2SiteVPNMultipleOptions>:setup	`Error`	0.00	test_vpc_vpn.py
ContextSuite context=TestVpcRemoteAccessVpn>:setup	`Error`	0.00	test_vpc_vpn.py
ContextSuite context=TestVpcSite2SiteVpn>:setup	`Error`	0.00	test_vpc_vpn.py
ContextSuite context=TestISOUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestLBRuleUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestNatRuleUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestPublicIPUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestSnapshotUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestVmUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestVolumeUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestVpnUsage>:setup	`Error`	0.00	test_usage.py
test_01_deploy_vm_on_specific_host	`Error`	2.89	test_vm_deployment_planner.py
test_02_deploy_vm_on_specific_cluster	`Error`	2.63	test_vm_deployment_planner.py
test_03_deploy_vm_on_specific_pod	`Error`	2.71	test_vm_deployment_planner.py
test_04_deploy_vm_on_host_override_pod_and_cluster	`Error`	2.74	test_vm_deployment_planner.py
test_05_deploy_vm_on_cluster_override_pod	`Error`	2.65	test_vm_deployment_planner.py
ContextSuite context=TestDeployVM>:setup	`Error`	0.00	test_vm_life_cycle.py
test_01_secure_vm_migration	`Error`	77.26	test_vm_life_cycle.py
test_02_unsecure_vm_migration	`Error`	217.74	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	`Error`	145.98	test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration	`Error`	145.84	test_vm_life_cycle.py
ContextSuite context=TestVMLifeCycle>:setup	`Error`	1.66	test_vm_life_cycle.py
ContextSuite context=TestVmSnapshot>:setup	`Error`	1.62	test_vm_snapshots.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	1.45	test_host_maintenance.py
test_03_cancel_host_maintenace_with_migration_jobs_failure	`Error`	1.49	test_host_maintenance.py
ContextSuite context=TestHostMaintenanceAgents>:setup	`Error`	1.53	test_host_maintenance.py
test_disable_oobm_ha_state_ineligible	`Error`	1511.77	test_hostha_kvm.py

yadvr · 2021-08-17T11:08:40Z

 String name = column != null ? column.name() : field;
-
+ if (!name.equals("id")) {
+ name = String.format("%s, id", name);


LGTM but I worry as this is used at so many places, @shwstppr - is there a more isolated way to fix the list API issue than this, i.e. what is causing the issue for the listServiceOfferings API response?

@rhtyd as mentioned in the PR desc, it is sorting issue when entries in table have same value for the column (for which order by is called). In case of list* APIs where resources have sort_key in their corresponding tables.
Since by default value for sort_key is 0, weirdly sorted results are returned when LIMIT offset is used. Change will add id as secondary sort criteria.
Only issue I see is the case when table doesn't have id column. Don't think that is the case with tables in ACS.

What if the table has no id column @shwstppr the change is in Filter class and could have widespread side-effects. Can we check the service layer handler specific to listServiceOfferings and do something isolated/specific to that?

@rhtyd changed the logic. No changes in Filter class now

For tables that use sort_key field, incorrect results can be returned by the list* APIs while using pagination and with the same values for sort-key field. (Default value for sort_key field remains 0). This change adds id column as secondary sort criteria in such cases. Fixesapache#5316 Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2021-08-17T12:38:14Z

@blueorangutan package

blueorangutan · 2021-08-17T12:39:03Z

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-08-17T13:04:33Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 893

weizhouapache

code lgtm

nvazquez

LGTM

weizhouapache · 2021-08-17T14:16:51Z

@blueorangutan test

blueorangutan · 2021-08-17T14:18:02Z

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

blueorangutan · 2021-08-18T00:14:40Z

Trillian test result (tid-1680)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34120 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5320-t1680-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 89 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

nvazquez · 2021-08-18T02:57:15Z

@borisstoyanov can you please repeat your tests after the refactor?

borisstoyanov

LGTM, rerun the tests with latest changes and it all seems fine

[root@pr5320-t1674-kvm-centos7-mgmt1 ~]# grep -x -i -f page1 page2 | wc
0 0 0
[root@pr5320-t1674-kvm-centos7-mgmt1 ~]# grep -x -i -f page1 page3 | wc
0 0 0
[root@pr5320-t1674-kvm-centos7-mgmt1 ~]# grep -x -i -f page1 page4 | wc
0 0 0
[root@pr5320-t1674-kvm-centos7-mgmt1 ~]# grep -x -i -f page2 page4 | wc
0 0 0
[root@pr5320-t1674-kvm-centos7-mgmt1 ~]# grep -x -i -f page3 page4 | wc
0 0 0

shwstppr closed this Aug 16, 2021

shwstppr reopened this Aug 16, 2021

apache deleted a comment from blueorangutan Aug 16, 2021

borisstoyanov approved these changes Aug 17, 2021
View reviewed changes

yadvr reviewed Aug 17, 2021
View reviewed changes

yadvr requested review from davidjumani, nvazquez, sureshanaparti and weizhouapache August 17, 2021 11:42

shwstppr force-pushed the fix-listapi-usesortkey branch from 137f37f to 169411eCompare August 17, 2021 12:36

shwstppr changed the title ~~db: use id column as secondary sort criteria~~server: use id column as secondary sort criteria with sortKey Aug 17, 2021

weizhouapache approved these changes Aug 17, 2021
View reviewed changes

nvazquez approved these changes Aug 17, 2021
View reviewed changes

borisstoyanov approved these changes Aug 18, 2021
View reviewed changes

yadvr approved these changes Aug 18, 2021
View reviewed changes

apache deleted a comment from blueorangutan Aug 18, 2021

yadvr added this to the 4.16.0.0 milestone Aug 18, 2021

yadvr merged commit 7bfc7f0 into apache:mainAug 18, 2021

nvazquez added type:bug Severity:Major labels Oct 14, 2021

Uh oh!

Conversation

shwstppr commented Aug 16, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

shwstppr commented Aug 16, 2021

Uh oh!

blueorangutan commented Aug 16, 2021

Uh oh!

blueorangutan commented Aug 16, 2021

Uh oh!

nvazquez commented Aug 16, 2021

Uh oh!

blueorangutan commented Aug 16, 2021

Uh oh!

shwstppr commented Aug 17, 2021

Uh oh!

blueorangutan commented Aug 17, 2021

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Aug 17, 2021

Uh oh!

yadvrAug 17, 2021

Choose a reason for hiding this comment

Uh oh!

shwstpprAug 17, 2021

Choose a reason for hiding this comment

Uh oh!

yadvrAug 17, 2021

Choose a reason for hiding this comment

Uh oh!

shwstpprAug 17, 2021

Choose a reason for hiding this comment

Uh oh!

shwstppr commented Aug 17, 2021

Uh oh!

blueorangutan commented Aug 17, 2021

Uh oh!

blueorangutan commented Aug 17, 2021

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Aug 17, 2021

Uh oh!

blueorangutan commented Aug 17, 2021

Uh oh!

blueorangutan commented Aug 18, 2021

Uh oh!

nvazquez commented Aug 18, 2021

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shwstppr commented Aug 16, 2021•
edited
Loading