ostree-repo-pull: add options to configure retry behavior

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 these changes only apply when using libcurl. Finally this change adds a final option that affects all backends to control the max amount of connections of the fetcher "max-outstanding-fetcher-requests".
ostreedev · Oct 18, 2023 · 4fb6e6f · 4fb6e6f
1 parent befd844
commit 4fb6e6f
Show file tree

Hide file tree

Showing 9 changed files with 247 additions and 17 deletions.
diff --git a/man/ostree-pull.xml b/man/ostree-pull.xml
@@ -144,6 +144,42 @@ License along with this library. If not, see <https://www.gnu.org/licenses/>.
                 </para></listitem>
             </varlistentry>
 
+            <varlistentry>
+                <term><option>--disable-retry-on-network-errors</option></term>
+
+                <listitem><para>
+                    Do not retry when network issues happen, instead fail automatically. (Currently only affects libcurl)
+                </para></listitem>
+            </varlistentry>
+
+            <varlistentry>
+                <term><option>--low-speed-limit-bytes</option>=N</term>
+
+                <listitem><para>
+                    The average transfer speed per second of a transfer during the
+                    time set via 'low-speed-time-seconds' for libcurl to abort
+                    (default: 1000)
+                </para></listitem>
+            </varlistentry>
+
+            <varlistentry>
+                <term><option>--low-speed-time-seconds</option>=N</term>
+
+                <listitem><para>
+                    The time in number seconds that the transfer speed should be
+                    below the 'low-speed-limit-bytes' setting for libcurl to abort
+                    (default: 30)
+                </para></listitem>
+            </varlistentry>
+
+            <varlistentry>
+                <term><option>--max-outstanding-fetcher-requests</option>=N</term>
+
+                <listitem><para>
+                    The max amount of concurrent connections allowed. (default: 8)
+                </para></listitem>
+            </varlistentry>
+
             <varlistentry>
                 <term><option>--disable-verify-bindings</option></term>
 

diff --git a/src/libostree/ostree-fetcher-curl.c b/src/libostree/ostree-fetcher-curl.c
@@ -79,6 +79,10 @@ 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;
+  guint32 opt_max_outstanding_fetcher_requests;
 
   GMainContext *mainctx;
   CURLM *multi;
@@ -330,7 +334,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,
@@ -664,6 +674,31 @@ _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_max_outstanding_fetcher_requests (OstreeFetcher *self,
+                                                      guint32 opt_max_outstanding_fetcher_requests)
+{
+  self->opt_max_outstanding_fetcher_requests = opt_max_outstanding_fetcher_requests;
+}
+
 void
 _ostree_fetcher_set_cookie_jar (OstreeFetcher *self, const char *jar_path)
 {
@@ -912,14 +947,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);

diff --git a/src/libostree/ostree-fetcher-soup.c b/src/libostree/ostree-fetcher-soup.c
@@ -75,6 +75,8 @@ typedef struct
   /* Also protected by output_stream_set_lock. */
   guint64 total_downloaded;
 
+  guint32 opt_max_outstanding_fetcher_requests;
+
   GError *oob_error;
 
 } ThreadClosure;
@@ -360,6 +362,12 @@ session_thread_set_tls_database_cb (ThreadClosure *thread_closure, gpointer data
     }
 }
 
+static void
+session_thread_set_max_outstanding_fetcher_requests (ThreadClosure *thread_closure, gpointer data)
+{
+  thread_closure->opt_max_outstanding_fetcher_requests = GPOINTER_TO_UINT (data);
+}
+
 static void
 session_thread_set_extra_user_agent_cb (ThreadClosure *thread_closure, gpointer data)
 {
@@ -377,6 +385,33 @@ 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
+}
+
+void
+_ostree_fetcher_set_max_outstanding_fetcher_requests (OstreeFetcher *self,
+                                                      guint32 opt_max_outstanding_fetcher_requests)
+{
+  session_thread_idle_add (self->thread_closure,
+                           session_thread_set_max_outstanding_fetcher_requests,
+                           GUINT_TO_POINTER (opt_max_outstanding_fetcher_requests), NULL);
+}
+
 static void on_request_sent (GObject *object, GAsyncResult *result, gpointer user_data);
 
 static void
