Re: [PATCH] add ability to configure checker parameters in vs

See comments in-line

15.08.2014 14:46, Bjørnar Ness пишет:
> When using TUN or DR, parameters such as connect_ip,
> connect_port and so on are often duplicated in all checker
> config-blocks.
> 
> This patch adds possibility to define defaults for these settings
> in virtual_server block, which can be overwritten in checker blocks.
If we decide to solve the config size problem, shouldn't we focus first on the checker's parameters, like check method, URL and so on? They are usually the same for all the real servers no matter do we use DR/TUN or not.

> 
> patch attached..
> 
> -- Bj(/)rnar
> 
> 
> keepalived-def_conn_opts.diff
> 
> 
> diff --git a/keepalived/check/check_api.c b/keepalived/check/check_api.c
> index 6df05a2..209a742 100644
> --- a/keepalived/check/check_api.c
> +++ b/keepalived/check/check_api.c
> @@ -75,13 +75,20 @@ queue_checker(void (*free_func) (void *), void (*dump_func) (void *)
>  	      , conn_opts_t *co)
>  {
>  	virtual_server_t *vs = LIST_TAIL_DATA(check_data->vs);
> +	conn_opts_t *def_co = vs->def_co;
>  	real_server_t *rs = LIST_TAIL_DATA(vs->rs);
>  	checker_t *checker = (checker_t *) MALLOC(sizeof (checker_t));
>  
>  	/* Set default dst = RS, timeout = 5 */
>  	if (co) {
> -		co->dst = rs->addr;
> -		co->connection_to = 5 * TIMER_HZ;
I suggest to make def_co a member struct, not a pointer. In that case the following if would not be necessary, and unconditional memcpy could be used. The def_co.connection_to would be always set to the default value. Only the RS address assignment would stay conditional here.

> +		/* copy conn_opts from vs to current checker if set */
> +		if(def_co)
> +			memcpy(co, def_co, sizeof(struct _conn_opts));
> +		else
> +			co->connection_to = 5 * TIMER_HZ;
> +
> +		if(! vs->def_co_ip_set)
> +			co->dst = rs->addr;
>  	}
>  
>  	checker->free_func = free_func;
> diff --git a/keepalived/check/check_data.c b/keepalived/check/check_data.c
> index 5cc2ed8..a226527 100644
> --- a/keepalived/check/check_data.c
> +++ b/keepalived/check/check_data.c
> @@ -159,6 +159,7 @@ free_vs(void *data)
>  	free_list(vs->rs);
>  	FREE_PTR(vs->quorum_up);
>  	FREE_PTR(vs->quorum_down);
> +	FREE_PTR(vs->def_co);
this would not be necessary, too
>  	FREE(vs);
>  }
>  static void
> @@ -241,6 +242,8 @@ alloc_vs(char *ip, char *port)
>  	new->delay_loop = KEEPALIVED_DEFAULT_DELAY;
>  	strncpy(new->timeout_persistence, "0", 1);
>  	new->virtualhost = NULL;
> +	new->def_co = NULL;
> +	new->def_co_ip_set = 0;
here we would assign the default connection_to, without duplicating of this constant in def_co_timeout_handler.
>  	new->alpha = 0;
>  	new->omega = 0;
>  	new->quorum_up = NULL;
> diff --git a/keepalived/check/check_parser.c b/keepalived/check/check_parser.c
> index cc270ab..3cd20dc 100644
> --- a/keepalived/check/check_parser.c
> +++ b/keepalived/check/check_parser.c
> @@ -163,6 +163,77 @@ virtualhost_handler(vector_t *strvec)
>  	vs->virtualhost = set_value(strvec);
>  }
>  
> +conn_opts_t *
> +def_co_get(virtual_server_t *vs)
> +{
> +	conn_opts_t *co;
> +
> +	if(vs->def_co)
> +		return vs->def_co;
> +
> +	co = CHECKER_NEW_CO();
> +	vs->def_co = co;
> +	/* Set default timeout = 5 */
> +	co->connection_to = 5 * TIMER_HZ;
> +
> +	return co;
> +}
> +static void
> +def_co_ip_handler(vector_t *strvec)
> +{
> +	virtual_server_t *vs = LIST_TAIL_DATA(check_data->vs);
> +	conn_opts_t *co = def_co_get(vs);
> +
> +	inet_stosockaddr(vector_slot(strvec,1), 0, &co->dst);
> +	vs->def_co_ip_set = 1;
> +}
> +static void
> +def_co_port_handler(vector_t *strvec)
> +{
> +	virtual_server_t *vs = LIST_TAIL_DATA(check_data->vs);
> +	conn_opts_t *co = def_co_get(vs);
> +
> +	checker_set_dst_port(&co->dst, htons(CHECKER_VALUE_INT(strvec)));
> +}
> +static void
> +def_co_srcip_handler(vector_t *strvec)
> +{
> +	virtual_server_t *vs = LIST_TAIL_DATA(check_data->vs);
> +	conn_opts_t *co = def_co_get(vs);
> +
> +	inet_stosockaddr(vector_slot(strvec, 1), 0, &co->bindto);
> +}
> +static void
> +def_co_srcport_handler(vector_t *strvec)
> +{
> +	virtual_server_t *vs = LIST_TAIL_DATA(check_data->vs);
> +	conn_opts_t *co = def_co_get(vs);
> +
> +	checker_set_dst_port(&co->bindto, htons(CHECKER_VALUE_INT(strvec)));
> +}
> +static void
> +def_co_timeout_handler(vector_t *strvec)
> +{
> +	virtual_server_t *vs = LIST_TAIL_DATA(check_data->vs);
> +	conn_opts_t *co = def_co_get(vs);
> +
> +	co->connection_to = CHECKER_VALUE_INT(strvec) * TIMER_HZ;
> +
> +	/* do not allow 0 timeout */
> +	if(! co->connection_to)
> +		co->connection_to = TIMER_HZ;
> +}
> +#ifdef _WITH_SO_MARK_
> +static void
> +def_co_fwmark_handler(vector_t *strvec)
> +{
> +	virtual_server_t *vs = LIST_TAIL_DATA(check_data->vs);
> +	conn_opts_t *co = def_co_get(vs);
> +
> +	co->fwmark = CHECKER_VALUE_INT(strvec);
> +}
> +#endif
> +
>  /* Sorry Servers handlers */
>  static void
>  ssvr_handler(vector_t *strvec)
> @@ -312,6 +383,15 @@ check_init_keywords(void)
>  	install_keyword("ops", &ops_handler);
>  	install_keyword("virtualhost", &virtualhost_handler);
>  
> +	install_keyword("connect_ip", &def_co_ip_handler);
> +	install_keyword("connect_port", &def_co_port_handler);
> +	install_keyword("bindto", &def_co_srcip_handler);
> +	install_keyword("bind_port", &def_co_srcport_handler);
> +	install_keyword("connect_timeout", &def_co_timeout_handler);
> +#ifdef _WITH_SO_MARK_
> +	install_keyword("fwmark", &def_co_fwmark_handler);
> +#endif
Here we duplicate the connection options list. I think we can invent a method to use universal connpots handlers, thus def_*_handler functions would not appear and the install_connect_keywords() call could be used here. To do so, the global var containing a pointer to the current conn_opts_t structure could be used.
> +
>  	/* Pool regression detection and handling. */
>  	install_keyword("alpha", &alpha_handler);
>  	install_keyword("omega", &omega_handler);
> diff --git a/keepalived/include/check_data.h b/keepalived/include/check_data.h
> index f634633..e4a3ecd 100644
> --- a/keepalived/include/check_data.h
> +++ b/keepalived/include/check_data.h
> @@ -49,6 +49,7 @@
>  
>  /* Typedefs */
>  typedef unsigned int checker_id_t;
> +typedef struct _conn_opts conn_opts_t;
May be just move the conn_opt_t definition to the check_data.h ?
>  
>  /* Daemon dynamic data structure definition */
>  #define MAX_TIMEOUT_LENGTH		5
> @@ -124,6 +125,8 @@ typedef struct _virtual_server {
>  	uint32_t			nat_mask;
>  	uint32_t			granularity_persistence;
>  	char				*virtualhost;
> +	conn_opts_t			*def_co;
> +	int				def_co_ip_set;
def_co_ip_set field is not necessary, we could check if af != AF_UNSPEC
>  	list				rs;
>  	int				alive;
>  	unsigned			alpha;		/* Alpha mode enabled. */
> 
> 
> 
> ------------------------------------------------------------------------------


-- 
Best regards,
Alexey 

------------------------------------------------------------------------------
_______________________________________________
Keepalived-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/keepalived-devel