Skip to content

Commit

Permalink
ostree-repo-pull: add options to configure retry behavior
Browse files Browse the repository at this point in the history
This introduces the retry-all-network-errors option which
is enabled by default. This is a behavior change as now
ostree will retry on requests that fail except when
they fail with NOT_FOUND. It also introduces the options
low-speed-limit-bytes and low-speed-time-seconds these
map to CURL options only at the moment. Which have defaults
set following librepo:
https://github.com/rpm-software-management/librepo/blob/7c9af219abd49f8961542b7622fc82cfdaa572e3/librepo/handle.h#L90
https://github.com/rpm-software-management/librepo/blob/7c9af219abd49f8961542b7622fc82cfdaa572e3/librepo/handle.h#L96
Currently this changes only apply when using libcurl.
  • Loading branch information
jmarrero committed Oct 10, 2023
1 parent 5fe050f commit 8044abb
Show file tree
Hide file tree
Showing 6 changed files with 119 additions and 13 deletions.
41 changes: 32 additions & 9 deletions src/libostree/ostree-fetcher-curl.c
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ struct OstreeFetcher
int tmpdir_dfd;
bool force_anonymous;
char *custom_user_agent;
guint32 opt_low_speed_limit;
guint32 opt_low_speed_time;
gboolean opt_retry_all;

