You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by DaanHoogland <gi...@git.apache.org> on 2016/01/18 18:18:49 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
GitHub user DaanHoogland opened a pull request:
https://github.com/apache/cloudstack/pull/1348
CLOUDSTACK-9142 Migrate VM changes xmlDesc in a safe way
The problem arises when the origin hypervisor has an ip addres that ends with 1, like '10.10.10.1' and the VM is having a disk on an NFS share that has that as part of its address, '10.10.10.100' for instance.
now migrating to '10.10.10.10' will change both addresses in the xml description file for qemu. It is fixed and unit tests are added. I am not sure yet how to integration test this. Regression will probably work so creating a PR now.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/DaanHoogland/cloudstack CLOUDSTACK-9142
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1348.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1348
----
commit 2df0379d8ca3754fe926c168e304e2ff70f5e204
Author: Daan Hoogland <da...@onecht.net>
Date: 2016-01-18T16:43:43Z
CLOUDSTACK-9142 Migrate VM changes xmlDesc in a safe way
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-214995585
@DaanHoogland yes that or something similar so test data can be separate to its own file
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-215075290
@rhtyd as a part of loading from resource I am restructuring the project layout to adhere to maven standards. Doing so it seems not appropriate to include such a change in this bug fix.@swill I think this can be merged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-215185737
Ok perfect. I will add this to my merge queue. Thanks for clarifying guys...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-215179261
Thanks guys. I am fine with merging it as is, but does it make sense to wait a bit to see what the discussion brings up in the #1521 PR? Or will it not affect this PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-172624897
@DaanHoogland Please check, unit test error:
```
-------------------------------------------------------
T E S T S
-------------------------------------------------------
Running com.cloud.hypervisor.kvm.resource.LibvirtComputingResourceTest
log4j:WARN No appenders could be found for logger (org.reflections.Reflections).
log4j:WARN Please initialize the log4j system properly.
log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info.
Tests run: 144, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.726 sec <<< FAILURE! - in com.cloud.hypervisor.kvm.resource.LibvirtComputingResourceTest
testMigrateCommand(com.cloud.hypervisor.kvm.resource.LibvirtComputingResourceTest) Time elapsed: 0.013 sec <<< ERROR!
java.lang.StringIndexOutOfBoundsException: String index out of range: -1
at java.lang.String.substring(String.java:1960)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1348#discussion_r56420155
--- Diff: plugins/hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapperTest.java ---
@@ -0,0 +1,306 @@
+//
+// 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.
+//
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Test;
+
+public class LibvirtMigrateCommandWrapperTest {
+ String fullfile =
+"<domain type='kvm' id='4'>\n" +
--- End diff --
Move this string into a separate file under resource/ for this test case. Read the file from the resource path.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-173357364
Looking at this test I see the problem indeed. I actually created this regression while fixing it.
One thing though, you mean that the IP of the NFS server is part of the XML desc, can you show me an example? I can't think of a way though.
The code seems good to me, but running a integration test on this is indeed hard to do.
This would btw also apply for a Ceph cluster. If a Ceph monitor has a IP which 'matches' that of the host the same situation could arrise.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-173360077
Except for the concern that there may be multiple graphics element where the method would clearly fail, LGTM. Please also squash your commits.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-173392186
@remibergsma that is serious. migration is failing for us with the tested code and the test is failing with the fix. I have work to do it seems. :) Can you send me the full log for comparison, please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-173358933
Hi @DaanHoogland,
I would only suggest you extracting those magic numbers at line 97 to constant variables (using some descriptive names).
I also have a doubt,
Are we using that “@author” directive? Such as the one you have at line 26 of “LibvirtMigrateCommandWrapperTest”
BTW: I really liked the “replaceIpForVNCInDescFile” method. Very nice and descriptive method name, comprehensive java doc, test cases and the method itself is not complicated. I believe that should be our code quality goal. Congratulations !
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1348#discussion_r50334011
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java ---
@@ -190,4 +196,28 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
return new MigrateAnswer(command, result == null, result, null);
}
-}
\ No newline at end of file
+
+ /**
+ * This function assumes an qemu machine desription containing a single graphics element like
--- End diff --
@bhaisaab not in a cloudstack generated desc file. But you are right inprinciple it could exist.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1348#discussion_r50325672
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java ---
@@ -48,6 +49,9 @@
@ResourceWrapper(handles = MigrateCommand.class)
public final class LibvirtMigrateCommandWrapper extends CommandWrapper<MigrateCommand, Answer, LibvirtComputingResource> {
+ private static final String CONTENTS_WILDCARD = ".*";
+ private static final String GRAPHICS_ELEM_END = "/graphics>";
--- End diff --
This doesn't look correct... ?
"<" missing?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-173398341
@rafaelweingartner I have no idea about 'those magic numbers' I didn't touch them.
I can remove the @author. it was auto generated.
thanks for the compliment, though I myself think the javadoc could have been even more terse.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1348
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-173389882
@wido the problem we encountered in our env is not with NFS servers but with RBD. sorry for the misdirection.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-215182527
No it won't. the fix in here is valid whichever way we go.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-215170748
@swill yes that was a good to have suggestion; keep large string based test data separated from test, though since this request is cosmetic in nature I'm okay if we decide to merge
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-215084100
So if I am following this correctly, we are all set. @rhtyd requested a change to move the test data to an overridable location, but did not require this change for his LGTM. @DaanHoogland would have to restructure the plugin in order to make that happen and feels it is outside the scope of this fix and would prefer to address that to a future improvement. Does that all make sense?
I will put this in my merge queue, but will wait for confirmation that we are good to go before merging. Thanks guys...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by bvbharatk <gi...@git.apache.org>.
Github user bvbharatk commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-200300953
### ACS CI BVT Run
**Sumarry:**
Build Number 122
Hypervisor xenserver
NetworkType Advanced
Passed=105
Failed=13
Skipped=4
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_deploy_vgpu_enabled_vm
test_06_copy_template
test_06_copy_iso
**Passed test suits:**
integration.smoke.test_deploy_vm_with_userdata.TestDeployVmWithUserData
integration.smoke.test_affinity_groups_projects.TestDeployVmWithAffinityGroup
integration.smoke.test_portable_publicip.TestPortablePublicIPAcquire
integration.smoke.test_over_provisioning.TestUpdateOverProvision
integration.smoke.test_global_settings.TestUpdateConfigWithScope
integration.smoke.test_scale_vm.TestScaleVm
integration.smoke.test_service_offerings.TestCreateServiceOffering
integration.smoke.test_loadbalance.TestLoadBalance
integration.smoke.test_routers.TestRouterServices
integration.smoke.test_reset_vm_on_reboot.TestResetVmOnReboot
integration.smoke.test_snapshots.TestSnapshotRootDisk
integration.smoke.test_deploy_vms_with_varied_deploymentplanners.TestDeployVmWithVariedPlanners
integration.smoke.test_network.TestDeleteAccount
integration.smoke.test_non_contigiousvlan.TestUpdatePhysicalNetwork
integration.smoke.test_deploy_vm_iso.TestDeployVMFromISO
integration.smoke.test_public_ip_range.TestDedicatePublicIPRange
integration.smoke.test_multipleips_per_nic.TestDeployVM
integration.smoke.test_regions.TestRegions
integration.smoke.test_affinity_groups.TestDeployVmWithAffinityGroup
integration.smoke.test_network_acl.TestNetworkACL
integration.smoke.test_pvlan.TestPVLAN
integration.smoke.test_volumes.TestCreateVolume
integration.smoke.test_ssvm.TestSSVMs
integration.smoke.test_nic.TestNic
integration.smoke.test_deploy_vm_root_resize.TestDeployVM
integration.smoke.test_resource_detail.TestResourceDetail
integration.smoke.test_secondary_storage.TestSecStorageServices
integration.smoke.test_vm_life_cycle.TestDeployVM
integration.smoke.test_disk_offerings.TestCreateDiskOffering
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-173364862
@DaanHoogland This test is failing:
```
Test migrate VM ... === TestName: test_08_migrate_vm | Status : EXCEPTION ===
ERROR
```
Details:
```
======================================================================
ERROR: Test migrate VM
----------------------------------------------------------------------
Traceback (most recent call last):
File "/data/git/cs1/cloudstack/test/integration/smoke/test_vm_life_cycle.py", line 594, in test_08_migrate_vm
self.vm_to_migrate.migrate(self.apiclient, migrate_host.id)
File "/usr/lib/python2.7/site-packages/marvin/lib/base.py", line 653, in migrate
apiclient.migrateVirtualMachine(cmd)
File "/usr/lib/python2.7/site-packages/marvin/cloudstackAPI/cloudstackAPIClient.py", line 772, in migrateVirtualMachine
response = self.connection.marvinRequest(command, response_type=response, method=method)
File "/usr/lib/python2.7/site-packages/marvin/cloudstackConnection.py", line 379, in marvinRequest
raise e
Exception: Job failed: {jobprocstatus : 0, created : u'2016-01-20T21:19:08+0000', cmd : u'org.apache.cloudstack.api.command.admin.vm.MigrateVMCmd', userid : u'95279fda-bed0-11e5-90a2-5254001daa61', jobstatus : 2, jobid : u'e7a1bb0f-39a5-4868-97c9-8fda5f8a8aa1', jobresultcode : 530, jobresulttype : u'object', jobresult : {errorcode : 530, errortext : u"org.libvirt.LibvirtException: internal error: process exited while connecting to monitor: 2016-01-20T21:19:09.852246Z qemu-kvm: Failed to start VNC server on `192.168.22.22:0,password': Failed to bind socket: Cannot assign requested address\n"}, accountid : u'95278d2a-bed0-11e5-90a2-5254001daa61'}
```
Tried the test again and it failed again. Can you investigate please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1348#discussion_r50335474
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java ---
@@ -190,4 +196,28 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
return new MigrateAnswer(command, result == null, result, null);
}
-}
\ No newline at end of file
+
+ /**
+ * This function assumes an qemu machine desription containing a single graphics element like
+ * <graphics type='vnc' port='5900' autoport='yes' listen='10.10.10.1'>
+ * <listen type='address' address='10.10.10.1'/>
+ * </graphics>
+ * @param xmlDesc the qemu xml description
+ * @param source the ip address to migrate from
--- End diff --
not used?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1348#discussion_r50333689
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java ---
@@ -1,4 +1,5 @@
//
+
--- End diff --
doesn't seem intentional, will have a look
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-214858450
LGTM, though it would be great if @DaanHoogland can move part of the test string from the Test to an xml file under resources
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by davidamorimfaria <gi...@git.apache.org>.
Github user davidamorimfaria commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-187342982
LGTM, 4.7.1 with this fix was installed on a live environment with success. Instances can now be migrated to and from the kvm node that has an IP address which partially matches one of the ceph monitor nodes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-176010507
@wido yes it was fixed. The problem was the newlines in the pattern to search for. I had not taken that in account for in the unit test. We are about to put this in production but first I have to check how the status of the tests are.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1348#discussion_r50333804
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java ---
@@ -48,6 +49,9 @@
@ResourceWrapper(handles = MigrateCommand.class)
public final class LibvirtMigrateCommandWrapper extends CommandWrapper<MigrateCommand, Answer, LibvirtComputingResource> {
+ private static final String CONTENTS_WILDCARD = ".*";
+ private static final String GRAPHICS_ELEM_END = "/graphics>";
--- End diff --
why would that be needed, the match is conclusive enough without it. Adding it would be purely cosmetic, wouldn't it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-197555778
It looks like @davidamorimfaria is using this in production now without issue, so that is a good sign.
@remibergsma do the tests that were previously failing now pass?
Thanks...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1348#discussion_r56420080
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java ---
@@ -190,4 +195,27 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
return new MigrateAnswer(command, result == null, result, null);
}
-}
\ No newline at end of file
+
+ /**
+ * This function assumes an qemu machine description containing a single graphics element like
+ * <graphics type='vnc' port='5900' autoport='yes' listen='10.10.10.1'>
+ * <listen type='address' address='10.10.10.1'/>
+ * </graphics>
+ * @param xmlDesc the qemu xml description
+ * @param target the ip address to migrate to
+ * @return the new xmlDesc
+ */
+ String replaceIpForVNCInDescFile(String xmlDesc, final String target) {
+ final int begin = xmlDesc.indexOf(GRAPHICS_ELEM_START);
--- End diff --
While this should work for most cases, the code is not defensive. For example, it will fail for multiple graphics nodes or if there are any whitespaces between closing brackets. Consider using a dom parser.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-172661987
looking into this, probably been working on to much at the same time today, sorry
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-215087562
That's correct. I am doing the restructure now based on this PR's source branch and will make a new PR shortly. If it gets tested before this one I will close this one but it is really a pedantic play-with-myself kind of thing and the wether outside is fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1348#discussion_r50325617
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java ---
@@ -1,4 +1,5 @@
//
+
--- End diff --
Why?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-173519263
@remibergsma I reproduced the error. I had not copied the newlines from the problematic system into my unit test. will push a fix without hurry.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1348#discussion_r50319348
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java ---
@@ -190,4 +196,28 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
return new MigrateAnswer(command, result == null, result, null);
}
-}
\ No newline at end of file
+
+ /**
+ * This function assumes an qemu machine desription containing a single graphics element like
--- End diff --
What is there are multiple graphics element? Can we ever hit a case where there may be more than one <graphics> element?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-214855601
Update on this? Is this ready to be merged? We are missing one LGTM.
Code looks good to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-175824842
@DaanHoogland Have you been able to fix this? Code-wise it still seems good.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-187167486
@remibergsma @wido can we merge this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1348#discussion_r50336727
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java ---
@@ -190,4 +196,28 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
return new MigrateAnswer(command, result == null, result, null);
}
-}
\ No newline at end of file
+
+ /**
+ * This function assumes an qemu machine desription containing a single graphics element like
+ * <graphics type='vnc' port='5900' autoport='yes' listen='10.10.10.1'>
+ * <listen type='address' address='10.10.10.1'/>
+ * </graphics>
+ * @param xmlDesc the qemu xml description
+ * @param source the ip address to migrate from
--- End diff --
no, will remove. tnx. it was used in the initial version.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1348#issuecomment-214889603
@rhtyd tomorrow #kingsday, I will have some free time ;) loadResourceFromClassPath you are talking about, huh?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1348#discussion_r56490248
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java ---
@@ -190,4 +195,27 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
return new MigrateAnswer(command, result == null, result, null);
}
-}
\ No newline at end of file
+
+ /**
+ * This function assumes an qemu machine description containing a single graphics element like
+ * <graphics type='vnc' port='5900' autoport='yes' listen='10.10.10.1'>
+ * <listen type='address' address='10.10.10.1'/>
+ * </graphics>
+ * @param xmlDesc the qemu xml description
+ * @param target the ip address to migrate to
+ * @return the new xmlDesc
+ */
+ String replaceIpForVNCInDescFile(String xmlDesc, final String target) {
+ final int begin = xmlDesc.indexOf(GRAPHICS_ELEM_START);
--- End diff --
@bhaisaab that is out of scope I think. The node is under ACS control. Adding a xml parser would be nice, I agree.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---