Cleaned class “com.cloud.hypervisor.xenserver.resource.XcpOssResource” #560
Conversation
that seemed to be forgotten during the evolution of the ACS. It was removed a few methods that were already coded properly in its parent class “com.cloud.hypervisor.xenserver.resource.CitrixResourceBase”. It was also removed some methods that seemed to cause weird behaviors. The methods removed/fixed are detailed as follows: • com.cloud.hypervisor.xenserver.resource.XcpOssResource.fillHostInfo(Connection, StartupRoutingCommand) – it was removed, because it always added the string “, hvm” to the host capabilities. Therefore, if one uses XCP hypervisor it could cause a lot of trouble when deploying HVM virtual machines in an environment that has PV and HVM clusters. The method is already properly coded in parent class. • com.cloud.hypervisor.xenserver.resource.XcpOssResource.launchHeartBeat(Connection) – It was removed. It was not performing anything and always returns a true value. The method of parent class is properly coded and works for XCP environments. The heartbeat plugin exists in XCP environment. • com.cloud.hypervisor.xenserver.resource.XcpOssResource.initializeLocalSR(Connection) – it was removed. The method of the parent class works properly for XCP environments. • com.cloud.hypervisor.xenserver.resource.XcpOssResource.createPatchVbd(Connection, String, VM) – It was removed. This method causes a bug in XCP environments, because of its half-implementation, it was not possible to migrate system VMs. The parent class implementation works properly for XCP. • com.cloud.hypervisor.xenserver.resource.XcpOssResource.execute(NetworkUsageCommand) – removed, hence it was already coded into parent class and its respective wrappers (“com.cloud.hypervisor.xenserver.resource.wrapper.xcp.XcpServerNetworkUsageCommandWrapper”). BTW: I noticed that the class XcpServerNetworkUsageCommandWrapper and XenServer56NetworkUsageCommandWrapper are almost the same, with the exception that XenServer56NetworkUsageCommandWrapper deals with VPC. I believe that those wrappers could be converted into one, and moved to parent. I am not doing that here because I do not have a XCP environment with advanced networking to test it. • com.cloud.hypervisor.xenserver.resource.XcpOssResource.executeRequest(Command) – removed, hence it is not needed anymor. • com.cloud.hypervisor.xenserver.resource.XcpOssResource.execute(StopCommand) – I did not understand that method. It seemed weird and its removal did not change any behavior of the environment I tested it with.
|
cloudstack-pull-requests #688 FAILURE |
|
The problems found do not seem to be caused by my commit, hence I just On Sat, Jul 4, 2015 at 12:22 PM, asfbot [email protected] wrote:
Rafael Weingärtner |
|
The XenServer plugin build failed:
I'm trying to find out why it did so. A way to move forward would be for me, or someone else, to try to test your PR. I'm busy with another PR now, but if nobody picks your change, I might have a look later today. Concerning your comments about XcpServerNetworkUsageCommandWrapper and XenServer56NetworkUsageCommandWrapper being almost the same, that's due to the refactor I did on the CitrixResourceBase class. The refactor looked only at change the structure of the code, not behaviour. If 2 wrappers look/are the same, we need to do further work. I will take some time to look into it as well. Cheers, |
|
Travis jobs from 3591.1 to 3591.5 (inclusive) failed due to time out. |
|
The reason why the build failed can be found here: https://builds.apache.org/job/cloudstack-pull-requests/688/org.apache.cloudstack$cloud-plugin-hypervisor-xenserver/console To sum it up: your changes did not pass the audit due to tab entries in the code. That's a code-style audit. Did you use Eclipse/IntelliJ to change the Java code? If so, you will need to change your code settings to cope with 4-spaces instead of 1-tab. After that, you will have to format your change and commit again to the same branch you used to create the PR. Jenkins should be fine and we can proceed with testing/merging your changes. Cheers, Erros below: /home/jenkins/jenkins-slave/workspace/cloudstack-pull-requests/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/XcpOssResource.java:38:1: Line contains a tab character. |
|
cloudstack-pull-rats #1 SUCCESS |
|
Dear wilderrodrigues, In regarding to classes: XcpServerNetworkUsageCommandWrapper and On Mon, Jul 6, 2015 at 1:20 PM, asfbot [email protected] wrote:
Rafael Weingärtner |
|
I will take some time to merge them and will ping you for help. :) Once the build/travis is done, I will have a look at the changes and LGTM it. Cheers, |
|
cloudstack-pull-requests #696 SUCCESS |
|
Awesome, @rafaelweingartner Thanks for fixing it. LGTM @DaanHoogland would you mind to have a look at this one as well? Cheers, |
|
LGTM |
|
Nice to hear that, I will be waiting for CS 4.6 now ;) If you guys will check those classes “XcpServerNetworkUsageCommandWrapper” On Thu, Jul 9, 2015 at 9:00 AM, asfgit [email protected] wrote:
Rafael Weingärtner |
It seems that the class "XcpOssResource" was forgotten during the evolution of the ACS.
It was removed a few methods that were already coded properly in its
parent class
“com.cloud.hypervisor.xenserver.resource.CitrixResourceBase”. It was
also removed some methods that seemed to cause weird behaviors. The
methods removed/fixed are detailed as follows:
• com.cloud.hypervisor.xenserver.resource.XcpOssResource.fillHostInfo(Connection,
StartupRoutingCommand) – it was removed, because it always added the
string “, hvm” to the host capabilities. Therefore, if one uses XCP
hypervisor it could cause a lot of trouble when deploying HVM virtual
machines in an environment that has PV and HVM clusters. The method is
already properly coded in parent class.
• com.cloud.hypervisor.xenserver.resource.XcpOssResource.launchHeartBeat(Connection)
– It was removed. It was not performing anything and always returns a
true value. The method of parent class is properly coded and works for
XCP environments. The heartbeat plugin exists in XCP environment.
• com.cloud.hypervisor.xenserver.resource.XcpOssResource.initializeLocalSR(Connection)
– it was removed. The method of the parent class works properly for XCP
environments.
• com.cloud.hypervisor.xenserver.resource.XcpOssResource.createPatchVbd(Connection,
String, VM) – It was removed. This method causes a bug in XCP
environments, because of its half-implementation, it was not possible to
migrate system VMs. The parent class implementation works properly for
XCP.
• com.cloud.hypervisor.xenserver.resource.XcpOssResource.execute(NetworkUsageCommand)
– removed, hence it was already coded into parent class and its
respective wrappers
(“com.cloud.hypervisor.xenserver.resource.wrapper.xcp.XcpServerNetworkUsageCommandWrapper”).
BTW: I noticed that the class XcpServerNetworkUsageCommandWrapper and
XenServer56NetworkUsageCommandWrapper are almost the same, with the
exception that XenServer56NetworkUsageCommandWrapper deals with VPC. I
believe that those wrappers could be converted into one, and moved to
parent. I am not doing that here because I do not have a XCP environment
with advanced networking to test it.
• com.cloud.hypervisor.xenserver.resource.XcpOssResource.executeRequest(Command)
– removed, hence it is not needed anymor.
• com.cloud.hypervisor.xenserver.resource.XcpOssResource.execute(StopCommand)
– I did not understand that method. It seemed weird and its removal did
not change any behavior of the environment I tested it with.
I strongly encourage you guys to review the classes XcpServerNetworkUsageCommandWrapper and XenServer56NetworkUsageCommandWrapper, I think they can be merged. I also believe that the VPN code should be execute to XCP, which today is not happening.