search

apache / cloudstack

Apache CloudStack is an opensource Infrastructure as a Service (IaaS) cloud computing platform
https://cloudstack.apache.org/
Apache License 2.0
2k stars 1.09k forks source link

Change vm.stats.remove.batch.size to delete.batch.query.size & allow delete of volume_stats in batches #9283

Closed vishesh92 closed 3 months ago

vishesh92 commented 3 months ago

Description

This PR changes vm.stats.remove.batch.size configuration parameter to delete.batch.query.size to make it more generic.

Changes also allow deletion of volume_stats in batches using the delete.batch.query.size global setting.

For context, please check the conversation in PR: https://github.com/apache/cloudstack/pull/8740

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?

vishesh92 commented 3 months ago

@blueorangutan package

blueorangutan commented 3 months ago

@vishesh92 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 commented 3 months ago

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

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 11.76471% with 15 lines in your changes missing coverage. Please review.

Project coverage is 14.97%. Comparing base (0b857de) to head (745b212). Report is 160 commits behind head on 4.19.

Files Patch % Lines
...java/com/cloud/storage/dao/VolumeStatsDaoImpl.java 0.00% 12 Missing :warning:
...src/main/java/com/cloud/server/StatsCollector.java 33.33% 2 Missing :warning:
.../cloud/configuration/ConfigurationManagerImpl.java 50.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## 4.19 #9283 +/- ## ============================================= - Coverage 30.70% 14.97% -15.74% + Complexity 34232 11046 -23186 ============================================= Files 5376 5390 +14 Lines 378222 470973 +92751 Branches 55053 57438 +2385 ============================================= - Hits 116135 70512 -45623 - Misses 246719 392627 +145908 + Partials 15368 7834 -7534 ``` | [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/9283/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [simulator-marvin-tests](https://app.codecov.io/gh/apache/cloudstack/pull/9283/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | | | [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/9283/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.27% <ø> (-0.04%)` | :arrow_down: | | [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/9283/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `15.68% <11.76%> (-0.97%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

vishesh92 commented 3 months ago

@blueorangutan package

blueorangutan commented 3 months ago

@vishesh92 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 commented 3 months ago

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

JoaoJandre commented 3 months ago

Hey @vishesh92, As I said on #8740, I do not think that it makes sense to have a configuration with a generic name that only applies to a single scenario. This misleads the users into thinking that ACS will apply it everywhere, even if you add a note at the end of the description saying otherwise. My point on #8740 was that if we are applying the same concept on other deletes, then we can create a generic configuration. Just renaming a specific configuration to be generic and changing nothing else does not seem to make much sense.

vishesh92 commented 3 months ago

Hey @vishesh92, As I said on #8740, I do not think that it makes sense to have a configuration with a generic name that only applies to a single scenario. This misleads the users into thinking that ACS will apply it everywhere, even if you add a note at the end of the description saying otherwise. My point on #8740 was that if we are applying the same concept on other deletes, then we can create a generic configuration. Just renaming a specific configuration to be generic and changing nothing else does not seem to make much sense.

@JoaoJandre I think I misunderstood what you meant. I thought you didn't want to increase the scope of your PR. As I mentioned earlier in PR https://github.com/apache/cloudstack/pull/8740, I know it doesn't make sense now. But I am assuming we will be doing this for other tables as well in the future. Do you have any plan on how to deprecate the global settings in the future when we have similar global setting for different tables?

vishesh92 commented 3 months ago

@blueorangutan package

blueorangutan commented 3 months ago

@vishesh92 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.

sureshanaparti commented 3 months ago

Hi @JoaoJandre Can you review/check if your comments are addressed. It seems the config is applied for vm and volume stats now, this can be extended to other stats/tables later if required without any new config.

blueorangutan commented 3 months ago

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

vishesh92 commented 3 months ago

@blueorangutan test

blueorangutan commented 3 months ago

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

blueorangutan commented 3 months ago

[SF] Trillian test result (tid-10608) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 49474 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9283-t10608-kvm-centos7.zip Smoke tests completed. 130 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_vpc_site2site_vpn_multiple_options Failure 306.52 test_vpc_vpn.py
JoaoJandre commented 3 months ago

