From 41a16a478ab67fc170403026d2eaff3bb47b761b Mon Sep 17 00:00:00 2001 From: Kris McQueen Date: Fri, 1 Oct 2010 14:11:18 -0700 Subject: [PATCH] Fixes required for regressions found running automated tests. (1) method signatures that weren't properly refactored to new API framework (2) API request parameter types should always be specified lest they default to String which may or may not be desired (3) better exception handling with regard to Queued commands and generic exceptions (4) fix eventing to pass the proper accountId where neccessary --- server/src/com/cloud/api/ApiDispatcher.java | 12 +++--- server/src/com/cloud/api/ApiServer.java | 12 +++++- .../src/com/cloud/api/BaseAsyncCreateCmd.java | 2 +- .../CreatePortForwardingServiceRuleCmd.java | 2 +- .../com/cloud/network/NetworkManagerImpl.java | 14 +++++-- .../com/cloud/server/ManagementServer.java | 4 +- .../cloud/server/ManagementServerImpl.java | 39 +++++++++---------- 7 files changed, 48 insertions(+), 37 deletions(-) diff --git a/server/src/com/cloud/api/ApiDispatcher.java b/server/src/com/cloud/api/ApiDispatcher.java index 2bd15816db2..e437ed39201 100644 --- a/server/src/com/cloud/api/ApiDispatcher.java +++ b/server/src/com/cloud/api/ApiDispatcher.java @@ -31,6 +31,7 @@ import org.apache.log4j.Logger; import com.cloud.agent.manager.AgentManager; import com.cloud.api.BaseCmd.CommandType; +import com.cloud.async.AsyncCommandQueued; import com.cloud.configuration.ConfigurationManager; import com.cloud.consoleproxy.ConsoleProxyManager; import com.cloud.exception.InvalidParameterValueException; @@ -156,9 +157,6 @@ public class ApiDispatcher { } catch (IllegalArgumentException iArgEx) { s_logger.warn("Exception executing method " + methodName + " for command " + cmd.getClass().getSimpleName(), iArgEx); throw new ServerApiException(BaseCmd.INTERNAL_ERROR, "Unable to execute method " + methodName + " for command " + cmd.getClass().getSimpleName() + ", internal error in the implementation."); - } catch (Exception ex) { - s_logger.error("Unhandled exception invoking method " + methodName + " for command " + cmd.getClass().getSimpleName(), ex); - throw new ServerApiException(BaseCmd.INTERNAL_ERROR, "Unable to execute method " + methodName + " for command " + cmd.getClass().getSimpleName() + ", internal error in the implementation."); } } @@ -213,8 +211,11 @@ public class ApiDispatcher { s_logger.warn("Exception executing method " + methodName + " for command " + cmd.getClass().getSimpleName(), nsme); throw new ServerApiException(BaseCmd.INTERNAL_ERROR, "Unable to execute method " + methodName + " for command " + cmd.getClass().getSimpleName() + ", unable to find implementation."); } catch (InvocationTargetException ite) { - s_logger.warn("Exception executing method " + methodName + " for command " + cmd.getClass().getSimpleName(), ite); Throwable cause = ite.getCause(); + if (cause instanceof AsyncCommandQueued) { + throw (AsyncCommandQueued)cause; + } + s_logger.warn("Exception executing method " + methodName + " for command " + cmd.getClass().getSimpleName(), ite); if (cause instanceof InvalidParameterValueException) { throw new ServerApiException(BaseCmd.PARAM_ERROR, cause.getMessage()); } else if (cause instanceof PermissionDeniedException) { @@ -227,9 +228,6 @@ public class ApiDispatcher { } catch (IllegalArgumentException iArgEx) { s_logger.warn("Exception executing method " + methodName + " for command " + cmd.getClass().getSimpleName(), iArgEx); throw new ServerApiException(BaseCmd.INTERNAL_ERROR, "Unable to execute method " + methodName + " for command " + cmd.getClass().getSimpleName() + ", internal error in the implementation."); - } catch (Exception ex) { - s_logger.error("Unhandled exception invoking method " + methodName + " for command " + cmd.getClass().getSimpleName(), ex); - throw new ServerApiException(BaseCmd.INTERNAL_ERROR, "Unable to execute method " + methodName + " for command " + cmd.getClass().getSimpleName() + ", internal error in the implementation."); } } diff --git a/server/src/com/cloud/api/ApiServer.java b/server/src/com/cloud/api/ApiServer.java index c8041776897..9d055d6d5fc 100644 --- a/server/src/com/cloud/api/ApiServer.java +++ b/server/src/com/cloud/api/ApiServer.java @@ -350,6 +350,7 @@ public class ApiServer implements HttpRequestHandler { BaseAsyncCreateCmd createCmd = (BaseAsyncCreateCmd)cmdObj; objectId = _dispatcher.dispatchCreateCmd(createCmd, params); createCmd.setId(objectId); + params.put("id", objectId.toString()); } BaseAsyncCmd asyncCmd = (BaseAsyncCmd)cmdObj; @@ -366,8 +367,15 @@ public class ApiServer implements HttpRequestHandler { } AsyncJobVO job = new AsyncJobVO(); - job.setUserId(UserContext.current().getUserId()); - job.setAccountId(ctx.getAccountId()); + job.setUserId(userId); + if (account != null) { + job.setAccountId(ctx.getAccountId()); + } else { + // Just have SYSTEM own the job for now. Users won't be able to see this job, + // but in an admin case (like domain admin) they won't be able to see it anyway + // so no loss of service. + job.setAccountId(1L); + } job.setCmd(cmdObj.getClass().getName()); job.setCmdInfo(gson.toJson(params)); long jobId = _asyncMgr.submitAsyncJob(job); diff --git a/server/src/com/cloud/api/BaseAsyncCreateCmd.java b/server/src/com/cloud/api/BaseAsyncCreateCmd.java index 783e77022a3..02b7903f127 100644 --- a/server/src/com/cloud/api/BaseAsyncCreateCmd.java +++ b/server/src/com/cloud/api/BaseAsyncCreateCmd.java @@ -4,7 +4,7 @@ import com.cloud.api.response.ApiResponseSerializer; import com.cloud.api.response.CreateCmdResponse; public abstract class BaseAsyncCreateCmd extends BaseAsyncCmd { - @Parameter(name="id") + @Parameter(name="id", type=CommandType.LONG) private Long id; public Long getId() { diff --git a/server/src/com/cloud/api/commands/CreatePortForwardingServiceRuleCmd.java b/server/src/com/cloud/api/commands/CreatePortForwardingServiceRuleCmd.java index 5f08767945a..3561f86f17d 100644 --- a/server/src/com/cloud/api/commands/CreatePortForwardingServiceRuleCmd.java +++ b/server/src/com/cloud/api/commands/CreatePortForwardingServiceRuleCmd.java @@ -43,7 +43,7 @@ public class CreatePortForwardingServiceRuleCmd extends BaseAsyncCreateCmd { @Parameter(name="privateport", type=CommandType.STRING, required=true) private String privatePort; - @Parameter(name="protocol", type=CommandType.STRING, required=true) + @Parameter(name="protocol", type=CommandType.STRING) private String protocol; @Parameter(name="publicport", type=CommandType.STRING, required=true) diff --git a/server/src/com/cloud/network/NetworkManagerImpl.java b/server/src/com/cloud/network/NetworkManagerImpl.java index bbb952ad2ef..d66719b78de 100644 --- a/server/src/com/cloud/network/NetworkManagerImpl.java +++ b/server/src/com/cloud/network/NetworkManagerImpl.java @@ -1503,7 +1503,7 @@ public class NetworkManagerImpl implements NetworkManager, VirtualMachineManager } - @Override + @Override @DB public IPAddressVO associateIP(AssociateIPAddrCmd cmd) throws ResourceAllocationException, InsufficientAddressCapacityException, InvalidParameterValueException, InternalErrorException, PermissionDeniedException { String accountName = cmd.getAccountName(); Long domainId = cmd.getDomainId(); @@ -1962,7 +1962,7 @@ public class NetworkManagerImpl implements NetworkManager, VirtualMachineManager return _rulesDao.listIPForwarding(cmd.getIpAddress(), true); } - @Override + @Override @DB public void assignToLoadBalancer(AssignToLoadBalancerRuleCmd cmd) throws NetworkRuleConflictException, InternalErrorException, PermissionDeniedException, InvalidParameterValueException { Long loadBalancerId = cmd.getLoadBalancerId(); @@ -2195,7 +2195,7 @@ public class NetworkManagerImpl implements NetworkManager, VirtualMachineManager } } - @Override + @Override @DB public LoadBalancerVO createLoadBalancerRule(CreateLoadBalancerRuleCmd cmd) throws InvalidParameterValueException, PermissionDeniedException { String publicIp = cmd.getPublicIp(); @@ -3304,6 +3304,12 @@ public class NetworkManagerImpl implements NetworkManager, VirtualMachineManager throw new ServerApiException(BaseCmd.PARAM_ERROR, "No virtual machine id specified."); } + // if a single instanceId was given, add it to the list so we can always just process the list if instanceIds + if (instanceIds == null) { + instanceIds = new ArrayList(); + instanceIds.add(vmInstanceId); + } + if (userId == null) { userId = Long.valueOf(1); } @@ -3381,7 +3387,7 @@ public class NetworkManagerImpl implements NetworkManager, VirtualMachineManager description = "deleted load balancer rule [" + updatedRule.getPublicIpAddress() + ":" + updatedRule.getPublicPort() + "]->[" + updatedRule.getPrivateIpAddress() + ":" + updatedRule.getPrivatePort() + "]" + " " + updatedRule.getProtocol(); - EventUtils.saveEvent(userId, account.getId(), level, type, description); + EventUtils.saveEvent(userId, loadBalancer.getAccountId(), level, type, description); } } txn.commit(); diff --git a/server/src/com/cloud/server/ManagementServer.java b/server/src/com/cloud/server/ManagementServer.java index fbd29cf82c9..07e0d934dd9 100644 --- a/server/src/com/cloud/server/ManagementServer.java +++ b/server/src/com/cloud/server/ManagementServer.java @@ -773,10 +773,10 @@ public interface ManagementServer { /** * Apply a port forwarding service rule to all VMs that have the port forwarding service applied - * @param ruleId the id of the created rule to apply + * @param cmd the command object that wraps the id of the created rule to apply * @return the updated rule if successful, null otherwise */ - NetworkRuleConfigVO applyPortForwardingServiceRule(Long ruleId) throws NetworkRuleConflictException; + NetworkRuleConfigVO applyPortForwardingServiceRule(CreatePortForwardingServiceRuleCmd cmd) throws NetworkRuleConflictException; ConsoleProxyInfo getConsoleProxy(long dataCenterId, long userVmId); ConsoleProxyVO startConsoleProxy(long instanceId, long startEventId) throws InternalErrorException; diff --git a/server/src/com/cloud/server/ManagementServerImpl.java b/server/src/com/cloud/server/ManagementServerImpl.java index b947545a488..7731aac667f 100755 --- a/server/src/com/cloud/server/ManagementServerImpl.java +++ b/server/src/com/cloud/server/ManagementServerImpl.java @@ -4549,7 +4549,8 @@ public class ManagementServerImpl implements ManagementServer { } @Override - public NetworkRuleConfigVO applyPortForwardingServiceRule(Long ruleId) throws NetworkRuleConflictException { + public NetworkRuleConfigVO applyPortForwardingServiceRule(CreatePortForwardingServiceRuleCmd cmd) throws NetworkRuleConflictException { + Long ruleId = cmd.getId(); NetworkRuleConfigVO netRule = null; if (ruleId != null) { Long userId = UserContext.current().getUserId(); @@ -5476,30 +5477,28 @@ public class ManagementServerImpl implements ManagementServer { Long accountId = null; String portForwardingServiceName = cmd.getPortForwardingServiceName(); - if (account != null) { - if (isAdmin(account.getType())) { - if ((accountName != null) && (domainId != null)) { - if (!_domainDao.isChildDomain(account.getDomainId(), domainId)) { - throw new ServerApiException(BaseCmd.ACCOUNT_ERROR, "Unable to create port forwarding service in domain " + domainId + ", permission denied."); - } + if ((account == null) || isAdmin(account.getType())) { + if ((accountName != null) && (domainId != null)) { + if ((account != null) && !_domainDao.isChildDomain(account.getDomainId(), domainId)) { + throw new ServerApiException(BaseCmd.ACCOUNT_ERROR, "Unable to create port forwarding service in domain " + domainId + ", permission denied."); + } - Account userAccount = findActiveAccount(accountName, domainId); - if (userAccount != null) { - accountId = userAccount.getId(); - } else { - throw new InvalidParameterValueException("Unable to create port forwarding service " + portForwardingServiceName + ", could not find account " + accountName + " in domain " + domainId); - } + Account userAccount = findActiveAccount(accountName, domainId); + if (userAccount != null) { + accountId = userAccount.getId(); } else { - // the admin must be creating the security group - if (account != null) { - accountId = account.getId(); - domainId = account.getDomainId(); - } + throw new InvalidParameterValueException("Unable to create port forwarding service " + portForwardingServiceName + ", could not find account " + accountName + " in domain " + domainId); } } else { - accountId = account.getId(); - domainId = account.getDomainId(); + // the admin must be creating the security group + if (account != null) { + accountId = account.getId(); + domainId = account.getDomainId(); + } } + } else { + accountId = account.getId(); + domainId = account.getDomainId(); } if (accountId == null) {