CID-1338387: remove logicless execution cycles

[DaanHoogland]

I have removed the paramoia null check and created a specific exception as i think we always should for any specific condition. This would require careful consideration each time on whether and how specific the condition is of course ;)

[miguelaferreira]

Wouldn't it be better to make sure endPointSelector.selectHypervisorHost(...) never returns null?

[DaanHoogland]

It was put in there explicitely (return null;) so i trusted the intention. a review of the architecture could be justified.

[miguelaferreira]

That intention opens up an entire category of errors, null dereferencing. In addition forces defensive coding, just like what you introduce in this PR.
Why not try to, incrementally, move away from that instead of capitulating it?

[asfbot]

Daan Hoogland on dev@cloudstack.apache.org replies:
sure

[rafaelweingartner]

@DaanHoogland I agree with @miguelaferreira.
I have looked that code.
The select it runs is the following:
select h.id from host h where h.status = 'Up' and h.hypervisor_type is not null and h.data_center_id = ? ORDER by rand() limit 1

That makes me wondering, why ordering by random the projection?! That can make problems harder to debug. Whatever, that is not the focus here.
The only ways I see that select returning nothing are the following:
• If we have no hosts in the zone;
• If all of the hosts have hypervisor_type equal to null, is it even possible to have a host with hypervisor_type = null?
I would be concerned about the first case, a zone with no hosts. Thus, we should have to check when the method “com.cloud.hypervisor.ovm3.resources.Ovm3HypervisorGuru.performSideEffectsForDelegationOnCommand(long, Command)” is called.
What API method ends up in that code, or what action of ACS is going execute it?

[DaanHoogland]

@miguelaferreira@rafaelweingartner I totally agree with you that this can be dealt with better. I didn't want to give it a second thought as my intention is to get more coverity issues fixed then newly reported on every itteration. I am fine with not committing this as I first approached so will look into a better solution. please see #1057 and comment on that as well.

[DaanHoogland]

@rafaelweingartner after randomly oredering and limiting output to 1 the next step is looping over all (1) results and retrieving all hosts one by one ignoring previous results in the set.

@miguelaferreira growing this out my idea is to start by having selectHypervisorHost throw an exception on host == null and catch it one level up to just ignore the finding it again part. I'll implement it for arguments sake and force push

[DaanHoogland]

@miguelaferreira@rafaelweingartner you guys care to discuss this further? (this as in this case but also this as in this pattern)

[rafaelweingartner]

@ DaanHoogland, I noticed that the select is limiting the RS in one element. That is actually my point, if the register does not matter, I would rather order by id DESC or ASC and not at random (of course, I would maintain the limit 1 clause); because that way if something happens I think it would be easier to track the problem.

Additionally, as you pointed that out, I think if the host == null, throwing an exception is a better way to go.

BTW: I was staring at that code now, and something hit me.
The method “org.apache.cloudstack.storage.endpoint.DefaultEndPointSelector.selectHypervisorHost(Scope)” returns an object “EndPoint” that contains a random host id. If the host does not matter, why not use the host that we have already loaded in line 113? We could just check if the attribute “hypervisor_type” is not null for that host; that is what the select in “selectHypervisorHost” method is doing.

I also noticed that you created two new exceptions, would not it be better for us to use the already known “CloudRuntimeException”? Instead of creating other exception types.
I think there was a problem with Jenkins, JVM crash ?!
I will also take a look at PR #1057.

[miguelaferreira]

If there will be no error recovery for the exception thrown on host == null, then you will probably be better off with using Optional. It's in Java8, but also in Guava, I think.

[rafaelweingartner]

Are we already migrating to Java 8?

[DaanHoogland]

Optionals are great whether we need to handle or not, right? Especially in this one level case. I was thinking it would get more complicated in some cases. Even in this case if we realy think it through.

If we go and think of a good pattern to handle nulls with optionals we better make it very tempting to use all over the cloud. a bigger problem then a less then perfect solution is more then one solution to a problem. it will lead to confusion.

@rafaelweingartner yes we are going to 1.8. I think we can run on 1.8 now.

[rafaelweingartner]

That is great if we can use Java 8. However, I checked the maven compiler configuration, and it is using Java 1.7. Will that be changed in current master?

[DaanHoogland]

yeah, you had me at 'Are we'. I am now building with 1.8. change in the pom and et the java_home should be all

[rafaelweingartner]

