Skip to content

Commit 1e2e1ea

Browse files
kelvenykelveny
committed
CLOUDSTACK-5765: cleanup internal serialization and exception propagation issues
1 parent a05d71a commit 1e2e1ea

13 files changed

Lines changed: 131 additions & 62 deletions

File tree

‎api/src/com/cloud/deploy/DeployDestination.java‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
// under the License.
1717
package com.cloud.deploy;
1818

19+
import java.io.Serializable;
1920
import java.util.Map;
2021

2122
import com.cloud.dc.DataCenter;
@@ -26,7 +27,9 @@
2627
import com.cloud.storage.Volume;
2728
import com.cloud.utils.NumbersUtil;
2829

29-
public class DeployDestination {
30+
public class DeployDestination implements Serializable {
31+
private static final long serialVersionUID = 7113840781939014695L;
32+
3033
DataCenter _dc;
3134
Pod _pod;
3235
Cluster _cluster;
@@ -76,28 +79,28 @@ public int hashCode() {
7679
@Override
7780
public boolean equals(Object obj) {
7881
DeployDestination that = (DeployDestination)obj;
79-
if (this._dc == null || that._dc == null) {
82+
if (_dc == null || that._dc == null) {
8083
return false;
8184
}
82-
if (this._dc.getId() != that._dc.getId()) {
85+
if (_dc.getId() != that._dc.getId()) {
8386
return false;
8487
}
85-
if (this._pod == null || that._pod == null) {
88+
if (_pod == null || that._pod == null) {
8689
return false;
8790
}
88-
if (this._pod.getId() != that._pod.getId()) {
91+
if (_pod.getId() != that._pod.getId()) {
8992
return false;
9093
}
91-
if (this._cluster == null || that._cluster == null) {
94+
if (_cluster == null || that._cluster == null) {
9295
return false;
9396
}
94-
if (this._cluster.getId() != that._cluster.getId()) {
97+
if (_cluster.getId() != that._cluster.getId()) {
9598
return false;
9699
}
97-
if (this._host == null || that._host == null) {
100+
if (_host == null || that._host == null) {
98101
return false;
99102
}
100-
return this._host.getId() == that._host.getId();
103+
return _host.getId() == that._host.getId();
101104
}
102105

103106
@Override

‎api/src/com/cloud/deploy/DeploymentPlanner.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
// under the License.
1717
package com.cloud.deploy;
1818

19+
import java.io.Serializable;
1920
import java.util.Collection;
2021
import java.util.HashSet;
2122
import java.util.Set;
@@ -89,7 +90,9 @@ public enum PlannerResourceUsage {
8990
Shared, Dedicated;
9091
}
9192

92-
public static class ExcludeList {
93+
public static class ExcludeList implements Serializable {
94+
private static final long serialVersionUID = -482175549460148301L;
95+
9396
private Set<Long> _dcIds;
9497
private Set<Long> _podIds;
9598
private Set<Long> _clusterIds;

‎api/src/com/cloud/exception/CloudException.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
*/
2828

2929
public class CloudException extends Exception {
30+
private static final long serialVersionUID = 8784427323859682503L;
3031

3132
// This holds a list of uuids and their names. Add uuid:fieldname pairs
3233
protected ArrayList<String> idList = new ArrayList<String>();
@@ -58,10 +59,10 @@ public ArrayList<String> getIdProxyList() {
5859
}
5960

6061
public void setCSErrorCode(int cserrcode) {
61-
this.csErrorCode = cserrcode;
62+
csErrorCode = cserrcode;
6263
}
6364

6465
public int getCSErrorCode() {
65-
return this.csErrorCode;
66+
return csErrorCode;
6667
}
6768
}

‎api/src/com/cloud/exception/OperationTimedoutException.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,15 @@ public class OperationTimedoutException extends CloudException {
2828
long _agentId;
2929
long _seqId;
3030
int _time;
31-
Command[] _cmds;
31+
32+
// TODO
33+
// I did a reference search on usage of getCommands() and found none
34+
//
35+
// to prevent serialization problems across boundaries, I'm disabling serialization of _cmds here
36+
// getCommands() will still be available within the same serialization boundary, but it will be lost
37+
// when exception is propagated across job boundaries.
38+
//
39+
transient Command[] _cmds;
3240
boolean _isActive;
3341

3442
public OperationTimedoutException(Command[] cmds, long agentId, long seqId, int time, boolean isActive) {

‎engine/api/src/com/cloud/vm/VirtualMachineManager.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,9 @@ NicProfile addVmToNetwork(VirtualMachine vm, Network network, NicProfile request
190190
*/
191191
VirtualMachineTO toVmTO(VirtualMachineProfile profile);
192192

193-
VirtualMachine reConfigureVm(String vmUuid, ServiceOffering newServiceOffering, boolean sameHost) throws ResourceUnavailableException, ConcurrentOperationException;
193+
VirtualMachine reConfigureVm(String vmUuid, ServiceOffering newServiceOffering, boolean sameHost) throws ResourceUnavailableException, ConcurrentOperationException,
194+
InsufficientServerCapacityException;
195+
194196
void findHostAndMigrate(String vmUuid, Long newSvcOfferingId, DeploymentPlanner.ExcludeList excludeHostList) throws InsufficientCapacityException,
195197
ConcurrentOperationException, ResourceUnavailableException;
196198

‎engine/components-api/src/com/cloud/vm/VmWorkJobHandlerProxy.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,10 @@ public Pair<JobInfo.Status, String> handleVmWorkJob(VmWork work) throws Exceptio
116116

117117
// legacy CloudStack code relies on checked exception for error handling
118118
// we need to re-throw the real exception here
119-
if (e.getCause() != null && e.getCause() instanceof Exception)
119+
if (e.getCause() != null && e.getCause() instanceof Exception) {
120+
s_logger.info("Rethrow exception " + e.getCause());
120121
throw (Exception)e.getCause();
122+
}
121123

122124
throw e;
123125
}

‎engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java‎

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3670,7 +3670,7 @@ public boolean unplugNic(Network network, NicTO nic, VirtualMachineTO vm, Reserv
36703670
@Override
36713671
public VMInstanceVO reConfigureVm(String vmUuid, ServiceOffering oldServiceOffering,
36723672
boolean reconfiguringOnExistingHost)
3673-
throws ResourceUnavailableException, ConcurrentOperationException {
3673+
throws ResourceUnavailableException, InsufficientServerCapacityException, ConcurrentOperationException {
36743674

36753675
AsyncJobExecutionContext jobContext = AsyncJobExecutionContext.getCurrentExecutionContext();
36763676
if (!VmJobEnabled.value() || jobContext.isJobDispatchedBy(VmWorkConstants.VM_WORK_JOB_DISPATCHER)) {
@@ -3688,20 +3688,21 @@ public VMInstanceVO reConfigureVm(String vmUuid, ServiceOffering oldServiceOffer
36883688
throw new RuntimeException("Execution excetion", e);
36893689
}
36903690

3691-
AsyncJobVO jobVo = _entityMgr.findById(AsyncJobVO.class, outcome.getJob().getId());
3692-
if (jobVo.getResultCode() == JobInfo.Status.SUCCEEDED.ordinal()) {
3693-
return _entityMgr.findById(VMInstanceVO.class, vm.getId());
3694-
} else {
3695-
Object jobResult = _jobMgr.unmarshallResultObject(outcome.getJob());
3696-
if (jobResult != null) {
3697-
if (jobResult instanceof ResourceUnavailableException)
3698-
throw (ResourceUnavailableException)jobResult;
3699-
else if (jobResult instanceof ConcurrentOperationException)
3700-
throw (ConcurrentOperationException)jobResult;
3691+
Object jobResult = _jobMgr.unmarshallResultObject(outcome.getJob());
3692+
if (jobResult != null) {
3693+
if (jobResult instanceof ResourceUnavailableException)
3694+
throw (ResourceUnavailableException)jobResult;
3695+
else if (jobResult instanceof ConcurrentOperationException)
3696+
throw (ConcurrentOperationException)jobResult;
3697+
else if (jobResult instanceof InsufficientServerCapacityException)
3698+
throw (InsufficientServerCapacityException)jobResult;
3699+
else if (jobResult instanceof Throwable) {
3700+
s_logger.error("Unhandled exception", (Throwable)jobResult);
3701+
throw new RuntimeException("Unhandled exception", (Throwable)jobResult);
37013702
}
3702-
3703-
throw new RuntimeException("Failed with un-handled exception");
37043703
}
3704+
3705+
return (VMInstanceVO)vm;
37053706
}
37063707
}
37073708

@@ -4633,7 +4634,7 @@ public Object[] doInTransaction(TransactionStatus status) {
46334634
}
46344635

46354636
public Outcome<VirtualMachine> reconfigureVmThroughJobQueue(
4636-
final String vmUuid, final ServiceOffering oldServiceOffering, final boolean reconfiguringOnExistingHost) {
4637+
final String vmUuid, final ServiceOffering newServiceOffering, final boolean reconfiguringOnExistingHost) {
46374638

46384639
final CallContext context = CallContext.current();
46394640
final User user = context.getCallingUser();
@@ -4668,7 +4669,7 @@ public Object[] doInTransaction(TransactionStatus status) {
46684669

46694670
// save work context info (there are some duplications)
46704671
VmWorkReconfigure workInfo = new VmWorkReconfigure(user.getId(), account.getId(), vm.getId(),
4671-
VirtualMachineManagerImpl.VM_WORK_JOB_HANDLER, oldServiceOffering, reconfiguringOnExistingHost);
4672+
VirtualMachineManagerImpl.VM_WORK_JOB_HANDLER, newServiceOffering.getId(), reconfiguringOnExistingHost);
46724673
workJob.setCmdInfo(VmWorkSerializer.serialize(workInfo));
46734674

46744675
_jobMgr.submitAsyncJob(workJob, VmWorkConstants.VM_WORK_QUEUE, vm.getId());
@@ -4796,7 +4797,10 @@ private Pair<JobInfo.Status, String> orchestrateReconfigure(VmWorkReconfigure wo
47964797
s_logger.info("Unable to find vm " + work.getVmId());
47974798
}
47984799
assert (vm != null);
4799-
reConfigureVm(vm.getUuid(), work.getNewServiceOffering(),
4800+
4801+
ServiceOffering newServiceOffering = _offeringDao.findById(vm.getId(), work.getNewServiceOfferingId());
4802+
4803+
reConfigureVm(vm.getUuid(), newServiceOffering,
48004804
work.isSameHost());
48014805
return new Pair<JobInfo.Status, String>(JobInfo.Status.SUCCEEDED, null);
48024806
}

‎engine/orchestration/src/com/cloud/vm/VmWorkJobDispatcher.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
import org.apache.cloudstack.framework.jobs.AsyncJob;
2727
import org.apache.cloudstack.framework.jobs.AsyncJobDispatcher;
2828
import org.apache.cloudstack.framework.jobs.AsyncJobManager;
29-
import org.apache.cloudstack.framework.jobs.impl.JobSerializerHelper;
3029
import org.apache.cloudstack.jobs.JobInfo;
3130

3231
import com.cloud.utils.Pair;
@@ -105,9 +104,8 @@ public void runJob(AsyncJob job) {
105104
} catch(Throwable e) {
106105
s_logger.error("Unable to complete " + job + ", job origin:" + job.getRelated(), e);
107106

108-
String exceptionJson = JobSerializerHelper.toSerializedString(e);
109-
s_logger.info("Serialize exception object into json: " + exceptionJson + ", job origin: " + job.getRelated());
110-
_asyncJobMgr.completeAsyncJob(job.getId(), JobInfo.Status.FAILED, 0, exceptionJson);
107+
RuntimeException ex = new RuntimeException("Job failed due to exception " + e.getMessage());
108+
_asyncJobMgr.completeAsyncJob(job.getId(), JobInfo.Status.FAILED, 0, _asyncJobMgr.marshallResultObject(ex));
111109
} finally {
112110
CallContext.unregister();
113111
}

‎engine/orchestration/src/com/cloud/vm/VmWorkReconfigure.java‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,25 +16,24 @@
1616
// under the License.
1717
package com.cloud.vm;
1818

19-
import com.cloud.offering.ServiceOffering;
2019

2120
public class VmWorkReconfigure extends VmWork {
2221
private static final long serialVersionUID = -4517030323758086615L;
2322

24-
ServiceOffering newServiceOffering;
23+
Long newServiceOfferingId;
2524
boolean sameHost;
2625

2726
public VmWorkReconfigure(long userId, long accountId, long vmId, String handlerName,
28-
ServiceOffering newServiceOffering, boolean sameHost) {
27+
Long newServiceOfferingId, boolean sameHost) {
2928

3029
super(userId, accountId, vmId, handlerName);
3130

32-
this.newServiceOffering = newServiceOffering;
31+
this.newServiceOfferingId = newServiceOfferingId;
3332
this.sameHost = sameHost;
3433
}
3534

36-
public ServiceOffering getNewServiceOffering() {
37-
return newServiceOffering;
35+
public Long getNewServiceOfferingId() {
36+
return newServiceOfferingId;
3837
}
3938

4039
public boolean isSameHost() {

‎engine/schema/src/com/cloud/service/ServiceOfferingVO.java‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -97,8 +97,8 @@ public ServiceOfferingVO(String name, Integer cpu, Integer ramSize, Integer spee
9797
this.rateMbps = rateMbps;
9898
this.multicastRateMbps = multicastRateMbps;
9999
this.offerHA = offerHA;
100-
this.limitCpuUse = false;
101-
this.volatileVm = false;
100+
limitCpuUse = false;
101+
volatileVm = false;
102102
this.defaultUse = defaultUse;
103103
this.vmType = vmType == null ? null : vmType.toString().toLowerCase();
104104
}
@@ -174,16 +174,16 @@ public ServiceOfferingVO(ServiceOfferingVO offering) {
174174
offering.getSystemUse(),
175175
true,
176176
offering.getDomainId());
177-
this.cpu = offering.getCpu();
178-
this.ramSize = offering.getRamSize();
179-
this.speed = offering.getSpeed();
180-
this.rateMbps = offering.getRateMbps();
181-
this.multicastRateMbps = offering.getMulticastRateMbps();
182-
this.offerHA = offering.getOfferHA();
183-
this.limitCpuUse = offering.getLimitCpuUse();
184-
this.volatileVm = offering.getVolatileVm();
185-
this.hostTag = offering.getHostTag();
186-
this.vmType = offering.getSystemVmType();
177+
cpu = offering.getCpu();
178+
ramSize = offering.getRamSize();
179+
speed = offering.getSpeed();
180+
rateMbps = offering.getRateMbps();
181+
multicastRateMbps = offering.getMulticastRateMbps();
182+
offerHA = offering.getOfferHA();
183+
limitCpuUse = offering.getLimitCpuUse();
184+
volatileVm = offering.getVolatileVm();
185+
hostTag = offering.getHostTag();
186+
vmType = offering.getSystemVmType();
187187
}
188188

189189
@Override
@@ -325,6 +325,6 @@ public boolean isDynamic() {
325325
}
326326

327327
public void setDynamicFlag(boolean isdynamic) {
328-
this.isDynamic = isdynamic;
328+
isDynamic = isdynamic;
329329
}
330330
}

0 commit comments

Comments
 (0)