@@ -511,7 +546,7 @@ ostree_fetcher_session_thread (gpointer data)
    * by spreading requests across mirrors. */
   gint max_conns;
   g_object_get (closure->session, "max-conns-per-host", &max_conns, NULL);
-  if (max_conns < _OSTREE_MAX_OUTSTANDING_FETCHER_REQUESTS)
+  if (max_conns < closure->opt_max_outstanding_fetcher_requests)
     {
       /* We download a lot of small objects in ostree, so this
        * helps a lot.  Also matches what most modern browsers do.
@@ -522,7 +557,7 @@ ostree_fetcher_session_thread (gpointer data)
        * currently conservative #define SOUP_SESSION_MAX_CONNS_PER_HOST_DEFAULT 2 (as of
        * 2018-02-14).
        */
-      max_conns = _OSTREE_MAX_OUTSTANDING_FETCHER_REQUESTS;
+      max_conns = closure->opt_max_outstanding_fetcher_requests;
       g_object_set (closure->session, "max-conns-per-host", max_conns, NULL);
     }
 

diff --git a/src/libostree/ostree-fetcher-soup3.c b/src/libostree/ostree-fetcher-soup3.c
@@ -84,6 +84,7 @@ struct OstreeFetcher
   char *user_agent;
 
   guint64 bytes_transferred;
+  guint32 opt_max_outstanding_fetcher_requests;
 };
 
 enum
@@ -380,6 +381,31 @@ _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
+}
+
+void
+_ostree_fetcher_set_max_outstanding_fetcher_requests (OstreeFetcher *self,
+                                                      guint32 opt_max_outstanding_fetcher_requests)
+{
+  self->opt_max_outstanding_fetcher_requests = opt_max_outstanding_fetcher_requests;
+}
+
 static gboolean
 finish_stream (FetcherRequest *request, GCancellable *cancellable, GError **error)
 {
@@ -667,7 +693,7 @@ create_soup_session (OstreeFetcher *self)
   const char *user_agent = self->user_agent ?: OSTREE_FETCHER_USERAGENT_STRING;
   SoupSession *session = soup_session_new_with_options (
       "user-agent", user_agent, "timeout", 60, "idle-timeout", 60, "max-conns-per-host",
-      _OSTREE_MAX_OUTSTANDING_FETCHER_REQUESTS, NULL);
+      self->opt_max_outstanding_fetcher_requests, NULL);
 
   /* SoupContentDecoder is included in the session by default. Remove it
    * if gzip compression isn't in use.

diff --git a/src/libostree/ostree-fetcher.h b/src/libostree/ostree-fetcher.h
@@ -99,6 +99,16 @@ 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_max_outstanding_fetcher_requests (OstreeFetcher *self,
+                                                      guint32 opt_max_outstanding_fetcher_requests);
+
 void _ostree_fetcher_set_tls_database (OstreeFetcher *self, const char *tlsdb_path);
 
 void _ostree_fetcher_set_extra_headers (OstreeFetcher *self, GVariant *extra_headers);

diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h
@@ -33,7 +33,6 @@ G_BEGIN_DECLS
 #define _OSTREE_SUMMARY_CACHE_DIR "summaries"
 #define _OSTREE_CACHE_DIR "cache"
 
-#define _OSTREE_MAX_OUTSTANDING_FETCHER_REQUESTS 8
 #define _OSTREE_MAX_OUTSTANDING_DELTAPART_REQUESTS 2
 
 /* We want some parallelism with disk writes, but we also

diff --git a/src/libostree/ostree-repo-pull-private.h b/src/libostree/ostree-repo-pull-private.h
@@ -54,6 +54,10 @@ typedef struct
 
   GVariant *extra_headers;
   char *append_user_agent;
+  guint32 low_speed_limit;
+  guint32 low_speed_time;
+  gboolean retry_all;
+  guint32 max_outstanding_fetcher_requests;
 
   gboolean dry_run;
   gboolean dry_run_emitted_progress;