cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vijay Venkatachalam" <vijay.venkatacha...@citrix.com>
Subject Re: Review Request: AWS Style HealthCheck feature BugID : 664
Date Wed, 06 Mar 2013 12:40:09 GMT

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



plugins/network-elements/netscaler/src/com/cloud/network/resource/NetscalerResource.java
<https://reviews.apache.org/r/9165/#comment37046>

    Check for the the HealthCheck Policy TO object's revoke flag instead of passing null for
health check policy. Because it might mean 2 things, either policy is not present or have
to be removed.



plugins/network-elements/netscaler/src/com/cloud/network/resource/NetscalerResource.java
<https://reviews.apache.org/r/9165/#comment37045>

    This will cause failure for lb vservers which does not have monitors.



plugins/network-elements/netscaler/src/com/cloud/network/resource/NetscalerResource.java
<https://reviews.apache.org/r/9165/#comment37047>

    You could use getMonitorIfExisits() != null



plugins/network-elements/netscaler/src/com/cloud/network/resource/NetscalerResource.java
<https://reviews.apache.org/r/9165/#comment37048>

    No need to check for this exception!



plugins/network-elements/netscaler/src/com/cloud/network/resource/NetscalerResource.java
<https://reviews.apache.org/r/9165/#comment37049>

    No need to check for this exception



plugins/network-elements/netscaler/src/com/cloud/network/resource/NetscalerResource.java
<https://reviews.apache.org/r/9165/#comment37050>

    Checking state for each service, is expensive. Please use lbvserver_service_binding resource
for querying all the service bindings of a lbvserver and then check for curState property
on each binding. Please NOTE: the NetScaler doc wrongly claims it as LB state



plugins/network-elements/netscaler/src/com/cloud/network/resource/NetscalerResource.java
<https://reviews.apache.org/r/9165/#comment37051>

    This exception not required to be checked



plugins/network-elements/netscaler/src/com/cloud/network/resource/NetscalerResource.java
<https://reviews.apache.org/r/9165/#comment37052>

    You might not be required to do the delete twice. Just check by setting resp_code to null
and it should work



plugins/network-elements/netscaler/src/com/cloud/network/resource/NetscalerResource.java
<https://reviews.apache.org/r/9165/#comment37053>

    exception check not required



plugins/network-elements/netscaler/src/com/cloud/network/resource/NetscalerResource.java
<https://reviews.apache.org/r/9165/#comment37029>

    Looks like the monitor name conflicts with AutoScale Monitor! Check generateAutoScaleVmGroupIdentifier()



server/src/com/cloud/network/ExternalLoadBalancerDeviceManagerImpl.java
<https://reviews.apache.org/r/9165/#comment37026>

    I guess it has to be !isAutoScaleConfig(). Or it has to be ignored inside NetScaler Resource!



server/src/com/cloud/network/dao/LBHealthCheckPolicyDaoImpl.java
<https://reviews.apache.org/r/9165/#comment37062>

    You might want to remove this if not used!



server/src/com/cloud/network/dao/LBHealthCheckPolicyDaoImpl.java
<https://reviews.apache.org/r/9165/#comment37054>

    Looks like an unused public function



server/src/com/cloud/network/lb/LBHealthCheckManagerImpl.java
<https://reviews.apache.org/r/9165/#comment37056>

    Might not be required to set it to 600.



server/src/com/cloud/network/lb/LBHealthCheckManagerImpl.java
<https://reviews.apache.org/r/9165/#comment37057>

    why DB?



server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java
<https://reviews.apache.org/r/9165/#comment37058>

    Is there a genuine requirement to suppress rawtypes?



server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java
<https://reviews.apache.org/r/9165/#comment37037>

    lbRule object created here does not seemed to be used?



server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java
<https://reviews.apache.org/r/9165/#comment37039>

    no DB annotation?



server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java
<https://reviews.apache.org/r/9165/#comment37040>

    Typo: Only health check is removed!



server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java
<https://reviews.apache.org/r/9165/#comment37059>

    Please give a code comment that this function is primary function which collects the status
of all service VMs for all LBs



server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java
<https://reviews.apache.org/r/9165/#comment37061>

    Please avoid checking NetScaler provider here. I understand you want to do some optimization
but you could better structure it. You can use the Capability infrastructure already available.
    
    Basically, you need to introduce a new Lb Capability. It will be in Network.java =>