Hey @vishesh92, As I said on #8740, I do not think that it makes sense to have a configuration with a generic name that only applies to a single scenario. This misleads the users into thinking that ACS will apply it everywhere, even if you add a note at the end of the description saying otherwise. My point on #8740 was that if we are applying the same concept on other deletes, then we can create a generic configuration. Just renaming a specific configuration to be generic and changing nothing else does not seem to make much sense.

@JoaoJandre I think I misunderstood what you meant. I thought you didn't want to increase the scope of your PR. As I mentioned earlier in PR #8740, I know it doesn't make sense now. But I am assuming we will be doing this for other tables as well in the future. Do you have any plan on how to deprecate the global settings in the future when we have similar global setting for different tables?

When I said I did not want to increase the scope of the PR, I was talking about applying the configuration to other deletes.

Regarding global setting deprecation, this would be better discussed on #8970, but it could be a something like: introduce the new configuration while deprecating the older one on one release, then remove the older one on the next release.

In any case, I see you added support for the volume_stats table. As the description of the PR says that the idea is to make the configuration generic, will you be adding support for other tables as well?

vishesh92 commented 3 months ago

In any case, I see you added support for the volume_stats table. As the description of the PR says that the idea is to make the configuration generic, will you be adding support for other tables as well?

I am not planning to add support for other tables in this PR. I will try to add support for other tables in the future.

sureshanaparti commented 3 months ago

@JoaoJandre we can keep the config that is updated in this PR, and later its description will to be updated if support is added for other tables instead to keeping separate config for tables. Agree?

sureshanaparti commented 3 months ago

@blueorangutan package

blueorangutan commented 3 months ago

@sureshanaparti 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 commented 3 months ago

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

sureshanaparti commented 3 months ago

@blueorangutan package

blueorangutan commented 3 months ago

@sureshanaparti 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 commented 3 months ago

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

sureshanaparti commented 3 months ago

@blueorangutan test

blueorangutan commented 3 months ago

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

blueorangutan commented 3 months ago

[SF] Trillian Build Failed (tid-10657)

vishesh92 commented 3 months ago

@blueorangutan package

blueorangutan commented 3 months ago

@vishesh92 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 commented 3 months ago

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

sureshanaparti commented 3 months ago

@blueorangutan test

blueorangutan commented 3 months ago

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

blueorangutan commented 3 months ago

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

Test Result Time (s) Test File
ContextSuite context=TestTemplateHierarchy>:setup Error 8.40 test_accounts.py
test_01_events_resource Failure 0.02 test_events_resource.py
ContextSuite context=TestDeployVmWithAffinityGroup>:setup Error 0.00 test_affinity_groups.py
ContextSuite context=TestMultipleVolumeAttach>:setup Error 0.00 test_attach_multiple_volumes.py
ContextSuite context=TestDummyBackupAndRecovery>:setup Error 0.00 test_backup_recovery_dummy.py
ContextSuite context=TestVeeamBackupAndRecovery>:setup Error 0.00 test_backup_recovery_veeam.py
test_01_condensed_drs_algorithm Error 0.00 test_cluster_drs.py
test_02_balanced_drs_algorithm Error 0.00 test_cluster_drs.py
ContextSuite context=TestConsoleEndpoint>:setup Error 0.00 test_console_endpoint.py
test_01_deploy_vm_with_extraconfig_throws_exception_kvm Error 0.09 test_deploy_vm_extra_config_data.py
test_02_deploy_vm_with_extraconfig_kvm Error 0.07 test_deploy_vm_extra_config_data.py
test_03_update_vm_with_extraconfig_kvm Error 0.07 test_deploy_vm_extra_config_data.py
ContextSuite context=TestDeployVmRootSize>:setup Error 0.00 test_deploy_vm_root_resize.py
ContextSuite context=TestHostControlState>:setup Error 27.26 test_host_control_state.py
ContextSuite context=TestImportAndUnmanageVolumes>:setup Error 0.00 test_import_unmanage_volumes.py
test_11_test_unmanaged_cluster_lifecycle Error 3.26 test_kubernetes_clusters.py
ContextSuite context=TestListVolumes>:setup Error 0.00 test_list_volumes.py
ContextSuite context=TestNetworkMigration>:setup Error 0.00 test_migration.py
test_03_network_operations_on_created_vm_of_otheruser Error 1.22 test_network_permissions.py
test_04_deploy_vm_for_other_user_and_test_vm_operations Failure 0.05 test_network_permissions.py
ContextSuite context=TestSharedNetwork>:setup Error 46.45 test_network.py
test_01_nic Error 0.06 test_nic.py
test_01_non_strict_host_anti_affinity Error 2.25 test_nonstrict_affinity_group.py
test_02_non_strict_host_affinity Error 1.16 test_nonstrict_affinity_group.py
ContextSuite context=TestL2PersistentNetworks>:setup Error 0.00 test_persistent_network.py
test_01_add_primary_storage_disabled_host Failure 0.06 test_primary_storage.py
test_01_primary_storage_iscsi Failure 0.06 test_primary_storage.py
test_01_primary_storage_nfs Failure 0.06 test_primary_storage.py
ContextSuite context=TestStorageTags>:setup Error 0.13 test_primary_storage.py
ContextSuite context=TestPrivateGwACLOvsGRE>:setup Error 0.00 test_privategw_acl_ovs_gre.py
ContextSuite context=TestTemplates>:setup Error 34.65 test_templates.py
test_01_positive_tests_usage Failure 3.07 test_usage_events.py
ContextSuite context=TestLBRuleUsage>:setup Error 2.27 test_usage.py
ContextSuite context=TestNatRuleUsage>:setup Error 2.33 test_usage.py
ContextSuite context=TestPublicIPUsage>:setup Error 2.39 test_usage.py
ContextSuite context=TestSnapshotUsage>:setup Error 2.45 test_usage.py
ContextSuite context=TestTemplateUsage>:setup Error 2.51 test_usage.py
ContextSuite context=TestVmUsage>:setup Error 2.56 test_usage.py
ContextSuite context=TestVolumeUsage>:setup Error 2.67 test_usage.py
ContextSuite context=TestVpnUsage>:setup Error 2.73 test_usage.py
ContextSuite context=TestVmAutoScaling>:setup Error 0.00 test_vm_autoscaling.py
test_01_deploy_vm_on_specific_host Error 0.04 test_vm_deployment_planner.py
test_02_deploy_vm_on_specific_cluster Error 0.03 test_vm_deployment_planner.py
test_03_deploy_vm_on_specific_pod Error 0.07 test_vm_deployment_planner.py
test_04_deploy_vm_on_host_override_pod_and_cluster Error 0.09 test_vm_deployment_planner.py
test_05_deploy_vm_on_cluster_override_pod Error 0.06 test_vm_deployment_planner.py
ContextSuite context=TestDeployVM>:setup Error 0.00 test_vm_life_cycle.py
ContextSuite context=TestKVMLiveMigration>:setup Error 0.00 test_vm_life_cycle.py
ContextSuite context=TestMigrateVMwithVolume>:setup Error 0.00 test_vm_life_cycle.py
ContextSuite context=TestSecuredVmMigration>:setup Error 0.00 test_vm_life_cycle.py
ContextSuite context=TestVMLifeCycle>:setup Error 0.06 test_vm_life_cycle.py
ContextSuite context=TestUnmanageVM>:setup Error 0.00 test_vm_lifecycle_unmanage_import.py
ContextSuite context=TestVMSchedule>:setup Error 0.00 test_vm_schedule.py
ContextSuite context=TestVmSnapshot>:setup Error 0.06 test_vm_snapshots.py
ContextSuite context=TestVnfTemplates>:setup Error 0.00 test_vnf_templates.py
ContextSuite context=TestCreateVolume>:setup Error 0.00 test_volumes.py
ContextSuite context=TestVolumeEncryption>:setup Error 0.00 test_volumes.py
ContextSuite context=TestVolumes>:setup Error 0.00 test_volumes.py
test_02_cancel_host_maintenace_with_migration_jobs Failure 0.12 test_host_maintenance.py
test_03_cancel_host_maintenace_with_migration_jobs_failure Error 0.20 test_host_maintenance.py
ContextSuite context=TestHostMaintenanceAgents>:setup Error 0.28 test_host_maintenance.py
sureshanaparti commented 3 months ago

@blueorangutan LLtest

blueorangutan commented 3 months ago

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

sureshanaparti commented 3 months ago

[SF] Trillian test result (tid-10664) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 35009 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9283-t10664-kvm-centos7.zip Smoke tests completed. 97 look OK, 34 have errors, 0 did not run

these failures are due to templates listing issue & builtin template deleted during the tests.

sureshanaparti commented 3 months ago

Manually verified the config change.

  • © Githubissues.
  • Githubissues is a development platform for aggregating issues.