From 55f8b32aa00baf0609f54891230269ed97bedb5a Mon Sep 17 00:00:00 2001 From: Nitin Kumar Maharana Date: Tue, 29 Dec 2015 16:45:41 +0530 Subject: [PATCH 1/2] CLOUDSTACK-9132: API createVolume takes empty string for name parameter Added conditions to check if the name is empty or blank. If it is empty or blank, then it generates a random name. Made the name field as optional in UI as well as in API. Added required unit tests. --- .../command/user/volume/CreateVolumeCmd.java | 2 +- .../cloud/storage/VolumeApiServiceImpl.java | 24 +++++++++++++--- .../storage/VolumeApiServiceImplTest.java | 28 +++++++++++++++++++ ui/scripts/docs.js | 2 +- ui/scripts/storage.js | 5 +--- 5 files changed, 51 insertions(+), 10 deletions(-) diff --git a/api/src/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java b/api/src/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java index 1e3c01cec9e..54c376e2265 100644 --- a/api/src/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java +++ b/api/src/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java @@ -79,7 +79,7 @@ public class CreateVolumeCmd extends BaseAsyncCreateCustomIdCmd { description = "the ID of the disk offering. Either diskOfferingId or snapshotId must be passed in.") private Long diskOfferingId; - @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "the name of the disk volume") + @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, description = "the name of the disk volume") private String volumeName; @Parameter(name = ApiConstants.SIZE, type = CommandType.LONG, description = "Arbitrary volume size") diff --git a/server/src/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/com/cloud/storage/VolumeApiServiceImpl.java index afa5cc40089..5a53f9c531d 100644 --- a/server/src/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/com/cloud/storage/VolumeApiServiceImpl.java @@ -476,6 +476,25 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic }); } + /** + * Retrieves the volume name from CreateVolumeCmd object. + * + * If the retrieved volume name is null, empty or blank, then A random name + * will be generated using getRandomVolumeName method. + * + * @param cmd + * @return Either the retrieved name or a random name. + */ + public String getVolumeNameFromCommand(CreateVolumeCmd cmd) { + String userSpecifiedName = cmd.getVolumeName(); + + if (org.apache.commons.lang.StringUtils.isBlank(userSpecifiedName)) { + userSpecifiedName = getRandomVolumeName(); + } + + return userSpecifiedName; + } + /* * Just allocate a volume in the database, don't send the createvolume cmd * to hypervisor. The volume will be finally created only when it's attached @@ -671,10 +690,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic throw new InvalidParameterValueException("Zone is not configured to use local storage but volume's disk offering " + diskOffering.getName() + " uses it"); } - String userSpecifiedName = cmd.getVolumeName(); - if (userSpecifiedName == null) { - userSpecifiedName = getRandomVolumeName(); - } + String userSpecifiedName = getVolumeNameFromCommand(cmd); VolumeVO volume = commitVolume(cmd, caller, owner, displayVolume, zoneId, diskOfferingId, provisioningType, size, minIops, maxIops, parentVolume, userSpecifiedName, _uuidMgr.generateUuid(Volume.class, cmd.getCustomId())); diff --git a/server/test/com/cloud/storage/VolumeApiServiceImplTest.java b/server/test/com/cloud/storage/VolumeApiServiceImplTest.java index 0e46142fe2c..4b502a404dc 100644 --- a/server/test/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/test/com/cloud/storage/VolumeApiServiceImplTest.java @@ -29,6 +29,8 @@ import java.util.UUID; import javax.inject.Inject; import com.cloud.user.User; +import junit.framework.Assert; +import org.apache.cloudstack.api.command.user.volume.CreateVolumeCmd; import org.junit.After; import org.junit.Before; import org.junit.Rule; @@ -97,6 +99,8 @@ public class VolumeApiServiceImplTest { SnapshotInfo snapshotInfoMock; @Mock VolumeService volService; + @Mock + CreateVolumeCmd createVol; DetachVolumeCmd detachCmd = new DetachVolumeCmd(); Class _detachCmdClass = detachCmd.getClass(); @@ -355,6 +359,30 @@ public class VolumeApiServiceImplTest { _svc.takeSnapshot(5L, Snapshot.MANUAL_POLICY_ID, 3L, null, false); } + @Test + public void testNullGetVolumeNameFromCmd() { + when(createVol.getVolumeName()).thenReturn(null); + Assert.assertNotNull(_svc.getVolumeNameFromCommand(createVol)); + } + + @Test + public void testEmptyGetVolumeNameFromCmd() { + when(createVol.getVolumeName()).thenReturn(""); + Assert.assertNotNull(_svc.getVolumeNameFromCommand(createVol)); + } + + @Test + public void testBlankGetVolumeNameFromCmd() { + when(createVol.getVolumeName()).thenReturn(" "); + Assert.assertNotNull(_svc.getVolumeNameFromCommand(createVol)); + } + + @Test + public void testNonEmptyGetVolumeNameFromCmd() { + when(createVol.getVolumeName()).thenReturn("abc"); + Assert.assertSame(_svc.getVolumeNameFromCommand(createVol), "abc"); + } + @After public void tearDown() { CallContext.unregister(); diff --git a/ui/scripts/docs.js b/ui/scripts/docs.js index ed6ab0c938c..30e123b950b 100755 --- a/ui/scripts/docs.js +++ b/ui/scripts/docs.js @@ -1008,7 +1008,7 @@ cloudStack.docs = { }, // Add volume helpVolumeName: { - desc: 'Give the volume a unique name so you can find it later.', + desc: 'Give a unique volume name. If it is not provided, a name will be generated randomly.', externalLink: '' }, helpVolumeAvailabilityZone: { diff --git a/ui/scripts/storage.js b/ui/scripts/storage.js index 2f93917319c..39dfccf2ebe 100644 --- a/ui/scripts/storage.js +++ b/ui/scripts/storage.js @@ -80,10 +80,7 @@ fields: { name: { docID: 'helpVolumeName', - label: 'label.name', - validation: { - required: true - } + label: 'label.name' }, availabilityZone: { label: 'label.availability.zone', From 750c11b494fe9100f233c8e9a7ef89c346ccaf2b Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Thu, 7 Jan 2016 15:52:41 +0530 Subject: [PATCH 2/2] ui: improve metrics view implementation - Implement Search boxes in all Metrics views - Fix threshold calculations for host and storage pool metrics view - Consider overcommit ratios for calculation allocated thresholds - Save/pass context while navigating across resources and metrics view Signed-off-by: Rohit Yadav --- ui/scripts/metrics.js | 50 ++++++----- ui/scripts/ui-custom/metricsView.js | 125 +++++++++++++++++++++------- 2 files changed, 127 insertions(+), 48 deletions(-) diff --git a/ui/scripts/metrics.js b/ui/scripts/metrics.js index 609022a81a3..6ac376e6f15 100644 --- a/ui/scripts/metrics.js +++ b/ui/scripts/metrics.js @@ -575,8 +575,8 @@ label: 'label.metrics.allocated', thresholdcolor: true, thresholds: { - notification: 'cpunotificationthreshold', - disable: 'cpudisablethreshold' + notification: 'cpuallocatednotificationthreshold', + disable: 'cpuallocateddisablethreshold' } } } @@ -600,8 +600,8 @@ label: 'label.metrics.allocated', thresholdcolor: true, thresholds: { - notification: 'memnotificationthreshold', - disable: 'memdisablethreshold' + notification: 'memallocatednotificationthreshold', + disable: 'memallocateddisablethreshold' } } } @@ -656,11 +656,31 @@ items[idx].networkwrite = ''; } + var cpuOverCommit = 1.0; + var memOverCommit = 1.0; + $.ajax({ + url: createURL('listClusters'), + data: {clusterid: host.clusterid, listAll: true}, + success: function(json) { + if (json.listclustersresponse && json.listclustersresponse.cluster) { + var cluster = json.listclustersresponse.cluster[0]; + cpuOverCommit = parseFloat(cluster.cpuovercommitratio); + memOverCommit = parseFloat(cluster.memoryovercommitratio); + } + }, + async: false + }); + // Threshold color coding items[idx].cpunotificationthreshold = 0.75 * parseFloat(items[idx].cputotal); items[idx].cpudisablethreshold = 0.95 * parseFloat(items[idx].cputotal); + items[idx].cpuallocatednotificationthreshold = 0.75 * cpuOverCommit * parseFloat(items[idx].cputotal); + items[idx].cpuallocateddisablethreshold = 0.95 * cpuOverCommit * parseFloat(items[idx].cputotal); + items[idx].memnotificationthreshold = 0.75 * parseFloat(items[idx].memtotal); items[idx].memdisablethreshold = 0.95 * parseFloat(items[idx].memtotal); + items[idx].memallocatednotificationthreshold = 0.75 * memOverCommit * parseFloat(items[idx].memtotal); + items[idx].memallocateddisablethreshold = 0.95 * memOverCommit * parseFloat(items[idx].memtotal); $.ajax({ url: createURL('listConfigurations'), @@ -671,15 +691,19 @@ switch (config.name) { case 'cluster.cpu.allocated.capacity.disablethreshold': items[idx].cpudisablethreshold = parseFloat(config.value) * parseFloat(items[idx].cputotal); + items[idx].cpuallocateddisablethreshold = parseFloat(config.value) * cpuOverCommit * parseFloat(items[idx].cputotal); break; case 'cluster.cpu.allocated.capacity.notificationthreshold': items[idx].cpunotificationthreshold = parseFloat(config.value) * parseFloat(items[idx].cputotal); + items[idx].cpuallocatednotificationthreshold = parseFloat(config.value) * cpuOverCommit * parseFloat(items[idx].cputotal); break; case 'cluster.memory.allocated.capacity.disablethreshold': items[idx].memdisablethreshold = parseFloat(config.value) * parseFloat(items[idx].memtotal); + items[idx].memallocateddisablethreshold = parseFloat(config.value) * memOverCommit * parseFloat(items[idx].memtotal); break; case 'cluster.memory.allocated.capacity.notificationthreshold': items[idx].memnotificationthreshold = parseFloat(config.value) * parseFloat(items[idx].memtotal); + items[idx].memallocatednotificationthreshold = parseFloat(config.value) * memOverCommit * parseFloat(items[idx].memtotal); break; } }); @@ -688,20 +712,6 @@ async: false }); - var cpuOverCommit = 1.0; - var memOverCommit = 1.0; - $.ajax({ - url: createURL('listClusters'), - data: {clusterid: host.clusterid, listAll: true}, - success: function(json) { - if (json.listclustersresponse && json.listclustersresponse.cluster) { - var cluster = json.listclustersresponse.cluster[0]; - cpuOverCommit = cluster.cpuovercommitratio; - memOverCommit = cluster.memoryovercommitratio; - } - }, - async: false - }); items[idx].cputotal = items[idx].cputotal + ' Ghz (x' + cpuOverCommit + ')'; items[idx].memtotal = items[idx].memtotal + ' (x' + memOverCommit + ')'; @@ -1081,13 +1091,13 @@ $.each(json.listconfigurationsresponse.configuration, function(i, config) { switch (config.name) { case 'cluster.storage.allocated.capacity.notificationthreshold': - items[idx].storageallocatednotificationthreshold = parseFloat(config.value) * parseFloat(items[idx].disksizetotal); + items[idx].storageallocatednotificationthreshold = parseFloat(config.value) * items[idx].overprovisionfactor * parseFloat(items[idx].disksizetotal); break; case 'cluster.storage.capacity.notificationthreshold': items[idx].storagenotificationthreshold = parseFloat(config.value) * parseFloat(items[idx].disksizetotal); break; case 'pool.storage.allocated.capacity.disablethreshold': - items[idx].storageallocateddisablethreshold = parseFloat(config.value) * parseFloat(items[idx].disksizetotal); + items[idx].storageallocateddisablethreshold = parseFloat(config.value) * items[idx].overprovisionfactor * parseFloat(items[idx].disksizetotal); break; case 'pool.storage.capacity.disablethreshold': items[idx].storagedisablethreshold = parseFloat(config.value) * parseFloat(items[idx].disksizetotal); diff --git a/ui/scripts/ui-custom/metricsView.js b/ui/scripts/ui-custom/metricsView.js index ef5dbba4ba4..fccbd00648d 100644 --- a/ui/scripts/ui-custom/metricsView.js +++ b/ui/scripts/ui-custom/metricsView.js @@ -17,28 +17,79 @@ (function($, cloudStack) { cloudStack.uiCustom.metricsView = function(args) { - return function() { + return function(ctxArgs) { + var metricsListView = cloudStack.sections.metrics.listView; var metricsLabel = _l('label.metrics'); + var context = {}; + if (ctxArgs && ctxArgs.hasOwnProperty('context')) { + context = ctxArgs.context; + } if (args.resource == 'zones') { - metricsListView = cloudStack.sections.metrics.zones.listView; metricsLabel = _l('label.zones') + ' ' + metricsLabel; + metricsListView = cloudStack.sections.metrics.zones.listView; + } else if (args.resource == 'clusters') { - metricsListView = cloudStack.sections.metrics.clusters.listView; metricsLabel = _l('label.clusters') + ' ' + metricsLabel; + metricsListView = cloudStack.sections.metrics.clusters.listView; + } else if (args.resource == 'hosts') { - metricsListView = cloudStack.sections.metrics.hosts.listView; metricsLabel = _l('label.hosts') + ' ' + metricsLabel; + metricsListView = cloudStack.sections.metrics.hosts.listView; + + if (context && !context.filterBy) { + if (context.hasOwnProperty('clusters') && context.clusters[0]) { + context.filterBy = 'clusterid'; + context.id = context.clusters[0].id; + } + if (context.hasOwnProperty('instances') && context.instances[0]) { + context.filterBy = 'virtualmachineid'; + context.id = context.instances[0].id; + } + } } else if (args.resource == 'storagepool') { - metricsListView = cloudStack.sections.metrics.storagepool.listView; metricsLabel = _l('label.primary.storage') + ' ' + metricsLabel; + metricsListView = cloudStack.sections.metrics.storagepool.listView; + } else if (args.resource == 'vms') { - metricsListView = cloudStack.sections.metrics.instances.listView; metricsLabel = _l('label.instances') + ' ' + metricsLabel; + metricsListView = cloudStack.sections.metrics.instances.listView; + metricsListView.advSearchFields = cloudStack.sections.instances.listView.advSearchFields; + + if (context && !context.filterBy) { + if (context.hasOwnProperty('hosts') && context.hosts[0]) { + context.filterBy = 'hostid'; + context.id = context.hosts[0].id; + } + } } else if (args.resource == 'volumes') { - metricsListView = cloudStack.sections.metrics.volumes.listView; metricsLabel = _l('label.volumes') + ' ' + metricsLabel; + metricsListView = cloudStack.sections.metrics.volumes.listView; + metricsListView.advSearchFields = cloudStack.sections.storage.sections.volumes.listView.advSearchFields; + metricsListView.groupableColumns = false; + + if (context && !context.filterBy) { + if (context.hasOwnProperty('instances') && context.instances[0]) { + context.filterBy = 'virtualmachineid'; + context.id = context.instances[0].id; + } + if (context.hasOwnProperty('primarystorages') && context.primarystorages[0]) { + context.filterBy = 'storageid'; + context.id = context.primarystorages[0].id; + } + } + } + + if (context.metricsFilterData) { + delete context.metricsFilterData; + } + + if (context.filterBy) { + context.metricsFilterData = { + key: context.filterBy, + value: context.id + }; } // list view refresh button @@ -56,24 +107,40 @@ } }; - metricsListView.hideSearchBar = true; + metricsListView.hideSearchBar = false; metricsListView.needsRefresh = true; metricsListView.noSplit = true; metricsListView.horizontalOverflow = true; metricsListView.groupableColumns = true; - if (args.resource == 'volumes') { - metricsListView.groupableColumns = false; - } + if (args.resource != 'vms' && args.resource != 'volumes' && args.resource != 'zones') { + metricsListView.advSearchFields = { + name: { + label: 'label.name' + }, + zoneid: { + label: 'label.zone', + select: function(args) { + $.ajax({ + url: createURL('listZones'), + data: { + listAll: true + }, + success: function(json) { + var zones = json.listzonesresponse.zone ? json.listzonesresponse.zone : []; - var metricsContext = cloudStack.context; - if (metricsContext.metricsFilterData) { - delete metricsContext.metricsFilterData; - } - if (args.filterBy) { - metricsContext.metricsFilterData = { - key: args.filterBy, - value: args.id + args.response.success({ + data: $.map(zones, function(zone) { + return { + id: zone.id, + description: zone.name + }; + }) + }); + } + }); + } + } }; } @@ -84,7 +151,7 @@ complete: function($newPanel) { $newPanel.listView({ $browser: $browser, - context: metricsContext, + context: context, listView: metricsListView }); // Make metrics tables horizontally scrollable @@ -105,7 +172,7 @@ } } $browser.cloudBrowser('removeLastPanel', {}); - var refreshedPanel = cloudStack.uiCustom.metricsView(args)(); + var refreshedPanel = cloudStack.uiCustom.metricsView(args)(ctxArgs); if (wasSorted && thClassName) { refreshedPanel.find('th.' + thClassName).filter(function() { return $(this).index() == thIndex; @@ -113,23 +180,25 @@ } }); - var filterMetricView = metricsListView.browseBy; - if (filterMetricView) { + var browseBy = metricsListView.browseBy; + if (browseBy) { $newPanel.bind('click', function(event) { event.stopPropagation(); var $target = $(event.target); var id = $target.closest('tr').data('list-view-item-id'); var jsonObj = $target.closest('tr').data('jsonObj'); - if (filterMetricView.filterKey && jsonObj) { - if (jsonObj.hasOwnProperty(filterMetricView.filterKey)) { - id = jsonObj[filterMetricView.filterKey]; + if (browseBy.filterKey && jsonObj) { + if (jsonObj.hasOwnProperty(browseBy.filterKey)) { + id = jsonObj[browseBy.filterKey]; } else { return; // return if provided key is missing } } if (id && ($target.hasClass('first') || $target.parent().hasClass('first')) && ($target.is('td') || $target.parent().is('td'))) { - filterMetricView.id = id; - cloudStack.uiCustom.metricsView(filterMetricView)(); + context.id = id; + context.filterBy = browseBy.filterBy; + ctxArgs.context = context; + cloudStack.uiCustom.metricsView({resource: browseBy.resource})(ctxArgs); } }); }