From 7b76cd2fb40d7b26bd786a4a0f8724d8c316a9d3 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 10 Nov 2015 12:53:35 +0100 Subject: [PATCH 1/4] CID-1338387: paranoia null checking --- .../hypervisor/ovm3/resources/Ovm3HypervisorGuru.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/plugins/hypervisors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/Ovm3HypervisorGuru.java b/plugins/hypervisors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/Ovm3HypervisorGuru.java index 3711bbf65f14..f455de5a1492 100755 --- a/plugins/hypervisors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/Ovm3HypervisorGuru.java +++ b/plugins/hypervisors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/Ovm3HypervisorGuru.java @@ -112,8 +112,12 @@ void performSideEffectsForDelegationOnCommand(long hostId, Command cmd) { if (srcStore instanceof NfsTO && destStore instanceof NfsTO) { HostVO host = hostDao.findById(hostId); EndPoint ep = endPointSelector.selectHypervisorHost(new ZoneScope(host.getDataCenterId())); - host = hostDao.findById(ep.getId()); - hostDao.loadDetails(host); + if (ep != null) { + host = hostDao.findById(ep.getId()); + if(host != null) { + hostDao.loadDetails(host); + } + } } } } From a67fc10461b5d0b7482c4cd18267cc3a6bc020d2 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 10 Nov 2015 17:52:50 +0100 Subject: [PATCH 2/4] CID-1338387 specific exception for specific condition --- .../api/storage/EndPointSelector.java | 39 +++++++++++++- .../endpoint/DefaultEndPointSelector.java | 3 +- .../ovm3/resources/Ovm3HypervisorGuru.java | 10 ++-- .../com/cloud/hypervisor/XenServerGuru.java | 16 +++--- .../CloudstackInternalException.java | 52 +++++++++++++++++++ 5 files changed, 107 insertions(+), 13 deletions(-) create mode 100644 utils/src/main/java/org/apache/cloudstack/CloudstackInternalException.java diff --git a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/EndPointSelector.java b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/EndPointSelector.java index 4d6465b80678..cdf420dc771b 100644 --- a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/EndPointSelector.java +++ b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/EndPointSelector.java @@ -20,6 +20,8 @@ import java.util.List; +import org.apache.cloudstack.CloudstackInternalException; + public interface EndPointSelector { EndPoint select(DataObject srcData, DataObject destData); @@ -35,7 +37,42 @@ public interface EndPointSelector { EndPoint select(Scope scope, Long storeId); - EndPoint selectHypervisorHost(Scope scope); + EndPoint selectHypervisorHost(Scope scope) throws NoSuchEndPointException; EndPoint select(DataStore store, String downloadUrl); + + public class NoSuchEndPointException extends CloudstackInternalException { + + /** + * + */ + private static final long serialVersionUID = 1L; + EndPoint _ep; + + public NoSuchEndPointException() { + // TODO Auto-generated constructor stub + } + + public NoSuchEndPointException(String message) { + super(message); + // TODO Auto-generated constructor stub + } + + public NoSuchEndPointException(Throwable cause) { + super(cause); + // TODO Auto-generated constructor stub + } + + public NoSuchEndPointException(String message, Throwable cause) { + super(message, cause); + // TODO Auto-generated constructor stub + } + + public NoSuchEndPointException(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) { + super(message, cause, enableSuppression, writableStackTrace); + // TODO Auto-generated constructor stub + } + + } + } diff --git a/engine/storage/src/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java b/engine/storage/src/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java index d38aaed80ed6..98c1ae84a3d5 100644 --- a/engine/storage/src/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java +++ b/engine/storage/src/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java @@ -382,7 +382,7 @@ public List selectAll(DataStore store) { } @Override - public EndPoint selectHypervisorHost(Scope scope) { + public EndPoint selectHypervisorHost(Scope scope) throws NoSuchEndPointException { StringBuilder sbuilder = new StringBuilder(); sbuilder.append(findOneHypervisorHostInScope); if (scope.getScopeType() == ScopeType.ZONE) { @@ -408,6 +408,7 @@ public EndPoint selectHypervisorHost(Scope scope) { } } catch (SQLException e) { s_logger.warn("can't find endpoint", e); + throw new NoSuchEndPointException(e); } if (host == null) { diff --git a/plugins/hypervisors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/Ovm3HypervisorGuru.java b/plugins/hypervisors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/Ovm3HypervisorGuru.java index f455de5a1492..afd7ae806396 100755 --- a/plugins/hypervisors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/Ovm3HypervisorGuru.java +++ b/plugins/hypervisors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/Ovm3HypervisorGuru.java @@ -111,12 +111,12 @@ void performSideEffectsForDelegationOnCommand(long hostId, Command cmd) { DataStoreTO destStore = destData.getDataStore(); if (srcStore instanceof NfsTO && destStore instanceof NfsTO) { HostVO host = hostDao.findById(hostId); - EndPoint ep = endPointSelector.selectHypervisorHost(new ZoneScope(host.getDataCenterId())); - if (ep != null) { + try { + EndPoint ep = endPointSelector.selectHypervisorHost(new ZoneScope(host.getDataCenterId())); host = hostDao.findById(ep.getId()); - if(host != null) { - hostDao.loadDetails(host); - } + hostDao.loadDetails(host); + } catch (EndPointSelector.NoSuchEndPointException e) { + LOGGER.error("no endpoint found for zone " + host.getDataCenterId()); } } } diff --git a/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/XenServerGuru.java b/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/XenServerGuru.java index 9567f313492f..a9d455062acc 100644 --- a/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/XenServerGuru.java +++ b/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/XenServerGuru.java @@ -190,12 +190,16 @@ public Pair getCommandHostDelegation(long hostId, Command cmd) { DataStoreTO destStore = destData.getDataStore(); if (srcStore instanceof NfsTO && destStore instanceof NfsTO) { HostVO host = hostDao.findById(hostId); - EndPoint ep = endPointSelector.selectHypervisorHost(new ZoneScope(host.getDataCenterId())); - host = hostDao.findById(ep.getId()); - hostDao.loadDetails(host); - String snapshotHotFixVersion = host.getDetail(XenserverConfigs.XS620HotFix); - if (snapshotHotFixVersion != null && snapshotHotFixVersion.equalsIgnoreCase(XenserverConfigs.XSHotFix62ESP1004)) { - return new Pair(Boolean.TRUE, new Long(ep.getId())); + try { + EndPoint ep = endPointSelector.selectHypervisorHost(new ZoneScope(host.getDataCenterId())); + host = hostDao.findById(ep.getId()); + hostDao.loadDetails(host); + String snapshotHotFixVersion = host.getDetail(XenserverConfigs.XS620HotFix); + if (snapshotHotFixVersion != null && snapshotHotFixVersion.equalsIgnoreCase(XenserverConfigs.XSHotFix62ESP1004)) { + return new Pair(Boolean.TRUE, new Long(ep.getId())); + } + } catch (EndPointSelector.NoSuchEndPointException e) { + LOGGER.error("no endpoint found in zone " + host.getDataCenterId()); } } } diff --git a/utils/src/main/java/org/apache/cloudstack/CloudstackInternalException.java b/utils/src/main/java/org/apache/cloudstack/CloudstackInternalException.java new file mode 100644 index 000000000000..5756be350291 --- /dev/null +++ b/utils/src/main/java/org/apache/cloudstack/CloudstackInternalException.java @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.cloudstack; + +public class CloudstackInternalException extends Exception { + + /** + * + */ + private static final long serialVersionUID = 1L; + + public CloudstackInternalException() { + // TODO Auto-generated constructor stub + } + + public CloudstackInternalException(String message) { + super(message); + // TODO Auto-generated constructor stub + } + + public CloudstackInternalException(Throwable cause) { + super(cause); + // TODO Auto-generated constructor stub + } + + public CloudstackInternalException(String message, Throwable cause) { + super(message, cause); + // TODO Auto-generated constructor stub + } + + public CloudstackInternalException(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) { + super(message, cause, enableSuppression, writableStackTrace); + // TODO Auto-generated constructor stub + } + +} From 78d35465d7b24b6c9f569dbe83529c3fed2e2071 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 10 Nov 2015 18:02:55 +0100 Subject: [PATCH 3/4] forgotten details throw on host is null no iteration for retrieving only one element --- .../cloudstack/storage/endpoint/DefaultEndPointSelector.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/engine/storage/src/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java b/engine/storage/src/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java index 98c1ae84a3d5..3e6c0fdb90a5 100644 --- a/engine/storage/src/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java +++ b/engine/storage/src/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java @@ -402,7 +402,7 @@ public EndPoint selectHypervisorHost(Scope scope) throws NoSuchEndPointException PreparedStatement pstmt = txn.prepareStatement(sql); ResultSet rs = pstmt.executeQuery(); ) { - while (rs.next()) { + if (rs.next()) { long id = rs.getLong(1); host = hostDao.findById(id); } @@ -412,7 +412,7 @@ public EndPoint selectHypervisorHost(Scope scope) throws NoSuchEndPointException } if (host == null) { - return null; + throw new NoSuchEndPointException("no host found in scope " + scope.getScopeId()); } return RemoteHostEndPoint.getHypervisorHostEndPoint(host); From b0bbba21d3bd1e1332dc72a1caeb4a4cd85d7f4b Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Wed, 11 Nov 2015 18:28:02 +0100 Subject: [PATCH 4/4] CID-1338387 removal of unnecessary code --- .../api/storage/EndPointSelector.java | 38 +------------- .../endpoint/DefaultEndPointSelector.java | 5 +- .../ovm3/resources/Ovm3HypervisorGuru.java | 29 ----------- .../com/cloud/hypervisor/XenServerGuru.java | 16 +++--- .../CloudstackInternalException.java | 52 ------------------- 5 files changed, 9 insertions(+), 131 deletions(-) delete mode 100644 utils/src/main/java/org/apache/cloudstack/CloudstackInternalException.java diff --git a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/EndPointSelector.java b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/EndPointSelector.java index cdf420dc771b..4903ed975957 100644 --- a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/EndPointSelector.java +++ b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/EndPointSelector.java @@ -20,8 +20,6 @@ import java.util.List; -import org.apache.cloudstack.CloudstackInternalException; - public interface EndPointSelector { EndPoint select(DataObject srcData, DataObject destData); @@ -37,42 +35,8 @@ public interface EndPointSelector { EndPoint select(Scope scope, Long storeId); - EndPoint selectHypervisorHost(Scope scope) throws NoSuchEndPointException; + EndPoint selectHypervisorHost(Scope scope); EndPoint select(DataStore store, String downloadUrl); - public class NoSuchEndPointException extends CloudstackInternalException { - - /** - * - */ - private static final long serialVersionUID = 1L; - EndPoint _ep; - - public NoSuchEndPointException() { - // TODO Auto-generated constructor stub - } - - public NoSuchEndPointException(String message) { - super(message); - // TODO Auto-generated constructor stub - } - - public NoSuchEndPointException(Throwable cause) { - super(cause); - // TODO Auto-generated constructor stub - } - - public NoSuchEndPointException(String message, Throwable cause) { - super(message, cause); - // TODO Auto-generated constructor stub - } - - public NoSuchEndPointException(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) { - super(message, cause, enableSuppression, writableStackTrace); - // TODO Auto-generated constructor stub - } - - } - } diff --git a/engine/storage/src/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java b/engine/storage/src/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java index 3e6c0fdb90a5..b198133c21d2 100644 --- a/engine/storage/src/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java +++ b/engine/storage/src/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java @@ -382,7 +382,7 @@ public List selectAll(DataStore store) { } @Override - public EndPoint selectHypervisorHost(Scope scope) throws NoSuchEndPointException { + public EndPoint selectHypervisorHost(Scope scope) { StringBuilder sbuilder = new StringBuilder(); sbuilder.append(findOneHypervisorHostInScope); if (scope.getScopeType() == ScopeType.ZONE) { @@ -408,11 +408,10 @@ public EndPoint selectHypervisorHost(Scope scope) throws NoSuchEndPointException } } catch (SQLException e) { s_logger.warn("can't find endpoint", e); - throw new NoSuchEndPointException(e); } if (host == null) { - throw new NoSuchEndPointException("no host found in scope " + scope.getScopeId()); + return null; } return RemoteHostEndPoint.getHypervisorHostEndPoint(host); diff --git a/plugins/hypervisors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/Ovm3HypervisorGuru.java b/plugins/hypervisors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/Ovm3HypervisorGuru.java index afd7ae806396..f35024c527c3 100755 --- a/plugins/hypervisors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/Ovm3HypervisorGuru.java +++ b/plugins/hypervisors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/Ovm3HypervisorGuru.java @@ -20,20 +20,12 @@ import javax.ejb.Local; import javax.inject.Inject; -import org.apache.cloudstack.engine.subsystem.api.storage.EndPoint; import org.apache.cloudstack.engine.subsystem.api.storage.EndPointSelector; -import org.apache.cloudstack.engine.subsystem.api.storage.ZoneScope; -import org.apache.cloudstack.storage.command.CopyCommand; import org.apache.cloudstack.storage.command.StorageSubSystemCommand; import org.apache.log4j.Logger; import com.cloud.agent.api.Command; -import com.cloud.agent.api.to.DataObjectType; -import com.cloud.agent.api.to.DataStoreTO; -import com.cloud.agent.api.to.DataTO; -import com.cloud.agent.api.to.NfsTO; import com.cloud.agent.api.to.VirtualMachineTO; -import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; import com.cloud.hypervisor.Hypervisor.HypervisorType; import com.cloud.hypervisor.HypervisorGuru; @@ -100,26 +92,5 @@ void performSideEffectsForDelegationOnCommand(long hostId, Command cmd) { StorageSubSystemCommand c = (StorageSubSystemCommand)cmd; c.setExecuteInSequence(true); } - if (cmd instanceof CopyCommand) { - CopyCommand cpyCommand = (CopyCommand)cmd; - DataTO srcData = cpyCommand.getSrcTO(); - DataTO destData = cpyCommand.getDestTO(); - - if (srcData.getObjectType() == DataObjectType.SNAPSHOT && destData.getObjectType() == DataObjectType.TEMPLATE) { - LOGGER.debug("Snapshot to Template: " + cmd); - DataStoreTO srcStore = srcData.getDataStore(); - DataStoreTO destStore = destData.getDataStore(); - if (srcStore instanceof NfsTO && destStore instanceof NfsTO) { - HostVO host = hostDao.findById(hostId); - try { - EndPoint ep = endPointSelector.selectHypervisorHost(new ZoneScope(host.getDataCenterId())); - host = hostDao.findById(ep.getId()); - hostDao.loadDetails(host); - } catch (EndPointSelector.NoSuchEndPointException e) { - LOGGER.error("no endpoint found for zone " + host.getDataCenterId()); - } - } - } - } } } diff --git a/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/XenServerGuru.java b/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/XenServerGuru.java index a9d455062acc..9567f313492f 100644 --- a/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/XenServerGuru.java +++ b/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/XenServerGuru.java @@ -190,16 +190,12 @@ public Pair getCommandHostDelegation(long hostId, Command cmd) { DataStoreTO destStore = destData.getDataStore(); if (srcStore instanceof NfsTO && destStore instanceof NfsTO) { HostVO host = hostDao.findById(hostId); - try { - EndPoint ep = endPointSelector.selectHypervisorHost(new ZoneScope(host.getDataCenterId())); - host = hostDao.findById(ep.getId()); - hostDao.loadDetails(host); - String snapshotHotFixVersion = host.getDetail(XenserverConfigs.XS620HotFix); - if (snapshotHotFixVersion != null && snapshotHotFixVersion.equalsIgnoreCase(XenserverConfigs.XSHotFix62ESP1004)) { - return new Pair(Boolean.TRUE, new Long(ep.getId())); - } - } catch (EndPointSelector.NoSuchEndPointException e) { - LOGGER.error("no endpoint found in zone " + host.getDataCenterId()); + EndPoint ep = endPointSelector.selectHypervisorHost(new ZoneScope(host.getDataCenterId())); + host = hostDao.findById(ep.getId()); + hostDao.loadDetails(host); + String snapshotHotFixVersion = host.getDetail(XenserverConfigs.XS620HotFix); + if (snapshotHotFixVersion != null && snapshotHotFixVersion.equalsIgnoreCase(XenserverConfigs.XSHotFix62ESP1004)) { + return new Pair(Boolean.TRUE, new Long(ep.getId())); } } } diff --git a/utils/src/main/java/org/apache/cloudstack/CloudstackInternalException.java b/utils/src/main/java/org/apache/cloudstack/CloudstackInternalException.java deleted file mode 100644 index 5756be350291..000000000000 --- a/utils/src/main/java/org/apache/cloudstack/CloudstackInternalException.java +++ /dev/null @@ -1,52 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.cloudstack; - -public class CloudstackInternalException extends Exception { - - /** - * - */ - private static final long serialVersionUID = 1L; - - public CloudstackInternalException() { - // TODO Auto-generated constructor stub - } - - public CloudstackInternalException(String message) { - super(message); - // TODO Auto-generated constructor stub - } - - public CloudstackInternalException(Throwable cause) { - super(cause); - // TODO Auto-generated constructor stub - } - - public CloudstackInternalException(String message, Throwable cause) { - super(message, cause); - // TODO Auto-generated constructor stub - } - - public CloudstackInternalException(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) { - super(message, cause, enableSuppression, writableStackTrace); - // TODO Auto-generated constructor stub - } - -}