----------------------------------------------------------- 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 > >