GMainContext *mainctx;
CURLM *multi;
Expand Down Expand Up @@ -330,7 +333,13 @@ check_multi_info (OstreeFetcher *fetcher)
}
else
{
g_task_return_new_error (task, G_IO_ERROR, G_IO_ERROR_FAILED,
/* When it is not a file, we want to retry the request.
* We accomplish that by using G_IO_ERROR_TIMED_OUT.
*/
gboolean opt_retry_all = req->fetcher->opt_retry_all;
int g_io_error_code
= (is_file || !opt_retry_all) ? G_IO_ERROR_FAILED : G_IO_ERROR_TIMED_OUT;
g_task_return_new_error (task, G_IO_ERROR, g_io_error_code,
"While fetching %s: [%u] %s", eff_url, curlres,
curl_easy_strerror (curlres));
_ostree_fetcher_journal_failure (req->fetcher->remote_name, eff_url,
Expand Down Expand Up @@ -664,6 +673,24 @@ _ostree_fetcher_set_proxy (OstreeFetcher *self, const char *http_proxy)
self->proxy = g_strdup (http_proxy);
}

void
_ostree_fetcher_set_low_speed_time (OstreeFetcher *self, guint32 opt_low_speed_time)
{
self->opt_low_speed_time = opt_low_speed_time;
}

void
_ostree_fetcher_set_low_speed_limit (OstreeFetcher *self, guint32 opt_low_speed_limit)
{
self->opt_low_speed_limit = opt_low_speed_limit;
}

void
_ostree_fetcher_set_retry_all (OstreeFetcher *self, gboolean opt_retry_all)
{
self->opt_retry_all = opt_retry_all;
}

void
_ostree_fetcher_set_cookie_jar (OstreeFetcher *self, const char *jar_path)
{
Expand Down Expand Up @@ -912,14 +939,10 @@ initiate_next_curl_request (FetcherRequest *req, GTask *task)
g_assert_cmpint (rc, ==, CURLM_OK);
rc = curl_easy_setopt (req->easy, CURLOPT_CONNECTTIMEOUT, 30L);
g_assert_cmpint (rc, ==, CURLM_OK);
/* We used to set CURLOPT_LOW_SPEED_LIMIT and CURLOPT_LOW_SPEED_TIME
* here, but see https://github.com/ostreedev/ostree/issues/878#issuecomment-347228854
* basically those options don't play well with HTTP2 at the moment
* where we can have lots of outstanding requests. Further,
* we could implement that functionality at a higher level
* more consistently too.
*/

rc = curl_easy_setopt (req->easy, CURLOPT_LOW_SPEED_LIMIT, req->fetcher->opt_low_speed_limit);
g_assert_cmpint (rc, ==, CURLM_OK);
rc = curl_easy_setopt (req->easy, CURLOPT_LOW_SPEED_TIME, req->fetcher->opt_low_speed_time);
g_assert_cmpint (rc, ==, CURLM_OK);
/* closure bindings -> task */
rc = curl_easy_setopt (req->easy, CURLOPT_PRIVATE, task);
g_assert_cmpint (rc, ==, CURLM_OK);
Expand Down
18 changes: 18 additions & 0 deletions src/libostree/ostree-fetcher-soup.c
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,24 @@ session_thread_set_extra_user_agent_cb (ThreadClosure *thread_closure, gpointer
}
}

void
_ostree_fetcher_set_low_speed_time (OstreeFetcher *self, guint32 opt_low_speed_time)
{
// TODO
}

void
_ostree_fetcher_set_low_speed_limit (OstreeFetcher *self, guint32 opt_low_speed_limit)
{
// TODO
}

void
_ostree_fetcher_set_retry_all (OstreeFetcher *self, gboolean opt_retry_all)
{
// TODO
}

static void on_request_sent (GObject *object, GAsyncResult *result, gpointer user_data);

static void
Expand Down
18 changes: 18 additions & 0 deletions src/libostree/ostree-fetcher-soup3.c
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,24 @@ _ostree_fetcher_set_extra_user_agent (OstreeFetcher *self, const char *extra_use
self->user_agent = g_strdup_printf ("%s %s", OSTREE_FETCHER_USERAGENT_STRING, extra_user_agent);
}

void
_ostree_fetcher_set_low_speed_time (OstreeFetcher *self, guint32 opt_low_speed_time)
{
// TODO
}

void
_ostree_fetcher_set_low_speed_limit (OstreeFetcher *self, guint32 opt_low_speed_limit)
{
// TODO
}

void
_ostree_fetcher_set_retry_all (OstreeFetcher *self, gboolean opt_retry_all)
{
// TODO
}

static gboolean
finish_stream (FetcherRequest *request, GCancellable *cancellable, GError **error)
{
Expand Down
6 changes: 6 additions & 0 deletions src/libostree/ostree-fetcher.h
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,12 @@ void _ostree_fetcher_set_proxy (OstreeFetcher *fetcher, const char *proxy);
void _ostree_fetcher_set_client_cert (OstreeFetcher *fetcher, const char *cert_path,
const char *key_path);

void _ostree_fetcher_set_low_speed_limit (OstreeFetcher *self, guint32 opt_low_speed_limit);

void _ostree_fetcher_set_low_speed_time (OstreeFetcher *self, guint32 opt_low_speed_time);

void _ostree_fetcher_set_retry_all (OstreeFetcher *self, gboolean opt_retry_all);

void _ostree_fetcher_set_tls_database (OstreeFetcher *self, const char *tlsdb_path);

void _ostree_fetcher_set_extra_headers (OstreeFetcher *self, GVariant *extra_headers);
Expand Down
3 changes: 3 additions & 0 deletions src/libostree/ostree-repo-pull-private.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ typedef struct

GVariant *extra_headers;
char *append_user_agent;
guint32 low_speed_limit;
guint32 low_speed_time;
gboolean retry_all;

gboolean dry_run;
gboolean dry_run_emitted_progress;
Expand Down
46 changes: 42 additions & 4 deletions src/libostree/ostree-repo-pull.c
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@
* _ostree_fetcher_should_retry_request(). This is the default value for the
* `n-network-retries` pull option. */
#define DEFAULT_N_NETWORK_RETRIES 5
#define OPT_LOWSPEEDLIMIT_DEFAULT 1000
#define OPT_LOWSPEEDTIME_DEFAULT 30
#define OPT_RETRYALL_DEFAULT TRUE

typedef struct
{
Expand Down Expand Up @@ -2928,7 +2931,9 @@ _ostree_repo_cache_summary (OstreeRepo *self, const char *remote, GBytes *summar
static OstreeFetcher *
_ostree_repo_remote_new_fetcher (OstreeRepo *self, const char *remote_name, gboolean gzip,
GVariant *extra_headers, const char *append_user_agent,
OstreeFetcherSecurityState *out_state, GError **error)
guint32 low_speed_limit, guint32 low_speed_time,
gboolean retry_all, OstreeFetcherSecurityState *out_state,
GError **error)
{
OstreeFetcher *fetcher = NULL;
OstreeFetcherConfigFlags fetcher_flags = 0;
Expand Down Expand Up @@ -2995,6 +3000,10 @@ _ostree_repo_remote_new_fetcher (OstreeRepo *self, const char *remote_name, gboo
}
}

_ostree_fetcher_set_low_speed_limit (fetcher, low_speed_limit);
_ostree_fetcher_set_low_speed_time (fetcher, low_speed_time);
_ostree_fetcher_set_retry_all (fetcher, retry_all);

{
g_autofree char *tls_ca_path = NULL;

Expand Down Expand Up @@ -3221,6 +3230,7 @@ reinitialize_fetcher (OtPullData *pull_data, const char *remote_name, GError **e
g_clear_object (&pull_data->fetcher);
pull_data->fetcher = _ostree_repo_remote_new_fetcher (
pull_data->repo, remote_name, FALSE, pull_data->extra_headers, pull_data->append_user_agent,
pull_data->low_speed_limit, pull_data->low_speed_time, pull_data->retry_all,
&pull_data->fetcher_security_state, error);
if (pull_data->fetcher == NULL)
return FALSE;
Expand Down Expand Up @@ -3450,6 +3460,12 @@ all_requested_refs_have_commit (
* * `n-network-retries` (`u`): Number of times to retry each download on receiving
* a transient network error, such as a socket timeout; default is 5, 0
* means return errors without retrying. Since: 2018.6
* * `low-speed-limit-bytes` (`u`): The average transfer speed per second of a transfer
during the time set via "low-speed-time-seconds" for libcurl to abort.
* * `low-speed-time-seconds` (`u`): The time in number seconds that the transfer
speed should be below the "low-speed-limit-bytes" setting for libcurl to abort.
* * `retry-all-network-errors` (`b`): Retry when network issues happen, instead of
* failing automatically. Currently only affects libcurl. (Default set to true)
* * `ref-keyring-map` (`a(sss)`): Array of (collection ID, ref name, keyring
* remote name) tuples specifying which remote's keyring should be used when
* doing GPG verification of each collection-ref. This is useful to prevent a
Expand Down Expand Up @@ -3506,6 +3522,9 @@ ostree_repo_pull_with_options (OstreeRepo *self, const char *remote_name_or_base
gboolean opt_gpg_verify_summary_set = FALSE;
gboolean opt_collection_refs_set = FALSE;
gboolean opt_n_network_retries_set = FALSE;
gboolean opt_low_speed_limit_set = FALSE;
gboolean opt_low_speed_time_set = FALSE;
gboolean opt_retry_all_set = FALSE;
gboolean opt_ref_keyring_map_set = FALSE;
gboolean disable_sign_verify = FALSE;
gboolean disable_sign_verify_summary = FALSE;
Expand Down Expand Up @@ -3570,6 +3589,12 @@ ostree_repo_pull_with_options (OstreeRepo *self, const char *remote_name_or_base
&pull_data->timestamp_check_from_rev);
(void)g_variant_lookup (options, "max-metadata-size", "t", &pull_data->max_metadata_size);
(void)g_variant_lookup (options, "append-user-agent", "s", &pull_data->append_user_agent);
opt_low_speed_limit_set
= g_variant_lookup (options, "low-speed-limit-bytes", "u", &pull_data->low_speed_limit);
opt_low_speed_time_set
= g_variant_lookup (options, "low-speed-time-seconds", "u", &pull_data->low_speed_time);
opt_retry_all_set
= g_variant_lookup (options, "retry-all-network-errors", "b", &pull_data->retry_all);
opt_n_network_retries_set
= g_variant_lookup (options, "n-network-retries", "u", &pull_data->n_network_retries);
opt_ref_keyring_map_set
Expand Down Expand Up @@ -3644,6 +3669,12 @@ ostree_repo_pull_with_options (OstreeRepo *self, const char *remote_name_or_base

if (!opt_n_network_retries_set)
pull_data->n_network_retries = DEFAULT_N_NETWORK_RETRIES;
if (!opt_low_speed_limit_set)
pull_data->low_speed_limit = OPT_LOWSPEEDLIMIT_DEFAULT;
if (!opt_low_speed_time_set)
pull_data->low_speed_time = OPT_LOWSPEEDTIME_DEFAULT;
if (!opt_retry_all_set)
pull_data->retry_all = OPT_RETRYALL_DEFAULT;

pull_data->repo = self;
pull_data->progress = progress;
Expand Down Expand Up @@ -5693,7 +5724,7 @@ find_remotes_cb (GObject *obj, GAsyncResult *async_result, gpointer user_data)
goto error;

fetcher = _ostree_repo_remote_new_fetcher (self, result->remote->name, TRUE, NULL,
NULL, NULL, &error);
NULL, 0, 0, TRUE, NULL, &error);
if (fetcher == NULL)
goto error;

Expand Down Expand Up @@ -6307,6 +6338,9 @@ ostree_repo_remote_fetch_summary_with_options (OstreeRepo *self, const char *nam
g_autoptr (GVariant) extra_headers = NULL;
g_autoptr (GPtrArray) mirrorlist = NULL;
const char *append_user_agent = NULL;
guint32 low_speed_limit = OPT_LOWSPEEDLIMIT_DEFAULT;
guint32 low_speed_time = OPT_LOWSPEEDTIME_DEFAULT;
gboolean retry_all = OPT_RETRYALL_DEFAULT;
guint n_network_retries = DEFAULT_N_NETWORK_RETRIES;
gboolean summary_sig_not_modified = FALSE;
g_autofree char *summary_sig_if_none_match = NULL;
Expand All @@ -6331,6 +6365,9 @@ ostree_repo_remote_fetch_summary_with_options (OstreeRepo *self, const char *nam
(void)g_variant_lookup (options, "http-headers", "@a(ss)", &extra_headers);
(void)g_variant_lookup (options, "append-user-agent", "&s", &append_user_agent);
(void)g_variant_lookup (options, "n-network-retries", "u", &n_network_retries);
(void)g_variant_lookup (options, "low-speed-limit-bytes", "u", &low_speed_limit);
(void)g_variant_lookup (options, "low-speed-time-seconds", "u", &low_speed_time);
(void)g_variant_lookup (options, "retry-all-network-errors", "b", &retry_all);
}

if (!ostree_repo_remote_get_gpg_verify_summary (self, name, &gpg_verify_summary, error))
Expand All @@ -6342,8 +6379,9 @@ ostree_repo_remote_fetch_summary_with_options (OstreeRepo *self, const char *nam
mainctx = _ostree_main_context_new_default ();
(void)mainctx; // Used for autocleanup

fetcher = _ostree_repo_remote_new_fetcher (self, name, TRUE, extra_headers, append_user_agent,
NULL, error);
fetcher
= _ostree_repo_remote_new_fetcher (self, name, TRUE, extra_headers, append_user_agent,
low_speed_limit, low_speed_time, retry_all, NULL, error);
if (fetcher == NULL)
return FALSE;

Expand Down

0 comments on commit 8044abb

Please sign in to comment.