Got it. Shouldn’t we create a PR changing the Java compilation level to 1.8?

@DaanHoogland, I still have a doubt about the need of that “selectHypervisorHost” method call. If you take a look at the stack of calls, the method that starts the process is “com.cloud.hypervisor.HypervisorGuruManagerImpl.getGuruProcessedCommandTargetHost(long, Command)” that method makes me think that we could just use the host that it receives as a parameter instead of looking for another one.

What is the point on getting a host at random? There was not even a check if the hypervisor type of the host that was selected is the same as the host that initiated the whole process.

Moreover, line 79 of “getGuruProcessedCommandTargetHost”, already uses the hypervisor type of the host that is being sent as a parameter; I am not seeing why that call “endPointSelector.selectHypervisorHost” is needed. It seems that we could just remove lines 114 and 115 in Ovm3HypervisorGuru.java (prior to the modifications that @DaanHoogland did) and we would be good to go, without much complications.

The same happens for XenServerGuru.java, lines 193 and 194.

[DaanHoogland]

@rafaelweingartner I think the method configures the host to have the right storage endpoint. It is kind of an obfuscated side effect but I think the call to endPointSelector.selectHypervisorHost(new ZoneScope(host.getDataCenterId())); is not totally without effect.

[DaanHoogland]

@rafaelweingartner the compile of cloudstack with 1.8 completes but not with unit tests. I think we need to work hard for this.
in jenkins it works however: http://jenkins.buildacloud.org/job/build-master-jdk18/configure
it runs mvn -Psystemvm clean test -Dmaven.compiler.source-1.8 -Dmaven.compiler.target=1.8
and usually passes

[rafaelweingartner]

@DaanHoogland, sorry if I am being stubborn, but I really do not see what the method “endPointSelector.selectHypervisorHost” is used for; at the end we get the “EndPoint” and use just the host_id of that object, then we load the host using that id and the details of that host.

My point is that, if the host does not matter, why not use the host we have already loaded in line 113, and then load the details of that host. If we do that, we can remove the “endPointSelector.selectHypervisorHost” method.

The configure method you talked about (org.apache.cloudstack.storage.RemoteHostEndPoint.configure(Host)) will load the “publicAddress”,if the host it receives as a parameter is a SSVM. However, the select “select h.id from host h where h.status = 'Up' and h.hypervisor_type is not null” that is executed in (endPointSelector.selectHypervisorHost) will not return any of the system VMs, because they have “hypervisor_type” value as null. Therefore, we are using that method without needing it; that method will just return a random host of the environment.

[miguelaferreira]

We don't need Java 8 to use optional. And yes, optional is meant for expressing that a value may be present or not. The API is quite clear, I don't see the confusion.

https://github.com/google/guava/wiki/UsingAndAvoidingNullExplained

[DaanHoogland]

@miguelaferreira Optional is very clear indeed. So is exception handling. When you need to handle a possible null return one level deep optional is fine. Newbees look at the code of one hypervisor guru as example of the code for an other hypervisor guru. Let's not implement a different solution strategy in each but find a solution that works in a generic way, avoiding confusion on the part of the spectator.

@rafaelweingartner your stubbornness is much appreciated. I want to understand exactly what happens as well. You have not convinced me that the call is without side effects yet. At the very least an EndPoint object is created by the spring framework. I don't say that this side effect is desired but I want to be sure before just cutting the code away. The endpoint in a storage subsystem is not intended to be the same as a hypervisor and maybe I let let myself be misled by that.

[miguelaferreira]

@DaanHoogland both Optional and Exception will give the opportunity to handle the "null case", the difference is in the semantics. Optional means that you expect to either have or not have a value. Exception means that no value is an error state/case. So you should use one or the other depending on what does it mean semantically to not have a value. Newbies or otherwise will understand this.

Regarding your point about different solutions in different places, I would say that it argues for stagnation. You present two options: either apply a better solution to all places, or keep replicating the current solution. I propose a different approach. An approach that will introduce a better solution incrementally, one (or a few) places at a time. I don't see any problem with this, since we actually keep history everyone can see that one change happened in 2010 and the other in 2015. Then people can ask about it, or better yet we can tell them upfront.

We just have to be willing to do the best we can at all times.

[DaanHoogland]

@miguelaferreira you are assigning claims to me that I did not make: "You present two options: "

[miguelaferreira]

