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
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
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 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
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
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
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
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
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
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
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,
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
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
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
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 @@
+//
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
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
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
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
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
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
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
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).
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
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
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
```
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
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
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
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
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
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 @@
//
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 @@
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
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
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 @@
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 @@
//
+
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
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
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
40 matches
Mail list logo
