[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[asfgit]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[swill]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[swill]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[rhtyd]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[swill]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[rhtyd]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[rhtyd]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[wido]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[bvbharatk]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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
+ * 
+ *   
+ * 
+ * @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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[bhaisaab]

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 =
+"\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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[bhaisaab]

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
+ * 
+ *   
+ * 
+ * @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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[swill]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[davidamorimfaria]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[wido]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[rafaelweingartner]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[bhaisaab]

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  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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[bhaisaab]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[remibergsma]

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+', 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[wido]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[borisroman]

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
+ * 
+ *   
+ * 
+ * @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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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
+ * 
+ *   
+ * 
+ * @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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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 {
 
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[borisroman]

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 {
 
+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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[borisroman]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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 
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[remibergsma]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9142 Migrate VM changes xmlDes...

[DaanHoogland]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---