From 2e1c2821a8234530bebfc288177753d010c4bcb0 Mon Sep 17 00:00:00 2001 From: Charles Queiroz Date: Thu, 24 Aug 2023 09:13:47 +0100 Subject: [PATCH 01/13] UI: Add central project store and watch functionality (#7900) Added a new getter 'allProjects' and mutation 'RELOAD_ALL_PROJECTS' to centralize the tracking of available projects in the state. This eliminates direct manipulation of the Project list in separate components and improves data consistency across the application. A watcher in ProjectMenu.vue has been implemented to handle changes to the 'allProjects' getter. The 'RELOAD_ALL_PROJECTS' mutation was also added where necessary to ensure projects list is updated in the state whenever changes occur. --- ui/src/components/header/ProjectMenu.vue | 14 ++++++++++++-- ui/src/store/getters.js | 3 ++- ui/src/store/modules/app.js | 13 +++++++++++-- ui/src/views/AutogenView.vue | 4 +++- 4 files changed, 28 insertions(+), 6 deletions(-) diff --git a/ui/src/components/header/ProjectMenu.vue b/ui/src/components/header/ProjectMenu.vue index 95a12ffeb1d..9182303c765 100644 --- a/ui/src/components/header/ProjectMenu.vue +++ b/ui/src/components/header/ProjectMenu.vue @@ -102,9 +102,8 @@ export default { getNextPage() } }).finally(() => { - this.projects = _.orderBy(projects, ['displaytext'], ['asc']) - this.projects.unshift({ name: this.$t('label.default.view') }) this.loading = false + this.$store.commit('RELOAD_ALL_PROJECTS', projects) }) } getNextPage() @@ -125,6 +124,17 @@ export default { filterProject (input, option) { return option.label.toLowerCase().indexOf(input.toLowerCase()) >= 0 } + }, + mounted () { + this.$store.watch( + (state, getters) => getters.allProjects, + (newValue, oldValue) => { + if (oldValue !== newValue && newValue !== undefined) { + this.projects = _.orderBy(newValue, ['displaytext'], ['asc']) + this.projects.unshift({ name: this.$t('label.default.view') }) + } + } + ) } } diff --git a/ui/src/store/getters.js b/ui/src/store/getters.js index f6a6dab8ad2..0273fd02b19 100644 --- a/ui/src/store/getters.js +++ b/ui/src/store/getters.js @@ -47,7 +47,8 @@ const getters = { twoFaEnabled: state => state.user.twoFaEnabled, twoFaProvider: state => state.user.twoFaProvider, twoFaIssuer: state => state.user.twoFaIssuer, - loginFlag: state => state.user.loginFlag + loginFlag: state => state.user.loginFlag, + allProjects: (state) => state.app.allProjects } export default getters diff --git a/ui/src/store/modules/app.js b/ui/src/store/modules/app.js index 8ed27f88b4c..806e11b2adb 100644 --- a/ui/src/store/modules/app.js +++ b/ui/src/store/modules/app.js @@ -30,7 +30,8 @@ import { USE_BROWSER_TIMEZONE, SERVER_MANAGER, VUE_VERSION, - CUSTOM_COLUMNS + CUSTOM_COLUMNS, + RELOAD_ALL_PROJECTS } from '@/store/mutation-types' const app = { @@ -50,7 +51,8 @@ const app = { metrics: false, listAllProjects: false, server: '', - vueVersion: '' + vueVersion: '', + allProjects: [] }, mutations: { SET_SIDEBAR_TYPE: (state, type) => { @@ -121,6 +123,10 @@ const app = { SET_CUSTOM_COLUMNS: (state, customColumns) => { vueProps.$localStorage.set(CUSTOM_COLUMNS, customColumns) state.customColumns = customColumns + }, + RELOAD_ALL_PROJECTS: (state, allProjects = []) => { + vueProps.$localStorage.set(RELOAD_ALL_PROJECTS, allProjects) + state.allProjects = allProjects } }, actions: { @@ -177,6 +183,9 @@ const app = { }, SetCustomColumns ({ commit }, bool) { commit('SET_CUSTOM_COLUMNS', bool) + }, + ReloadAllProjects ({ commit, allProjects }) { + commit('RELOAD_ALL_PROJECTS', allProjects) } } } diff --git a/ui/src/views/AutogenView.vue b/ui/src/views/AutogenView.vue index c8e931b6be0..4bc0c43616f 100644 --- a/ui/src/views/AutogenView.vue +++ b/ui/src/views/AutogenView.vue @@ -693,8 +693,10 @@ export default { } api('listProjects', { id: projectId, listall: true, details: 'min' }).then(json => { if (!json || !json.listprojectsresponse || !json.listprojectsresponse.project) return + const projects = json.listprojectsresponse.project const project = json.listprojectsresponse.project[0] this.$store.dispatch('SetProject', project) + this.$store.commit('RELOAD_ALL_PROJECTS', projects) this.$store.dispatch('ToggleTheme', project.id === undefined ? 'light' : 'dark') this.$message.success(`${this.$t('message.switch.to')} "${project.name}"`) const query = Object.assign({}, this.$route.query) @@ -937,6 +939,7 @@ export default { } if (this.apiName === 'listProjects' && this.items.length > 0) { + this.$store.commit('RELOAD_ALL_PROJECTS', this.items) this.columns.map(col => { if (col.title === 'Account') { col.title = this.$t('label.project.owner') @@ -1808,7 +1811,6 @@ export default { this.rules[field.name].push(rule) break default: - console.log('hererere') rule.required = field.required rule.message = this.$t('message.error.required.input') this.rules[field.name].push(rule) From a0702279aae011316a5a5af7e206ad0309f06b41 Mon Sep 17 00:00:00 2001 From: Marcus Sorensen Date: Thu, 24 Aug 2023 02:45:39 -0600 Subject: [PATCH 02/13] server Don't allow inadvertent deletion of hidden details via API (#7880) * Don't allow inadvertent deletion of hidden details via API * Update VM details unit test ensuring system/hidden details not removed * Update test/integration/component/test_update_vm.py --------- Co-authored-by: Marcus Sorensen Co-authored-by: dahn --- .../java/com/cloud/vm/UserVmManagerImpl.java | 25 +++- .../com/cloud/vm/UserVmManagerImplTest.java | 29 +++- test/integration/component/test_update_vm.py | 124 ++++++++++++++++-- 3 files changed, 152 insertions(+), 26 deletions(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 94ceb0de363..c1840065603 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -2756,13 +2756,18 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir final List userReadOnlySettings = Stream.of(QueryService.UserVMReadOnlyDetails.value().split(",")) .map(item -> (item).trim()) .collect(Collectors.toList()); + List existingDetails = userVmDetailsDao.listDetails(id); if (cleanupDetails){ if (caller != null && caller.getType() == Account.Type.ADMIN) { - userVmDetailsDao.removeDetails(id); + for (final UserVmDetailVO detail : existingDetails) { + if (detail != null && detail.isDisplay()) { + userVmDetailsDao.removeDetail(id, detail.getName()); + } + } } else { - for (final UserVmDetailVO detail : userVmDetailsDao.listDetails(id)) { + for (final UserVmDetailVO detail : existingDetails) { if (detail != null && !userDenyListedSettings.contains(detail.getName()) - && !userReadOnlySettings.contains(detail.getName())) { + && !userReadOnlySettings.contains(detail.getName()) && detail.isDisplay()) { userVmDetailsDao.removeDetail(id, detail.getName()); } } @@ -2782,15 +2787,25 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir if (userReadOnlySettings.contains(detailName)) { throw new InvalidParameterValueException("You're not allowed to add or edit the read-only setting: " + detailName); } + if (existingDetails.stream().anyMatch(d -> Objects.equals(d.getName(), detailName) && !d.isDisplay())){ + throw new InvalidParameterValueException("You're not allowed to add or edit the non-displayable setting: " + detailName); + } } - // Add any hidden/denied or read-only detail - for (final UserVmDetailVO detail : userVmDetailsDao.listDetails(id)) { + // Add any existing user denied or read-only details. We do it here because admins would already provide these (or can delete them). + for (final UserVmDetailVO detail : existingDetails) { if (userDenyListedSettings.contains(detail.getName()) || userReadOnlySettings.contains(detail.getName())) { details.put(detail.getName(), detail.getValue()); } } } + // ensure details marked as non-displayable are maintained, regardless of admin or not + for (final UserVmDetailVO existingDetail : existingDetails) { + if (!existingDetail.isDisplay()) { + details.put(existingDetail.getName(), existingDetail.getValue()); + } + } + verifyVmLimits(vmInstance, details); vmInstance.setDetails(details); _vmDao.saveDetails(vmInstance); diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index 18b4a891917..22a9bed7764 100644 --- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java @@ -149,7 +149,7 @@ public class UserVmManagerImplTest { private EntityManager entityManager; @Mock - private UserVmDetailsDao userVmDetailVO; + private UserVmDetailsDao userVmDetailsDao; @Mock private UserVmVO userVmVoMock; @@ -300,7 +300,7 @@ public class UserVmManagerImplTest { verifyMethodsThatAreAlwaysExecuted(); Mockito.verify(userVmManagerImpl).updateDisplayVmFlag(false, vmId, userVmVoMock); - Mockito.verify(userVmDetailVO, Mockito.times(0)).removeDetails(vmId); + Mockito.verify(userVmDetailsDao, Mockito.times(0)).removeDetail(anyLong(), anyString()); } @Test @@ -310,12 +310,15 @@ public class UserVmManagerImplTest { Mockito.when(_serviceOfferingDao.findById(Mockito.anyLong(), Mockito.anyLong())).thenReturn((ServiceOfferingVO) offering); Mockito.when(updateVmCommand.isCleanupDetails()).thenReturn(true); Mockito.lenient().doNothing().when(userVmManagerImpl).updateDisplayVmFlag(false, vmId, userVmVoMock); - Mockito.doNothing().when(userVmDetailVO).removeDetails(vmId); + Mockito.when(updateVmCommand.getUserdataId()).thenReturn(null); + prepareExistingDetails(vmId, "userdetail"); + userVmManagerImpl.updateVirtualMachine(updateVmCommand); verifyMethodsThatAreAlwaysExecuted(); - Mockito.verify(userVmDetailVO).removeDetails(vmId); + Mockito.verify(userVmDetailsDao).removeDetail(vmId, "userdetail"); + Mockito.verify(userVmDetailsDao, Mockito.times(0)).removeDetail(vmId, "systemdetail"); Mockito.verify(userVmManagerImpl, Mockito.times(0)).updateDisplayVmFlag(false, vmId, userVmVoMock); } @@ -340,6 +343,16 @@ public class UserVmManagerImplTest { prepareAndExecuteMethodDealingWithDetails(false, false); } + private List prepareExistingDetails(Long vmId, String... existingDetailKeys) { + List existingDetails = new ArrayList<>(); + for (String detail : existingDetailKeys) { + existingDetails.add(new UserVmDetailVO(vmId, detail, "foo", true)); + } + existingDetails.add(new UserVmDetailVO(vmId, "systemdetail", "bar", false)); + Mockito.when(userVmDetailsDao.listDetails(vmId)).thenReturn(existingDetails); + return existingDetails; + } + private void prepareAndExecuteMethodDealingWithDetails(boolean cleanUpDetails, boolean isDetailsEmpty) throws ResourceUnavailableException, InsufficientCapacityException { configureDoNothingForMethodsThatWeDoNotWantToTest(); @@ -360,8 +373,9 @@ public class UserVmManagerImplTest { lenient().doNothing().when(_networkMgr).saveExtraDhcpOptions(anyString(), anyLong(), anyMap()); HashMap details = new HashMap<>(); if(!isDetailsEmpty) { - details.put("", ""); + details.put("newdetail", "foo"); } + prepareExistingDetails(vmId, "existingdetail"); Mockito.when(updateVmCommand.getUserdataId()).thenReturn(null); Mockito.when(updateVmCommand.getDetails()).thenReturn(details); Mockito.when(updateVmCommand.isCleanupDetails()).thenReturn(cleanUpDetails); @@ -371,14 +385,15 @@ public class UserVmManagerImplTest { verifyMethodsThatAreAlwaysExecuted(); Mockito.verify(userVmVoMock, Mockito.times(cleanUpDetails || isDetailsEmpty ? 0 : 1)).setDetails(details); - Mockito.verify(userVmDetailVO, Mockito.times(cleanUpDetails ? 1: 0)).removeDetails(vmId); + Mockito.verify(userVmDetailsDao, Mockito.times(cleanUpDetails ? 1 : 0)).removeDetail(vmId, "existingdetail"); + Mockito.verify(userVmDetailsDao, Mockito.times(0)).removeDetail(vmId, "systemdetail"); Mockito.verify(userVmDao, Mockito.times(cleanUpDetails || isDetailsEmpty ? 0 : 1)).saveDetails(userVmVoMock); Mockito.verify(userVmManagerImpl, Mockito.times(0)).updateDisplayVmFlag(false, vmId, userVmVoMock); } private void configureDoNothingForDetailsMethod() { Mockito.lenient().doNothing().when(userVmManagerImpl).updateDisplayVmFlag(false, vmId, userVmVoMock); - Mockito.doNothing().when(userVmDetailVO).removeDetails(vmId); + Mockito.doNothing().when(userVmDetailsDao).removeDetail(anyLong(), anyString()); Mockito.doNothing().when(userVmDao).saveDetails(userVmVoMock); } diff --git a/test/integration/component/test_update_vm.py b/test/integration/component/test_update_vm.py index ca20a5ad3f8..4d167c63330 100644 --- a/test/integration/component/test_update_vm.py +++ b/test/integration/component/test_update_vm.py @@ -15,11 +15,11 @@ # specific language governing permissions and limitations # under the License. - from marvin.cloudstackTestCase import cloudstackTestCase from marvin.lib.base import Account, VirtualMachine, ServiceOffering -from marvin.lib.utils import cleanup_resources +from marvin.lib.utils import (validateList, cleanup_resources) from marvin.lib.common import get_zone, get_domain, get_template +from marvin.codes import PASS from nose.plugins.attrib import attr class TestData(object): @@ -59,6 +59,7 @@ class TestUpdateVirtualMachine(cloudstackTestCase): def setUp(self): self.testdata = TestData().testdata self.apiclient = self.testClient.getApiClient() + self.dbclient = self.testClient.getDbConnection() # Get Zone, Domain and Default Built-in template self.domain = get_domain(self.apiclient) @@ -106,18 +107,7 @@ class TestUpdateVirtualMachine(cloudstackTestCase): templateid=self.template.id ) - list_vms = VirtualMachine.list(self.apiclient, id=self.virtual_machine.id) - self.assertEqual( - isinstance(list_vms, list), - True, - "List VM response was not a valid list" - ) - self.assertNotEqual( - len(list_vms), - 0, - "List VM response was empty" - ) - vm = list_vms[0] + vm = self.listVmById(self.virtual_machine.id) self.debug( "VirtualMachine launched with id, name, displayname: %s %s %s"\ @@ -152,6 +142,112 @@ class TestUpdateVirtualMachine(cloudstackTestCase): self.assertEqual(vmnew.displayname, vmnewstarted.displayname, msg="display name changed on start, displayname is %s" % vmnewstarted.displayname) + @attr(tags=['advanced', 'simulator', 'basic', 'sg', 'details'], required_hardware="false") + def test_update_vm_details_admin(self): + """Test Update VirtualMachine Details + + # Set up a VM + # Set up hidden detail in DB for VM + + # Validate the following: + # 1. Can add two details (detail1, detail2) + # 2. Can fetch new details on VM + # 3. Can delete detail1 + # 4. Hidden detail not removed + # 6. The detail2 remains + # 7. Ensure cleanup parameter doesn't remove hidden details + """ + hidden_detail_name = "configDriveLocation" + detail1 = "detail1" + detail2 = "detail2" + + # set up a VM + self.virtual_machine = VirtualMachine.create( + self.apiclient, + self.testdata["virtual_machine"], + accountid=self.account.name, + zoneid=self.zone.id, + domainid=self.account.domainid, + serviceofferingid=self.service_offering.id, + templateid=self.template.id + ) + self.cleanup.append(self.virtual_machine) + + vm = self.listVmById(self.virtual_machine.id) + + self.debug( + "VirtualMachine launched with id, name, displayname: %s %s %s" \ + % (self.virtual_machine.id, vm.name, vm.displayname) + ) + + # set up a hidden detail + dbresult = self.dbclient.execute("select id from vm_instance where uuid='%s'" % vm.id) + self.assertEqual(validateList(dbresult)[0], PASS, "sql query returned invalid response") + vm_db_id = dbresult[0][0] + self.debug("VM has database id %d" % vm_db_id) + + self.dbclient.execute("insert into user_vm_details (vm_id, name, value, display) values (%d,'%s','HOST', 0)" % (vm_db_id, hidden_detail_name)) + + vm = self.listVmById(self.virtual_machine.id) + self.debug("VirtualMachine fetched with details: %s of type %s" % (vm.details, type(vm.details))) + + self.assertIsNone(vm.details[hidden_detail_name], "hidden detail should be hidden") + + # add two details by appending to what was returned via API + updating_vm_details = vm.details.__dict__ + updating_vm_details[detail1] = "foo" + updating_vm_details[detail2] = "bar" + + self.debug("Updating VM to new details: %s" % updating_vm_details) + vm = self.virtual_machine.update(self.apiclient, details=[updating_vm_details]) + + self.assertIsNotNone(vm.details[detail1], "Expect " + detail1) + self.assertIsNotNone(vm.details[detail2], "Expect " + detail2) + self.assertIsNone(vm.details[hidden_detail_name], "hidden detail should be hidden") + self.assertTrue(self.detailInDatabase(vm_db_id, hidden_detail_name), "hidden detail should still exist in db") + + # delete one detail + updating_vm_details = vm.details.__dict__ + del updating_vm_details["detail1"] + + self.debug("Deleting one detail by updating details: %s" % updating_vm_details) + vm = self.virtual_machine.update(self.apiclient, details=[updating_vm_details]) + + self.assertIsNone(vm.details[detail1], "Do not expect " + detail1) + self.assertIsNotNone(vm.details[detail2], "Expect " + detail2) + self.assertIsNone(vm.details[hidden_detail_name], "hidden detail should be hidden") + self.assertTrue(self.detailInDatabase(vm_db_id, hidden_detail_name), "hidden detail should still exist in db") + + # cleanup, ensure hidden detail is not deleted + vm = self.virtual_machine.update(self.apiclient, cleanupdetails="true") + self.assertIsNone(vm.details[detail1], "Do not expect " + detail1) + self.assertIsNone(vm.details[detail2], "Do not expect " + detail2) + self.assertIsNone(vm.details[hidden_detail_name], "hidden detail should be hidden") + self.assertTrue(self.detailInDatabase(vm_db_id, hidden_detail_name), "hidden detail should still exist in db") + + + def detailInDatabase(self, vm_id, detail_name): + dbresult = self.dbclient.execute("select id from user_vm_details where vm_id=%s and name='%s'" % (vm_id, detail_name)) + self.debug("Detail %s for VM %s: %s" % (detail_name, vm_id, dbresult)) + if validateList(dbresult)[0] == PASS: + return True + return False + + + def listVmById(self, id): + list_vms = VirtualMachine.list(self.apiclient, id=id) + self.assertEqual( + isinstance(list_vms, list), + True, + "List VM response was not a valid list" + ) + self.assertNotEqual( + len(list_vms), + 0, + "List VM response was empty" + ) + return list_vms[0] + def tearDown(self): try: cleanup_resources(self.apiclient, self.cleanup) From 8ad1009ad2d59571f48a89b94d9c1424a46030f9 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Thu, 24 Aug 2023 14:59:25 +0530 Subject: [PATCH 03/13] ui: fix notification list reordering intermittently (#7609) Signed-off-by: Abhishek Kumar --- ui/src/components/header/HeaderNotice.vue | 2 +- ui/src/store/modules/user.js | 6 +++++- ui/src/utils/plugins.js | 9 ++++++--- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/ui/src/components/header/HeaderNotice.vue b/ui/src/components/header/HeaderNotice.vue index fe9c579581e..82dc4d45edc 100644 --- a/ui/src/components/header/HeaderNotice.vue +++ b/ui/src/components/header/HeaderNotice.vue @@ -109,7 +109,7 @@ export default { (state, getters) => getters.headerNotices, (newValue, oldValue) => { if (oldValue !== newValue && newValue !== undefined) { - this.notices = newValue.reverse() + this.notices = newValue } } ) diff --git a/ui/src/store/modules/user.js b/ui/src/store/modules/user.js index 3e9aef7af08..3994a3fc29d 100644 --- a/ui/src/store/modules/user.js +++ b/ui/src/store/modules/user.js @@ -348,9 +348,13 @@ const user = { if (noticeIdx === -1) { noticeArray.push(noticeJson) } else { + const existingNotice = noticeArray[noticeIdx] + noticeJson.timestamp = existingNotice.timestamp noticeArray[noticeIdx] = noticeJson } - + noticeArray.sort(function (a, b) { + return new Date(b.timestamp) - new Date(a.timestamp) + }) commit('SET_HEADER_NOTICES', noticeArray) }, ProjectView ({ commit }, projectid) { diff --git a/ui/src/utils/plugins.js b/ui/src/utils/plugins.js index 5cc4bc6269a..7223f050368 100644 --- a/ui/src/utils/plugins.js +++ b/ui/src/utils/plugins.js @@ -65,7 +65,8 @@ export const pollJobPlugin = { key: jobId, title, description, - status: 'progress' + status: 'progress', + timestamp: new Date() }) eventBus.on('update-job-details', (args) => { @@ -107,7 +108,8 @@ export const pollJobPlugin = { title, description, status: 'done', - duration: 2 + duration: 2, + timestamp: new Date() }) eventBus.emit('update-job-details', { jobId, resourceId }) // Ensure we refresh on the same / parent page @@ -157,7 +159,8 @@ export const pollJobPlugin = { title, description: desc, status: 'failed', - duration: 2 + duration: 2, + timestamp: new Date() }) eventBus.emit('update-job-details', { jobId, resourceId }) // Ensure we refresh on the same / parent page From 47b6f0fd0565fc15a6034baaaad52f8a70163f7d Mon Sep 17 00:00:00 2001 From: Vishesh Date: Thu, 24 Aug 2023 18:37:11 +0530 Subject: [PATCH 04/13] Fix tungsten unit test (#7904) --- .../api/command/ListTungstenFabricAddressGroupCmdTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/network-elements/tungsten/src/test/java/org/apache/cloudstack/network/tungsten/api/command/ListTungstenFabricAddressGroupCmdTest.java b/plugins/network-elements/tungsten/src/test/java/org/apache/cloudstack/network/tungsten/api/command/ListTungstenFabricAddressGroupCmdTest.java index 775a7bbd40c..d797d29d7a0 100644 --- a/plugins/network-elements/tungsten/src/test/java/org/apache/cloudstack/network/tungsten/api/command/ListTungstenFabricAddressGroupCmdTest.java +++ b/plugins/network-elements/tungsten/src/test/java/org/apache/cloudstack/network/tungsten/api/command/ListTungstenFabricAddressGroupCmdTest.java @@ -53,6 +53,7 @@ public class ListTungstenFabricAddressGroupCmdTest { ReflectionTestUtils.setField(listTungstenFabricAddressGroupCmd, "addressGroupUuid", "test"); ReflectionTestUtils.setField(listTungstenFabricAddressGroupCmd, "page", 1); ReflectionTestUtils.setField(listTungstenFabricAddressGroupCmd, "pageSize", 10); + ReflectionTestUtils.setField(listTungstenFabricAddressGroupCmd, "s_maxPageSize", -1L); } @After From c683de4a55b1c96f58527c86523f5e8fd24802a9 Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Thu, 24 Aug 2023 18:11:04 +0200 Subject: [PATCH 05/13] kvm: fix unit test LibvirtReplugNicCommandWrapperTest (#7908) --- .../wrapper/LibvirtReplugNicCommandWrapperTest.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtReplugNicCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtReplugNicCommandWrapperTest.java index 50864e4bb8c..8a6827f4139 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtReplugNicCommandWrapperTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtReplugNicCommandWrapperTest.java @@ -236,6 +236,8 @@ public class LibvirtReplugNicCommandWrapperTest { bridgeVifDriver.configure(params); ovsVifDriver.configure(params); } + + LibvirtVMDef.setGlobalLibvirtVersion(6400000L); } @Test @@ -246,6 +248,10 @@ public class LibvirtReplugNicCommandWrapperTest { + "\n" + "\n" + "\n" + + "\n" + + "\n" + + "\n" + + "\n" + "\n" + "\n"; final String expectedAttachXml = From 3c38ed7a6567b95f3c18c094094e0447231c0aa2 Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Thu, 24 Aug 2023 18:12:01 +0200 Subject: [PATCH 06/13] server: allow user to list available IPs on shared networks (#7898) This fixes #7817 --- .../java/com/cloud/network/IpAddressManager.java | 4 ++++ .../com/cloud/network/IpAddressManagerImpl.java | 2 +- .../com/cloud/server/ManagementServerImpl.java | 16 +++++++++++++++- 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/engine/components-api/src/main/java/com/cloud/network/IpAddressManager.java b/engine/components-api/src/main/java/com/cloud/network/IpAddressManager.java index 2fa66d7166b..73d3de0ef0c 100644 --- a/engine/components-api/src/main/java/com/cloud/network/IpAddressManager.java +++ b/engine/components-api/src/main/java/com/cloud/network/IpAddressManager.java @@ -56,6 +56,10 @@ public interface IpAddressManager { "Set placement of vrouter ips in redundant mode in vpc tiers, this can be 3 value: `first` to use first ips in tiers, `last` to use last ips in tiers and `random` to take random ips in tiers.", true, ConfigKey.Scope.Account, null, null, null, null, null, ConfigKey.Kind.Select, "first,last,random"); + ConfigKey AllowUserListAvailableIpsOnSharedNetwork = new ConfigKey("Advanced", Boolean.class, "allow.user.list.available.ips.on.shared.network", "false", + "Determines whether users can list available IPs on shared networks", + true, ConfigKey.Scope.Global); + /** * Assigns a new public ip address. * diff --git a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java index 5436dd6acb1..60e7c5d12bf 100644 --- a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java +++ b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java @@ -2342,7 +2342,7 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage @Override public ConfigKey[] getConfigKeys() { - return new ConfigKey[] {UseSystemPublicIps, RulesContinueOnError, SystemVmPublicIpReservationModeStrictness, VrouterRedundantTiersPlacement}; + return new ConfigKey[] {UseSystemPublicIps, RulesContinueOnError, SystemVmPublicIpReservationModeStrictness, VrouterRedundantTiersPlacement, AllowUserListAvailableIpsOnSharedNetwork}; } /** diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index c6ace852bd3..31a78744153 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -2323,6 +2323,7 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe isAllocated = Boolean.TRUE; } } + boolean isAllocatedTemp = isAllocated; VlanType vlanType = null; if (forVirtualNetwork != null) { @@ -2333,6 +2334,7 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe final Account caller = getCaller(); List addrs = new ArrayList<>(); + NetworkVO network = null; // shared network if (vlanType == VlanType.DirectAttached && networkId == null && ipId == null) { // only root admin can list public ips in all shared networks if (caller.getType() != Account.Type.ADMIN) { @@ -2341,7 +2343,6 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe } else if (vlanType == VlanType.DirectAttached) { // list public ip address on shared network // access control. admin: all Ips, domain admin/user: all Ips in shared network in the domain/sub-domain/user - NetworkVO network = null; if (networkId == null) { IPAddressVO ip = _publicIpAddressDao.findById(ipId); if (ip == null) { @@ -2475,7 +2476,20 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe for (IPAddressVO addr: freeAddrs) { freeAddrIds.add(addr.getId()); } + } else if (vlanType == VlanType.DirectAttached && network != null && !isAllocatedTemp && isAllocated) { + if (caller.getType() != Account.Type.ADMIN && !IpAddressManager.AllowUserListAvailableIpsOnSharedNetwork.value()) { + s_logger.debug("Non-admin users are not allowed to list available IPs on shared networks"); + } else { + final SearchBuilder searchBuilder = _publicIpAddressDao.createSearchBuilder(); + buildParameters(searchBuilder, cmd, false); + + SearchCriteria searchCriteria = searchBuilder.create(); + setParameters(searchCriteria, cmd, vlanType, false); + searchCriteria.setParameters("state", IpAddress.State.Free.name()); + addrs.addAll(_publicIpAddressDao.search(searchCriteria, searchFilter)); // Free IPs on shared network + } } + if (freeAddrIds.size() > 0) { final SearchBuilder sb2 = _publicIpAddressDao.createSearchBuilder(); buildParameters(sb2, cmd, false); From e964395bd45c390e648f7c9cd567943c45276d69 Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Fri, 25 Aug 2023 11:36:03 +0530 Subject: [PATCH 07/13] vmware: improve solidfire storage plugin integration and fix cases (#3) (#7761) This fixes the following cases in which Solidfire storage integration caused issues when using Solidfire datadisks with VMware: 1. Take Volume Snapshot of Solidfire data disk 2. Delete an active Instance with Solidfire data disk attached 3. Attach used existing Solidfire data disk to a running/stopped VM 4. Stop and Start an instance with Solidfire data disks attached 5. Expand disk by resizing Solidfire data disk by providing size 6. Expand disk by changing disk offering for the Solidfire data disk Additional changes: - Use VMFS6 as managed datastore type if the host supports - Refactor detection and splitting of managed storage ds name in storage processor - Restrict storage rescanning for managed datastore when resizing Signed-off-by: Rohit Yadav --- .../orchestration/VolumeOrchestrator.java | 4 +-- .../manager/VmwareStorageManagerImpl.java | 30 ++++++++++++------- .../vmware/resource/VmwareResource.java | 5 ++++ .../resource/VmwareStorageProcessor.java | 12 ++++++-- .../manager/VmwareStorageManagerImplTest.java | 11 +++++++ .../SolidFirePrimaryDataStoreDriver.java | 30 +++++++++++++++++-- .../vmware/mo/HostDatastoreSystemMO.java | 17 ++++++----- .../hypervisor/vmware/util/VmwareHelper.java | 1 + 8 files changed, 86 insertions(+), 24 deletions(-) diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java index 52bcf5033af..c3908795f7c 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java @@ -1906,8 +1906,8 @@ public class VolumeOrchestrator extends ManagerBase implements VolumeOrchestrati throw new StorageAccessException(String.format("Unable to grant access to volume [%s] on host [%s].", volToString, host)); } } else { - // This might impact other managed storages, grant access for PowerFlex storage pool only - if (pool.getPoolType() == Storage.StoragePoolType.PowerFlex) { + // This might impact other managed storages, grant access for PowerFlex and Iscsi/Solidfire storage pool only + if (pool.getPoolType() == Storage.StoragePoolType.PowerFlex || pool.getPoolType() == Storage.StoragePoolType.Iscsi) { try { volService.grantAccess(volFactory.getVolume(vol.getId()), host, (DataStore)pool); } catch (Exception e) { diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/manager/VmwareStorageManagerImpl.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/manager/VmwareStorageManagerImpl.java index 5b94858b403..d7a736e80e4 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/manager/VmwareStorageManagerImpl.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/manager/VmwareStorageManagerImpl.java @@ -1107,12 +1107,25 @@ public class VmwareStorageManagerImpl implements VmwareStorageManager { return "snapshots/" + accountId + "/" + volumeId; } + protected boolean isManagedStorageDatastorePath(final String datastorePath) { + // ex. [-iqn.2010-01.com.solidfire:3p53.data-9999.97-0] i-2-9999-VM + return datastorePath != null && datastorePath.startsWith("[-iqn."); + } + + protected String getManagedDatastoreName(final String datastorePath) { + // ex. [-iqn.2010-01.com.solidfire:3p53.data-9999.97-0] + return datastorePath == null ? datastorePath : datastorePath.split(" ")[0]; + } + private long getVMSnapshotChainSize(VmwareContext context, VmwareHypervisorHost hyperHost, String fileName, ManagedObjectReference morDs, String exceptFileName, String vmName) throws Exception { long size = 0; DatastoreMO dsMo = new DatastoreMO(context, morDs); HostDatastoreBrowserMO browserMo = dsMo.getHostDatastoreBrowserMO(); String datastorePath = (new DatastoreFile(dsMo.getName(), vmName)).getPath(); + if (isManagedStorageDatastorePath(datastorePath)) { + datastorePath = getManagedDatastoreName(datastorePath); + } HostDatastoreBrowserSearchSpec searchSpec = new HostDatastoreBrowserSearchSpec(); FileQueryFlags fqf = new FileQueryFlags(); fqf.setFileSize(true); @@ -1241,11 +1254,9 @@ public class VmwareStorageManagerImpl implements VmwareStorageManager { String vmdkName = null; // if this is managed storage - if (fullPath.startsWith("[-iqn.")) { // ex. [-iqn.2010-01.com.company:3y8w.vol-10.64-0] -iqn.2010-01.com.company:3y8w.vol-10.64-0-000001.vmdk - baseName = fullPath.split(" ")[0]; // ex. [-iqn.2010-01.com.company:3y8w.vol-10.64-0] - - // remove '[' and ']' - baseName = baseName.substring(1, baseName.length() - 1); + if (isManagedStorageDatastorePath(fullPath)) { + baseName = getManagedDatastoreName(fullPath); + baseName = baseName.substring(1, baseName.length() - 1); // remove '[' and ']' vmdkName = fullPath; // for managed storage, vmdkName == fullPath } else { @@ -1288,12 +1299,9 @@ public class VmwareStorageManagerImpl implements VmwareStorageManager { } } else { Map mapNewDisk = getNewDiskMap(vmMo); - // if this is managed storage - if (path.startsWith("[-iqn.")) { // ex. [-iqn.2010-01.com.company:3y8w.vol-10.64-0] -iqn.2010-01.com.company:3y8w.vol-10.64-0-000001.vmdk - path = path.split(" ")[0]; // ex. [-iqn.2010-01.com.company:3y8w.vol-10.64-0] - - // remove '[' and ']' - baseName = path.substring(1, path.length() - 1); + if (isManagedStorageDatastorePath(path)) { + path = getManagedDatastoreName(path); + baseName = path.substring(1, path.length() - 1); // remove '[' and ']' } else { baseName = VmwareHelper.trimSnapshotDeltaPostfix(path); } diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java index 01d6f6816e0..e76b34954c6 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java @@ -940,6 +940,11 @@ public class VmwareResource extends ServerResourceBase implements StoragePoolRes ManagedObjectReference morDS = HypervisorHostHelper.findDatastoreWithBackwardsCompatibility(hyperHost, VmwareResource.getDatastoreName(iScsiName)); DatastoreMO dsMo = new DatastoreMO(hyperHost.getContext(), morDS); + if (path.startsWith("[-iqn.")) { + // Rescan 1:1 LUN that VMware may not know the LUN was recently resized + _storageProcessor.rescanAllHosts(context, lstHosts, true, true); + } + _storageProcessor.expandDatastore(hostDatastoreSystem, dsMo); } diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java index 7b4d5f118b4..ba9d16f8186 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java @@ -2825,7 +2825,15 @@ public class VmwareStorageProcessor implements StorageProcessor { morDs = firstHostDatastoreSystemMO.findDatastoreByName(datastoreName); if (morDs == null) { - morDs = firstHostDatastoreSystemMO.createVmfsDatastore(datastoreName, hostScsiDisk); + final String hostVersion = firstHostMO.getProductVersion(); + if (hostVersion.compareTo(VmwareHelper.MIN_VERSION_VMFS6) >= 0) { + morDs = firstHostDatastoreSystemMO.createVmfs6Datastore(datastoreName, hostScsiDisk); + } else { + morDs = firstHostDatastoreSystemMO.createVmfs5Datastore(datastoreName, hostScsiDisk); + } + } else { + // in case of iSCSI/solidfire 1:1 VMFS datastore could be inaccessible + mountVmfsDatastore(new DatastoreMO(context, morDs), lstHosts); } if (morDs != null) { @@ -3364,7 +3372,7 @@ public class VmwareStorageProcessor implements StorageProcessor { } } - private void rescanAllHosts(VmwareContext context, List> lstHostPairs, boolean rescanHba, boolean rescanVmfs) throws Exception { + public void rescanAllHosts(VmwareContext context, List> lstHostPairs, boolean rescanHba, boolean rescanVmfs) throws Exception { List hosts = new ArrayList<>(lstHostPairs.size()); for (Pair hostPair : lstHostPairs) { diff --git a/plugins/hypervisors/vmware/src/test/java/com/cloud/hypervisor/vmware/manager/VmwareStorageManagerImplTest.java b/plugins/hypervisors/vmware/src/test/java/com/cloud/hypervisor/vmware/manager/VmwareStorageManagerImplTest.java index 1d207e37bca..c72f23c628e 100644 --- a/plugins/hypervisors/vmware/src/test/java/com/cloud/hypervisor/vmware/manager/VmwareStorageManagerImplTest.java +++ b/plugins/hypervisors/vmware/src/test/java/com/cloud/hypervisor/vmware/manager/VmwareStorageManagerImplTest.java @@ -116,4 +116,15 @@ public class VmwareStorageManagerImplTest { public void testSetVolumeToPathAndSizeDatastoreClusterDifferentChildStore() { testCommon(Storage.StoragePoolType.PreSetup, Storage.StoragePoolType.DatastoreCluster, true); } + + @Test + public void testIsManagedStorageDatastorePath() { + Assert.assertTrue("Test if [-iqn... is a managed storage", storageManager.isManagedStorageDatastorePath("[-iqn.2010-01.com.solidfire:3p53.data-9999.97-0] i-2-9999-VM.vmdk")); + Assert.assertFalse("Test if [SomeDS] is not a managed storage", storageManager.isManagedStorageDatastorePath("[SomeDS] i-2-9999-VM/disk.vmdk")); + } + + @Test + public void testGetManagedDatastoreName() { + Assert.assertEquals("[-iqn.2010-01.com.solidfire:3p53.data-9999.97-0]", storageManager.getManagedDatastoreName("[-iqn.2010-01.com.solidfire:3p53.data-9999.97-0] i-2-9999-VM.vmdk")); + } } diff --git a/plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/driver/SolidFirePrimaryDataStoreDriver.java b/plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/driver/SolidFirePrimaryDataStoreDriver.java index 702bdc3669a..4478dc98ca4 100644 --- a/plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/driver/SolidFirePrimaryDataStoreDriver.java +++ b/plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/driver/SolidFirePrimaryDataStoreDriver.java @@ -17,6 +17,7 @@ package org.apache.cloudstack.storage.datastore.driver; import java.text.NumberFormat; +import java.util.Arrays; import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -85,6 +86,8 @@ import com.cloud.user.dao.AccountDao; import com.cloud.utils.Pair; import com.cloud.utils.db.GlobalLock; import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.vm.dao.VMInstanceDao; +import com.cloud.vm.VirtualMachine; import com.google.common.base.Preconditions; public class SolidFirePrimaryDataStoreDriver implements PrimaryDataStoreDriver { @@ -111,6 +114,7 @@ public class SolidFirePrimaryDataStoreDriver implements PrimaryDataStoreDriver { @Inject private PrimaryDataStoreDao storagePoolDao; @Inject private StoragePoolDetailsDao storagePoolDetailsDao; @Inject private VMTemplatePoolDao vmTemplatePoolDao; + @Inject private VMInstanceDao vmDao; @Inject private VolumeDao volumeDao; @Inject private VolumeDetailsDao volumeDetailsDao; @Inject private VolumeDataFactory volumeFactory; @@ -187,13 +191,33 @@ public class SolidFirePrimaryDataStoreDriver implements PrimaryDataStoreDriver { } } + private boolean isRevokeAccessNotNeeded(DataObject dataObject) { + // Workaround: don't unplug iscsi lun when volume is attached to a VM + // This is regression workaround from upper layers which are calling + // a releaseVmResources() method that calls the revoke on an attached disk + if (dataObject.getType() == DataObjectType.VOLUME) { + Volume volume = volumeDao.findById(dataObject.getId()); + if (volume.getInstanceId() != null) { + VirtualMachine vm = vmDao.findById(volume.getInstanceId()); + if (vm != null && !Arrays.asList(VirtualMachine.State.Destroyed, VirtualMachine.State.Expunging, VirtualMachine.State.Error).contains(vm.getState())) { + return true; + } + } + } + return false; + } + @Override - public void revokeAccess(DataObject dataObject, Host host, DataStore dataStore) - { + public void revokeAccess(DataObject dataObject, Host host, DataStore dataStore) { if (dataObject == null || host == null || dataStore == null) { return; } + if (isRevokeAccessNotNeeded(dataObject)) { + LOGGER.debug("Skipping revoke access for Solidfire data object type:" + dataObject.getType() + " id:" + dataObject.getId()); + return; + } + long sfVolumeId = getSolidFireVolumeId(dataObject, false); long clusterId = host.getClusterId(); long storagePoolId = dataStore.getId(); @@ -210,6 +234,8 @@ public class SolidFirePrimaryDataStoreDriver implements PrimaryDataStoreDriver { throw new CloudRuntimeException(errMsg); } + LOGGER.debug("Revoking access for Solidfire data object type:" + dataObject.getType() + " id:" + dataObject.getId()); + try { SolidFireUtil.SolidFireConnection sfConnection = SolidFireUtil.getSolidFireConnection(storagePoolId, storagePoolDetailsDao); diff --git a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HostDatastoreSystemMO.java b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HostDatastoreSystemMO.java index 30798e31e19..c2fe3f4e54c 100644 --- a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HostDatastoreSystemMO.java +++ b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HostDatastoreSystemMO.java @@ -199,18 +199,21 @@ public class HostDatastoreSystemMO extends BaseMO { return _context.getService().queryAvailableDisksForVmfs(_mor, null); } - public ManagedObjectReference createVmfsDatastore(String datastoreName, HostScsiDisk hostScsiDisk) throws Exception { - // just grab the first instance of VmfsDatastoreOption - VmfsDatastoreOption vmfsDatastoreOption = _context.getService().queryVmfsDatastoreCreateOptions(_mor, hostScsiDisk.getDevicePath(), 5).get(0); - + public ManagedObjectReference createVmfsDatastore(String datastoreName, HostScsiDisk hostScsiDisk, Integer vmfsVersion) throws Exception { + VmfsDatastoreOption vmfsDatastoreOption = _context.getService().queryVmfsDatastoreCreateOptions(_mor, hostScsiDisk.getDevicePath(), vmfsVersion).get(0); VmfsDatastoreCreateSpec vmfsDatastoreCreateSpec = (VmfsDatastoreCreateSpec)vmfsDatastoreOption.getSpec(); - - // set the name of the datastore to be created vmfsDatastoreCreateSpec.getVmfs().setVolumeName(datastoreName); - return _context.getService().createVmfsDatastore(_mor, vmfsDatastoreCreateSpec); } + public ManagedObjectReference createVmfs5Datastore(String datastoreName, HostScsiDisk hostScsiDisk) throws Exception { + return createVmfsDatastore(datastoreName, hostScsiDisk, 5); + } + + public ManagedObjectReference createVmfs6Datastore(String datastoreName, HostScsiDisk hostScsiDisk) throws Exception { + return createVmfsDatastore(datastoreName, hostScsiDisk, 6); + } + public boolean deleteDatastore(String name) throws Exception { ManagedObjectReference morDatastore = findDatastore(name); if (morDatastore != null) { diff --git a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java index 4a81beeff98..841d914af32 100644 --- a/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java +++ b/vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java @@ -98,6 +98,7 @@ public class VmwareHelper { public static final int MAX_SUPPORTED_DEVICES_SCSI_CONTROLLER = MAX_ALLOWED_DEVICES_SCSI_CONTROLLER - 1; // One device node is unavailable for hard disks or SCSI devices public static final int MAX_USABLE_SCSI_CONTROLLERS = 2; public static final String MIN_VERSION_UEFI_LEGACY = "5.5"; + public static final String MIN_VERSION_VMFS6 = "6.5"; public static boolean isReservedScsiDeviceNumber(int deviceNumber) { // The SCSI controller is assigned to virtual device node (z:7), so that device node is unavailable for hard disks or SCSI devices. From f5a1f4130d554c648d030c14cdd7ad212976e9cf Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Fri, 25 Aug 2023 11:35:31 +0200 Subject: [PATCH 08/13] server: fix global setting system.vm.public.ip.reservation.mode.strictness is not really dynamic (#7909) If the original value is `false`, and search build is configured without the condition. Now change the value to `true`, it will not get effective due to missing condition. --- .../main/java/com/cloud/network/IpAddressManagerImpl.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java index 60e7c5d12bf..bb5371ed271 100644 --- a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java +++ b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java @@ -496,9 +496,8 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage AssignIpAddressSearch.and("dc", AssignIpAddressSearch.entity().getDataCenterId(), Op.EQ); AssignIpAddressSearch.and("allocated", AssignIpAddressSearch.entity().getAllocatedTime(), Op.NULL); AssignIpAddressSearch.and("vlanId", AssignIpAddressSearch.entity().getVlanId(), Op.IN); - if (SystemVmPublicIpReservationModeStrictness.value()) { - AssignIpAddressSearch.and("forSystemVms", AssignIpAddressSearch.entity().isForSystemVms(), Op.EQ); - } + AssignIpAddressSearch.and("forSystemVms", AssignIpAddressSearch.entity().isForSystemVms(), Op.EQ); + SearchBuilder vlanSearch = _vlanDao.createSearchBuilder(); vlanSearch.and("type", vlanSearch.entity().getVlanType(), Op.EQ); vlanSearch.and("networkId", vlanSearch.entity().getNetworkId(), Op.EQ); From 8dc5fdd067d555e3fdb04704627bca043b766174 Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Fri, 25 Aug 2023 11:36:39 +0200 Subject: [PATCH 09/13] server: fix cannot get systemvm ips in dedicated ranges (#7144) This fixes #6698 --- .../java/com/cloud/api/ApiResponseHelper.java | 2 +- .../ConfigurationManagerImpl.java | 8 +- .../cloud/network/IpAddressManagerImpl.java | 275 +++++++++--------- .../integration/smoke/test_public_ip_range.py | 3 +- ui/public/locales/en.json | 1 + .../views/infra/network/IpRangesTabPublic.vue | 6 +- 6 files changed, 151 insertions(+), 144 deletions(-) diff --git a/server/src/main/java/com/cloud/api/ApiResponseHelper.java b/server/src/main/java/com/cloud/api/ApiResponseHelper.java index 05aee6b6ccb..17c465d40ce 100644 --- a/server/src/main/java/com/cloud/api/ApiResponseHelper.java +++ b/server/src/main/java/com/cloud/api/ApiResponseHelper.java @@ -901,7 +901,7 @@ public class ApiResponseHelper implements ResponseGenerator { Long networkId = vlan.getNetworkId(); if (networkId != null) { Network network = _ntwkModel.getNetwork(networkId); - if (network != null) { + if (network != null && TrafficType.Guest.equals(network.getTrafficType())) { Long accountId = network.getAccountId(); populateAccount(vlanResponse, accountId); populateDomain(vlanResponse, ApiDBUtils.findAccountById(accountId).getDomainId()); diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java index 737cdc769f6..455a964b8d7 100644 --- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java +++ b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java @@ -5397,10 +5397,11 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati // Check if any of the Public IP addresses is allocated to another // account - boolean forSystemVms = false; final List ips = _publicIpAddressDao.listByVlanId(vlanDbId); for (final IPAddressVO ip : ips) { - forSystemVms = ip.isForSystemVms(); + if (ip.isForSystemVms()) { + throw new InvalidParameterValueException(ip.getAddress() + " Public IP address in range is dedicated to system vms "); + } final Long allocatedToAccountId = ip.getAllocatedToAccountId(); if (allocatedToAccountId != null) { if (vlanOwner != null && allocatedToAccountId != vlanOwner.getId()) { @@ -5425,7 +5426,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati UsageEventUtils.publishUsageEvent(EventTypes.EVENT_NET_IP_ASSIGN, vlanOwner.getId(), ip.getDataCenterId(), ip.getId(), ip.getAddress().toString(), ip.isSourceNat(), vlan.getVlanType().toString(), ip.getSystem(), usageHidden, ip.getClass().getName(), ip.getUuid()); } - } else if (domain != null && !forSystemVms) { + } else if (domain != null) { // Create an DomainVlanMapVO entry DomainVlanMapVO domainVlanMapVO = new DomainVlanMapVO(domain.getId(), vlan.getId()); _domainVlanMapDao.persist(domainVlanMapVO); @@ -7250,7 +7251,6 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati @Override public Domain getVlanDomain(long vlanId) { Vlan vlan = _vlanDao.findById(vlanId); - Long domainId = null; // if vlan is Virtual Domain specific, get vlan information from the // accountVlanMap; otherwise get account information diff --git a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java index bb5371ed271..9882ed50353 100644 --- a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java +++ b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java @@ -334,19 +334,10 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage @DB private IPAddressVO assignAndAllocateIpAddressEntry(final Account owner, final VlanType vlanUse, final Long guestNetworkId, final boolean sourceNat, final boolean allocate, final boolean isSystem, - final Long vpcId, final Boolean displayIp, final boolean fetchFromDedicatedRange, + final Long vpcId, final Boolean displayIp, final List addressVOS) throws CloudRuntimeException { return Transaction.execute((TransactionCallbackWithException) status -> { IPAddressVO finalAddress = null; - if (!fetchFromDedicatedRange && VlanType.VirtualNetwork.equals(vlanUse)) { - // Check that the maximum number of public IPs for the given accountId will not be exceeded - try { - _resourceLimitMgr.checkResourceLimit(owner, ResourceType.public_ip); - } catch (ResourceAllocationException ex) { - s_logger.warn("Failed to allocate resource of type " + ex.getResourceType() + " for account " + owner); - throw new AccountLimitException("Maximum number of public IP addresses for account: " + owner.getAccountName() + " has been exceeded."); - } - } for (IPAddressVO possibleAddr : addressVOS) { if (possibleAddr.getState() != State.Free) { @@ -826,6 +817,10 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage throws InsufficientAddressCapacityException { List addrs = listAvailablePublicIps(dcId, podId, vlanDbIds, owner, vlanUse, guestNetworkId, sourceNat, assign, allocate, requestedIp, requestedGateway, isSystem, vpcId, displayIp, forSystemVms, true); IPAddressVO addr = addrs.get(0); + if (assign) { + addr = assignAndAllocateIpAddressEntry(owner, vlanUse, guestNetworkId, sourceNat, allocate, + isSystem,vpcId, displayIp, addrs); + } if (vlanUse == VlanType.VirtualNetwork) { _firewallMgr.addSystemFirewallRules(addr, owner); } @@ -837,128 +832,99 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage public List listAvailablePublicIps(final long dcId, final Long podId, final List vlanDbIds, final Account owner, final VlanType vlanUse, final Long guestNetworkId, final boolean sourceNat, final boolean assign, final boolean allocate, final String requestedIp, final String requestedGateway, final boolean isSystem, final Long vpcId, final Boolean displayIp, final boolean forSystemVms, final boolean lockOneRow) throws InsufficientAddressCapacityException { - return Transaction.execute(new TransactionCallbackWithException, InsufficientAddressCapacityException>() { - @Override - public List doInTransaction(TransactionStatus status) throws InsufficientAddressCapacityException { - StringBuilder errorMessage = new StringBuilder("Unable to get ip address in "); - boolean fetchFromDedicatedRange = false; - List dedicatedVlanDbIds = new ArrayList(); - List nonDedicatedVlanDbIds = new ArrayList(); - DataCenter zone = _entityMgr.findById(DataCenter.class, dcId); - SearchCriteria sc = null; - if (podId != null) { - sc = AssignIpAddressFromPodVlanSearch.create(); - sc.setJoinParameters("podVlanMapSB", "podId", podId); - errorMessage.append(" pod id=" + podId); - } else { - sc = AssignIpAddressSearch.create(); - errorMessage.append(" zone id=" + dcId); - } + StringBuilder errorMessage = new StringBuilder("Unable to get ip address in "); + boolean fetchFromDedicatedRange = false; + List dedicatedVlanDbIds = new ArrayList(); + List nonDedicatedVlanDbIds = new ArrayList(); + DataCenter zone = _entityMgr.findById(DataCenter.class, dcId); - // If owner has dedicated Public IP ranges, fetch IP from the dedicated range - // Otherwise fetch IP from the system pool - Network network = _networksDao.findById(guestNetworkId); - //Checking if network is null in the case of system VM's. At the time of allocation of IP address to systemVm, no network is present. - if(network == null || !(network.getGuestType() == GuestType.Shared && zone.getNetworkType() == NetworkType.Advanced)) { - List maps = _accountVlanMapDao.listAccountVlanMapsByAccount(owner.getId()); - for (AccountVlanMapVO map : maps) { - if (vlanDbIds == null || vlanDbIds.contains(map.getVlanDbId())) - dedicatedVlanDbIds.add(map.getVlanDbId()); - } - } - List domainMaps = _domainVlanMapDao.listDomainVlanMapsByDomain(owner.getDomainId()); - for (DomainVlanMapVO map : domainMaps) { + SearchCriteria sc = null; + if (podId != null) { + sc = AssignIpAddressFromPodVlanSearch.create(); + sc.setJoinParameters("podVlanMapSB", "podId", podId); + errorMessage.append(" pod id=" + podId); + } else { + sc = AssignIpAddressSearch.create(); + errorMessage.append(" zone id=" + dcId); + } + + sc.setParameters("dc", dcId); + + // for direct network take ip addresses only from the vlans belonging to the network + if (vlanUse == VlanType.DirectAttached) { + sc.setJoinParameters("vlan", "networkId", guestNetworkId); + errorMessage.append(", network id=" + guestNetworkId); + } + if (requestedGateway != null) { + sc.setJoinParameters("vlan", "vlanGateway", requestedGateway); + errorMessage.append(", requested gateway=" + requestedGateway); + } + sc.setJoinParameters("vlan", "type", vlanUse); + + Network network = _networksDao.findById(guestNetworkId); + String routerIpAddress = null; + if (network != null) { + NetworkDetailVO routerIpDetail = _networkDetailsDao.findDetail(network.getId(), ApiConstants.ROUTER_IP); + routerIpAddress = routerIpDetail != null ? routerIpDetail.getValue() : null; + } + if (requestedIp != null) { + sc.addAnd("address", SearchCriteria.Op.EQ, requestedIp); + errorMessage.append(": requested ip " + requestedIp + " is not available"); + } else if (routerIpAddress != null) { + sc.addAnd("address", Op.NEQ, routerIpAddress); + } + + boolean ascOrder = ! forSystemVms; + Filter filter = new Filter(IPAddressVO.class, "forSystemVms", ascOrder, 0l, 1l); + + filter.addOrderBy(IPAddressVO.class,"vlanId", true); + + List addrs = new ArrayList<>(); + + if (forSystemVms) { + // Get Public IPs for system vms in dedicated ranges + sc.setParameters("forSystemVms", true); + if (lockOneRow) { + addrs = _ipAddressDao.lockRows(sc, filter, true); + } else { + addrs = new ArrayList<>(_ipAddressDao.search(sc, null)); + } + } + if ((!lockOneRow || (lockOneRow && CollectionUtils.isEmpty(addrs))) && + !(forSystemVms && SystemVmPublicIpReservationModeStrictness.value())) { + sc.setParameters("forSystemVms", false); + // If owner has dedicated Public IP ranges, fetch IP from the dedicated range + // Otherwise fetch IP from the system pool + // Checking if network is null in the case of system VM's. At the time of allocation of IP address to systemVm, no network is present. + if (network == null || !(network.getGuestType() == GuestType.Shared && zone.getNetworkType() == NetworkType.Advanced)) { + List maps = _accountVlanMapDao.listAccountVlanMapsByAccount(owner.getId()); + for (AccountVlanMapVO map : maps) { if (vlanDbIds == null || vlanDbIds.contains(map.getVlanDbId())) dedicatedVlanDbIds.add(map.getVlanDbId()); } - List nonDedicatedVlans = _vlanDao.listZoneWideNonDedicatedVlans(dcId); - for (VlanVO nonDedicatedVlan : nonDedicatedVlans) { - if (vlanDbIds == null || vlanDbIds.contains(nonDedicatedVlan.getId())) - nonDedicatedVlanDbIds.add(nonDedicatedVlan.getId()); - } - - if (vlanUse == VlanType.VirtualNetwork) { - if (!dedicatedVlanDbIds.isEmpty()) { - fetchFromDedicatedRange = true; - sc.setParameters("vlanId", dedicatedVlanDbIds.toArray()); - errorMessage.append(", vlanId id=" + Arrays.toString(dedicatedVlanDbIds.toArray())); - } else if (!nonDedicatedVlanDbIds.isEmpty()) { - sc.setParameters("vlanId", nonDedicatedVlanDbIds.toArray()); - errorMessage.append(", vlanId id=" + Arrays.toString(nonDedicatedVlanDbIds.toArray())); - } else { - if (podId != null) { - InsufficientAddressCapacityException ex = new InsufficientAddressCapacityException("Insufficient address capacity", Pod.class, podId); - ex.addProxyObject(ApiDBUtils.findPodById(podId).getUuid()); - throw ex; - } - s_logger.warn(errorMessage.toString()); - InsufficientAddressCapacityException ex = new InsufficientAddressCapacityException("Insufficient address capacity", DataCenter.class, dcId); - ex.addProxyObject(ApiDBUtils.findZoneById(dcId).getUuid()); - throw ex; - } - } - - sc.setParameters("dc", dcId); - - // for direct network take ip addresses only from the vlans belonging to the network - if (vlanUse == VlanType.DirectAttached) { - sc.setJoinParameters("vlan", "networkId", guestNetworkId); - errorMessage.append(", network id=" + guestNetworkId); - } - if (requestedGateway != null) { - sc.setJoinParameters("vlan", "vlanGateway", requestedGateway); - errorMessage.append(", requested gateway=" + requestedGateway); - } - sc.setJoinParameters("vlan", "type", vlanUse); - String routerIpAddress = null; - if (network != null) { - NetworkDetailVO routerIpDetail = _networkDetailsDao.findDetail(network.getId(), ApiConstants.ROUTER_IP); - routerIpAddress = routerIpDetail != null ? routerIpDetail.getValue() : null; - } - if (requestedIp != null) { - sc.addAnd("address", SearchCriteria.Op.EQ, requestedIp); - errorMessage.append(": requested ip " + requestedIp + " is not available"); - } else if (routerIpAddress != null) { - sc.addAnd("address", Op.NEQ, routerIpAddress); - } - - boolean ascOrder = ! forSystemVms; - Filter filter = new Filter(IPAddressVO.class, "forSystemVms", ascOrder, 0l, 1l); - if (SystemVmPublicIpReservationModeStrictness.value()) { - sc.setParameters("forSystemVms", forSystemVms); - } - - filter.addOrderBy(IPAddressVO.class,"vlanId", true); - - List addrs; - - if (lockOneRow) { - addrs = _ipAddressDao.lockRows(sc, filter, true); + } + List domainMaps = _domainVlanMapDao.listDomainVlanMapsByDomain(owner.getDomainId()); + for (DomainVlanMapVO map : domainMaps) { + if (vlanDbIds == null || vlanDbIds.contains(map.getVlanDbId())) + dedicatedVlanDbIds.add(map.getVlanDbId()); + } + List nonDedicatedVlans = _vlanDao.listZoneWideNonDedicatedVlans(dcId); + for (VlanVO nonDedicatedVlan : nonDedicatedVlans) { + if (vlanDbIds == null || vlanDbIds.contains(nonDedicatedVlan.getId())) + nonDedicatedVlanDbIds.add(nonDedicatedVlan.getId()); + } + if (vlanUse == VlanType.VirtualNetwork) { + if (!dedicatedVlanDbIds.isEmpty()) { + fetchFromDedicatedRange = true; + sc.setParameters("vlanId", dedicatedVlanDbIds.toArray()); + errorMessage.append(", vlanId id=" + Arrays.toString(dedicatedVlanDbIds.toArray())); + } else if (!nonDedicatedVlanDbIds.isEmpty()) { + sc.setParameters("vlanId", nonDedicatedVlanDbIds.toArray()); + errorMessage.append(", vlanId id=" + Arrays.toString(nonDedicatedVlanDbIds.toArray())); } else { - addrs = new ArrayList<>(_ipAddressDao.search(sc, null)); - } - - // If all the dedicated IPs of the owner are in use fetch an IP from the system pool - if ((!lockOneRow || (lockOneRow && addrs.size() == 0)) && fetchFromDedicatedRange && vlanUse == VlanType.VirtualNetwork) { - // Verify if account is allowed to acquire IPs from the system - boolean useSystemIps = UseSystemPublicIps.valueIn(owner.getId()); - if (useSystemIps && !nonDedicatedVlanDbIds.isEmpty()) { - fetchFromDedicatedRange = false; - sc.setParameters("vlanId", nonDedicatedVlanDbIds.toArray()); - errorMessage.append(", vlanId id=" + Arrays.toString(nonDedicatedVlanDbIds.toArray())); - if (lockOneRow) { - addrs = _ipAddressDao.lockRows(sc, filter, true); - } else { - addrs.addAll(_ipAddressDao.search(sc, null)); - } - } - } - - if (lockOneRow && addrs.size() == 0) { if (podId != null) { InsufficientAddressCapacityException ex = new InsufficientAddressCapacityException("Insufficient address capacity", Pod.class, podId); - // for now, we hardcode the table names, but we should ideally do a lookup for the tablename from the VO object. ex.addProxyObject(ApiDBUtils.findPodById(podId).getUuid()); throw ex; } @@ -967,17 +933,58 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage ex.addProxyObject(ApiDBUtils.findZoneById(dcId).getUuid()); throw ex; } - - if (lockOneRow) { - assert (addrs.size() == 1) : "Return size is incorrect: " + addrs.size(); - } - if (assign) { - assignAndAllocateIpAddressEntry(owner, vlanUse, guestNetworkId, sourceNat, allocate, - isSystem,vpcId, displayIp, fetchFromDedicatedRange, addrs); - } - return addrs; } - }); + if (lockOneRow) { + addrs = _ipAddressDao.lockRows(sc, filter, true); + } else { + addrs = new ArrayList<>(_ipAddressDao.search(sc, null)); + } + + // If all the dedicated IPs of the owner are in use fetch an IP from the system pool + if ((!lockOneRow || (lockOneRow && addrs.size() == 0)) && fetchFromDedicatedRange && vlanUse == VlanType.VirtualNetwork) { + // Verify if account is allowed to acquire IPs from the system + boolean useSystemIps = UseSystemPublicIps.valueIn(owner.getId()); + if (useSystemIps && !nonDedicatedVlanDbIds.isEmpty()) { + fetchFromDedicatedRange = false; + sc.setParameters("vlanId", nonDedicatedVlanDbIds.toArray()); + errorMessage.append(", vlanId id=" + Arrays.toString(nonDedicatedVlanDbIds.toArray())); + if (lockOneRow) { + addrs = _ipAddressDao.lockRows(sc, filter, true); + } else { + addrs.addAll(_ipAddressDao.search(sc, null)); + } + } + } + } + + if (lockOneRow && addrs.size() == 0) { + if (podId != null) { + InsufficientAddressCapacityException ex = new InsufficientAddressCapacityException("Insufficient address capacity", Pod.class, podId); + // for now, we hardcode the table names, but we should ideally do a lookup for the tablename from the VO object. + ex.addProxyObject(ApiDBUtils.findPodById(podId).getUuid()); + throw ex; + } + s_logger.warn(errorMessage.toString()); + InsufficientAddressCapacityException ex = new InsufficientAddressCapacityException("Insufficient address capacity", DataCenter.class, dcId); + ex.addProxyObject(ApiDBUtils.findZoneById(dcId).getUuid()); + throw ex; + } + + if (lockOneRow) { + assert (addrs.size() == 1) : "Return size is incorrect: " + addrs.size(); + } + + if (assign && !fetchFromDedicatedRange && VlanType.VirtualNetwork.equals(vlanUse)) { + // Check that the maximum number of public IPs for the given accountId will not be exceeded + try { + _resourceLimitMgr.checkResourceLimit(owner, ResourceType.public_ip); + } catch (ResourceAllocationException ex) { + s_logger.warn("Failed to allocate resource of type " + ex.getResourceType() + " for account " + owner); + throw new AccountLimitException("Maximum number of public IP addresses for account: " + owner.getAccountName() + " has been exceeded."); + } + } + + return addrs; } @DB diff --git a/test/integration/smoke/test_public_ip_range.py b/test/integration/smoke/test_public_ip_range.py index 69baf5f2fc2..19edc4c164f 100644 --- a/test/integration/smoke/test_public_ip_range.py +++ b/test/integration/smoke/test_public_ip_range.py @@ -128,9 +128,8 @@ class TestDedicatePublicIPRange(cloudstackTestCase): id=self.public_ip_range.vlan.id ) public_ip_response = list_public_ip_range_response[0] - self.assertEqual( + self.assertIsNone( public_ip_response.account, - "system", "Check account name is system account in listVlanIpRanges" ) return diff --git a/ui/public/locales/en.json b/ui/public/locales/en.json index 1469170ae01..528ea521c53 100644 --- a/ui/public/locales/en.json +++ b/ui/public/locales/en.json @@ -1901,6 +1901,7 @@ "label.suspend.project": "Suspend project", "label.switch.type": "Switch type", "label.sync.storage": "Sync storage pool", +"label.system.ip.pool": "System Pool", "label.system.offering": "System offering", "label.system.offerings": "System offerings", "label.system.service.offering": "System service offering", diff --git a/ui/src/views/infra/network/IpRangesTabPublic.vue b/ui/src/views/infra/network/IpRangesTabPublic.vue index 247a8faf0d4..d7f743fda13 100644 --- a/ui/src/views/infra/network/IpRangesTabPublic.vue +++ b/ui/src/views/infra/network/IpRangesTabPublic.vue @@ -47,21 +47,21 @@ {{ record.endip || record.endipv6 }}