[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive URL: https://github.com/apache/cloudstack/pull/3976#discussion_r402943211 ## File path: engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java ## @@ -299,7 +299,7 @@ void implementNetworkElementsAndResources(DeployDestination dest, ReservationCon * service provider is ConfigDrive or VirtualRouter * @param vm * @param dest Review comment: please add descriptions or remove This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r401877966
##
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
##
@@ -141,6 +146,12 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
xmlDesc = dm.getXMLDesc(xmlFlag);
xmlDesc = replaceIpForVNCInDescFile(xmlDesc, target);
+String oldIsoVolumePath = getOldVolumePath(disks, vmName);
+String newIsoVolumePath =
getNewVolumePathIfDatastoreHasChanged(libvirtComputingResource, conn, to);
+if (newIsoVolumePath != null &&
!newIsoVolumePath.equals(oldIsoVolumePath)) {
+s_logger.debug("Editing mount path");
+xmlDesc = replaceDiskSourceFile(xmlDesc, newIsoVolumePath,
vmName);
+}
Review comment:
I think even this part could be factorred out. the method is still almost
200 lines. (no biggy)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive URL: https://github.com/apache/cloudstack/pull/3976#discussion_r401873299 ## File path: engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java ## @@ -294,6 +294,15 @@ void implementNetworkElementsAndResources(DeployDestination dest, ReservationCon void finalizeUpdateInSequence(Network network, boolean success); +/** + * Adds hypervisor hostname to a file - hypervisor-host-name if the userdata + * service provider is ConfigDrive or VirtualRouter + * @param vm + * @param dest + * @throws ResourceUnavailableException Review comment: please remove or add description of use/reason for throwing This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399329465
##
File path:
server/src/main/java/com/cloud/network/router/CommandSetupHelper.java
##
@@ -177,23 +180,30 @@
private VlanDao _vlanDao;
@Inject
private IPAddressDao _ipAddressDao;
-
@Inject
private RouterControlHelper _routerControlHelper;
+@Inject
+private HostDao _hostDao;
@Autowired
@Qualifier("networkHelper")
protected NetworkHelper _networkHelper;
public void createVmDataCommand(final VirtualRouter router, final UserVm
vm, final NicVO nic, final String publicKey, final Commands cmds) {
-final String serviceOffering =
_serviceOfferingDao.findByIdIncludingRemoved(vm.getId(),
vm.getServiceOfferingId()).getDisplayText();
-final String zoneName =
_dcDao.findById(router.getDataCenterId()).getName();
-final IPAddressVO staticNatIp =
_ipAddressDao.findByVmIdAndNetworkId(nic.getNetworkId(), vm.getId());
-cmds.addCommand(
-"vmdata",
-generateVmDataCommand(router, nic.getIPv4Address(),
vm.getUserData(), serviceOffering, zoneName,
-staticNatIp == null || staticNatIp.getState() !=
IpAddress.State.Allocated ? null : staticNatIp.getAddress().addr(),
vm.getHostName(), vm.getInstanceName(),
-vm.getId(), vm.getUuid(), publicKey,
nic.getNetworkId()));
+if (vm != null && router != null && nic != null) {
Review comment:
so why wasn't this needed before?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399333572
##
File path: test/integration/component/test_vr_metadata.py
##
@@ -0,0 +1,347 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+
+# this script will cover VMdeployment with Userdata tests
+
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.base import *
+from marvin.lib.utils import (validateList, cleanup_resources)
+from marvin.lib.common import *
+from nose.plugins.attrib import attr
+from marvin.codes import PASS,FAIL
+
+_multiprocess_shared_ = True
+
+class Services:
+def __init__(self):
+self.services = {
+"virtual_machine": {
+"displayname": "TesVM1",
+"username": "root",
+"password": "password",
+"ssh_port": 22,
+"hypervisor": 'XenServer',
+"privateport": 22,
+"publicport": 22,
+"protocol": 'TCP',
+},
+"ostype": 'CentOS 5.5 (64-bit)',
+"service_offering": {
+"name": "Tiny Instance",
+"displaytext": "Tiny Instance",
+"cpunumber": 1,
+"cpuspeed": 100,
+"memory": 256,
+},
+}
+
+class TestDeployVmWithMetaData(cloudstackTestCase):
+@classmethod
+def setUpClass(cls):
+cls.testclient = super(TestDeployVmWithMetaData,
cls).getClsTestClient()
+cls.apiclient = cls.testclient.getApiClient()
+cls._cleanup = []
+#cls.services = Services().services
+cls.services = cls.testclient.getParsedTestDataConfig()
+cls.zone = get_zone(cls.apiclient, cls.testclient.getZoneForTests())
+cls.service_offering = ServiceOffering.create(
+cls.apiclient,
+cls.services["service_offering"]
+)
+
+cls.template = get_template(
+cls.apiclient,
+cls.zone.id,
+cls.services["ostype"]
+)
+
+
+@classmethod
+def tearDownClass(cls):
+try:
+cls._cleanup = cls._cleanup[::-1]
+cleanup_resources(cls.apiclient, cls._cleanup)
+except Exception as e:
+raise Exception("Warning: Exception during cleanup : %s" % e)
+
+def setUp(self):
+self.apiclient = self.testClient.getApiClient()
+self.cleanup = []
+
+def tearDown(self):
+try:
+self.cleanup = self.cleanup[::-1]
+cleanup_resources(self.apiclient, self.cleanup)
+except Exception as e:
+raise Exception("Warning: Exception during cleanup : %s" % e)
+return
+
+def migrate_VM(self, vm):
+"""Migrates VM to another host, if available"""
+self.debug("+++ Migrating one of the VMs in the created "
+ "VPC Tier network to another host, if available...")
+self.debug("Checking if a host is available for migration...")
+hosts = Host.listForMigration(self.apiclient, virtualmachineid=vm.id)
+if hosts:
+self.assertEqual(isinstance(hosts, list), True,
+ "List hosts should return a valid list"
+ )
+host = hosts[0]
+self.debug("Migrating VM with ID: "
+ "%s to Host: %s" % (vm.id, host.id))
+try:
+vm.migrate(self.apiclient, hostid=host.id)
+except Exception as e:
+self.fail("Failed to migrate instance, %s" % e)
+self.debug("Migrated VM with ID: "
+ "%s to Host: %s" % (vm.id, host.id))
+else:
+self.debug("No host available for migration. "
+ "Test requires at-least 2 hosts")
+return host
+
+def list_nics(self, vm_id):
+list_vm_res = VirtualMachine.list(self.apiclient, id=vm_id)
+self.assertEqual(validateList(list_vm_res)[0], PASS, "List vms
returned invalid response")
+nics = list_vm_res[0].nic
+for nic
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399276119
##
File path:
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##
@@ -1610,6 +1612,26 @@ public void finalizeUpdateInSequence(Network network,
boolean success) {
}
}
+@Override
+public void addNewDisk(VirtualMachineProfile vm, DeployDestination dest)
throws ResourceUnavailableException {
+final List nics = _nicDao.listByVmId(vm.getId());
+for (final NicVO nic : nics) {
+final NetworkVO network =
_networksDao.findById(nic.getNetworkId());
+final Integer networkRate =
_networkModel.getNetworkRate(network.getId(), vm.getId());
+final NicProfile profile = new NicProfile(nic, network,
nic.getBroadcastUri(), nic.getIsolationUri(), networkRate,
_networkModel.isSecurityGroupSupportedInNetwork(network),
+_networkModel.getNetworkTag(vm.getHypervisorType(),
network));
+for (final NetworkElement element : networkElements) {
+if
(_networkModel.areServicesSupportedInNetwork(network.getId(), Service.UserData)
+&& element instanceof UserDataServiceProvider &&
+(element instanceof ConfigDriveNetworkElement ||
element instanceof VirtualRouterElement)) {
+final UserDataServiceProvider sp =
(UserDataServiceProvider) element;
+if (!sp.addNewDisk(profile, network, vm, dest)) {
+throw new CloudRuntimeException("Failed to create New
Iso Disk");
+}
+}
+}
Review comment:
so if we have an element that fits, do we still iterate over the rest, or
should we break on the outer `if`? which makes me think this method has a high
complexity can you disect it for readability, please?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399329854 ## File path: server/src/main/java/com/cloud/network/rules/UserdataToRouterRules.java ## @@ -50,6 +50,7 @@ public boolean accept(final NetworkTopologyVisitor visitor, final VirtualRouter UserVmDao userVmDao = visitor.getVirtualNetworkApplianceFactory().getUserVmDao(); _userVM = userVmDao.findById(_profile.getVirtualMachine().getId()); + Review comment: Why? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399300984
##
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
##
@@ -572,4 +606,49 @@ private String getXml(Document doc) throws
TransformerException {
return byteArrayOutputStream.toString();
}
+
+private String replaceDiskSourceFile(String xmlDesc, String isoPath,
String vmName) throws IOException, SAXException, ParserConfigurationException,
TransformerException {
+InputStream in = IOUtils.toInputStream(xmlDesc);
+
+DocumentBuilderFactory docFactory =
DocumentBuilderFactory.newInstance();
+DocumentBuilder docBuilder = docFactory.newDocumentBuilder();
+Document doc = docBuilder.parse(in);
+
+// Get the root element
+Node domainNode = doc.getFirstChild();
+
+NodeList domainChildNodes = domainNode.getChildNodes();
+
+for (int i = 0; i < domainChildNodes.getLength(); i++) {
+Node domainChildNode = domainChildNodes.item(i);
+
+if ("devices".equals(domainChildNode.getNodeName())) {
+NodeList devicesChildNodes = domainChildNode.getChildNodes();
+
+for (int x = 0; x < devicesChildNodes.getLength(); x++) {
+Node deviceChildNode = devicesChildNodes.item(x);
+if ("disk".equals(deviceChildNode.getNodeName())) {
+Node diskNode = deviceChildNode;
+String sourceText = getSourceText(diskNode);
+NodeList diskChildNodes = diskNode.getChildNodes();
+for (int z = 0; z < diskChildNodes.getLength(); z++) {
+Node diskChildNode = diskChildNodes.item(z);
+if ("source".equals(diskChildNode.getNodeName())) {
+Node sourceNode = diskChildNode;
+NamedNodeMap sourceNodeAttributes =
sourceNode.getAttributes();
+Node sourceNodeAttribute =
sourceNodeAttributes.getNamedItem("file");
+if (
sourceNodeAttribute.getNodeValue().contains(vmName)) {
+diskNode.removeChild(diskChildNode);
+Element newChildSourceNode =
doc.createElement("source");
+newChildSourceNode.setAttribute("file",
isoPath);
+diskNode.appendChild(newChildSourceNode);
+}
+}
+}
+}
+}
+}
+}
+return getXml(doc);
+}
Review comment:
for - if - for - if - for - if - if, this should definitely be dissected and
simplified.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399309518
##
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
##
@@ -572,4 +606,49 @@ private String getXml(Document doc) throws
TransformerException {
return byteArrayOutputStream.toString();
}
+
+private String replaceDiskSourceFile(String xmlDesc, String isoPath,
String vmName) throws IOException, SAXException, ParserConfigurationException,
TransformerException {
+InputStream in = IOUtils.toInputStream(xmlDesc);
+
+DocumentBuilderFactory docFactory =
DocumentBuilderFactory.newInstance();
+DocumentBuilder docBuilder = docFactory.newDocumentBuilder();
+Document doc = docBuilder.parse(in);
+
+// Get the root element
+Node domainNode = doc.getFirstChild();
+
+NodeList domainChildNodes = domainNode.getChildNodes();
+
+for (int i = 0; i < domainChildNodes.getLength(); i++) {
+Node domainChildNode = domainChildNodes.item(i);
+
+if ("devices".equals(domainChildNode.getNodeName())) {
+NodeList devicesChildNodes = domainChildNode.getChildNodes();
+
+for (int x = 0; x < devicesChildNodes.getLength(); x++) {
+Node deviceChildNode = devicesChildNodes.item(x);
+if ("disk".equals(deviceChildNode.getNodeName())) {
+Node diskNode = deviceChildNode;
+String sourceText = getSourceText(diskNode);
+NodeList diskChildNodes = diskNode.getChildNodes();
+for (int z = 0; z < diskChildNodes.getLength(); z++) {
+Node diskChildNode = diskChildNodes.item(z);
+if ("source".equals(diskChildNode.getNodeName())) {
+Node sourceNode = diskChildNode;
+NamedNodeMap sourceNodeAttributes =
sourceNode.getAttributes();
+Node sourceNodeAttribute =
sourceNodeAttributes.getNamedItem("file");
+if (
sourceNodeAttribute.getNodeValue().contains(vmName)) {
+diskNode.removeChild(diskChildNode);
+Element newChildSourceNode =
doc.createElement("source");
+newChildSourceNode.setAttribute("file",
isoPath);
+diskNode.appendChild(newChildSourceNode);
Review comment:
and how about when we are sure we have the right disk, do we just continue
processing or should we escape somehow and not process the rest?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399313914
##
File path:
server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java
##
@@ -327,6 +355,30 @@ public void rollbackMigration(NicProfile nic, Network
network, VirtualMachinePro
public void commitMigration(NicProfile nic, Network network,
VirtualMachineProfile vm, ReservationContext src, ReservationContext dst) {
}
+private void recreateConfigDriveIso(NicProfile nic, Network network,
VirtualMachineProfile vm, DeployDestination dest) throws
ResourceUnavailableException {
+if (nic.isDefaultNic() &&
_networkModel.getUserDataUpdateProvider(network).getProvider().equals(Provider.ConfigDrive))
{
+DiskTO diskToUse = null;
+for (DiskTO disk : vm.getDisks()) {
+if (disk.getType() == Volume.Type.ISO && disk.getPath() !=
null && disk.getPath().contains("configdrive")) {
+diskToUse = disk;
+break;
+}
+}
+final UserVmVO userVm = _userVmDao.findById(vm.getId());
+final Account caller = CallContext.current().getCallingAccount();
+
+if (userVm != null) {
+final boolean isWindows =
_guestOSCategoryDao.findById(_guestOSDao.findById(userVm.getGuestOSId()).getCategoryId()).getName().equalsIgnoreCase("Windows");
+String destHostname =
(VirtualMachineManager.AllowExposeHypervisorHostname.value() &&
VirtualMachineManager.AllowExposeHypervisorHostnameAccountLevel.valueIn(caller.getId()))
? dest.getHost().getName() : null;
Review comment:
triple code ;)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399327546 ## File path: server/src/main/java/com/cloud/network/element/VpcVirtualRouterElement.java ## @@ -711,4 +711,5 @@ public boolean stopVpn(final RemoteAccessVpn vpn) throws ResourceUnavailableExce } return result; } -} \ No newline at end of file + Review comment: This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399326270
##
File path:
server/src/main/java/com/cloud/network/element/VirtualRouterElement.java
##
@@ -765,6 +772,33 @@ public boolean saveSSHKey(final Network network, final
NicProfile nic, final Vir
return result;
}
+@Override
+public boolean addNewDisk(NicProfile nicProfile, Network network,
VirtualMachineProfile vm, DeployDestination dest) throws
ResourceUnavailableException {
Review comment:
this one is certainly not adding any disk. we need to change that name.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399333572
##
File path: test/integration/component/test_vr_metadata.py
##
@@ -0,0 +1,347 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+
+# this script will cover VMdeployment with Userdata tests
+
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.base import *
+from marvin.lib.utils import (validateList, cleanup_resources)
+from marvin.lib.common import *
+from nose.plugins.attrib import attr
+from marvin.codes import PASS,FAIL
+
+_multiprocess_shared_ = True
+
+class Services:
+def __init__(self):
+self.services = {
+"virtual_machine": {
+"displayname": "TesVM1",
+"username": "root",
+"password": "password",
+"ssh_port": 22,
+"hypervisor": 'XenServer',
+"privateport": 22,
+"publicport": 22,
+"protocol": 'TCP',
+},
+"ostype": 'CentOS 5.5 (64-bit)',
+"service_offering": {
+"name": "Tiny Instance",
+"displaytext": "Tiny Instance",
+"cpunumber": 1,
+"cpuspeed": 100,
+"memory": 256,
+},
+}
+
+class TestDeployVmWithMetaData(cloudstackTestCase):
+@classmethod
+def setUpClass(cls):
+cls.testclient = super(TestDeployVmWithMetaData,
cls).getClsTestClient()
+cls.apiclient = cls.testclient.getApiClient()
+cls._cleanup = []
+#cls.services = Services().services
+cls.services = cls.testclient.getParsedTestDataConfig()
+cls.zone = get_zone(cls.apiclient, cls.testclient.getZoneForTests())
+cls.service_offering = ServiceOffering.create(
+cls.apiclient,
+cls.services["service_offering"]
+)
+
+cls.template = get_template(
+cls.apiclient,
+cls.zone.id,
+cls.services["ostype"]
+)
+
+
+@classmethod
+def tearDownClass(cls):
+try:
+cls._cleanup = cls._cleanup[::-1]
+cleanup_resources(cls.apiclient, cls._cleanup)
+except Exception as e:
+raise Exception("Warning: Exception during cleanup : %s" % e)
+
+def setUp(self):
+self.apiclient = self.testClient.getApiClient()
+self.cleanup = []
+
+def tearDown(self):
+try:
+self.cleanup = self.cleanup[::-1]
+cleanup_resources(self.apiclient, self.cleanup)
+except Exception as e:
+raise Exception("Warning: Exception during cleanup : %s" % e)
+return
+
+def migrate_VM(self, vm):
+"""Migrates VM to another host, if available"""
+self.debug("+++ Migrating one of the VMs in the created "
+ "VPC Tier network to another host, if available...")
+self.debug("Checking if a host is available for migration...")
+hosts = Host.listForMigration(self.apiclient, virtualmachineid=vm.id)
+if hosts:
+self.assertEqual(isinstance(hosts, list), True,
+ "List hosts should return a valid list"
+ )
+host = hosts[0]
+self.debug("Migrating VM with ID: "
+ "%s to Host: %s" % (vm.id, host.id))
+try:
+vm.migrate(self.apiclient, hostid=host.id)
+except Exception as e:
+self.fail("Failed to migrate instance, %s" % e)
+self.debug("Migrated VM with ID: "
+ "%s to Host: %s" % (vm.id, host.id))
+else:
+self.debug("No host available for migration. "
+ "Test requires at-least 2 hosts")
+return host
+
+def list_nics(self, vm_id):
+list_vm_res = VirtualMachine.list(self.apiclient, id=vm_id)
+self.assertEqual(validateList(list_vm_res)[0], PASS, "List vms
returned invalid response")
+nics = list_vm_res[0].nic
+for nic
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r39961
##
File path: test/integration/component/test_vr_metadata.py
##
@@ -0,0 +1,347 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+
+# this script will cover VMdeployment with Userdata tests
+
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.base import *
+from marvin.lib.utils import (validateList, cleanup_resources)
+from marvin.lib.common import *
+from nose.plugins.attrib import attr
+from marvin.codes import PASS,FAIL
+
+_multiprocess_shared_ = True
+
+class Services:
+def __init__(self):
+self.services = {
+"virtual_machine": {
+"displayname": "TesVM1",
+"username": "root",
+"password": "password",
+"ssh_port": 22,
+"hypervisor": 'XenServer',
+"privateport": 22,
+"publicport": 22,
+"protocol": 'TCP',
+},
+"ostype": 'CentOS 5.5 (64-bit)',
+"service_offering": {
+"name": "Tiny Instance",
+"displaytext": "Tiny Instance",
+"cpunumber": 1,
+"cpuspeed": 100,
+"memory": 256,
+},
+}
+
+class TestDeployVmWithMetaData(cloudstackTestCase):
+@classmethod
+def setUpClass(cls):
+cls.testclient = super(TestDeployVmWithMetaData,
cls).getClsTestClient()
+cls.apiclient = cls.testclient.getApiClient()
+cls._cleanup = []
+#cls.services = Services().services
+cls.services = cls.testclient.getParsedTestDataConfig()
+cls.zone = get_zone(cls.apiclient, cls.testclient.getZoneForTests())
+cls.service_offering = ServiceOffering.create(
+cls.apiclient,
+cls.services["service_offering"]
+)
+
+cls.template = get_template(
+cls.apiclient,
+cls.zone.id,
+cls.services["ostype"]
+)
+
+
+@classmethod
+def tearDownClass(cls):
+try:
+cls._cleanup = cls._cleanup[::-1]
+cleanup_resources(cls.apiclient, cls._cleanup)
+except Exception as e:
+raise Exception("Warning: Exception during cleanup : %s" % e)
+
+def setUp(self):
+self.apiclient = self.testClient.getApiClient()
+self.cleanup = []
+
+def tearDown(self):
+try:
+self.cleanup = self.cleanup[::-1]
+cleanup_resources(self.apiclient, self.cleanup)
+except Exception as e:
+raise Exception("Warning: Exception during cleanup : %s" % e)
+return
+
+def migrate_VM(self, vm):
+"""Migrates VM to another host, if available"""
+self.debug("+++ Migrating one of the VMs in the created "
+ "VPC Tier network to another host, if available...")
+self.debug("Checking if a host is available for migration...")
+hosts = Host.listForMigration(self.apiclient, virtualmachineid=vm.id)
+if hosts:
+self.assertEqual(isinstance(hosts, list), True,
+ "List hosts should return a valid list"
+ )
+host = hosts[0]
+self.debug("Migrating VM with ID: "
+ "%s to Host: %s" % (vm.id, host.id))
+try:
+vm.migrate(self.apiclient, hostid=host.id)
+except Exception as e:
+self.fail("Failed to migrate instance, %s" % e)
+self.debug("Migrated VM with ID: "
+ "%s to Host: %s" % (vm.id, host.id))
+else:
+self.debug("No host available for migration. "
+ "Test requires at-least 2 hosts")
+return host
+
+def list_nics(self, vm_id):
+list_vm_res = VirtualMachine.list(self.apiclient, id=vm_id)
+self.assertEqual(validateList(list_vm_res)[0], PASS, "List vms
returned invalid response")
+nics = list_vm_res[0].nic
+for nic
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399311522 ## File path: server/src/main/java/com/cloud/network/element/CloudZonesNetworkElement.java ## @@ -217,11 +220,12 @@ public boolean addPasswordAndUserdata(Network network, NicProfile nic, VirtualMa } String serviceOffering = _serviceOfferingDao.findByIdIncludingRemoved(uservm.getServiceOfferingId()).getDisplayText(); String zoneName = _dcDao.findById(network.getDataCenterId()).getName(); - +final Account caller = CallContext.current().getCallingAccount(); +String destHostname = (VirtualMachineManager.AllowExposeHypervisorHostname.value() && VirtualMachineManager.AllowExposeHypervisorHostnameAccountLevel.valueIn(caller.getId())) ? dest.getHost().getName() : null; Review comment: again, no override but a double condition. It also constitutes double code, this is a value that should be returned by the VirtualMachineManager and not be implemented in two places, obviously. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399329465
##
File path:
server/src/main/java/com/cloud/network/router/CommandSetupHelper.java
##
@@ -177,23 +180,30 @@
private VlanDao _vlanDao;
@Inject
private IPAddressDao _ipAddressDao;
-
@Inject
private RouterControlHelper _routerControlHelper;
+@Inject
+private HostDao _hostDao;
@Autowired
@Qualifier("networkHelper")
protected NetworkHelper _networkHelper;
public void createVmDataCommand(final VirtualRouter router, final UserVm
vm, final NicVO nic, final String publicKey, final Commands cmds) {
-final String serviceOffering =
_serviceOfferingDao.findByIdIncludingRemoved(vm.getId(),
vm.getServiceOfferingId()).getDisplayText();
-final String zoneName =
_dcDao.findById(router.getDataCenterId()).getName();
-final IPAddressVO staticNatIp =
_ipAddressDao.findByVmIdAndNetworkId(nic.getNetworkId(), vm.getId());
-cmds.addCommand(
-"vmdata",
-generateVmDataCommand(router, nic.getIPv4Address(),
vm.getUserData(), serviceOffering, zoneName,
-staticNatIp == null || staticNatIp.getState() !=
IpAddress.State.Allocated ? null : staticNatIp.getAddress().addr(),
vm.getHostName(), vm.getInstanceName(),
-vm.getId(), vm.getUuid(), publicKey,
nic.getNetworkId()));
+if (vm != null && router != null && nic != null) {
Review comment:
so why wasn't this need before?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399308898
##
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
##
@@ -572,4 +606,49 @@ private String getXml(Document doc) throws
TransformerException {
return byteArrayOutputStream.toString();
}
+
+private String replaceDiskSourceFile(String xmlDesc, String isoPath,
String vmName) throws IOException, SAXException, ParserConfigurationException,
TransformerException {
+InputStream in = IOUtils.toInputStream(xmlDesc);
+
+DocumentBuilderFactory docFactory =
DocumentBuilderFactory.newInstance();
+DocumentBuilder docBuilder = docFactory.newDocumentBuilder();
+Document doc = docBuilder.parse(in);
+
+// Get the root element
+Node domainNode = doc.getFirstChild();
+
+NodeList domainChildNodes = domainNode.getChildNodes();
+
+for (int i = 0; i < domainChildNodes.getLength(); i++) {
+Node domainChildNode = domainChildNodes.item(i);
+
+if ("devices".equals(domainChildNode.getNodeName())) {
+NodeList devicesChildNodes = domainChildNode.getChildNodes();
+
+for (int x = 0; x < devicesChildNodes.getLength(); x++) {
+Node deviceChildNode = devicesChildNodes.item(x);
+if ("disk".equals(deviceChildNode.getNodeName())) {
+Node diskNode = deviceChildNode;
+String sourceText = getSourceText(diskNode);
+NodeList diskChildNodes = diskNode.getChildNodes();
+for (int z = 0; z < diskChildNodes.getLength(); z++) {
+Node diskChildNode = diskChildNodes.item(z);
+if ("source".equals(diskChildNode.getNodeName())) {
+Node sourceNode = diskChildNode;
+NamedNodeMap sourceNodeAttributes =
sourceNode.getAttributes();
+Node sourceNodeAttribute =
sourceNodeAttributes.getNamedItem("file");
+if (
sourceNodeAttribute.getNodeValue().contains(vmName)) {
Review comment:
are we sure there will be only one? how about a VM with multiple disks/isos
mounted? are we sure this contains() is selective enough?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399299689
##
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
##
@@ -250,6 +281,9 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
} catch (final TransformerException e) {
s_logger.debug("TransformerException: " + e.getMessage());
result = e.getMessage();
+} catch (URISyntaxException e) {
+s_logger.debug("UriSyntaxException: "+ e.getMessage());
+result = e.getMessage();
Review comment:
this is a c of a bad pattern for two reasons:
1. the message if available should be explaining that it is a uri syntax
issue.
1. as in most of the other catch clauses this can be replaced by something
like `String.format("%s: %s", e.getClass().getSimpleName(), e.getMessage())`.
And we should then unify the catch clauses.
I suggest replacing with (something like):
```
} catch (final TimeoutException e) {
s_logger.debug("Timed out while migrating domain: " +
e.getMessage());
result = e.getMessage();
} catch (final IOException
| ParserConfigurationException
| SAXException
| TransformerException
| URISyntaxException e) {
s_logger.debug(String.format("%s: %s",
e.getClass().getSimpleName(), e.getMessage()));
result = "Exception during migrate, see hypervisor for details;
" + e.getMessage();
} finally {
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399288447
##
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
##
@@ -141,6 +168,10 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
xmlDesc = dm.getXMLDesc(xmlFlag);
xmlDesc = replaceIpForVNCInDescFile(xmlDesc, target);
+if (newIsoVolumePath != null && oldIsoVolumePath !=
newIsoVolumePath) {
+s_logger.debug("editing mount path");
+xmlDesc = replaceDiskSourceFile(xmlDesc, newIsoVolumePath,
vmName);
+}
Review comment:
please extract, and if not to much trouble other bits as well.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399320104
##
File path:
server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java
##
@@ -424,8 +476,25 @@ private Long findAgentId(VirtualMachineProfile profile,
DeployDestination dest,
return agentId;
}
-private boolean createConfigDriveIso(VirtualMachineProfile profile,
DeployDestination dest) throws ResourceUnavailableException {
-final DataStore dataStore = findDataStore(profile, dest);
+private boolean createConfigDriveIso(VirtualMachineProfile profile,
DeployDestination dest, DiskTO disk) throws ResourceUnavailableException {
+DataStore dataStore = null;
+if (disk != null) {
+String dId = disk.getData().getDataStore().getUuid();
+if (VirtualMachineManager.VmConfigDriveOnPrimaryPool.value()) {
+dataStore = _dataStoreMgr.getDataStore(dId,
DataStoreRole.Primary);
+} else {
+List dataStores = _dataStoreMgr.listImageStores();
+String url = disk.getData().getDataStore().getUrl();
+for(DataStore ds : dataStores) {
+if (url.equals(ds.getUri()) &&
DataStoreRole.Image.equals(ds.getRole())) {
+dataStore = ds;
+break;
+}
+}
+}
+} else {
+dataStore = findDataStore(profile, dest);
+}
Review comment:
can you put in `getOrDecideOnWhichDataStoreToUseOrSomeNameLikeThat()`?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399276728 ## File path: engine/schema/src/main/java/com/cloud/vm/dao/UserVmDaoImpl.java ## @@ -1,4 +1,4 @@ -// Licensed to the Apache Software Foundation (ASF) under one +// Licensed to the Apache Software Foundation (ASF) under one Review comment: space spilled over in the file ;) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399320617
##
File path:
server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java
##
@@ -514,9 +584,11 @@ private boolean configureConfigDriveData(final
VirtualMachineProfile profile, fi
final String sshPublicKey = getSshKey(profile);
final String serviceOffering =
_serviceOfferingDao.findByIdIncludingRemoved(vm.getId(),
vm.getServiceOfferingId()).getDisplayText();
boolean isWindows =
_guestOSCategoryDao.findById(_guestOSDao.findById(vm.getGuestOSId()).getCategoryId()).getName().equalsIgnoreCase("Windows");
-
+String hostname = _hostDao.findById(vm.getHostId()).getName();
+final Account caller = CallContext.current().getCallingAccount();
+String destHostname =
(VirtualMachineManager.AllowExposeHypervisorHostname.value() &&
VirtualMachineManager.AllowExposeHypervisorHostnameAccountLevel.valueIn(caller.getId()))
? hostname : null;
Review comment:
quadruple code...
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399279684
##
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
##
@@ -115,6 +118,30 @@ public Answer execute(final MigrateCommand command, final
LibvirtComputingResour
conn = libvirtUtilitiesHelper.getConnectionByVmName(vmName);
ifaces = libvirtComputingResource.getInterfaces(conn, vmName);
disks = libvirtComputingResource.getDisks(conn, vmName);
+
+String oldIsoVolumePath = null;
+for (DiskDef disk : disks) {
+if (disk.getDiskPath() != null &&
disk.getDiskPath().contains(vmName)) {
+oldIsoVolumePath = disk.getDiskPath();
+break;
+}
+}
+
+VirtualMachineTO to = command.getVirtualMachine();
+
+DiskTO newDisk = null;
+for (DiskTO disk : to.getDisks()) {
+if (disk.getPath() != null &&
disk.getPath().contains("configdrive")) {
+newDisk = disk;
+break;
+}
+}
+
+String newIsoVolumePath = null;
+if (newDisk != null) {
+ newIsoVolumePath =
libvirtComputingResource.getVolumePath(conn, newDisk);
+}
+
Review comment:
this method is already 200 lines, please add your logic in a new
method/class or module.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399330489
##
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##
@@ -4352,9 +4352,10 @@ public boolean
finalizeVirtualMachineProfile(VirtualMachineProfile profile, Depl
if (_networkModel.isSharedNetworkWithoutServices(network.getId()))
{
final String serviceOffering =
_serviceOfferingDao.findByIdIncludingRemoved(vm.getId(),
vm.getServiceOfferingId()).getDisplayText();
boolean isWindows =
_guestOSCategoryDao.findById(_guestOSDao.findById(vm.getGuestOSId()).getCategoryId()).getName().equalsIgnoreCase("Windows");
-
+final Account caller =
CallContext.current().getCallingAccount();
+String destHostname =
(VirtualMachineManager.AllowExposeHypervisorHostname.value() &&
VirtualMachineManager.AllowExposeHypervisorHostnameAccountLevel.valueIn(caller.getId()))
? dest.getHost().getName() : null;
Review comment:
method (sixth use)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399303730
##
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
##
@@ -572,4 +606,49 @@ private String getXml(Document doc) throws
TransformerException {
return byteArrayOutputStream.toString();
}
+
+private String replaceDiskSourceFile(String xmlDesc, String isoPath,
String vmName) throws IOException, SAXException, ParserConfigurationException,
TransformerException {
+InputStream in = IOUtils.toInputStream(xmlDesc);
+
+DocumentBuilderFactory docFactory =
DocumentBuilderFactory.newInstance();
+DocumentBuilder docBuilder = docFactory.newDocumentBuilder();
+Document doc = docBuilder.parse(in);
+
+// Get the root element
+Node domainNode = doc.getFirstChild();
+
+NodeList domainChildNodes = domainNode.getChildNodes();
+
+for (int i = 0; i < domainChildNodes.getLength(); i++) {
+Node domainChildNode = domainChildNodes.item(i);
+
+if ("devices".equals(domainChildNode.getNodeName())) {
+NodeList devicesChildNodes = domainChildNode.getChildNodes();
+
+for (int x = 0; x < devicesChildNodes.getLength(); x++) {
+Node deviceChildNode = devicesChildNodes.item(x);
+if ("disk".equals(deviceChildNode.getNodeName())) {
+Node diskNode = deviceChildNode;
+String sourceText = getSourceText(diskNode);
+NodeList diskChildNodes = diskNode.getChildNodes();
+for (int z = 0; z < diskChildNodes.getLength(); z++) {
+Node diskChildNode = diskChildNodes.item(z);
Review comment:
and finally `for (Node diskChildNode : diskChildNodes)`
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399321199
##
File path:
server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java
##
@@ -514,9 +584,11 @@ private boolean configureConfigDriveData(final
VirtualMachineProfile profile, fi
final String sshPublicKey = getSshKey(profile);
final String serviceOffering =
_serviceOfferingDao.findByIdIncludingRemoved(vm.getId(),
vm.getServiceOfferingId()).getDisplayText();
boolean isWindows =
_guestOSCategoryDao.findById(_guestOSDao.findById(vm.getGuestOSId()).getCategoryId()).getName().equalsIgnoreCase("Windows");
-
+String hostname = _hostDao.findById(vm.getHostId()).getName();
+final Account caller = CallContext.current().getCallingAccount();
+String destHostname =
(VirtualMachineManager.AllowExposeHypervisorHostname.value() &&
VirtualMachineManager.AllowExposeHypervisorHostnameAccountLevel.valueIn(caller.getId()))
? hostname : null;
Review comment:
maybe the hostname retrieval and caller account should be added inside the
call as well.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399284687
##
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
##
@@ -115,6 +118,30 @@ public Answer execute(final MigrateCommand command, final
LibvirtComputingResour
conn = libvirtUtilitiesHelper.getConnectionByVmName(vmName);
ifaces = libvirtComputingResource.getInterfaces(conn, vmName);
disks = libvirtComputingResource.getDisks(conn, vmName);
+
+String oldIsoVolumePath = null;
+for (DiskDef disk : disks) {
+if (disk.getDiskPath() != null &&
disk.getDiskPath().contains(vmName)) {
+oldIsoVolumePath = disk.getDiskPath();
+break;
+}
Review comment:
this means the VM could not have another iso mounted?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399314758
##
File path:
server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java
##
@@ -327,6 +355,30 @@ public void rollbackMigration(NicProfile nic, Network
network, VirtualMachinePro
public void commitMigration(NicProfile nic, Network network,
VirtualMachineProfile vm, ReservationContext src, ReservationContext dst) {
}
+private void recreateConfigDriveIso(NicProfile nic, Network network,
VirtualMachineProfile vm, DeployDestination dest) throws
ResourceUnavailableException {
+if (nic.isDefaultNic() &&
_networkModel.getUserDataUpdateProvider(network).getProvider().equals(Provider.ConfigDrive))
{
+DiskTO diskToUse = null;
+for (DiskTO disk : vm.getDisks()) {
+if (disk.getType() == Volume.Type.ISO && disk.getPath() !=
null && disk.getPath().contains("configdrive")) {
+diskToUse = disk;
+break;
+}
+}
+final UserVmVO userVm = _userVmDao.findById(vm.getId());
+final Account caller = CallContext.current().getCallingAccount();
+
+if (userVm != null) {
+final boolean isWindows =
_guestOSCategoryDao.findById(_guestOSDao.findById(userVm.getGuestOSId()).getCategoryId()).getName().equalsIgnoreCase("Windows");
Review comment:
seems like this statement could do with its own method for readability (will
be optimised out by the compiler anyway).
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399306118
##
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
##
@@ -572,4 +606,49 @@ private String getXml(Document doc) throws
TransformerException {
return byteArrayOutputStream.toString();
}
+
+private String replaceDiskSourceFile(String xmlDesc, String isoPath,
String vmName) throws IOException, SAXException, ParserConfigurationException,
TransformerException {
+InputStream in = IOUtils.toInputStream(xmlDesc);
+
+DocumentBuilderFactory docFactory =
DocumentBuilderFactory.newInstance();
+DocumentBuilder docBuilder = docFactory.newDocumentBuilder();
+Document doc = docBuilder.parse(in);
+
+// Get the root element
+Node domainNode = doc.getFirstChild();
+
+NodeList domainChildNodes = domainNode.getChildNodes();
+
+for (int i = 0; i < domainChildNodes.getLength(); i++) {
+Node domainChildNode = domainChildNodes.item(i);
+
+if ("devices".equals(domainChildNode.getNodeName())) {
+NodeList devicesChildNodes = domainChildNode.getChildNodes();
+
+for (int x = 0; x < devicesChildNodes.getLength(); x++) {
+Node deviceChildNode = devicesChildNodes.item(x);
+if ("disk".equals(deviceChildNode.getNodeName())) {
Review comment:
extract here again (as `process, search, replaceChildDisks()`)?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399328454
##
File path:
server/src/main/java/com/cloud/network/router/CommandSetupHelper.java
##
@@ -177,23 +180,30 @@
private VlanDao _vlanDao;
@Inject
private IPAddressDao _ipAddressDao;
-
@Inject
private RouterControlHelper _routerControlHelper;
+@Inject
+private HostDao _hostDao;
@Autowired
@Qualifier("networkHelper")
protected NetworkHelper _networkHelper;
public void createVmDataCommand(final VirtualRouter router, final UserVm
vm, final NicVO nic, final String publicKey, final Commands cmds) {
-final String serviceOffering =
_serviceOfferingDao.findByIdIncludingRemoved(vm.getId(),
vm.getServiceOfferingId()).getDisplayText();
-final String zoneName =
_dcDao.findById(router.getDataCenterId()).getName();
-final IPAddressVO staticNatIp =
_ipAddressDao.findByVmIdAndNetworkId(nic.getNetworkId(), vm.getId());
-cmds.addCommand(
-"vmdata",
-generateVmDataCommand(router, nic.getIPv4Address(),
vm.getUserData(), serviceOffering, zoneName,
-staticNatIp == null || staticNatIp.getState() !=
IpAddress.State.Allocated ? null : staticNatIp.getAddress().addr(),
vm.getHostName(), vm.getInstanceName(),
-vm.getId(), vm.getUuid(), publicKey,
nic.getNetworkId()));
+if (vm != null && router != null && nic != null) {
+final String serviceOffering =
_serviceOfferingDao.findByIdIncludingRemoved(vm.getId(),
vm.getServiceOfferingId()).getDisplayText();
+final String zoneName =
_dcDao.findById(router.getDataCenterId()).getName();
+final IPAddressVO staticNatIp =
_ipAddressDao.findByVmIdAndNetworkId(nic.getNetworkId(), vm.getId());
+
+Host host = _hostDao.findById(vm.getHostId());
+final Account caller = CallContext.current().getCallingAccount();
+String destHostname =
(VirtualMachineManager.AllowExposeHypervisorHostname.value() &&
VirtualMachineManager.AllowExposeHypervisorHostnameAccountLevel.valueIn(caller.getId()))
? host.getName() : null;
Review comment:
fifth
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399305021
##
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
##
@@ -572,4 +606,49 @@ private String getXml(Document doc) throws
TransformerException {
return byteArrayOutputStream.toString();
}
+
+private String replaceDiskSourceFile(String xmlDesc, String isoPath,
String vmName) throws IOException, SAXException, ParserConfigurationException,
TransformerException {
+InputStream in = IOUtils.toInputStream(xmlDesc);
+
+DocumentBuilderFactory docFactory =
DocumentBuilderFactory.newInstance();
+DocumentBuilder docBuilder = docFactory.newDocumentBuilder();
+Document doc = docBuilder.parse(in);
+
+// Get the root element
+Node domainNode = doc.getFirstChild();
+
+NodeList domainChildNodes = domainNode.getChildNodes();
+
+for (int i = 0; i < domainChildNodes.getLength(); i++) {
+Node domainChildNode = domainChildNodes.item(i);
+
+if ("devices".equals(domainChildNode.getNodeName())) {
Review comment:
extract from here? by name of `process- or search- or replaceChildDevices()`?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399267259
##
File path:
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##
@@ -1610,6 +1612,26 @@ public void finalizeUpdateInSequence(Network network,
boolean success) {
}
}
+@Override
+public void addNewDisk(VirtualMachineProfile vm, DeployDestination dest)
throws ResourceUnavailableException {
Review comment:
in view of what this method does, the name "addNewDisk" seems completely
wrong. Why is it named like this?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399272418 ## File path: engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java ## @@ -294,6 +294,8 @@ void implementNetworkElementsAndResources(DeployDestination dest, ReservationCon void finalizeUpdateInSequence(Network network, boolean success); +void addNewDisk(VirtualMachineProfile vm, DeployDestination dest) throws ResourceUnavailableException; Review comment: please add javadoc. the purpose of this method is not obvious from the name, parameters or return type. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399199919
##
File path: engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java
##
@@ -61,6 +61,12 @@
ConfigKey ResoureCountRunningVMsonly = new
ConfigKey("Advanced", Boolean.class,
"resource.count.running.vms.only", "false",
"Count the resources of only running VMs in resource limitation.",
true);
+ConfigKey AllowExposeHypervisorHostnameAccoutLevel = new
ConfigKey("Advanced", Boolean.class, "allow.expose.host.hostname",
+"false", "If set to true, it allows the hypervisor host name on
which the VM is spawned on to be exposed to the VM", true,
ConfigKey.Scope.Account);
+
+ConfigKey AllowExposeHypervisorHostname = new
ConfigKey("Advanced", Boolean.class, "allow.expose.host.hostname",
Review comment:
Two config keys at different scopes, with the same name. I will want to see
tests for that (unit and integration)
It might work but is not by design and we need to protect this or signal if
it fails after any kind of refactor.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399255992
##
File path: engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java
##
@@ -61,6 +61,12 @@
ConfigKey ResoureCountRunningVMsonly = new
ConfigKey("Advanced", Boolean.class,
"resource.count.running.vms.only", "false",
"Count the resources of only running VMs in resource limitation.",
true);
+ConfigKey AllowExposeHypervisorHostnameAccoutLevel = new
ConfigKey("Advanced", Boolean.class, "allow.expose.host.hostname",
Review comment:
also, I know this was countered before, but I woud like to see account in
the name of this setting. i.e. "account.allow.expose.host.hostname" and
"general.allow.expose.host.hostname"
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399258922
##
File path:
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##
@@ -2802,9 +2802,10 @@ private void orchestrateMigrateWithStorage(final String
vmUuid, final long srcHo
if
(_networkModel.isSharedNetworkWithoutServices(network.getId())) {
final String serviceOffering =
_serviceOfferingDao.findByIdIncludingRemoved(vm.getId(),
vm.getServiceOfferingId()).getDisplayText();
boolean isWindows =
_guestOSCategoryDao.findById(_guestOSDao.findById(vm.getGuestOSId()).getCategoryId()).getName().equalsIgnoreCase("Windows");
-
+final Account caller =
CallContext.current().getCallingAccount();
+String destHostname =
(VirtualMachineManager.AllowExposeHypervisorHostname.value() &&
VirtualMachineManager.AllowExposeHypervisorHostnameAccoutLevel.valueIn(caller.getId()))
? destination.getHost().getName() : null;
Review comment:
it looks like both must be true so the account level cannot be used to
override the global setting. Is that intentional? (this is both a functional as
well as a technical question)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399199291
##
File path: engine/api/src/main/java/com/cloud/vm/VirtualMachineManager.java
##
@@ -61,6 +61,12 @@
ConfigKey ResoureCountRunningVMsonly = new
ConfigKey("Advanced", Boolean.class,
"resource.count.running.vms.only", "false",
"Count the resources of only running VMs in resource limitation.",
true);
+ConfigKey AllowExposeHypervisorHostnameAccoutLevel = new
ConfigKey("Advanced", Boolean.class, "allow.expose.host.hostname",
Review comment:
spello : var name ending in `AccoutLevel`, missing an 'n'
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #3976: Enable sending hypervior host name via metadata - VR and Config Drive
DaanHoogland commented on a change in pull request #3976: Enable sending
hypervior host name via metadata - VR and Config Drive
URL: https://github.com/apache/cloudstack/pull/3976#discussion_r399198496
##
File path: core/src/main/java/com/cloud/agent/api/routing/VmDataCommand.java
##
@@ -72,5 +72,4 @@ public String getVmIpAddress() {
public void addVmData(String folder, String file, String contents) {
vmData.add(new String[] {folder, file, contents});
}
-
Review comment:
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
