Repository: eclipse-bluechi/bluechi (language: python)

Issue:
Make agent heartbeat interval a config option
The agent heartbeat has a #define in protocol.h.  This should be changed to a config option.


Discussion / hints:

Fix patch:
diff --git a/config/agent/agent.conf b/config/agent/agent.conf
index e5d05ee3d9..cbeea185a3 100644
--- a/config/agent/agent.conf
+++ b/config/agent/agent.conf
@@ -26,6 +26,10 @@ ManagerHost=
 # CLI options.
 #ManagerAddress=
 
+#
+# Defines the interval between two heartbeat signals sent to hirte in milliseconds. 
+#HeartbeatInterval=
+
 #
 # The level used for logging. Supported values are: DEBUG, INFO, WARN and ERROR.
 #LogLevel=INFO
diff --git a/config/agent/hirte-default.conf b/config/agent/hirte-default.conf
index f212b0d34e..69943b71fb 100644
--- a/config/agent/hirte-default.conf
+++ b/config/agent/hirte-default.conf
@@ -22,6 +22,10 @@ ManagerPort=842
 # CLI options.
 ManagerAddress=
 
+#
+# Defines the interval between two heartbeat signals sent to hirte in milliseconds. 
+HeartbeatInterval=
+
 #
 # The level used for logging. Supported values are: DEBUG, INFO, WARN and ERROR.
 LogLevel=INFO
diff --git a/doc/man/hirte-agent.1.md b/doc/man/hirte-agent.1.md
index 209f887208..ad6a51f1a8 100644
--- a/doc/man/hirte-agent.1.md
+++ b/doc/man/hirte-agent.1.md
@@ -39,6 +39,10 @@ Overrides any setting of `ManagerHost` or `ManagerPort` defined in the configura
 
 The unique name of this `hirte-agent` used for registering at `hirte`. This option will overwrite the port defined in the configuration file.
 
+#### **--interval**, **-i**
+
+The interval between two heartbeat signals sent to hirte in milliseconds. This option will overwrite the heartbeat interval defined in the configuration file. 
+
 #### **--config**, **-c**
 
 Path to the configuration file, see `hirte-agent.conf(5)`.
@@ -78,8 +82,4 @@ TBD
 
 ## CONFIGURATION FILES
 
