From fc29c194d1f9dd052cd14a92f0ce37505606c93c Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Fri, 26 Apr 2024 12:10:03 +0530 Subject: [PATCH 1/4] api: improve listVM API handling and response speed - Changes behaviour of details param handling for: - listVirtualMachines API: when the detail param is not provided, it uses `all` details except `stats` - listVirtualMachinesMetrics API: when the detail param is not provided, it uses `all` details including `stats` - Remove ConfigKey vm.stats.increment.metrics.in.memory which was renamed to `vm.stats.increment.metrics` in #5984 - Changes default value of VM stats accumulation setting `vm.stats.increment.metrics` to false until a better solution emerges. Since #5984, this is true and during the execution of listVM APIs the stats are clubbed/calculated which can immensely slow down list VM API calls. - Fix UI that uses listVirtualMachinesMetrics to not call `stats` detail when in list view without metrics selected. These changes see 2-4x gains in the listVirtualMachines APIs call and in the UI. For environment where code changes are not possible, disabling `vm.stats.increment.metrics` global setting saw 2-4x speed gain in the list API calls. Signed-off-by: Rohit Yadav --- .../cloudstack/api/command/user/vm/ListVMsCmd.java | 9 +++++---- .../org/apache/cloudstack/api/ListVMsMetricsCmd.java | 12 ++++++++++++ .../com/cloud/api/query/dao/UserVmJoinDaoImpl.java | 3 +-- .../main/java/com/cloud/server/StatsCollector.java | 7 ++----- ui/src/config/section/compute.js | 4 ++-- ui/src/views/AutogenView.vue | 1 + 6 files changed, 23 insertions(+), 13 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java index 6a5ec28d1bae..e3f0632c71da 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java @@ -96,10 +96,11 @@ public class ListVMsCmd extends BaseListRetrieveOnlyResourceCountCmd implements @Parameter(name = ApiConstants.DETAILS, type = CommandType.LIST, collectionType = CommandType.STRING, - description = "comma separated list of vm details requested, " - + "value can be a list of [all, group, nics, stats, secgrp, tmpl, servoff, diskoff, backoff, iso, volume, min, affgrp]." - + " If no parameter is passed in, the details will be defaulted to all") - private List viewDetails; + description = "comma separated list of host details requested, " + + "value can be a list of [all, group, nics, stats, secgrp, tmpl, servoff, diskoff, iso, volume, min, affgrp]." + + " If no parameter is passed in, the details will be defaulted to, all details excluding stats " + + " for the listVirtualMachines API, and all details including stats for the listVirtualMachinesMetrics API") + protected List viewDetails; @Parameter(name = ApiConstants.TEMPLATE_ID, type = CommandType.UUID, entityType = TemplateResponse.class, description = "list vms by template") private Long templateId; diff --git a/plugins/metrics/src/main/java/org/apache/cloudstack/api/ListVMsMetricsCmd.java b/plugins/metrics/src/main/java/org/apache/cloudstack/api/ListVMsMetricsCmd.java index 55af69e623c2..179bf9245315 100644 --- a/plugins/metrics/src/main/java/org/apache/cloudstack/api/ListVMsMetricsCmd.java +++ b/plugins/metrics/src/main/java/org/apache/cloudstack/api/ListVMsMetricsCmd.java @@ -17,10 +17,12 @@ package org.apache.cloudstack.api; +import java.util.EnumSet; import java.util.List; import javax.inject.Inject; +import com.cloud.exception.InvalidParameterValueException; import org.apache.cloudstack.acl.RoleType; import org.apache.cloudstack.api.command.user.UserCmd; import org.apache.cloudstack.api.command.user.vm.ListVMsCmd; @@ -56,6 +58,16 @@ public String getCommandName() { return APINAME.toLowerCase() + BaseCmd.RESPONSE_SUFFIX; } + @Override + public EnumSet getDetails() throws InvalidParameterValueException { + final EnumSet dv = super.getDetails(); + // Include stats detail when 'all' details are asked + if (dv.contains(ApiConstants.VMDetails.all)) { + dv.add(ApiConstants.VMDetails.stats); + } + return dv; + } + @Override public void execute() { ListResponse userVms = _queryService.searchForUserVMs(this); diff --git a/server/src/main/java/com/cloud/api/query/dao/UserVmJoinDaoImpl.java b/server/src/main/java/com/cloud/api/query/dao/UserVmJoinDaoImpl.java index e5cc9ee72342..5b5ee021cbfc 100644 --- a/server/src/main/java/com/cloud/api/query/dao/UserVmJoinDaoImpl.java +++ b/server/src/main/java/com/cloud/api/query/dao/UserVmJoinDaoImpl.java @@ -251,7 +251,7 @@ public UserVmResponse newUserVmResponse(ResponseView view, String objectName, Us userVmResponse.setOsDisplayName(guestOS.getDisplayName()); } - if (details.contains(VMDetails.all) || details.contains(VMDetails.stats)) { + if (details.contains(VMDetails.stats)) { // stats calculation VmStats vmStats = ApiDBUtils.getVmStatistics(userVm.getId(), accumulateStats); if (vmStats != null) { @@ -268,7 +268,6 @@ public UserVmResponse newUserVmResponse(ResponseView view, String objectName, Us userVmResponse.setMemoryKBs(totalMemory); userVmResponse.setMemoryIntFreeKBs(correctedFreeMemory); userVmResponse.setMemoryTargetKBs((long)vmStats.getTargetMemoryKBs()); - } } diff --git a/server/src/main/java/com/cloud/server/StatsCollector.java b/server/src/main/java/com/cloud/server/StatsCollector.java index 2467416155a1..6adfc82cfc54 100644 --- a/server/src/main/java/com/cloud/server/StatsCollector.java +++ b/server/src/main/java/com/cloud/server/StatsCollector.java @@ -278,12 +278,10 @@ public String toString() { private static final ConfigKey statsOutputUri = new ConfigKey<>("Advanced", String.class, "stats.output.uri", "", "URI to send StatsCollector statistics to. The collector is defined on the URI scheme. Example: graphite://graphite-hostaddress:port or influxdb://influxdb-hostaddress/dbname. Note that the port is optional, if not added the default port for the respective collector (graphite or influxdb) will be used. Additionally, the database name '/dbname' is also optional; default db name is 'cloudstack'. You must create and configure the database if using influxdb.", true); - protected static ConfigKey vmStatsIncrementMetrics = new ConfigKey<>("Advanced", Boolean.class, "vm.stats.increment.metrics", "true", + protected static ConfigKey vmStatsIncrementMetrics = new ConfigKey<>("Advanced", Boolean.class, "vm.stats.increment.metrics", "false", "When set to 'true', VM metrics(NetworkReadKBs, NetworkWriteKBs, DiskWriteKBs, DiskReadKBs, DiskReadIOs and DiskWriteIOs) that are collected from the hypervisor are summed before being returned." + "On the other hand, when set to 'false', the VM metrics API will just display the latest metrics collected.", true); - private static final ConfigKey VM_STATS_INCREMENT_METRICS_IN_MEMORY = new ConfigKey<>("Advanced", Boolean.class, "vm.stats.increment.metrics.in.memory", "true", - "When set to 'true', VM metrics(NetworkReadKBs, NetworkWriteKBs, DiskWriteKBs, DiskReadKBs, DiskReadIOs and DiskWriteIOs) that are collected from the hypervisor are summed and stored in memory. " - + "On the other hand, when set to 'false', the VM metrics API will just display the latest metrics collected.", true); + protected static ConfigKey vmStatsMaxRetentionTime = new ConfigKey<>("Advanced", Integer.class, "vm.stats.max.retention.time", "720", "The maximum time (in minutes) for keeping VM stats records in the database. The VM stats cleanup process will be disabled if this is set to 0 or less than 0.", true); @@ -2131,7 +2129,6 @@ public String getConfigComponentName() { public ConfigKey[] getConfigKeys() { return new ConfigKey[] {vmDiskStatsInterval, vmDiskStatsIntervalMin, vmNetworkStatsInterval, vmNetworkStatsIntervalMin, StatsTimeout, statsOutputUri, vmStatsIncrementMetrics, vmStatsMaxRetentionTime, vmStatsCollectUserVMOnly, vmDiskStatsRetentionEnabled, vmDiskStatsMaxRetentionTime, - VM_STATS_INCREMENT_METRICS_IN_MEMORY, MANAGEMENT_SERVER_STATUS_COLLECTION_INTERVAL, DATABASE_SERVER_STATUS_COLLECTION_INTERVAL, DATABASE_SERVER_LOAD_HISTORY_RETENTION_NUMBER}; diff --git a/ui/src/config/section/compute.js b/ui/src/config/section/compute.js index db17b20ef9dc..54810f216fbb 100644 --- a/ui/src/config/section/compute.js +++ b/ui/src/config/section/compute.js @@ -31,9 +31,9 @@ export default { permission: ['listVirtualMachinesMetrics'], resourceType: 'UserVm', params: () => { - var params = { details: 'servoff,tmpl,iso,nics,backoff' } + var params = { details: 'group,nics,secgrp,tmpl,servoff,diskoff,iso,volume,affgrp' } if (store.getters.metrics) { - params = { details: 'servoff,tmpl,iso,nics,backoff,stats' } + params = { details: 'all' } } params.isvnf = false return params diff --git a/ui/src/views/AutogenView.vue b/ui/src/views/AutogenView.vue index be6c0f24cd17..58a8a5adce4c 100644 --- a/ui/src/views/AutogenView.vue +++ b/ui/src/views/AutogenView.vue @@ -904,6 +904,7 @@ export default { if (['listVirtualMachinesMetrics'].includes(this.apiName) && this.dataView) { delete params.details delete params.isvnf + params.details = 'group,nics,secgrp,tmpl,servoff,diskoff,iso,volume,affgrp' } this.loading = true From 085038ebd16723d405c6a7115d0bc340ce243ce4 Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Sat, 27 Apr 2024 10:10:17 +0530 Subject: [PATCH 2/4] address review comments Signed-off-by: Rohit Yadav --- .../apache/cloudstack/api/command/user/vm/ListVMsCmd.java | 5 +++-- .../main/java/org/apache/cloudstack/query/QueryService.java | 3 +++ .../resources/META-INF/db/schema-41900to41910-cleanup.sql | 2 ++ .../src/main/java/com/cloud/api/query/QueryManagerImpl.java | 2 +- .../main/java/com/cloud/api/query/dao/UserVmJoinDaoImpl.java | 2 +- 5 files changed, 10 insertions(+), 4 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java index e3f0632c71da..559abfa15203 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java @@ -98,8 +98,9 @@ public class ListVMsCmd extends BaseListRetrieveOnlyResourceCountCmd implements collectionType = CommandType.STRING, description = "comma separated list of host details requested, " + "value can be a list of [all, group, nics, stats, secgrp, tmpl, servoff, diskoff, iso, volume, min, affgrp]." - + " If no parameter is passed in, the details will be defaulted to, all details excluding stats " - + " for the listVirtualMachines API, and all details including stats for the listVirtualMachinesMetrics API") + + " If no parameter is passed in, the details will be defaulted to all details excluding/including stats " + + " for the listVirtualMachines API as determined by list.vm.default.details.stats setting (default: false)." + + " However, all details including stats are returned for the listVirtualMachinesMetrics API") protected List viewDetails; @Parameter(name = ApiConstants.TEMPLATE_ID, type = CommandType.UUID, entityType = TemplateResponse.class, description = "list vms by template") diff --git a/api/src/main/java/org/apache/cloudstack/query/QueryService.java b/api/src/main/java/org/apache/cloudstack/query/QueryService.java index 3299e7537a2e..030e402fea32 100644 --- a/api/src/main/java/org/apache/cloudstack/query/QueryService.java +++ b/api/src/main/java/org/apache/cloudstack/query/QueryService.java @@ -125,6 +125,9 @@ public interface QueryService { static final ConfigKey SharePublicTemplatesWithOtherDomains = new ConfigKey<>("Advanced", Boolean.class, "share.public.templates.with.other.domains", "true", "If false, templates of this domain will not show up in the list templates of other domains.", true, ConfigKey.Scope.Domain); + ConfigKey AllowStatsInDefaultDetailsForListVMs = new ConfigKey<>("Advanced", Boolean.class, "list.vm.default.details.stats", "false", + "Determines whether VM stats should be returned when details are not specified in listVirtualMachines API request", true, ConfigKey.Scope.Global); + ListResponse searchForUsers(ListUsersCmd cmd) throws PermissionDeniedException; ListResponse searchForUsers(Long domainId, boolean recursive) throws PermissionDeniedException; diff --git a/engine/schema/src/main/resources/META-INF/db/schema-41900to41910-cleanup.sql b/engine/schema/src/main/resources/META-INF/db/schema-41900to41910-cleanup.sql index b580d42686f0..8297dbab0b44 100644 --- a/engine/schema/src/main/resources/META-INF/db/schema-41900to41910-cleanup.sql +++ b/engine/schema/src/main/resources/META-INF/db/schema-41900to41910-cleanup.sql @@ -18,3 +18,5 @@ --; -- Schema upgrade cleanup from 4.19.0.0 to 4.19.1.0 --; + +DELETE from cloud.configuration where name='vm.stats.increment.metrics.in.memory'; diff --git a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java index 9aee1636c2a5..6b2f23cdde63 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -5686,6 +5686,6 @@ public String getConfigComponentName() { @Override public ConfigKey[] getConfigKeys() { return new ConfigKey[] {AllowUserViewDestroyedVM, UserVMDeniedDetails, UserVMReadOnlyDetails, SortKeyAscending, - AllowUserViewAllDomainAccounts, SharePublicTemplatesWithOtherDomains}; + AllowUserViewAllDomainAccounts, SharePublicTemplatesWithOtherDomains, AllowStatsInDefaultDetailsForListVMs}; } } diff --git a/server/src/main/java/com/cloud/api/query/dao/UserVmJoinDaoImpl.java b/server/src/main/java/com/cloud/api/query/dao/UserVmJoinDaoImpl.java index 5b5ee021cbfc..c0f741734df5 100644 --- a/server/src/main/java/com/cloud/api/query/dao/UserVmJoinDaoImpl.java +++ b/server/src/main/java/com/cloud/api/query/dao/UserVmJoinDaoImpl.java @@ -251,7 +251,7 @@ public UserVmResponse newUserVmResponse(ResponseView view, String objectName, Us userVmResponse.setOsDisplayName(guestOS.getDisplayName()); } - if (details.contains(VMDetails.stats)) { + if (details.contains(VMDetails.stats) || (QueryService.AllowStatsInDefaultDetailsForListVMs.value() && details.contains(VMDetails.all))) { // stats calculation VmStats vmStats = ApiDBUtils.getVmStatistics(userVm.getId(), accumulateStats); if (vmStats != null) { From d3677aa2f69a1ab0826b14102a6387bda85a4d93 Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Sat, 27 Apr 2024 10:25:23 +0530 Subject: [PATCH 3/4] address default global setting to allow faster list VMs response time Signed-off-by: Rohit Yadav --- .../main/resources/META-INF/db/schema-41900to41910-cleanup.sql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/engine/schema/src/main/resources/META-INF/db/schema-41900to41910-cleanup.sql b/engine/schema/src/main/resources/META-INF/db/schema-41900to41910-cleanup.sql index 8297dbab0b44..cf0600b9e5e1 100644 --- a/engine/schema/src/main/resources/META-INF/db/schema-41900to41910-cleanup.sql +++ b/engine/schema/src/main/resources/META-INF/db/schema-41900to41910-cleanup.sql @@ -19,4 +19,6 @@ -- Schema upgrade cleanup from 4.19.0.0 to 4.19.1.0 --; +-- List VMs response optimisation https://github.com/apache/cloudstack/pull/8985 +UPDATE cloud.configuration set value='false' where name='vm.stats.increment.metrics'; DELETE from cloud.configuration where name='vm.stats.increment.metrics.in.memory'; From a2cf0be680dbb8e3f66b0c5ab1404cf3a8310883 Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Mon, 29 Apr 2024 13:18:28 +0530 Subject: [PATCH 4/4] reduce number of config/db calls Signed-off-by: Rohit Yadav --- .../org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java | 4 ++++ .../main/java/com/cloud/api/query/dao/UserVmJoinDaoImpl.java | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java index 559abfa15203..5201cde6cc0a 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java @@ -44,6 +44,7 @@ import org.apache.cloudstack.api.response.UserVmResponse; import org.apache.cloudstack.api.response.VpcResponse; import org.apache.cloudstack.api.response.ZoneResponse; +import org.apache.cloudstack.query.QueryService; import org.apache.commons.lang3.BooleanUtils; import org.apache.log4j.Logger; @@ -245,6 +246,9 @@ public EnumSet getDetails() throws InvalidParameterValueException { EnumSet dv; if (viewDetails == null || viewDetails.size() <= 0) { dv = EnumSet.of(VMDetails.all); + if (QueryService.AllowStatsInDefaultDetailsForListVMs.value()) { + dv.add(VMDetails.stats); + } } else { try { ArrayList dc = new ArrayList(); diff --git a/server/src/main/java/com/cloud/api/query/dao/UserVmJoinDaoImpl.java b/server/src/main/java/com/cloud/api/query/dao/UserVmJoinDaoImpl.java index c0f741734df5..5b5ee021cbfc 100644 --- a/server/src/main/java/com/cloud/api/query/dao/UserVmJoinDaoImpl.java +++ b/server/src/main/java/com/cloud/api/query/dao/UserVmJoinDaoImpl.java @@ -251,7 +251,7 @@ public UserVmResponse newUserVmResponse(ResponseView view, String objectName, Us userVmResponse.setOsDisplayName(guestOS.getDisplayName()); } - if (details.contains(VMDetails.stats) || (QueryService.AllowStatsInDefaultDetailsForListVMs.value() && details.contains(VMDetails.all))) { + if (details.contains(VMDetails.stats)) { // stats calculation VmStats vmStats = ApiDBUtils.getVmStatistics(userVm.getId(), accumulateStats); if (vmStats != null) {