cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sheng Yang" <sh...@yasker.org>
Subject Re: Review Request 19038: CLOUDSTACK-6090: Virtual Router Service Failure Alerting
Date Tue, 18 Mar 2014 22:30:50 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19038/#review37641
-----------------------------------------------------------



core/src/com/cloud/agent/api/GetRouterAlertsAnswer.java
<https://reviews.apache.org/r/19038/#comment69265>

    What's use of routerName field here? Is it the same as NetworkElementCommand.ROUTER_NAME?



core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java
<https://reviews.apache.org/r/19038/#comment69270>

    Put it in query command rather than here. It would demand immediate answer with details
rather than a simple true/false answer. See GetDomRVersionCommand.



core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java
<https://reviews.apache.org/r/19038/#comment69268>

    This comment is wrong. result.isSuccess() would only indicate the execution of scripts
in the VR is success, regardless of it provides result or not. The result is still success
if it provides output as well. If result is not success, it would cause a error.
    
    Though your logic seems still honor the success? But what if command fail due to e.g.
communication fail? I didn't see you have a place for failed answer.



core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java
<https://reviews.apache.org/r/19038/#comment69269>

    You won't need generateConfig. This command should be categoried as querycommand since
it would demand immediately result. We cannot aggregate it. 



server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java
<https://reviews.apache.org/r/19038/#comment69284>

    I think you can use listRunningBy() and skip the routers which are not managed by this
mgmt server? Get both GuestType routers separately seems strange.



server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java
<https://reviews.apache.org/r/19038/#comment69285>

    you can just avoid this null by generate the object and putting a timestamp in it, e.g.
1970-01-01 00:00:00 UTC
    



systemvm/patches/debian/config/opt/cloud/bin/getRouterAlerts.sh
<https://reviews.apache.org/r/19038/#comment69277>

    You can use "tac" or alike tools to reverse the reading file, and stop when find old alerts.



systemvm/patches/debian/config/opt/cloud/bin/getRouterAlerts.sh
<https://reviews.apache.org/r/19038/#comment69279>

    Just compare the seconds since epoch. "date +%s"



systemvm/patches/debian/config/opt/cloud/bin/getRouterAlerts.sh
<https://reviews.apache.org/r/19038/#comment69282>

    It should be fine to always use alerts="${alerts}\n${line}" anyway... You don't need so
complex if statement here.


- Sheng Yang


On March 14, 2014, 9:36 a.m., Harikrishna Patnala wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19038/
> -----------------------------------------------------------
> 
> (Updated March 14, 2014, 9:36 a.m.)
> 
> 
> Review request for cloudstack, Chiradeep Vittal, Jayapal Reddy, and Murali Reddy.
> 
> 
> Bugs: CLOUDSTACK-6090
>     https://issues.apache.org/jira/browse/CLOUDSTACK-6090
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> CLOUDSTACK-6090: Virtual Router Service Failure Alerting
> Currently in CS we can monitor the running services on Virtual Router and ensure they
are running through the lifetime of VR. 
> Upon failure of any service in VR, monitoring service logs the alerts in VR logs. 
> With this feature those alerts will be sent to MS.
> 
> Added marvin tests
> 
> 
> Diffs
> -----
> 
>   core/src/com/cloud/agent/api/GetRouterAlertsAnswer.java PRE-CREATION 
>   core/src/com/cloud/agent/api/routing/GetRouterAlertsCommand.java PRE-CREATION 
>   core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java 3712aba

>   engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml
765d86c 
>   engine/schema/src/com/cloud/network/dao/OpRouterMonitorServiceDao.java PRE-CREATION

>   engine/schema/src/com/cloud/network/dao/OpRouterMonitorServiceDaoImpl.java PRE-CREATION

>   engine/schema/src/com/cloud/network/dao/OpRouterMonitorServiceVO.java PRE-CREATION

>   server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java eeab91d

>   setup/db/db/schema-430to440.sql 056c5f8 
>   systemvm/patches/debian/config/opt/cloud/bin/getRouterAlerts.sh PRE-CREATION 
>   systemvm/patches/debian/config/root/monitorServices.py 0319ece 
>   test/integration/smoke/test_VirtualRouter_alerts.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19038/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Harikrishna Patnala
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message