vrrp: T1297: improve gratuitous ARP default value handling and help strings

3 files changed, 46 insertions, 25 deletions

diff --git a/data/templates/high-availability/keepalived.conf.j2 b/data/templates/high-availability/keepalived.conf.j2
index 23abb66dc..6ea5f91d0 100644
--- a/data/templates/high-availability/keepalived.conf.j2
+++ b/data/templates/high-availability/keepalived.conf.j2
@@ -2,6 +2,7 @@
 # Do not edit this file, all your changes will be lost
 # on next commit or reboot
 
+# Global definitions configuration block
 global_defs {
     dynamic_interfaces
     script_user root
diff --git a/interface-definitions/include/vrrp/garp.xml.i b/interface-definitions/include/vrrp/garp.xml.i
index b321c9591..b56b490df 100644
--- a/interface-definitions/include/vrrp/garp.xml.i
+++ b/interface-definitions/include/vrrp/garp.xml.i
@@ -4,38 +4,42 @@
     <help>Gratuitous ARP parameters</help>
   </properties>
   <children>
-    <leafNode name="master-delay">
+    <leafNode name="interval">
       <properties>
-        <help>Delay for second set of gratuitous ARPs after transition to MASTER</help>
+        <help>Interval between Gratuitous ARP</help>
         <valueHelp>
-          <format>u32:1-1000</format>
-          <description>Delay for second set of gratuitous ARPs after transition to MASTER</description>
+          <format>&lt;0.000-1000&gt;</format>
+          <description>Interval in seconds, resolution microseconds</description>
         </valueHelp>
         <constraint>
-          <validator name="numeric" argument="--range 1-1000"/>
+          <validator name="numeric" argument="--range 0.000-1000 --float"/>
         </constraint>
       </properties>
-      <defaultValue>5</defaultValue>
+      <defaultValue>0</defaultValue>
     </leafNode>
-    <leafNode name="master-repeat">
+    <leafNode name="master-delay">
       <properties>
-        <help>Number of gratuitous ARP messages to send at a time after transition to MASTER</help>
+        <help>Delay for second set of gratuitous ARPs after transition to master</help>
         <valueHelp>
-          <format>u32:1-255</format>
-          <description>Number of gratuitous ARP messages to send at a time after transition to MASTER</description>
+          <format>u32:1-1000</format>
+          <description>Delay in seconds</description>
         </valueHelp>
         <constraint>
-          <validator name="numeric" argument="--range 1-255"/>
+          <validator name="numeric" argument="--range 1-1000"/>
         </constraint>
       </properties>
       <defaultValue>5</defaultValue>
     </leafNode>
     <leafNode name="master-refresh">
       <properties>
-        <help>Minimum time interval for refreshing gratuitous ARPs while MASTER. 0 means no refresh</help>
+        <help>Minimum time interval for refreshing gratuitous ARPs while beeing master</help>
+        <valueHelp>
+          <format>u32:0</format>
+          <description>No refresh</description>
+        </valueHelp>
         <valueHelp>
           <format>u32:1-255</format>
-          <description>Minimum time interval for refreshing gratuitous ARPs while MASTER. 0 means no refresh</description>
+          <description>Interval in seconds</description>
         </valueHelp>
         <constraint>
           <validator name="numeric" argument="--range 1-255"/>
@@ -45,10 +49,10 @@
     </leafNode>
     <leafNode name="master-refresh-repeat">
       <properties>
-        <help>Number of gratuitous ARP messages to send at a time while MASTER</help>
+        <help>Number of gratuitous ARP messages to send at a time while beeing master</help>
         <valueHelp>
           <format>u32:1-255</format>
-          <description>Number of gratuitous ARP messages to send at a time while MASTER</description>
+          <description>Number of gratuitous ARP messages</description>
         </valueHelp>
         <constraint>
           <validator name="numeric" argument="--range 1-255"/>
@@ -56,18 +60,18 @@
       </properties>
       <defaultValue>1</defaultValue>
     </leafNode>
-    <leafNode name="interval">
+    <leafNode name="master-repeat">
       <properties>
-        <help>Delay between gratuitous ARP messages sent on an interface</help>
+        <help>Number of gratuitous ARP messages to send at a time after transition to master</help>
         <valueHelp>
-          <format>&lt;0.000-1000&gt;</format>
-          <description>Delay between gratuitous ARP messages sent on an interface</description>
+          <format>u32:1-255</format>
+          <description>Number of gratuitous ARP messages</description>
         </valueHelp>
         <constraint>
-          <validator name="numeric" argument="--range 0.000-1000 --float"/>
+          <validator name="numeric" argument="--range 1-255"/>
         </constraint>
       </properties>
-      <defaultValue>0</defaultValue>
+      <defaultValue>5</defaultValue>
     </leafNode>
   </children>
 </node>
diff --git a/src/conf_mode/high-availability.py b/src/conf_mode/high-availability.py
index bc3e67b40..79e407efd 100755
--- a/src/conf_mode/high-availability.py
+++ b/src/conf_mode/high-availability.py
@@ -28,6 +28,7 @@ from vyos.template import render
 from vyos.template import is_ipv4
 from vyos.template import is_ipv6
 from vyos.util import call
+from vyos.util import dict_search
 from vyos.xml import defaults
 from vyos import ConfigError
 from vyos import airbag
@@ -49,12 +50,27 @@ def get_config(config=None):
     # We have gathered the dict representation of the CLI, but there are default
     # options which we need to update into the dictionary retrived.
     if 'vrrp' in ha:
+        if dict_search('vrrp.global_parameters.garp', ha) != None:
+            default_values = defaults(base_vrrp + ['global-parameters', 'garp'])
+            ha['vrrp']['global_parameters']['garp'] = dict_merge(
+                default_values, ha['vrrp']['global_parameters']['garp'])
+
         if 'group' in ha['vrrp']:
-            default_values_vrrp = defaults(base_vrrp + ['group'])
-            if 'garp' in default_values_vrrp:
-                del default_values_vrrp['garp']
+            default_values = defaults(base_vrrp + ['group'])
+            default_values_garp = defaults(base_vrrp + ['group', 'garp'])
+
+            # XXX: T2665: we can not safely rely on the defaults() when there are
+            # tagNodes in place, it is better to blend in the defaults manually.
+            if 'garp' in default_values:
+                del default_values['garp']
             for group in ha['vrrp']['group']:
-                ha['vrrp']['group'][group] = dict_merge(default_values_vrrp, ha['vrrp']['group'][group])
+                ha['vrrp']['group'][group] = dict_merge(default_values, ha['vrrp']['group'][group])
+
+                # XXX: T2665: we can not safely rely on the defaults() when there are
+                # tagNodes in place, it is better to blend in the defaults manually.
+                if 'garp' in ha['vrrp']['group'][group]:
+                    ha['vrrp']['group'][group]['garp'] = dict_merge(
+                        default_values_garp, ha['vrrp']['group'][group]['garp'])
 
     # Merge per virtual-server default values
     if 'virtual_server' in ha: