[patch libteam] teamd: Add option to change evaluation logic of multiple link-watchers

[Otto Hollmann]

Now, if multiple link watchers are used, link is up if any of the
link-watchers reports the link up.

Introduce new option "lw_policy" to change this behaviour. Possible values
are "any" and "all". If nothing specified, default value
"any" will be
used and there will be no change in current behaviour. If value "all" will
be set, link will be up only if ALL the link-watchers report the link up.

Signed-off-by: Otto Hollmann <otto.hollmann(a)suse.com&gt;
---
 .../activebackup_multi_lw_1.conf              |  1 +
 teamd/teamd.c                                 | 28 +++++++++++++++++++
 teamd/teamd.h                                 |  1 +
 teamd/teamd_link_watch.c                      | 27 ++++++++++++++----
 4 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/teamd/example_configs/activebackup_multi_lw_1.conf
b/teamd/example_configs/activebackup_multi_lw_1.conf
index c1645d2..c66e6d7 100644
--- a/teamd/example_configs/activebackup_multi_lw_1.conf
+++ b/teamd/example_configs/activebackup_multi_lw_1.conf
@@ -1,6 +1,7 @@
 {
 	"device":	"team0",
 	"runner":	{"name": "activebackup"},
+	"lw_policy":	"any",
 	"link_watch":	[
 		{
 		"name": "arp_ping",
diff --git a/teamd/teamd.c b/teamd/teamd.c
index a89b702..a8a347f 100644
--- a/teamd/teamd.c
+++ b/teamd/teamd.c
@@ -1805,6 +1805,30 @@ static int teamd_drop_privileges()
 
 #endif
 
+static int teamd_get_lw_policy(struct teamd_context *ctx)
+{
+	int err;
+	const char *lw_policy;
+
+	err = teamd_config_string_get(ctx, &lw_policy, "$.lw_policy");
+
+	if (!err) {
+		if (!strcmp(lw_policy, "all")) {
+			ctx->evaluate_all_watchers = true;
+		} else if (!strcmp(lw_policy, "any")) {
+			ctx->evaluate_all_watchers = false;
+		} else {
+			teamd_log_err("Unrecognized value for lw_policy.");
+			teamd_log_err("Only \"any\" or \"all\" are allowed but
\"%s\" found in config.", lw_policy);
+			return -EINVAL;
+		}
+	} else {
+		ctx->evaluate_all_watchers = false;
+		teamd_log_dbg(ctx, "No lw_policy specified in config, using
default value \"any\".");
+	}
+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	enum teamd_exit_code ret = TEAMD_EXIT_FAILURE;
@@ -1863,6 +1887,10 @@ int main(int argc, char **argv)
 	if (err)
 		goto config_free;
 
+	err = teamd_get_lw_policy(ctx);
+	if (err)
+		goto config_free;
+
 	err = teamd_set_default_pid_file(ctx);
 	if (err)
 		goto config_free;
diff --git a/teamd/teamd.h b/teamd/teamd.h
index 541d2a7..4ca40a1 100644
--- a/teamd/teamd.h
+++ b/teamd/teamd.h
@@ -110,6 +110,7 @@ struct teamd_context {
 	json_t *			config_json;
 	char *				pid_file;
 	char *				team_devname;
+	bool				evaluate_all_watchers;
 	char *				ident;
 	char *				argv0;
 	struct team_handle *		th;
diff --git a/teamd/teamd_link_watch.c b/teamd/teamd_link_watch.c
index cae6549..11f4697 100644
--- a/teamd/teamd_link_watch.c
+++ b/teamd/teamd_link_watch.c
@@ -133,11 +133,28 @@ bool teamd_link_watch_port_up(struct teamd_context *ctx,
 	if (!tdport)
 		return true;
 	link = true;
-	teamd_for_each_port_priv_by_creator(common_ppriv, tdport,
-					    LW_PORT_PRIV_CREATOR_PRIV) {
-		link = common_ppriv->link_up;
-		if (link)
-			return link;
+	if (ctx->evaluate_all_watchers) {
+		/*
+		 * If multiple link-watchers used at the same time,
+		 * link is up if ALL of the link-watchers reports the link up.
+		 */
+		teamd_for_each_port_priv_by_creator(common_ppriv, tdport,
+						    LW_PORT_PRIV_CREATOR_PRIV) {
+			link = common_ppriv->link_up;
+			if (!link)
+				return link;
+		}
+	} else {
+		/*
+		 * If multiple link-watchers used at the same time,
+		 * link is up if ANY of the link-watchers reports the link up.
+		 */
+		teamd_for_each_port_priv_by_creator(common_ppriv, tdport,
+						    LW_PORT_PRIV_CREATOR_PRIV) {
+			link = common_ppriv->link_up;
+			if (link)
+				return link;
+		}
 	}
 	return link;
 }
-- 
2.35.3

[Otto Hollmann]

Hi there,
can you please provide feedback on this feature request?

Also let me know if anything is missing or needs to be modified and I will fix
it.

Regards

Otto Hollmann

On Fri, 2022-12-23 at 19:56 +0100, Otto Hollmann wrote:
>  Now, if multiple link watchers are used, link is up if any of the
>  link-watchers reports the link up.
>  
>  Introduce new option "lw_policy" to change this behaviour. Possible values
>  are "any" and "all". If nothing specified, default value
> "any" will be
>  used and there will be no change in current behaviour. If value "all" will
>  be set, link will be up only if ALL the link-watchers report the link up.
>  
>  Signed-off-by: Otto Hollmann <otto.hollmann(a)suse.com&gt;
>  ---
>   .../activebackup_multi_lw_1.conf              |  1 +
>   teamd/teamd.c                                 | 28 +++++++++++++++++++
>   teamd/teamd.h                                 |  1 +
>   teamd/teamd_link_watch.c                      | 27 ++++++++++++++----
>   4 files changed, 52 insertions(+), 5 deletions(-)
>  
>  diff --git a/teamd/example_configs/activebackup_multi_lw_1.conf
>  b/teamd/example_configs/activebackup_multi_lw_1.conf
>  index c1645d2..c66e6d7 100644
>  --- a/teamd/example_configs/activebackup_multi_lw_1.conf
>  +++ b/teamd/example_configs/activebackup_multi_lw_1.conf
>  @@ -1,6 +1,7 @@
>   {
>   	"device":	"team0",
>   	"runner":	{"name": "activebackup"},
>  +	"lw_policy":	"any",
>   	"link_watch":	[
>   		{
>   		"name": "arp_ping",
>  diff --git a/teamd/teamd.c b/teamd/teamd.c
>  index a89b702..a8a347f 100644
>  --- a/teamd/teamd.c
>  +++ b/teamd/teamd.c
>  @@ -1805,6 +1805,30 @@ static int teamd_drop_privileges()
>   
>   #endif
>   
>  +static int teamd_get_lw_policy(struct teamd_context *ctx)
>  +{
>  +	int err;
>  +	const char *lw_policy;
>  +
>  +	err = teamd_config_string_get(ctx, &lw_policy, "$.lw_policy");
>  +
>  +	if (!err) {
>  +		if (!strcmp(lw_policy, "all")) {
>  +			ctx->evaluate_all_watchers = true;
>  +		} else if (!strcmp(lw_policy, "any")) {
>  +			ctx->evaluate_all_watchers = false;
>  +		} else {
>  +			teamd_log_err("Unrecognized value for lw_policy.");
>  +			teamd_log_err("Only \"any\" or \"all\" are allowed but
>  \"%s\" found in config.", lw_policy);
>  +			return -EINVAL;
>  +		}
>  +	} else {
>  +		ctx->evaluate_all_watchers = false;
>  +		teamd_log_dbg(ctx, "No lw_policy specified in config, using
>  default value \"any\".");
>  +	}
>  +	return 0;
>  +}
>  +
>   int main(int argc, char **argv)
>   {
>   	enum teamd_exit_code ret = TEAMD_EXIT_FAILURE;
>  @@ -1863,6 +1887,10 @@ int main(int argc, char **argv)
>   	if (err)
>   		goto config_free;
>   
>  +	err = teamd_get_lw_policy(ctx);
>  +	if (err)
>  +		goto config_free;
>  +
>   	err = teamd_set_default_pid_file(ctx);
>   	if (err)
>   		goto config_free;
>  diff --git a/teamd/teamd.h b/teamd/teamd.h
>  index 541d2a7..4ca40a1 100644
>  --- a/teamd/teamd.h
>  +++ b/teamd/teamd.h
>  @@ -110,6 +110,7 @@ struct teamd_context {
>   	json_t *			config_json;
>   	char *				pid_file;
>   	char *				team_devname;
>  +	bool				evaluate_all_watchers;
>   	char *				ident;
>   	char *				argv0;
>   	struct team_handle *		th;
>  diff --git a/teamd/teamd_link_watch.c b/teamd/teamd_link_watch.c
>  index cae6549..11f4697 100644
>  --- a/teamd/teamd_link_watch.c
>  +++ b/teamd/teamd_link_watch.c
>  @@ -133,11 +133,28 @@ bool teamd_link_watch_port_up(struct teamd_context *ctx,
>   	if (!tdport)
>   		return true;
>   	link = true;
>  -	teamd_for_each_port_priv_by_creator(common_ppriv, tdport,
>  -					    LW_PORT_PRIV_CREATOR_PRIV) {
>  -		link = common_ppriv->link_up;
>  -		if (link)
>  -			return link;
>  +	if (ctx->evaluate_all_watchers) {
>  +		/*
>  +		 * If multiple link-watchers used at the same time,
>  +		 * link is up if ALL of the link-watchers reports the link up.
>  +		 */
>  +		teamd_for_each_port_priv_by_creator(common_ppriv, tdport,
>  +						    LW_PORT_PRIV_CREATOR_PRIV) {
>  +			link = common_ppriv->link_up;
>  +			if (!link)
>  +				return link;
>  +		}
>  +	} else {
>  +		/*
>  +		 * If multiple link-watchers used at the same time,
>  +		 * link is up if ANY of the link-watchers reports the link up.
>  +		 */
>  +		teamd_for_each_port_priv_by_creator(common_ppriv, tdport,
>  +						    LW_PORT_PRIV_CREATOR_PRIV) {
>  +			link = common_ppriv->link_up;
>  +			if (link)
>  +				return link;
>  +		}
>   	}
>   	return link;
>   }
>  -- 
>  2.35.3
>  
>  _______________________________________________
>  libteam mailing list -- libteam(a)lists.fedorahosted.org
>  To unsubscribe send an email to libteam-leave(a)lists.fedorahosted.org
>  Fedora Code of Conduct: 
>  https://docs.fedoraproject.org/en-US/project/code-of-conduct/
>  List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
>  List Archives: 
>  https://lists.fedorahosted.org/archives/list/[e-mail]
>  Do not reply to spam, report it: 
>  https://pagure.io/fedora-infrastructure/new_issue

[Jiri Pirko]

Thu, Feb 09, 2023 at 06:34:47PM CET, otto.hollmann(a)suse.com wrote:
> Hi there,
> can you please provide feedback on this feature request?
>
> Also let me know if anything is missing or needs to be modified and I will fix
> it. 
I'm very sorry, I totally missed this for couple of months, not sure how
that happened...

[kindom random]

hello yoyu know its soo cool idea really i love it

[kindom random]

kljf hghhghghghghghghghghggggagiurrrrrrrrrr awygawrygpoawyg uarygywearguiygiuwayiugaywre

[Jiri Pirko]

Fri, Dec 23, 2022 at 07:56:33PM CET, otto.hollmann(a)suse.com wrote:
> Now, if multiple link watchers are used, link is up if any of the
> link-watchers reports the link up.
>
> Introduce new option "lw_policy" to change this behaviour. Possible values
> are "any" and "all". If nothing specified, default value
> "any" will be
> used and there will be no change in current behaviour. If value "all" will
> be set, link will be up only if ALL the link-watchers report the link up. 
In general, this looks fine.
Couple of nits below:

> Signed-off-by: Otto Hollmann <otto.hollmann(a)suse.com&gt;
> ---
>  .../activebackup_multi_lw_1.conf              |  1 +
>  teamd/teamd.c                                 | 28 +++++++++++++++++++
>  teamd/teamd.h                                 |  1 +
>  teamd/teamd_link_watch.c                      | 27 ++++++++++++++---- 
You are missing manpage edit here.


>  4 files changed, 52 insertions(+), 5 deletions(-)
>
> diff --git a/teamd/example_configs/activebackup_multi_lw_1.conf
> b/teamd/example_configs/activebackup_multi_lw_1.conf
> index c1645d2..c66e6d7 100644
> --- a/teamd/example_configs/activebackup_multi_lw_1.conf
> +++ b/teamd/example_configs/activebackup_multi_lw_1.conf
> @@ -1,6 +1,7 @@
>  {
>  	"device":	"team0",
>  	"runner":	{"name": "activebackup"},
> +	"lw_policy":	"any", 
We have "link_watch" with no abbr. Please name this
"link_watch_policy".
It's a bit longer, but consistent.



>  	"link_watch":	[
>  		{
>  		"name": "arp_ping", 
Please add nother example file with "all".


> diff --git a/teamd/teamd.c b/teamd/teamd.c
> index a89b702..a8a347f 100644
> --- a/teamd/teamd.c
> +++ b/teamd/teamd.c
> @@ -1805,6 +1805,30 @@ static int teamd_drop_privileges()
>  
>  #endif
>  
> +static int teamd_get_lw_policy(struct teamd_context *ctx)
> +{
> +	int err;
> +	const char *lw_policy;
> +
> +	err = teamd_config_string_get(ctx, &lw_policy, "$.lw_policy");
> + 
Empty line not needed here, remove.


> +	if (!err) {
> +		if (!strcmp(lw_policy, "all")) {
> +			ctx->evaluate_all_watchers = true;
> +		} else if (!strcmp(lw_policy, "any")) {
> +			ctx->evaluate_all_watchers = false;
> +		} else {
> +			teamd_log_err("Unrecognized value for lw_policy.");
> +			teamd_log_err("Only \"any\" or \"all\" are allowed but
> \"%s\" found in config.", lw_policy); 
Hmm, this is an odd line break. Email client broken? Perhaps better to
use git-send-email instead.


> +			return -EINVAL;
> +		}
> +	} else {
> +		ctx->evaluate_all_watchers = false; 
Struct is zeroed (see teamd_context_init()), why do you need to
have this assign?


> +		teamd_log_dbg(ctx, "No lw_policy specified in config, using
> default value \"any\".");
> +	}
> +	return 0;
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	enum teamd_exit_code ret = TEAMD_EXIT_FAILURE;
> @@ -1863,6 +1887,10 @@ int main(int argc, char **argv)
>  	if (err)
>  		goto config_free;
>  
> +	err = teamd_get_lw_policy(ctx);
> +	if (err)
> +		goto config_free;
> +
>  	err = teamd_set_default_pid_file(ctx);
>  	if (err)
>  		goto config_free;
> diff --git a/teamd/teamd.h b/teamd/teamd.h
> index 541d2a7..4ca40a1 100644
> --- a/teamd/teamd.h
> +++ b/teamd/teamd.h
> @@ -110,6 +110,7 @@ struct teamd_context {
>  	json_t *			config_json;
>  	char *				pid_file;
>  	char *				team_devname;
> +	bool				evaluate_all_watchers; 
I would rather see this amond the rest of the bools a bit above this.
Or do you have any reason to put it here?


> 	char *				ident;
> 	char *				argv0;
> 	struct team_handle *		th;
>diff --git a/teamd/teamd_link_watch.c b/teamd/teamd_link_watch.c
>index cae6549..11f4697 100644
>--- a/teamd/teamd_link_watch.c
>+++ b/teamd/teamd_link_watch.c
>@@ -133,11 +133,28 @@ bool teamd_link_watch_port_up(struct teamd_context *ctx,
> 	if (!tdport)
> 		return true;
> 	link = true;
>-	teamd_for_each_port_priv_by_creator(common_ppriv, tdport,
>-					    LW_PORT_PRIV_CREATOR_PRIV) {
>-		link = common_ppriv->link_up;
>-		if (link)
>-			return link;
>+	if (ctx->evaluate_all_watchers) {
>+		/*
>+		 * If multiple link-watchers used at the same time,
>+		 * link is up if ALL of the link-watchers reports the link up.
>+		 */
>+		teamd_for_each_port_priv_by_creator(common_ppriv, tdport,
>+						    LW_PORT_PRIV_CREATOR_PRIV) {
>+			link = common_ppriv->link_up;
>+			if (!link)
>+				return link;
>+		}
>+	} else {
>+		/*
>+		 * If multiple link-watchers used at the same time,
>+		 * link is up if ANY of the link-watchers reports the link up.
>+		 */
>+		teamd_for_each_port_priv_by_creator(common_ppriv, tdport,
>+						    LW_PORT_PRIV_CREATOR_PRIV) {
>+			link = common_ppriv->link_up;
>+			if (link)
>+				return link;
>+		}
> 	}
> 	return link;
> }
>-- 
>2.35.3
>
>

[Otto Hollmann]

I sent a v2 patch with all your nits fixed.

Thanks

On Mon, 2023-03-13 at 13:13 +0100, Jiri Pirko wrote:
>  Fri, Dec 23, 2022 at 07:56:33PM CET, otto.hollmann(a)suse.com wrote:
>  > Now, if multiple link watchers are used, link is up if any of the
>  > link-watchers reports the link up.
>  > 
>  > Introduce new option "lw_policy" to change this behaviour. Possible
> values
>  > are "any" and "all". If nothing specified, default value
> "any" will be
>  > used and there will be no change in current behaviour. If value "all"
> will
>  > be set, link will be up only if ALL the link-watchers report the link up.
>  
>  In general, this looks fine.
>  Couple of nits below:
>  
>  > 
>  > Signed-off-by: Otto Hollmann <otto.hollmann(a)suse.com&gt;
>  > ---
>  > .../activebackup_multi_lw_1.conf              |  1 +
>  > teamd/teamd.c                                 | 28 +++++++++++++++++++
>  > teamd/teamd.h                                 |  1 +
>  > teamd/teamd_link_watch.c                      | 27 ++++++++++++++----
>  
>  You are missing manpage edit here.
>  
>  
>  > 4 files changed, 52 insertions(+), 5 deletions(-)
>  > 
>  > diff --git a/teamd/example_configs/activebackup_multi_lw_1.conf
>  > b/teamd/example_configs/activebackup_multi_lw_1.conf
>  > index c1645d2..c66e6d7 100644
>  > --- a/teamd/example_configs/activebackup_multi_lw_1.conf
>  > +++ b/teamd/example_configs/activebackup_multi_lw_1.conf
>  > @@ -1,6 +1,7 @@
>  > {
>  >         "device":       "team0",
>  >         "runner":       {"name": "activebackup"},
>  > +       "lw_policy":    "any",
>  
>  We have "link_watch" with no abbr. Please name this
> "link_watch_policy".
>  It's a bit longer, but consistent.
>  
>  
>  
>  >         "link_watch":   [
>  >                 {
>  >                 "name": "arp_ping",
>  
>  Please add nother example file with "all".
>  
>  
>  > diff --git a/teamd/teamd.c b/teamd/teamd.c
>  > index a89b702..a8a347f 100644
>  > --- a/teamd/teamd.c
>  > +++ b/teamd/teamd.c
>  > @@ -1805,6 +1805,30 @@ static int teamd_drop_privileges()
>  > 
>  > #endif
>  > 
>  > +static int teamd_get_lw_policy(struct teamd_context *ctx)
>  > +{
>  > +       int err;
>  > +       const char *lw_policy;
>  > +
>  > +       err = teamd_config_string_get(ctx, &lw_policy,
> "$.lw_policy");
>  > +
>  
>  Empty line not needed here, remove.
>  
>  
>  > +       if (!err) {
>  > +               if (!strcmp(lw_policy, "all")) {
>  > +                       ctx->evaluate_all_watchers = true;
>  > +               } else if (!strcmp(lw_policy, "any")) {
>  > +                       ctx->evaluate_all_watchers = false;
>  > +               } else {
>  > +                       teamd_log_err("Unrecognized value for
> lw_policy.");
>  > +                       teamd_log_err("Only \"any\" or
> \"all\" are allowed
>  > but
>  > \"%s\" found in config.", lw_policy);
>  
>  Hmm, this is an odd line break. Email client broken? Perhaps better to
>  use git-send-email instead.
>  
>  
>  > +                       return -EINVAL;
>  > +               }
>  > +       } else {
>  > +               ctx->evaluate_all_watchers = false;
>  
>  Struct is zeroed (see teamd_context_init()), why do you need to
>  have this assign?
>  
>  
>  > +               teamd_log_dbg(ctx, "No lw_policy specified in config, using
>  > default value \"any\".");
>  > +       }
>  > +       return 0;
>  > +}
>  > +
>  > int main(int argc, char **argv)
>  > {
>  >         enum teamd_exit_code ret = TEAMD_EXIT_FAILURE;
>  > @@ -1863,6 +1887,10 @@ int main(int argc, char **argv)
>  >         if (err)
>  >                 goto config_free;
>  > 
>  > +       err = teamd_get_lw_policy(ctx);
>  > +       if (err)
>  > +               goto config_free;
>  > +
>  >         err = teamd_set_default_pid_file(ctx);
>  >         if (err)
>  >                 goto config_free;
>  > diff --git a/teamd/teamd.h b/teamd/teamd.h
>  > index 541d2a7..4ca40a1 100644
>  > --- a/teamd/teamd.h
>  > +++ b/teamd/teamd.h
>  > @@ -110,6 +110,7 @@ struct teamd_context {
>  >         json_t *                        config_json;
>  >         char *                          pid_file;
>  >         char *                          team_devname;
>  > +       bool                            evaluate_all_watchers;
>  
>  I would rather see this amond the rest of the bools a bit above this.
>  Or do you have any reason to put it here?
>  
>  
>  >         char *                          ident;
>  >         char *                          argv0;
>  >         struct team_handle *            th;
>  > diff --git a/teamd/teamd_link_watch.c b/teamd/teamd_link_watch.c
>  > index cae6549..11f4697 100644
>  > --- a/teamd/teamd_link_watch.c
>  > +++ b/teamd/teamd_link_watch.c
>  > @@ -133,11 +133,28 @@ bool teamd_link_watch_port_up(struct teamd_context
>  > *ctx,
>  >         if (!tdport)
>  >                 return true;
>  >         link = true;
>  > -       teamd_for_each_port_priv_by_creator(common_ppriv, tdport,
>  > -                                           LW_PORT_PRIV_CREATOR_PRIV) {
>  > -               link = common_ppriv->link_up;
>  > -               if (link)
>  > -                       return link;
>  > +       if (ctx->evaluate_all_watchers) {
>  > +               /*
>  > +                * If multiple link-watchers used at the same time,
>  > +                * link is up if ALL of the link-watchers reports the link
>  > up.
>  > +                */
>  > +               teamd_for_each_port_priv_by_creator(common_ppriv, tdport,
>  > +                                                  
>  > LW_PORT_PRIV_CREATOR_PRIV) {
>  > +                       link = common_ppriv->link_up;
>  > +                       if (!link)
>  > +                               return link;
>  > +               }
>  > +       } else {
>  > +               /*
>  > +                * If multiple link-watchers used at the same time,
>  > +                * link is up if ANY of the link-watchers reports the link
>  > up.
>  > +                */
>  > +               teamd_for_each_port_priv_by_creator(common_ppriv, tdport,
>  > +                                                  
>  > LW_PORT_PRIV_CREATOR_PRIV) {
>  > +                       link = common_ppriv->link_up;
>  > +                       if (link)
>  > +                               return link;
>  > +               }
>  >         }
>  >         return link;
>  > }
>  > -- 
>  > 2.35.3
>  > 
>  > 
>  _______________________________________________
>  libteam mailing list -- libteam(a)lists.fedorahosted.org
>  To unsubscribe send an email to libteam-leave(a)lists.fedorahosted.org
>  Fedora Code of Conduct:
>  https://docs.fedoraproject.org/en-US/project/code-of-conduct/
>  List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
>  List Archives:
>  https://lists.fedorahosted.org/archives/list/[e-mail]
>  Do not reply to spam, report it:
>  https://pagure.io/fedora-infrastructure/new_issue