I don't think so @DaanHoogland. But if I've misinterpreted what you said, I'll be happy to have you explain to me what that is.

[rafaelweingartner]

Hi @DaanHoogland,
About the method: “endPointSelector.selectHypervisorHost”.

I debugged the use of that method in my environment. I am using Xen; and that method is used when creating a template from a snapshot; the CS will execute the code: “com.cloud.hypervisor.XenServerGuru.getCommandHostDelegation(long, Command)”
ACS: 4.3.2
Xen (4.1) + XAPI (1.6), those components together are basically the same as a xenserver 6.0. The difference is that we use Debian as our OS and not CentOS.

Look at what happened in my test, the hostHd received in the “XenServerGuru.getCommandHostDelegation” method was (100).

“XenServerGuru.getCommandHostDelegation” loaded the host data from host table to retrieve the dataCenterID, and then executed “endPointSelector.selectHypervisorHost”, that ended up selecting a host that has the id (87) which is in a different cluster. Then it created the “EndPoint” object that does not do much; there is a configure method “org.apache.cloudstack.storage.RemoteHostEndPoint.configure(Host)” that is called, but it merely retrieves some data from the host object that was sent as a parameter. After that in line 161 of “XenServerGuru.getCommandHostDelegation” it is loaded the host data using the host id of the “EndPoint” object; then we check if the host (87) has a fix “snapshotHotFix”; in my case, the host did not have, so it ended up returning the original host id (100) at line 170.
Even if the host (87) had the “snapshotHotFix”, I do not understand why it is getting a host at random in the environment; If the host does not matter, why not use the host that the method already received as a parameter?

The name of the method “org.apache.cloudstack.storage.endpoint.DefaultEndPointSelector.selectHypervisorHost” does not say much, and its execution does not make much sense to me. I believe it can be removed without any harm.

[DaanHoogland]

@rafaelweingartner nice analysis. meaning, all of my change is senseless. I think the conclusion of your research is that the side effects for CopyCommand are none and we can actually remove this bit of the code. @snuf can you concur?

[DaanHoogland]

running the regression tests on this but these are not in ovm(yet) will look into that.

[rafaelweingartner]

Cool,
It seems that the JVM crashed during Jenkins execution.

[DaanHoogland]

see INFRA-10703 at the apache jira :) and cast your vote for it

[DaanHoogland]

did the sbp regression tests on this:

nosetests --with-marvin --marvin-config=${marvinCfg} -s -a tags=advanced,required_hardware=false \
smoke/test_routers.py \
smoke/test_network_acl.py \
smoke/test_privategw_acl.py \
smoke/test_reset_vm_on_reboot.py \
smoke/test_vm_life_cycle.py \
smoke/test_service_offerings.py \
smoke/test_network.py \
component/test_vpc_offerings.py \
component/test_vpc_routers.py

----------------------------------------------------------------------
Ran 42 tests in 9311.074s
OK

and

nosetests --with-marvin --marvin-config=${marvinCfg} -s -a tags=advanced,required_hardware=true \
component/test_vpc_redundant.py \
component/test_routers_iptables_default_policy.py \
component/test_routers_network_ops.py \
component/test_vpc_router_nics.py \
component/test_password_server.py \
component/test_router_dhcphosts.py \
smoke/test_loadbalance.py \
smoke/test_internal_lb.py \
smoke/test_ssvm.py \
smoke/test_vpc_vpn.py \
smoke/test_network.py

----------------------------------------------------------------------
Ran 33 tests in 17445.193s
OK

these do not include ovm tests however and were only run with a 2 kvm host config. will run with a 2 xen host config, now

[rafaelweingartner]

Before you run the Xen tests, I think you should apply the changes we talked about in "XenServerGuru.java" and remove the method "EndPoint selectHypervisorHost(Scope scope);" from "EndPointSelector.java"

[DaanHoogland]

@rafaelweingartner too late 😢 is running, next spin.

[rafaelweingartner]

@DaanHoogland, would you like me to finish the changes that we need, so we can proceed with this PR?

[DaanHoogland]

@rafaelweingartner please go ahead

[DaanHoogland]

#1177 obsoleted this

[rafaelweingartner]

Actually, the one that obsoleted this is #1124

[DaanHoogland]

@rafaelweingartner you better read #1177, part of #1124 will no longer merge cleanly because of the ovm3 changes.

[rafaelweingartner]

@DaanHoogland PR #1124 rebased on top of master.