[prev in list] [next in list] [prev in thread] [next in thread] 

List:       keepalived-devel
Subject:    Re: [Keepalived-devel] [PATCH] add ability to configure checker parameters in vs
From:       Alexey Andriyanov <alan () al-an ! info>
Date:       2014-08-16 9:11:21
Message-ID: 53EF2039.5090400 () al-an ! info
[Download RAW message or body]

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
Keepalived-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/keepalived-devel


[prev in list] [next in list] [prev in thread] [next in thread]