-TBD
-
-## SEE ALSO
-
 **[hirte-agent.conf(5)](https://github.com/containers/hirte/blob/main/doc/man/hirte-agent.conf.5.md)**
diff --git a/doc/man/hirte-agent.conf.5.md b/doc/man/hirte-agent.conf.5.md
index 808fdf052c..78ea9f2e94 100644
--- a/doc/man/hirte-agent.conf.5.md
+++ b/doc/man/hirte-agent.conf.5.md
@@ -29,7 +29,6 @@ SD Bus address used by `hirte-agent` to connect to `hirte`. See `man sd_bus_set_
 Overrides any setting of `ManagerHost` or `ManagerPort` defined in the configuration file as well as the respective CLI
 options. The option doesn't have a default value.
 
-
 #### **ManagerHost** (string)
 
 The host used by `hirte-agent` to connect to `hirte`. Must be a valid IPv4 or IPv6. The option doesn't have a default
@@ -40,6 +39,10 @@ value, it's mandatory to set this option for each hirte-agent.
 The port on which `hirte` is listening for connection request and the `hirte-agent` is connecting to. By default port
 `842` is used.
 
+#### **HeartbeatInterval** (long)
+
+The interval between two heartbeat signals sent to hirte in milliseconds. This option will overwrite the heartbeat interval defined in the configuration file. 
+
 #### **LogLevel** (string)
 
 The level used for logging. Supported values are:
diff --git a/doc/man/hirte.1.md b/doc/man/hirte.1.md
index 20004a7558..7e97cd74c3 100644
--- a/doc/man/hirte.1.md
+++ b/doc/man/hirte.1.md
@@ -61,8 +61,4 @@ TBD
 
 ## CONFIGURATION FILES
 
-TBD
-
-## SEE ALSO
-
 **[hirte.conf(5)](https://github.com/containers/hirte/blob/main/doc/man/hirte.conf.5.md)**
diff --git a/src/agent/agent.c b/src/agent/agent.c
index 164bba5785..90c2d4ed01 100644
--- a/src/agent/agent.c
+++ b/src/agent/agent.c
@@ -102,7 +102,7 @@ static int agent_reset_heartbeat_timer(Agent *agent, sd_event_source **event_sou
                         agent->event,
                         event_source,
                         CLOCK_BOOTTIME,
-                        AGENT_HEARTBEAT_INTERVAL_USEC,
+                        agent->heartbeat_interval_msec * USEC_PER_MSEC,
                         0,
                         agent_heartbeat_timer_callback,
                         agent,
@@ -321,6 +321,7 @@ Agent *agent_new(void) {
         agent->event = steal_pointer(&event);
         agent->api_bus_service_name = steal_pointer(&service_name);
         agent->port = HIRTE_DEFAULT_PORT;
+        agent->heartbeat_interval_msec = AGENT_HEARTBEAT_INTERVAL_MSEC;
         LIST_HEAD_INIT(agent->outstanding_requests);
         LIST_HEAD_INIT(agent->tracked_jobs);
         LIST_HEAD_INIT(agent->proxy_services);
@@ -448,6 +449,18 @@ bool agent_set_name(Agent *agent, const char *name) {
         return copy_str(&agent->name, name);
 }
 
+bool agent_set_heartbeat_interval(Agent *agent, const char *interval_msec) {
+        long interval = 0;
+
+        if (!parse_long(interval_msec, &interval)) {
+                hirte_log_errorf("Invalid heartbeat interval format '%s'", interval_msec);
+                return false;
+        }
+        agent->heartbeat_interval_msec = interval;
+        return true;
+}
+
+
 void agent_set_systemd_user(Agent *agent, bool systemd_user) {
         agent->systemd_user = systemd_user;
 }
@@ -523,6 +536,13 @@ bool agent_parse_config(Agent *agent, const char *configfile) {
                 }
         }
 
+        value = cfg_get_value(agent->config, CFG_HEARTBEAT_INTERVAL);
+        if (value) {
+                if (!agent_set_heartbeat_interval(agent, value)) {
+                        return false;
+                }
+        }
+
         return true;
 }
 
diff --git a/src/agent/agent.h b/src/agent/agent.h
index edbffe5fc3..434f227a60 100644
--- a/src/agent/agent.h
+++ b/src/agent/agent.h
@@ -52,6 +52,7 @@ struct Agent {
         char *name;
         char *host;
         int port;
+        long heartbeat_interval_msec;
 
         AgentConnectionState connection_state;
         uint64_t connection_retry_count;
@@ -94,6 +95,7 @@ bool agent_set_port(Agent *agent, const char *port);
 bool agent_set_host(Agent *agent, const char *host);
 bool agent_set_orch_address(Agent *agent, const char *address);
 bool agent_set_name(Agent *agent, const char *name);
+bool agent_set_heartbeat_interval(Agent *agent, const char *interval_msec);
 void agent_set_systemd_user(Agent *agent, bool systemd_user);
 bool agent_parse_config(Agent *agent, const char *configfile);
 
diff --git a/src/agent/main.c b/src/agent/main.c
index 5f0cfa31f2..bc74f9ee23 100644
--- a/src/agent/main.c
+++ b/src/agent/main.c
@@ -14,6 +14,7 @@ const struct option options[] = { { ARG_HOST, required_argument, 0, ARG_HOST_SHO
                                   { ARG_PORT, required_argument, 0, ARG_PORT_SHORT },
                                   { ARG_ADDRESS, required_argument, 0, ARG_ADDRESS_SHORT },
                                   { ARG_NAME, required_argument, 0, ARG_NAME_SHORT },
+                                  { ARG_HEARTBEAT_INTERVAL, required_argument, 0, ARG_HEARTBEAT_INTERVAL_SHORT },
                                   { ARG_CONFIG, required_argument, 0, ARG_CONFIG_SHORT },
                                   { ARG_USER, no_argument, 0, ARG_USER_SHORT },
                                   { ARG_HELP, no_argument, 0, ARG_HELP_SHORT },
@@ -21,7 +22,7 @@ const struct option options[] = { { ARG_HOST, required_argument, 0, ARG_HOST_SHO
 
 #define OPTIONS_STR                                                                               \
         ARG_PORT_SHORT_S ARG_HOST_SHORT_S ARG_ADDRESS_SHORT_S ARG_HELP_SHORT_S ARG_CONFIG_SHORT_S \
-                        ARG_NAME_SHORT_S ARG_USER_SHORT_S
+                        ARG_NAME_SHORT_S ARG_USER_SHORT_S ARG_HEARTBEAT_INTERVAL_SHORT_S
 
 static const char *opt_port = 0;
 static const char *opt_host = NULL;
@@ -29,9 +30,37 @@ static const char *opt_address = NULL;
 static const char *opt_name = NULL;
 static const char *opt_config = NULL;
 static bool opt_user = false;
+static const char *opt_heartbeat_interval = 0;
 
 static void usage(char *argv[]) {
-        printf("Usage: %s [-H host] [-p port] [-a address] [-c config] [-n name] [--user]\n", argv[0]);
+        printf("Usage:\n"
+               "\t%s [options...] \n"
+               "Available options are:\n"
+               "\t-%c %s\t\t Print this help message.\n"
+               "\t-%c %s\t\t The host of hirte to connect to. Must be a valid IPv4 or IPv6.\n"
+               "\t-%c %s\t\t The port of hirte to connect to.\n"
+               "\t-%c %s\t The DBus address of hirte to connect to. Overrides host and port.\n"
+               "\t-%c %s\t\t The unique name of this hirte-agent used to register at hirte.\n"
+               "\t-%c %s\t The interval between two heartbeat signals in milliseconds.\n"
+               "\t-%c %s\t A path to a config file used to bootstrap hirte-agent.\n"
+               "\t-%c %s\t\t Connect to the user systemd instance instead of the system one.\n",
+               argv[0],
+               ARG_HELP_SHORT,
+               ARG_HELP,
+               ARG_HOST_SHORT,
+               ARG_HOST,
+               ARG_PORT_SHORT,
+               ARG_PORT,
+               ARG_ADDRESS_SHORT,
+               ARG_ADDRESS,
+               ARG_NAME_SHORT,
+               ARG_NAME,
+               ARG_HEARTBEAT_INTERVAL_SHORT,
+               ARG_HEARTBEAT_INTERVAL,
+               ARG_CONFIG_SHORT,
+               ARG_CONFIG,
+               ARG_USER_SHORT,
+               ARG_USER);
 }
 
 static int get_opts(int argc, char *argv[]) {
@@ -41,7 +70,7 @@ static int get_opts(int argc, char *argv[]) {
                 switch (opt) {
                 case ARG_HELP_SHORT:
                         usage(argv);
-                        return 0;
+                        return 1;
 
                 case ARG_NAME_SHORT:
                         opt_name = optarg;
@@ -55,6 +84,10 @@ static int get_opts(int argc, char *argv[]) {
                         opt_address = optarg;
                         break;
 
+                case ARG_HEARTBEAT_INTERVAL_SHORT:
+                        opt_heartbeat_interval = optarg;
+                        break;
+
                 case ARG_CONFIG_SHORT:
                         opt_config = optarg;
                         break;
@@ -81,6 +114,8 @@ int main(int argc, char *argv[]) {
         int r = get_opts(argc, argv);
         if (r < 0) {
                 return EXIT_FAILURE;
+        } else if (r > 0) {
+                return EXIT_SUCCESS;
         }
 
         _cleanup_agent_ Agent *agent = agent_new();
@@ -112,10 +147,14 @@ int main(int argc, char *argv[]) {
                 return EXIT_FAILURE;
         }
 
-        if (!agent_start(agent)) {
+        if (opt_heartbeat_interval && !agent_set_heartbeat_interval(agent, opt_heartbeat_interval)) {
                 return EXIT_FAILURE;
         }
 
+        if (agent_start(agent)) {
+                return EXIT_SUCCESS;
+        }
+
         agent_stop(agent);
         return EXIT_SUCCESS;
 }
diff --git a/src/libhirte/common/cfg.h b/src/libhirte/common/cfg.h
index 4d6d7327ac..b512067a3d 100644
--- a/src/libhirte/common/cfg.h
+++ b/src/libhirte/common/cfg.h
@@ -16,6 +16,7 @@
 #define CFG_MANAGER_ADDRESS "ManagerAddress"
 #define CFG_NODE_NAME "NodeName"
 #define CFG_ALLOWED_NODE_NAMES "AllowedNodeNames"
+#define CFG_HEARTBEAT_INTERVAL "HeartbeatInterval"
 
 /*
  * Global section - this is used, when configuration options are specified in the configuration file
diff --git a/src/libhirte/common/opt.h b/src/libhirte/common/opt.h
index 57f097e400..f73d9f044f 100644
--- a/src/libhirte/common/opt.h
+++ b/src/libhirte/common/opt.h
@@ -13,6 +13,10 @@
 #define ARG_ADDRESS_SHORT 'a'
 #define ARG_ADDRESS_SHORT_S "a:"
 
+#define ARG_HEARTBEAT_INTERVAL "interval"
+#define ARG_HEARTBEAT_INTERVAL_SHORT 'i'
+#define ARG_HEARTBEAT_INTERVAL_SHORT_S "i:"
+
 #define ARG_CONFIG "config"
 #define ARG_CONFIG_SHORT 'c'
 #define ARG_CONFIG_SHORT_S "c:"
diff --git a/src/libhirte/common/parse-util.c b/src/libhirte/common/parse-util.c
index fee3c6811a..53b5b5f5ab 100644
--- a/src/libhirte/common/parse-util.c
+++ b/src/libhirte/common/parse-util.c
@@ -15,11 +15,11 @@ bool parse_long(const char *in, long *ret) {
         }
 
         errno = 0;
-        *ret = strtol(in, &x, base);
-
+        long r = strtol(in, &x, base);
         if (errno > 0 || !x || x == in || *x != 0) {
                 return false;
         }
+        *ret = r;
 
         return true;
 }
diff --git a/src/libhirte/common/protocol.h b/src/libhirte/common/protocol.h
index 9d76d25264..fc3e17a31a 100644
--- a/src/libhirte/common/protocol.h
+++ b/src/libhirte/common/protocol.h
@@ -48,6 +48,7 @@
 #define SYSTEMD_UNIT_IFACE "org.freedesktop.systemd1.Unit"
 
 #define USEC_PER_SEC 1000000
+#define USEC_PER_MSEC 1000
 #define HIRTE_DEFAULT_DBUS_TIMEOUT ((uint64_t) (USEC_PER_SEC * 30))
 
 /* Typestrings */
@@ -88,6 +89,5 @@ UnitActiveState active_state_from_string(const char *s);
 /* Agent to Hirte heartbeat signals */
 
 // Application-level heartbeat set at 2 seconds.
-#define AGENT_HEARTBEAT_INTERVAL_USEC (2000000)
-
+#define AGENT_HEARTBEAT_INTERVAL_MSEC (2000)
 #define AGENT_HEARTBEAT_SIGNAL_NAME "Heartbeat"
diff --git a/src/manager/main.c b/src/manager/main.c
index b03bb7455e..f8a5d6382c 100644
--- a/src/manager/main.c
+++ b/src/manager/main.c
@@ -20,7 +20,19 @@ static const char *opt_port = 0;
 static const char *opt_config = NULL;
 
 static void usage(char *argv[]) {
-        printf("Usage: %s [-p port] [-c config]\n", argv[0]);
+        printf("Usage:\n"
+               "\t%s [options...] \n"
+               "Available options are:\n"
+               "\t-%c %s\t\t Print this help message.\n"
+               "\t-%c %s\t\t The port of hirte to connect to.\n"
+               "\t-%c %s\t A path to a config file used to bootstrap hirte-agent.\n",
+               argv[0],
+               ARG_HELP_SHORT,
+               ARG_HELP,
+               ARG_PORT_SHORT,
+               ARG_PORT,
+               ARG_CONFIG_SHORT,
+               ARG_CONFIG);
 }
 
 static int get_opts(int argc, char *argv[]) {
@@ -30,7 +42,7 @@ static int get_opts(int argc, char *argv[]) {
                 switch (opt) {
                 case ARG_HELP_SHORT:
                         usage(argv);
-                        return 0;
+                        return 1;
 
                 case ARG_PORT_SHORT:
                         opt_port = optarg;
@@ -55,6 +67,8 @@ int main(int argc, char *argv[]) {
         int r = get_opts(argc, argv);
         if (r < 0) {
                 return EXIT_FAILURE;
+        } else if (r > 0) {
+                return EXIT_SUCCESS;
         }
 
         _cleanup_manager_ Manager *manager = manager_new();
diff --git a/src/proxy/main.c b/src/proxy/main.c
index 32e20d70c6..ebe13df103 100644
--- a/src/proxy/main.c
+++ b/src/proxy/main.c
@@ -71,7 +71,7 @@ static int get_opts(int argc, char *argv[]) {
                 switch (opt) {
                 case ARG_HELP_SHORT:
                         usage(argv);
-                        return 0;
+                        return 1;
                 case ARG_USER_SHORT:
                         opt_user = true;
                         break;
@@ -143,6 +143,8 @@ int main(int argc, char *argv[]) {
         int r = get_opts(argc, argv);
         if (r < 0) {
                 return EXIT_FAILURE;
+        } else if (r > 0) {
+                return EXIT_SUCCESS;
         }
 
         _cleanup_free_ char *node_name = NULL;


Test patch:
diff --git a/src/libhirte/test/parse-util_test.c b/src/libhirte/test/parse-util_test.c
index a81ada0c15..b7afc4b67a 100644
--- a/src/libhirte/test/parse-util_test.c
+++ b/src/libhirte/test/parse-util_test.c
@@ -18,13 +18,13 @@ bool test_parse_long(const char *input, bool expectedReturn, long expectedResult
         int ret = parse_long(input, &res);
         if ((ret != expectedReturn) || (res != expectedResult)) {
                 ret = asprintf(&msg,
-                         "FAILED: %s\n\tInput: '%s'\n\tExpected: Return=%d, Result=%ld\n\tGot: Return=%d, Result=%ld\n",
-                         __FUNCTION_NAME__,
-                         input,
-                         expectedReturn,
-                         expectedResult,
-                         ret,
-                         res);
+                               "FAILED: %s\n\tInput: '%s'\n\tExpected: Return=%d, Result=%ld\n\tGot: Return=%d, Result=%ld\n",
+                               __FUNCTION_NAME__,
+                               input,
+                               expectedReturn,
+                               expectedResult,
+                               ret,
+                               res);
         }
 
         if (msg != NULL) {
@@ -41,13 +41,13 @@ bool test_parse_port(const char *input, bool expected_return, uint16_t expected_
         bool ret = parse_port(input, &res);
         if ((ret != expected_return) || (res != expected_result)) {
                 ret = asprintf(&msg,
-                         "FAILED: %s\n\tInput: '%s'\n\tExpected: Return=%d, Result=%d\n\tGot: Return=%d, Result=%d\n",
-                         __FUNCTION_NAME__,
-                         input,
-                         expected_return,
-                         expected_result,
-                         ret,
-                         res);
+                               "FAILED: %s\n\tInput: '%s'\n\tExpected: Return=%d, Result=%d\n\tGot: Return=%d, Result=%d\n",
+                               __FUNCTION_NAME__,
+                               input,
+                               expected_return,
+                               expected_result,
+                               ret,
+                               res);
         }
 
         if (msg != NULL) {
@@ -59,12 +59,15 @@ bool test_parse_port(const char *input, bool expected_return, uint16_t expected_
 
 int main() {
         bool result = true;
+
         result = result && test_parse_long(NULL, false, 0L);
         result = result && test_parse_long("", false, 0L);
         result = result && test_parse_long("foo", false, 0L);
         result = result && test_parse_long("1234", true, 1234L);
-        result = result && test_parse_long("1234abc", false, 1234L);
+        result = result && test_parse_long("1234abc", false, 0L);
         result = result && test_parse_long("abc1234", false, 0L);
+        result = result && test_parse_long("-1234", true, -1234L);
+        result = result && test_parse_long("12.34", false, 0L);
 
         result = result && test_parse_port(NULL, false, 0);
         result = result && test_parse_port("", false, 0);
@@ -74,6 +77,8 @@ int main() {
         result = result && test_parse_port("65535", true, 65535);
         result = result && test_parse_port("65536", false, 0);
         result = result && test_parse_port("-2", false, 0);
+        result = result && test_parse_port("65.536", false, 0);
+
 
         if (!result) {
                 return EXIT_FAILURE;