Capability.HealthCheck.
    
    Here it will be 
    nwElmnt = _networkMgr.getElementForServiceInNetwork(network, Service.Lb)
    if(nwElmnt.getCapabalities().get(Service.Lb).get(Capability.Health).equals(true)) {
        // Here is the code for requesting for HealthCheck Updates
    }



server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java
<https://reviews.apache.org/r/9165/#comment37060>

    No need to package stickiness policies



server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java
<https://reviews.apache.org/r/9165/#comment37063>

    You might want to have have check access here and remove it in Cmd layer.



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

    Please check for the capability during creation of LBHealthCheckPolicy.



setup/db/db/schema-410to420.sql
<https://reviews.apache.org/r/9165/#comment37065>

    introduce INDEX for load_balancer_id


- Vijay Venkatachalam


On March 5, 2013, 6:56 p.m., Rajesh Battala wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9165/
> -----------------------------------------------------------
> 
> (Updated March 5, 2013, 6:56 p.m.)
> 
> 
> Review request for cloudstack and Vijay Venkatachalam.
> 
> 
> Description
> -------
> 
> Code Review for AWS Style Health Check Feature :
> Added API commands :
> 1. createLBHealthCheck
> 2. deleteLBHealthCheck
> 3. listLBHealthCheck
> 
> load_balancer_healthcheck_policies table will hold the data for healthcheck polices.

> 
> commands will take the lbrule id as mandatory param and execute the action.
> 1. createLBHealthCheck :
> ========================
>    LB ruleid is the mandatory param to the api.  Remaining params (pingpath, responstime,
request Healthcheck_interval, Healthy_thresshold, Unhealth_thresshold) have default values.
if not specified in the command.
>   It will create monitor (tcp/http) depending upon the LB protocol.
>   after creating the LB Monitor, it will bind the monitor to all the services present
in the LB rule.
>   NetScaler will take care of monitoring according to the monitor params.
>   Monitor name will be (Cloud-Mon-<LB IP>-<port>)
> 
>  Initially only one monitor is supported for an LB rule.
>  if createLBHealthCheck returns an error, it will cleanup the entry created in db.
>   
> 2. deleteLBHealthCheck:
> ========================
>    LB ruleid is the mandatory param to the api.
>    the command will first unbind all the services attached to it and then the monitor
will be deleted.
>    DB entry in load_balancer_healthcheck_policies will be deleted.
> 
> 3. listLBHealthChecks:
> ======================
>     LB ruleid is the mandatory param to the api.
> 	this command will list LB HealthChecks created on the LB rule.
> 
> LBHealthCheckManager:
> =====================
> A new field is introduce in the table load_balancer_vm_map (state of string type)
> 
> The task of the LBHealthCheckManager is at every period of time, it will fetch the service
status of LB rules and update them in the load_balancer_vm_map.
> The time interval fo the LBHealthManager can be configured from Global Settings(healthcheck.update.interval).
default value is 600 sec.  
> possible values UP, DOWN, UNKNOWN, BUSY, OUT OF SERVICE, GOING OUT OF SERVICE, DOWN WHEN
GOING OUT OF SERVICE
> 
> 
> This addresses bug https://issues.apache.org/jira/browse/CLOUDSTACK-664.
> 
> 
> Diffs
> -----
> 
>   api/src/com/cloud/agent/api/routing/HealthCheckLBConfigAnswer.java PRE-CREATION 
>   api/src/com/cloud/agent/api/routing/HealthCheckLBConfigCommand.java PRE-CREATION 
>   api/src/com/cloud/agent/api/to/LoadBalancerTO.java 2d166ea 
>   api/src/com/cloud/event/EventTypes.java 0087edc 
>   api/src/com/cloud/network/element/LoadBalancingServiceProvider.java 879ea0e 
>   api/src/com/cloud/network/lb/LoadBalancingRule.java fb1d988 
>   api/src/com/cloud/network/lb/LoadBalancingRulesService.java 3743aae 
>   api/src/com/cloud/network/rules/HealthCheckPolicy.java PRE-CREATION 
>   api/src/org/apache/cloudstack/api/ApiConstants.java 1b544fd 
>   api/src/org/apache/cloudstack/api/ResponseGenerator.java a602514 
>   api/src/org/apache/cloudstack/api/command/user/loadbalancer/CreateLBHealthCheckPolicyCmd.java
PRE-CREATION 
>   api/src/org/apache/cloudstack/api/command/user/loadbalancer/DeleteLBHealthCheckPolicyCmd.java
PRE-CREATION 
>   api/src/org/apache/cloudstack/api/command/user/loadbalancer/ListLBHealthCheckPoliciesCmd.java
PRE-CREATION 
>   api/src/org/apache/cloudstack/api/response/LBHealthCheckPolicyResponse.java PRE-CREATION

