Mailing-List: contact dev-help@cloudstack.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cloudstack.apache.org
From: GabrielBrascher <git@git.apache.org>
To: dev@cloudstack.apache.org
Reply-To: dev@cloudstack.apache.org
References: <git-pr-1278-cloudstack@git.apache.org>
In-Reply-To: <git-pr-1278-cloudstack@git.apache.org>
Subject: [GitHub] cloudstack issue #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...
Content-Type: text/plain
Message-Id: <20170303215413.D3588DFCA1@git1-us-west.apache.org>
Date: Fri,  3 Mar 2017 21:54:13 +0000 (UTC)
archived-at: Fri, 03 Mar 2017 21:54:16 -0000

Github user GabrielBrascher commented on the issue:

    https://github.com/apache/cloudstack/pull/1278
  
    @anshul1886 I would like to raise the point previously discussed by me and @rafaelweingartner. I think that we should pay attention if the change of user and caller will really do the job. So far I do not see how this PR changes the behavior.
    
    Basically this code changes two parameters in startVirtualRouter [_callerUser_ and _caller_ when calling startVirtualRouter(router, callerUser, caller, routerDeploymentDefinition.getParams())]. However, those parameters are only used inside startVirtualRouter when calling the method start(router, user, caller, params, null).
    
    ```
    if (router.getRole() != Role.VIRTUAL_ROUTER || !router.getIsRedundantRouter()) {
                return start(router, user, caller, params, null);
    }
    ```
    
    The problem is that the method **start** does not use either the _user_ and the _caller_ parameters in the overridden implementation (the one that you are using).
    
    ```
        protected DomainRouterVO start(DomainRouterVO router, final User user, final Account caller, final Map<Param, Object> params, final DeploymentPlan planToDeploy)
                throws StorageUnavailableException, InsufficientCapacityException, ConcurrentOperationException, ResourceUnavailableException {
            s_logger.debug("Starting router " + router);
            try {
                _itMgr.advanceStart(router.getUuid(), params, planToDeploy, null);
            } catch (final OperationTimedoutException e) {
                throw new ResourceUnavailableException("Starting router " + router + " failed! " + e.toString(), DataCenter.class, router.getDataCenterId());
            }
            if (router.isStopPending()) {
                s_logger.info("Clear the stop pending flag of router " + router.getHostName() + " after start router successfully!");
                router.setStopPending(false);
                router = _routerDao.persist(router);
            }
            // We don't want the failure of VPN Connection affect the status of
            // router, so we try to make connection
            // only after router start successfully
            final Long vpcId = router.getVpcId();
            if (vpcId != null) {
                _s2sVpnMgr.reconnectDisconnectedVpnByVpc(vpcId);
            }
            return _routerDao.findById(router.getId());
    }
    ```
    
    Sorry, but I can't see how your code alters the behavior as intended. Can you please show that by changing the parameters _user_ and _caller_ you are changing the behavior?
    
    Thanks in advance.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---