You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by koushik-das <gi...@git.apache.org> on 2015/08/21 13:13:47 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8754: VM migration triggered b...
GitHub user koushik-das opened a pull request:
https://github.com/apache/cloudstack/pull/725
CLOUDSTACK-8754: VM migration triggered by dynamic scaling is failing
This is caused by serialization failure for VmWorkMigrateForScale object. Replaced DeployDestination member present in VmWorkMigrateForScale with serializable types.
Haven't added any unit test as couldn't find a clean way to do it. Simply adding a test to do (de)serialization won't help catch any new member addition to the type which breaks serializability.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/koushik-das/cloudstack CLOUDSTACK-8754
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/725.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 #725
----
commit 3be2b6310333b8ee263ef7865d235d30ec9c804f
Author: Koushik Das <ko...@apache.org>
Date: 2015-08-21T11:01:44Z
CLOUDSTACK-8754: VM migration triggered by dynamic scaling is failing
This is caused by serialization failure for VmWorkMigrateForScale object. Replaced DeployDestination member
present in VmWorkMigrateForScale with serializable types.
----
---
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-8754: VM migration triggered b...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/725#issuecomment-133457425
Maybe a little out of scope for this PR, I have been looking a bit at the hierarchy. It contains a lot of primitives. somewhat related to our gson problem (arrays of primitive types) those should be removed.
@kishankavala I see your point on serialisation testing. But it doesn't mean that a simple test is useless. (no -1 for that) Let us think/search on applicable tests.
Did you test this code in any 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-8754: VM migration triggered b...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/725#issuecomment-134089286
@kishankavala a trivial test is better then none. for better tests, a jira ticket can be created and comments in the test class can be added.
that said LGTM
---
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-8754: VM migration triggered b...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/725#issuecomment-133449560
You mention replacing deployDestination with something serializable but I don't see it,
Why is a simple (de)serialization test not usefull? it will catch some things won't it?
A > serialize > desrialize > B
assertEquals(A,B);
btw removal if primitive types in serializable classes is something we do need.
---
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-8754: VM migration triggered b...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/725#issuecomment-134902453
FWIW, this LGTM; @koushik-das please send a different PR in future if you decide to add tests and do further enhancements.
Travis is green, merging now.
---
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-8754: VM migration triggered b...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/725#discussion_r37642249
--- Diff: engine/orchestration/src/com/cloud/vm/VmWorkMigrateForScale.java ---
@@ -18,30 +18,18 @@
import com.cloud.deploy.DeployDestination;
-public class VmWorkMigrateForScale extends VmWork {
+public class VmWorkMigrateForScale extends VmWorkMigrate {
private static final long serialVersionUID = 6854870395568389613L;
- long srcHostId;
- DeployDestination deployDestination;
--- End diff --
Serialization was broken anyway so doesn't matter.
---
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-8754: VM migration triggered b...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/725#issuecomment-133451646
@DaanHoogland check the base class
The test cannot catch a new addition to the class which may break serialization.
Suppose a new non-serializable member of type Object is added to the class and is initialized to null and only way to set it is through a new ctor or setter. Then the test would still pass. Ideally in this case you want the test to fail.
---
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-8754: VM migration triggered b...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/725#issuecomment-134038561
@DaanHoogland I have verified using the simulator based on the repro steps mentioned in the bug. I found some issues in the simulator while testing this issue end-to-end, will push the simulator fixes as a separate PR.
Also I had done a (de)serialization test for VmWorkMigrateForScale. But I don't want to have it as unit test unless I see a valid justification (or some better way to test 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-8754: VM migration triggered b...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/725#discussion_r37640687
--- Diff: engine/orchestration/src/com/cloud/vm/VmWorkMigrateForScale.java ---
@@ -18,30 +18,18 @@
import com.cloud.deploy.DeployDestination;
-public class VmWorkMigrateForScale extends VmWork {
+public class VmWorkMigrateForScale extends VmWorkMigrate {
private static final long serialVersionUID = 6854870395568389613L;
- long srcHostId;
- DeployDestination deployDestination;
--- End diff --
removing deployDestination warrants a new serialVersionUID, does 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-8754: VM migration triggered b...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/725#issuecomment-133552003
analysisfailures are unrelated to the changes AFAICT
---
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-8754: VM migration triggered b...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/725
---
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-8754: VM migration triggered b...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/725#issuecomment-133399741
Although I think the code is good and I want to give a LGTM, I'm just not 100% sure about this one.
---
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.
---