From: Alberto Leiva Popper Date: Thu, 17 Oct 2024 16:44:23 +0000 (-0600) Subject: Remove rsync & HTTP retries X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a3ad8d6757f5e3587b6207a8b0de32a7ca30fe2a;p=thirdparty%2FFORT-validator.git Remove rsync & HTTP retries Glad to ditch these. They'll be made redundant by the new threading. --- diff --git a/docs/usage.md b/docs/usage.md index ab1edd38..1846878e 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -46,8 +46,6 @@ description: Guide to use arguments of FORT Validator. 33. [`--validation-log.tag`](#--validation-logtag) 34. [`--http.enabled`](#--httpenabled) 35. [`--http.priority`](#--httppriority) - 36. [`--http.retry.count`](#--httpretrycount) - 37. [`--http.retry.interval`](#--httpretryinterval) 38. [`--http.user-agent`](#--httpuser-agent) 38. [`--http.max-redirs`](#--httpmax-redirs) 39. [`--http.connect-timeout`](#--httpconnect-timeout) @@ -63,8 +61,6 @@ description: Guide to use arguments of FORT Validator. 48. [`--thread-pool.server.max`](#--thread-poolservermax) 50. [`--rsync.enabled`](#--rsyncenabled) 51. [`--rsync.priority`](#--rsyncpriority) - 53. [`--rsync.retry.count`](#--rsyncretrycount) - 54. [`--rsync.retry.interval`](#--rsyncretryinterval) 40. [`--rsync.transfer-timeout`](#--rsynctransfer-timeout) 55. [`--configuration-file`](#--configuration-file) 56. [`rsync.program`](#rsyncprogram) @@ -102,13 +98,9 @@ description: Guide to use arguments of FORT Validator. [--server.deltas.lifetime=] [--rsync.enabled=true|false] [--rsync.priority=] - [--rsync.retry.count=] - [--rsync.retry.interval=] [--rsync.transfer-timeout=] [--http.enabled=true|false] [--http.priority=] - [--http.retry.count=] - [--http.retry.interval=] [--http.user-agent=] [--http.max-redirs=] [--http.connect-timeout=] @@ -646,26 +638,6 @@ HTTP's (and therefore RRDP's) precedence when choosing the protocol used to down See [`--rsync.priority`](#--rsyncpriority). -### `--http.retry.count` - -- **Type:** Integer -- **Availability:** `argv` and JSON -- **Default:** 1 -- **Range:** [0, [`UINT_MAX`](http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html)] - -Number of additional HTTP requests after a failed attempt. - -If a transient error is returned when Fort tries to perform an HTTP transfer, it will retry this number of times before giving up. Setting the number to 0 makes Fort do no retries (which is the default). "Transient error" is a timeout, an HTTP 408 response code, or an HTTP 5xx response code. - -### `--http.retry.interval` - -- **Type:** Integer -- **Availability:** `argv` and JSON -- **Default:** 4 -- **Range:** [0, [`UINT_MAX`](http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html)] - -Period of time (in seconds) to wait between each retry to request an HTTP URI. - ### `--http.user-agent` - **Type:** String @@ -916,28 +888,6 @@ RSYNC's precedence when choosing the protocol used to download files (assuming F See [`--http.priority`](#--httppriority). -### `--rsync.retry.count` - -- **Type:** Integer -- **Availability:** `argv` and JSON -- **Default:** 1 -- **Range:** [0, [`UINT_MAX`](http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html)] - -Maximum number of retries whenever there's an error executing an RSYNC. - -A value of **0** means **no retries**. - -Whenever is necessary to execute an RSYNC, the validator will try at least one time the execution. If there was an error executing the RSYNC, the validator will retry it at most `--rsync.retry.count` times, waiting [`--rsync.retry.interval`](#--rsyncretryinterval) seconds between each retry. - -### `--rsync.retry.interval` - -- **Type:** Integer -- **Availability:** `argv` and JSON -- **Default:** 4 -- **Range:** [0, [`UINT_MAX`](http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html)] - -Period of time (in seconds) to wait between each retry to execute an RSYNC. - ### `--rsync.transfer-timeout` - **Type:** Integer @@ -989,10 +939,6 @@ The configuration options are mostly the same as the ones from the `argv` interf "rsync": { "enabled": true, "priority": 50, - "retry": { - "count": 1, - "interval": 4 - }, "transfer-timeout": 0, "program": "rsync", "arguments-recursive": [ @@ -1017,10 +963,6 @@ The configuration options are mostly the same as the ones from the `argv` interf "http": { "enabled": true, "priority": 60, - "retry": { - "count": 1, - "interval": 4 - }, "user-agent": "fort/1.6.2", "max-redirs": 10, "connect-timeout": 30, diff --git a/src/config.c b/src/config.c index 2957c675..e49d4699 100644 --- a/src/config.c +++ b/src/config.c @@ -90,13 +90,6 @@ struct rpki_config { unsigned int priority; /* Deprecated; does nothing. */ char *strategy; - /* Retry conf, utilized on errors */ - struct { - /* Maximum number of retries on error */ - unsigned int count; - /* Interval (in seconds) between each retry */ - unsigned int interval; - } retry; unsigned int transfer_timeout; char *program; struct string_array args; @@ -107,13 +100,6 @@ struct rpki_config { bool enabled; /* Protocol preference; compared to rsync.priority */ unsigned int priority; - /* Retry conf, utilized on errors */ - struct { - /* Maximum number of retries on error */ - unsigned int count; - /* Interval (in seconds) between each retry */ - unsigned int interval; - } retry; /* HTTP User-Agent request header */ char *user_agent; /* Allowed redirects per request */ @@ -478,22 +464,6 @@ static const struct option_field options[] = { .doc = "Deprecated; does nothing.", .json_null_allowed = true, .deprecated = true, - }, { - .id = 3003, - .name = "rsync.retry.count", - .type = >_uint, - .offset = offsetof(struct rpki_config, rsync.retry.count), - .doc = "Maximum amount of retries whenever there's an RSYNC error", - .min = 0, - .max = UINT_MAX, - }, { - .id = 3004, - .name = "rsync.retry.interval", - .type = >_uint, - .offset = offsetof(struct rpki_config, rsync.retry.interval), - .doc = "Period (in seconds) to wait between retries after an RSYNC error ocurred", - .min = 0, - .max = UINT_MAX, }, { .id = 3005, .name = "rsync.program", @@ -545,22 +515,6 @@ static const struct option_field options[] = { .min = 0, .max = 100, /* XXX deprecated? */ - }, { - .id = 9002, - .name = "http.retry.count", - .type = >_uint, - .offset = offsetof(struct rpki_config, http.retry.count), - .doc = "Maximum amount of retries whenever there's an error requesting HTTP URIs", - .min = 0, - .max = UINT_MAX, - }, { - .id = 9003, - .name = "http.retry.interval", - .type = >_uint, - .offset = offsetof(struct rpki_config, http.retry.interval), - .doc = "Period (in seconds) to wait between retries after an error ocurred doing HTTP requests", - .min = 0, - .max = UINT_MAX, }, { .id = 9004, .name = "http.user-agent", @@ -999,8 +953,6 @@ set_default_values(void) rpki_config.rsync.enabled = true; rpki_config.rsync.priority = 50; rpki_config.rsync.strategy = pstrdup(""); - rpki_config.rsync.retry.count = 1; - rpki_config.rsync.retry.interval = 4; rpki_config.rsync.transfer_timeout = 900; rpki_config.rsync.program = pstrdup("rsync"); string_array_init(&rpki_config.rsync.args, trash, ARRAY_LEN(trash)); @@ -1008,8 +960,6 @@ set_default_values(void) rpki_config.http.enabled = true; /* Higher priority than rsync by default */ rpki_config.http.priority = 60; - rpki_config.http.retry.count = 1; - rpki_config.http.retry.interval = 4; rpki_config.http.user_agent = pstrdup(PACKAGE_NAME "/" PACKAGE_VERSION); rpki_config.http.max_redirs = 10; rpki_config.http.connect_timeout = 30; @@ -1429,18 +1379,6 @@ config_get_rsync_priority(void) return rpki_config.rsync.priority; } -unsigned int -config_get_rsync_retry_count(void) -{ - return rpki_config.rsync.retry.count; -} - -unsigned int -config_get_rsync_retry_interval(void) -{ - return rpki_config.rsync.retry.interval; -} - long config_get_rsync_transfer_timeout(void) { @@ -1465,18 +1403,6 @@ config_get_http_priority(void) return rpki_config.http.priority; } -unsigned int -config_get_http_retry_count(void) -{ - return rpki_config.http.retry.count; -} - -unsigned int -config_get_http_retry_interval(void) -{ - return rpki_config.http.retry.interval; -} - char const * config_get_http_user_agent(void) { diff --git a/src/config.h b/src/config.h index a67c9a3c..8a959408 100644 --- a/src/config.h +++ b/src/config.h @@ -45,14 +45,10 @@ char const *config_get_http_ca_path(void); unsigned int config_get_rrdp_delta_threshold(void); bool config_get_rsync_enabled(void); unsigned int config_get_rsync_priority(void); -unsigned int config_get_rsync_retry_count(void); -unsigned int config_get_rsync_retry_interval(void); long config_get_rsync_transfer_timeout(void); char const *config_get_rsync_program(void); bool config_get_http_enabled(void); unsigned int config_get_http_priority(void); -unsigned int config_get_http_retry_count(void); -unsigned int config_get_http_retry_interval(void); char const *config_get_output_roa(void); char const *config_get_output_bgpsec(void); enum output_format config_get_output_format(void); diff --git a/src/http.c b/src/http.c index 9b716929..1f22d0da 100644 --- a/src/http.c +++ b/src/http.c @@ -246,10 +246,17 @@ handle_http_response_code(long http_code) } /* - * Fetch data from @src and write result on @dst. + * Download @src into @dst; HTTP assumed. + * + * If @changed returns true, the file was downloaded normally. + * If @changed returns false, the file has not been modified since @ims. + * + * @ims can be 0, which means "no epoch." + * @changed can be NULL, which means "I don't care." + * If @changed is not NULL, initialize it to false. */ -static int -http_fetch(char const *src, char const *dst, curl_off_t ims, bool *changed) +int +http_download(char const *src, char const *dst, curl_off_t ims, bool *changed) { struct http_handler handler; struct write_callback_arg args; @@ -259,6 +266,8 @@ http_fetch(char const *src, char const *dst, curl_off_t ims, bool *changed) unsigned int r; int error; + pr_val_info("HTTP GET: %s -> %s", src, dst); + error = http_easy_init(&handler, ims); if (error) return error; @@ -369,64 +378,3 @@ end: http_easy_cleanup(&handler); free(redirect); return error; } - -/* - * Download @url into @path; HTTP assumed. - * - * If @changed returns true, the file was downloaded normally. - * If @changed returns false, the file has not been modified since @ims. - * - * @ims can be 0, which means "no epoch." - * @changed can be NULL, which means "I don't care." - * If @changed is not NULL, initialize it to false. - */ -int -http_download(char const *url, char const *path, curl_off_t ims, bool *changed) -{ - unsigned int r; - int error; - - pr_val_info("HTTP GET: %s -> %s", url, path); - - for (r = 0; true; r++) { - pr_val_debug("Download attempt #%u...", r + 1); - - error = http_fetch(url, path, ims, changed); - switch (error) { - case 0: - pr_val_debug("Download successful."); - return 0; /* Happy path */ - - case EAGAIN: - break; - - default: - pr_val_debug("Download failed."); - return error; - } - - if (r >= config_get_http_retry_count()) { - pr_val_debug("Download failed: Retries exhausted."); - return EIO; - } - - pr_val_warn("Download failed; retrying in %u seconds.", - config_get_http_retry_interval()); - /* - * TODO (fine) Wrong. This is slowing the entire tree traversal - * down; use a thread pool. - */ - sleep(config_get_http_retry_interval()); - } -} - -/* - * Downloads @remote to the absolute path @dest (no workspace nor directory - * structure is created). - */ -int -http_download_direct(char const *src, char const *dst) -{ - pr_val_info("HTTP GET: %s -> %s", src, dst); - return http_fetch(src, dst, 0, NULL); -} diff --git a/src/http.h b/src/http.h index ccab132f..22113740 100644 --- a/src/http.h +++ b/src/http.h @@ -8,6 +8,5 @@ int http_init(void); void http_cleanup(void); int http_download(char const *, char const *, curl_off_t, bool *); -int http_download_direct(char const *, char const *); #endif /* SRC_HTTP_H_ */ diff --git a/src/init.c b/src/init.c index 9839152b..8a3b0fcc 100644 --- a/src/init.c +++ b/src/init.c @@ -12,7 +12,7 @@ fetch_url(char const *url, char const *filename) path = path_join(config_get_tal(), filename); - error = http_download_direct(url, path); + error = http_download(url, path, 0, NULL); if (error) { fprintf(stderr, "Couldn't fetch '%s': %s\n", path, strerror(abs(error))); diff --git a/src/rsync.c b/src/rsync.c index 76e79ec9..72da3117 100644 --- a/src/rsync.c +++ b/src/rsync.c @@ -280,9 +280,8 @@ rsync_download(char const *url, char const *path) /* Descriptors to pipe stderr (first element) and stdout (second) */ int fork_fds[2][2]; pid_t child_pid; - unsigned int retries; unsigned int i; - int child_status; + int child; int error; /* Prepare everything for the child exec */ @@ -295,105 +294,64 @@ rsync_download(char const *url, char const *path) pr_val_debug(" %s", args[i]); } - retries = 0; - do { - child_status = 0; - - error = create_pipes(fork_fds); - if (error) - return error; - - /* Flush output (avoid locks between father and child) */ - log_flush(); - - /* We need to fork because execvp() magics the thread away. */ - child_pid = fork(); - if (child_pid == 0) { - /* - * This code is run by the child, and should try to - * call execvp() as soon as possible. - * - * Refer to - * https://pubs.opengroup.org/onlinepubs/9699919799/functions/fork.html - * "{..} to avoid errors, the child process may only - * execute async-signal-safe operations until such time - * as one of the exec functions is called." - */ - handle_child_thread(args, fork_fds); - } - if (child_pid < 0) { - error = errno; - pr_op_err_st("Couldn't fork to execute rsync: %s", - strerror(error)); - /* Close all ends from the created pipes */ - close(STDERR_READ(fork_fds)); - close(STDOUT_READ(fork_fds)); - close(STDERR_WRITE(fork_fds)); - close(STDOUT_WRITE(fork_fds)); - return error; - } + error = create_pipes(fork_fds); + if (error) + return error; + + /* Flush output (avoid locks between father and child) */ + log_flush(); + + /* We need to fork because execvp() magics the thread away. */ + child_pid = fork(); + if (child_pid == 0) { + /* + * This code is run by the child, and should try to + * call execvp() as soon as possible. + * + * Refer to + * https://pubs.opengroup.org/onlinepubs/9699919799/functions/fork.html + * "{..} to avoid errors, the child process may only + * execute async-signal-safe operations until such time + * as one of the exec functions is called." + */ + handle_child_thread(args, fork_fds); + } + if (child_pid < 0) { + error = errno; + pr_op_err_st("Couldn't fork to execute rsync: %s", + strerror(error)); + /* Close all ends from the created pipes */ + close(STDERR_READ(fork_fds)); + close(STDOUT_READ(fork_fds)); + close(STDERR_WRITE(fork_fds)); + close(STDOUT_WRITE(fork_fds)); + return error; + } - /* This code is run by us. */ - error = exhaust_pipes(fork_fds); - if (error) - kill(child_pid, SIGTERM); /* Stop the child */ - - error = waitpid(child_pid, &child_status, 0); - do { - if (error == -1) { - error = errno; - pr_op_err_st("The rsync sub-process returned error %d (%s)", - error, strerror(error)); - if (child_status > 0) - break; - return error; - } - } while (0); - - if (WIFEXITED(child_status)) { - /* Happy path (but also sad path sometimes). */ - error = WEXITSTATUS(child_status); - pr_val_debug("The rsync sub-process terminated with error code %d.", - error); - if (!error) - return 0; - - if (retries == config_get_rsync_retry_count()) { - if (retries > 0) - pr_val_warn("Max RSYNC retries (%u) reached on '%s', won't retry again.", - retries, url); - return EIO; - } - pr_val_warn("Retrying RSYNC '%s' in %u seconds, %u attempts remaining.", - url, - config_get_rsync_retry_interval(), - config_get_rsync_retry_count() - retries); - retries++; - sleep(config_get_rsync_retry_interval()); - continue; - } - break; - } while (true); - - if (WIFSIGNALED(child_status)) { - switch (WTERMSIG(child_status)) { - case SIGINT: - pr_op_err_st("RSYNC was user-interrupted. Guess I'll interrupt myself too."); - break; - case SIGQUIT: - pr_op_err_st("RSYNC received a quit signal. Guess I'll quit as well."); - break; - case SIGKILL: - pr_op_err_st("Killed."); - break; - default: - pr_op_err_st("The RSYNC was terminated by a signal [%d] I don't have a handler for. Dunno; guess I'll just die.", - WTERMSIG(child_status)); - break; - } - return -EINTR; /* Meh? */ + /* This code is run by us. */ + error = exhaust_pipes(fork_fds); + if (error) + kill(child_pid, SIGTERM); /* Stop the child */ + + child = 0; + if (waitpid(child_pid, &child, 0) < 0) { + error = errno; + pr_op_err("Could not wait for rsync: %s", strerror(error)); + return error; + } + + if (WIFEXITED(child)) { + /* Happy path (but also sad path sometimes) */ + error = WEXITSTATUS(child); + pr_val_debug("rsync ended. Result: %d", error); + return error ? EIO : 0; + } + + if (WIFSIGNALED(child)) { + pr_op_warn("rsync interrupted by signal %d.", WTERMSIG(child)); + return EINTR; /* Meh? */ } - pr_op_err_st("The RSYNC command died in a way I don't have a handler for. Dunno; guess I'll die as well."); + pr_op_err_st("rsync died in an unknown way."); return -EINVAL; } diff --git a/test/rsync_test.c b/test/rsync_test.c index 83c75717..e6053386 100644 --- a/test/rsync_test.c +++ b/test/rsync_test.c @@ -14,8 +14,6 @@ static char content[1024]; /* Mocks */ MOCK(config_get_rsync_program, char const *, "rsync", void) -MOCK_UINT(config_get_rsync_retry_count, 0, void) -MOCK_UINT(config_get_rsync_retry_interval, 10, void) MOCK(config_get_rsync_transfer_timeout, long, 4, void) void