>   api/src/org/apache/cloudstack/api/response/LBHealthCheckResponse.java PRE-CREATION

>   client/tomcatconf/commands.properties.in dd0c3f8 
>   client/tomcatconf/components.xml.in 1d3faf3 
>   plugins/network-elements/elastic-loadbalancer/src/com/cloud/network/element/ElasticLoadBalancerElement.java
abb36c3 
>   plugins/network-elements/elastic-loadbalancer/src/com/cloud/network/lb/ElasticLoadBalancerManagerImpl.java
81039d1 
>   plugins/network-elements/f5/src/com/cloud/network/element/F5ExternalLoadBalancerElement.java
94c098e 
>   plugins/network-elements/netscaler/src/com/cloud/network/element/NetscalerElement.java
8f902df 
>   plugins/network-elements/netscaler/src/com/cloud/network/resource/NetscalerResource.java
abea464 
>   server/src/com/cloud/api/ApiResponseHelper.java fbfc955 
>   server/src/com/cloud/configuration/Config.java 418f97d 
>   server/src/com/cloud/network/ExternalLoadBalancerDeviceManager.java d979f07 
>   server/src/com/cloud/network/ExternalLoadBalancerDeviceManagerImpl.java bcefccc 
>   server/src/com/cloud/network/LBHealthCheckPolicyVO.java PRE-CREATION 
>   server/src/com/cloud/network/NetworkManagerImpl.java a575183 
>   server/src/com/cloud/network/dao/LBHealthCheckPolicyDao.java PRE-CREATION 
>   server/src/com/cloud/network/dao/LBHealthCheckPolicyDaoImpl.java PRE-CREATION 
>   server/src/com/cloud/network/dao/LoadBalancerVMMapVO.java 8856993 
>   server/src/com/cloud/network/element/VirtualRouterElement.java 500d0b6 
>   server/src/com/cloud/network/lb/LBHealthCheckManager.java PRE-CREATION 
>   server/src/com/cloud/network/lb/LBHealthCheckManagerImpl.java PRE-CREATION 
>   server/src/com/cloud/network/lb/LoadBalancingRulesManager.java 9d7d22f 
>   server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java 531a428 
>   server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java b387ab5

>   server/src/com/cloud/server/ManagementServerImpl.java 3c615e1 
>   setup/db/db/schema-410to420.sql f3112a1 
> 
> Diff: https://reviews.apache.org/r/9165/diff/
> 
> 
> Testing
> -------
> 
> Testing Done:
> =============
> 
> 1. create LB rule of TCP protocol and assing instances, create lb healthcheck on lb rule.
A TCP monitor is created and it will binded to all the services.
> 2. create LB rule of HTTP protocol and assing instances, create lb healthcheck on lb
rule. HTTP monitor is created and it will binded to all the services.
> 3. create LB rule of HTTP protocol and assing instances, create lb healthcheck on lb
rule with pingpath and other params, HTTP monitor is created and it will binded to all the
services and its properties pingpath value will be present.
> 4.for an existing LB with an Monitor, add a new service to LB, monitor will be binded
to the newly added service.
> 5.for an existing LB with an Monitor, delete a service from LB, monitor will be unbinded
and service will be removed.
> 6.delete an monitor for LB rule, all the service binded to the monitor will be unbinded
and monitor will get removed.
> 7.delete LB rule, lb vserver will be deleted and the monitor will be deleted. 
> 8. list the LB rules giving the lb rule id, healtcheckpolicy created on the LB rule will
be returned. if not it will return empty list
> 9.Modify the healthcheck.update.interval to 1 min, at every one minute LBHealthCheckManager
will be updating the service state from the Netscaler.
> 
> 
> Thanks,
> 
> Rajesh Battala
> 
>


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