Libguestfs - Sep 2019 - [PATCH nbdkit v3 3/3] retry: Add a test of this filter.

v2 was here:
https://www.redhat.com/archives/libguestfs/2019-September/msg00221.html

I think this is more like "the one".  It handles reopen failing
correctly, and there is a second test for that.  I also ran my sshd
tests locally and it worked in all scenarios I could think up (except
of course sshd not being available at the start, but we want that to
fail).

Rich.

This is intended for use by the forthcoming retry filter to close and
reopen the backend chain.  It is handled entirely by server/backend.c
as no cooperation is needed with the plugin.

Note the explicit readonly parameter: An alternative would be to store
the previous readonly setting in the b_conn_handle struct.  However
passing it explicitly allows the retry filter to retry as readonly,
which might be useful.  This design does however require any filter
which might call .reopen to save the original readonly parameter from
the .open call.
---
 include/nbdkit-filter.h |  6 ++++++
 server/backend.c        | 12 ++++++++++++
 server/filters.c        |  8 ++++++++
 server/internal.h       |  4 ++++
 4 files changed, 30 insertions(+)

diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index babce0d..c1930c1 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -68,6 +68,12 @@ typedef int nbdkit_next_open (void *nxdata,
                               int readonly);
 
 struct nbdkit_next_ops {
+  /* Performs close + open on the underlying chain.
+   * Used by the retry filter.
+   */
+  int (*reopen) (void *nxdata, int readonly);
+
+  /* The rest of the next ops are the same as normal plugin operations. */
   int64_t (*get_size) (void *nxdata);
 
   int (*can_write) (void *nxdata);
diff --git a/server/backend.c b/server/backend.c
index 6c102f9..52b1734 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -233,6 +233,18 @@ backend_valid_range (struct backend *b, struct connection
*conn,
 
 /* Wrappers for all callbacks in a filter's struct nbdkit_next_ops. */
 
+int
+backend_reopen (struct backend *b, struct connection *conn, int readonly)
+{
+  struct b_conn_handle *h = &conn->handles[b->i];
+
+  debug ("%s: reopen", b->name);
+
+  if (h->handle != NULL)
+    backend_close (b, conn);
+  return backend_open (b, conn, readonly);
+}
+
 int64_t
 backend_get_size (struct backend *b, struct connection *conn)
 {
diff --git a/server/filters.c b/server/filters.c
index 1091c2d..78e32bc 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -237,6 +237,13 @@ filter_close (struct backend *b, struct connection *conn)
  * single ‘void *nxdata’ struct pointer (‘b_conn’).
  */
 
+static int
+next_reopen (void *nxdata, int readonly)
+{
+  struct b_conn *b_conn = nxdata;
+  return backend_reopen (b_conn->b, b_conn->conn, readonly);
+}
+
 static int64_t
 next_get_size (void *nxdata)
 {
@@ -373,6 +380,7 @@ next_cache (void *nxdata, uint32_t count, uint64_t offset,
 }
 
 static struct nbdkit_next_ops next_ops = {
+  .reopen = next_reopen,
   .get_size = next_get_size,
   .can_write = next_can_write,
   .can_flush = next_can_flush,
diff --git a/server/internal.h b/server/internal.h
index a376d73..c58b020 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -307,6 +307,7 @@ struct backend {
   int (*prepare) (struct backend *, struct connection *conn, int readonly);
   int (*finalize) (struct backend *, struct connection *conn);
   void (*close) (struct backend *, struct connection *conn);
+  int (*reopen) (struct backend *, struct connection *conn);
 
   int64_t (*get_size) (struct backend *, struct connection *conn);
   int (*can_write) (struct backend *, struct connection *conn);
@@ -360,6 +361,9 @@ extern bool backend_valid_range (struct backend *b, struct
connection *conn,
                                  uint64_t offset, uint32_t count)
   __attribute__((__nonnull__ (1, 2)));
 
+extern int backend_reopen (struct backend *b, struct connection *conn,
+                           int readonly)
+  __attribute__((__nonnull__ (1, 2)));
 extern int64_t backend_get_size (struct backend *b, struct connection *conn)
   __attribute__((__nonnull__ (1, 2)));
 extern int backend_can_write (struct backend *b, struct connection *conn)
-- 
2.23.0

This filter can be used to transparently reopen/retry when a plugin
fails.  The connection is closed and reopened which for most plugins
causes them to attempt to reconnect to their source.

For example if doing a long or slow SSH copy:

  nbdkit -U - ssh host=remote /var/tmp/test.iso \
    --run 'qemu-img convert -p -f raw $nbd -O qcow2 test.qcow2'

if the SSH connection or network goes down in the middle then the
whole operation will fail.

By adding the retry filter:

  nbdkit -U - ssh --filter=retry host=remote /var/tmp/test.iso \
    --run 'qemu-img convert -p -f raw $nbd -O qcow2 test.qcow2'

this operation can recover from temporary failures in at least some
circumstances.  The NBD connection (a local Unix domain socket in the
example above) is not interrupted during retries, so NBD clients don't
need to be taught how to retry - everything is handled internally by
nbdkit.
---
 TODO                                  |   6 +
 configure.ac                          |   2 +
 docs/nbdkit-captive.pod               |   3 +
 filters/retry/Makefile.am             |  67 ++++++
 filters/retry/nbdkit-retry-filter.pod | 103 +++++++++
 filters/retry/retry.c                 | 301 ++++++++++++++++++++++++++
 plugins/curl/nbdkit-curl-plugin.pod   |   1 +
 plugins/ssh/nbdkit-ssh-plugin.pod     |   1 +
 plugins/vddk/nbdkit-vddk-plugin.pod   |   2 +
 9 files changed, 486 insertions(+)

diff --git a/TODO b/TODO
index d2cf0ae..02a095e 100644
--- a/TODO
+++ b/TODO
@@ -136,6 +136,12 @@ nbdkit-rate-filter:
 * split large requests to avoid long, lumpy sleeps when request size
   is much larger than rate limit
 
+nbdkit-retry-filter:
+
+* allow user to specify which errors cause a retry and which ones are
+  passed through; for example there's probably no point retrying on
+  ENOMEM
+
 Filters for security
 --------------------
 
diff --git a/configure.ac b/configure.ac
index 8261a7f..7ce3dbb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -872,6 +872,7 @@ filters="\
         partition \
         rate \
         readahead \
+        retry \
         stats \
         truncate \
         xz \
@@ -952,6 +953,7 @@ AC_CONFIG_FILES([Makefile
                  filters/partition/Makefile
                  filters/rate/Makefile
                  filters/readahead/Makefile
+                 filters/retry/Makefile
                  filters/stats/Makefile
                  filters/truncate/Makefile
                  filters/xz/Makefile
diff --git a/docs/nbdkit-captive.pod b/docs/nbdkit-captive.pod
index 59df669..0adf0a0 100644
--- a/docs/nbdkit-captive.pod
+++ b/docs/nbdkit-captive.pod
@@ -92,6 +92,9 @@ help performance:
  nbdkit -U - --filter=readahead curl https://example.com/disk.img \
         --run 'qemu-img convert $nbd disk.img'
 
+If the source suffers from temporary network failures
+L<nbdkit-retry-filter(1)> may help.
+
 To overwrite a file inside an uncompressed tar file (the file being
 overwritten must be the same size), use L<nbdkit-tar-plugin(1)> like
 this:
diff --git a/filters/retry/Makefile.am b/filters/retry/Makefile.am
new file mode 100644
index 0000000..86011bb
--- /dev/null
+++ b/filters/retry/Makefile.am
@@ -0,0 +1,67 @@
+# nbdkit
+# Copyright (C) 2019 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+include $(top_srcdir)/common-rules.mk
+
+EXTRA_DIST = nbdkit-retry-filter.pod
+
+filter_LTLIBRARIES = nbdkit-retry-filter.la
+
+nbdkit_retry_filter_la_SOURCES = \
+	retry.c \
+	$(top_srcdir)/include/nbdkit-filter.h \
+	$(NULL)
+
+nbdkit_retry_filter_la_CPPFLAGS = \
+	-I$(top_srcdir)/include \
+	-I$(top_srcdir)/common/include \
+	-I$(top_srcdir)/common/utils \
+	$(NULL)
+nbdkit_retry_filter_la_CFLAGS = $(WARNINGS_CFLAGS)
+nbdkit_retry_filter_la_LDFLAGS = \
+	-module -avoid-version -shared \
+	-Wl,--version-script=$(top_srcdir)/filters/filters.syms \
+	$(NULL)
+nbdkit_retry_filter_la_LIBADD = \
+	$(top_builddir)/common/utils/libutils.la \
+	$(NULL)
+
+if HAVE_POD
+
+man_MANS = nbdkit-retry-filter.1
+CLEANFILES += $(man_MANS)
+
+nbdkit-retry-filter.1: nbdkit-retry-filter.pod
+	$(PODWRAPPER) --section=1 --man $@ \
+	    --html $(top_builddir)/html/$@.html \
+	    $<
+
+endif HAVE_POD
diff --git a/filters/retry/nbdkit-retry-filter.pod
b/filters/retry/nbdkit-retry-filter.pod
new file mode 100644
index 0000000..aeed79c
--- /dev/null
+++ b/filters/retry/nbdkit-retry-filter.pod
@@ -0,0 +1,103 @@
+=head1 NAME
+
+nbdkit-retry-filter - reopen connection on error
+
+=head1 SYNOPSIS
+
+ nbdkit --filter=retry PLUGIN
+
+=head1 DESCRIPTION
+
+C<nbdkit-retry-filter> is a filter that transparently reopens the
+plugin connection when an error is encountered.  It can be used to
+make long-running copy operations reliable in the presence of
+temporary network failures, without requiring any changes to the
+plugin.
+
+A number of parameters are available to control:
+
+=over 4
+
+=item *
+
+How many times we retry.
+
+=item *
+
+The delay between retries, and whether we wait longer each time (known
+as "exponential back-off").
+
+=item *
+
+If we reopen the plugin in read-only mode after the first failure.
+
+=back
+
+The default (no parameters) is designed to offer a happy medium
+between recovering from short temporary failures but not doing
+anything too bad when permanent or unrecoverable failures happen: We
+retry 5 times with exponential back-off, waiting in total about 1
+minute before we give up.
+
+=head1 EXAMPLE
+
+In this example we copy and convert a large file using
+L<nbdkit-ssh-plugin(1)>, L<qemu-img(1)> and
L<nbdkit-captive(1)>.
+
+ nbdkit -U - ssh --filter=retry host=remote /var/tmp/test.iso \
+   --run 'qemu-img convert -p -f raw $nbd -O qcow2 test.qcow2'
+
+Without I<--filter=retry> a temporary failure would cause the copy to
+fail (for example, the remote host’s firewall is restarted causing the
+SSH connection to be closed).  Adding this filter means that it may be
+possible to transparently recover.
+
+=head1 PARAMETERS
+
+=over 4
+
+=item B<retries=>N
+
+The number of times any single operation will be retried before we
+give up and fail the operation.  The default is 5.
+
+=item B<retry-delay=>N
+
+The number of seconds to wait before retrying.  The default is 2
+seconds.
+
+=item B<retry-exponential=yes>
+
+Use exponential back-off.  The retry delay is doubled between each
+retry.  This is the default.
+
+=item B<retry-exponential=no>
+
+Do not use exponential back-off.  The retry delay is the same between
+each retry.
+
+=item B<retry-readonly=yes>
+
+As soon as a failure occurs and we retry, switch the underlying plugin
+permanently to read-only mode.
+
+=item B<retry-readonly=no>
+
+Do not change the read-write/read-only mode of the plugin when
+retrying.  This is the default.
+
+=back
+
+=head1 SEE ALSO
+
+L<nbdkit(1)>,
+L<nbdkit-filter(3)>,
+L<nbdkit-readahead-filter(1)>.
+
+=head1 AUTHORS
+
+Richard W.M. Jones
+
+=head1 COPYRIGHT
+
+Copyright (C) 2019 Red Hat Inc.
diff --git a/filters/retry/retry.c b/filters/retry/retry.c
new file mode 100644
index 0000000..7ba3d96
--- /dev/null
+++ b/filters/retry/retry.c
@@ -0,0 +1,301 @@
+/* nbdkit
+ * Copyright (C) 2019 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdbool.h>
+#include <inttypes.h>
+#include <string.h>
+#include <sys/time.h>
+
+#include <nbdkit-filter.h>
+
+#include "cleanup.h"
+
+static int retries = 5;         /* 0 = filter is disabled */
+static int initial_delay = 2;
+static bool exponential_backoff = true;
+static bool force_readonly = false;
+
+static int
+retry_config (nbdkit_next_config *next, void *nxdata,
+              const char *key, const char *value)
+{
+  int r;
+
+  if (strcmp (key, "retries") == 0) {
+    if (sscanf (value, "%d", &retries) != 1 || retries < 0) {
+      nbdkit_error ("cannot parse retries: %s", value);
+      return -1;
+    }
+  }
+  else if (strcmp (key, "retry-delay") == 0) {
+    if (sscanf (value, "%d", &initial_delay) != 1 ||
initial_delay <= 0) {
+      nbdkit_error ("cannot parse retry-delay: %s", value);
+      return -1;
+    }
+  }
+  else if (strcmp (key, "retry-exponential") == 0) {
+    r = nbdkit_parse_bool (value);
+    if (r == -1)
+      return -1;
+    exponential_backoff = r;
+    return 0;
+  }
+  else if (strcmp (key, "retry-readonly") == 0) {
+    r = nbdkit_parse_bool (value);
+    if (r == -1)
+      return -1;
+    force_readonly = r;
+    return 0;
+  }
+
+  return next (nxdata, key, value);
+}
+
+#define retry_config_help \
+  "retries=<N>              Number of retries (default: 5).\n"
\
+  "retry-delay=<N>          Second to wait before retry (default:
2).\n" \
+  "retry-exponential=yes|no Exponential back-off (default: yes).\n" \
+  "retry-readonly=yes|no    Force read-only on failure (default:
no).\n"
+
+struct retry_handle {
+  int readonly;                 /* Save original readonly setting. */
+};
+
+static void *
+retry_open (nbdkit_next_open *next, void *nxdata, int readonly)
+{
+  struct retry_handle *h;
+
+  if (next (nxdata, readonly) == -1)
+    return NULL;
+
+  h = malloc (sizeof *h);
+  if (h == NULL) {
+    nbdkit_error ("malloc: %m");
+    return NULL;
+  }
+
+  h->readonly = readonly;
+
+  return h;
+}
+
+static void
+retry_close (void *handle)
+{
+  struct retry_handle *h = handle;
+
+  free (h);
+}
+
+/* This function encapsulates the common retry logic used across all
+ * data commands.  If it returns true then the data command will retry
+ * the operation.  ‘struct retry_data’ is stack data saved between
+ * retries within the same command, and is initialized to zero.
+ */
+struct retry_data {
+  int retry;                    /* Retry number (0 = first time). */
+  int delay;                    /* Seconds to wait before retrying. */
+};
+
+static bool
+do_retry (struct retry_handle *h,
+          struct retry_data *data,
+          struct nbdkit_next_ops *next_ops, void *nxdata,
+          int *err)
+{
+  /* If it's the first retry, initialize the other fields in *data. */
+  if (data->retry == 0)
+    data->delay = initial_delay;
+
+ again:
+  /* Log the original errno since it will be lost when we retry. */
+  nbdkit_debug ("retry %d: original errno = %d", data->retry+1,
*err);
+
+  if (data->retry >= retries) {
+    nbdkit_debug ("could not recover after %d retries", retries);
+    return false;
+  }
+
+  nbdkit_debug ("waiting %d seconds before retrying",
data->delay);
+  if (nbdkit_nanosleep (data->delay, 0) == -1) {
+    /* We could do this but it would overwrite the more important
+     * errno from the underlying data call.
+     */
+    /* *err = errno; */
+    return false;
+  }
+
+  /* Update *data in case we are called again. */
+  data->retry++;
+  if (exponential_backoff)
+    data->delay *= 2;
+
+  /* Reopen the connection. */
+  if (next_ops->reopen (nxdata, h->readonly || force_readonly) == -1) {
+    /* If the reopen fails we treat it the same way as a command
+     * failing.
+     */
+    goto again;
+  }
+
+  /* Retry the data command. */
+  return true;
+}
+
+static int
+retry_pread (struct nbdkit_next_ops *next_ops, void *nxdata,
+             void *handle, void *buf, uint32_t count, uint64_t offset,
+             uint32_t flags, int *err)
+{
+  struct retry_handle *h = handle;
+  struct retry_data data = {0};
+  int r;
+
+ again:
+  r = next_ops->pread (nxdata, buf, count, offset, flags, err);
+  if (r == -1 && do_retry (h, &data, next_ops, nxdata, err)) goto
again;
+
+  return r;
+}
+
+/* Write. */
+static int
+retry_pwrite (struct nbdkit_next_ops *next_ops, void *nxdata,
+              void *handle,
+              const void *buf, uint32_t count, uint64_t offset,
+              uint32_t flags, int *err)
+{
+  struct retry_handle *h = handle;
+  struct retry_data data = {0};
+  int r;
+
+ again:
+  r = next_ops->pwrite (nxdata, buf, count, offset, flags, err);
+  if (r == -1 && do_retry (h, &data, next_ops, nxdata, err)) goto
again;
+
+  return r;
+}
+
+/* Trim. */
+static int
+retry_trim (struct nbdkit_next_ops *next_ops, void *nxdata,
+            void *handle,
+            uint32_t count, uint64_t offset, uint32_t flags,
+            int *err)
+{
+  struct retry_handle *h = handle;
+  struct retry_data data = {0};
+  int r;
+
+ again:
+  r = next_ops->trim (nxdata, count, offset, flags, err);
+  if (r == -1 && do_retry (h, &data, next_ops, nxdata, err)) goto
again;
+
+  return r;
+}
+
+/* Zero. */
+static int
+retry_zero (struct nbdkit_next_ops *next_ops, void *nxdata,
+            void *handle,
+            uint32_t count, uint64_t offset, uint32_t flags,
+            int *err)
+{
+  struct retry_handle *h = handle;
+  struct retry_data data = {0};
+  int r;
+
+ again:
+  r = next_ops->zero (nxdata, count, offset, flags, err);
+  if (r == -1 && do_retry (h, &data, next_ops, nxdata, err)) goto
again;
+
+  return r;
+}
+
+/* Extents. */
+static int
+retry_extents (struct nbdkit_next_ops *next_ops, void *nxdata,
+               void *handle,
+               uint32_t count, uint64_t offset, uint32_t flags,
+               struct nbdkit_extents *extents, int *err)
+{
+  struct retry_handle *h = handle;
+  struct retry_data data = {0};
+  int r;
+
+ again:
+  r = next_ops->extents (nxdata, count, offset, flags, extents, err);
+  if (r == -1 && do_retry (h, &data, next_ops, nxdata, err)) goto
again;
+
+  return r;
+}
+
+/* Cache. */
+static int
+retry_cache (struct nbdkit_next_ops *next_ops, void *nxdata,
+             void *handle,
+             uint32_t count, uint64_t offset, uint32_t flags,
+             int *err)
+{
+  struct retry_handle *h = handle;
+  struct retry_data data = {0};
+  int r;
+
+ again:
+  r = next_ops->cache (nxdata, count, offset, flags, err);
+  if (r == -1 && do_retry (h, &data, next_ops, nxdata, err)) goto
again;
+
+  return r;
+}
+
+static struct nbdkit_filter filter = {
+  .name              = "retry",
+  .longname          = "nbdkit retry filter",
+  .config            = retry_config,
+  .config_help       = retry_config_help,
+  .open              = retry_open,
+  .close             = retry_close,
+  .pread             = retry_pread,
+  .pwrite            = retry_pwrite,
+  .trim              = retry_trim,
+  .zero              = retry_zero,
+  .extents           = retry_extents,
+  .cache             = retry_cache,
+};
+
+NBDKIT_REGISTER_FILTER(filter)
diff --git a/plugins/curl/nbdkit-curl-plugin.pod
b/plugins/curl/nbdkit-curl-plugin.pod
index 1c96b4a..2d5c564 100644
--- a/plugins/curl/nbdkit-curl-plugin.pod
+++ b/plugins/curl/nbdkit-curl-plugin.pod
@@ -159,6 +159,7 @@ L<CURLOPT_COOKIE(3)>
 L<CURLOPT_VERBOSE(3)>,
 L<nbdkit(1)>,
 L<nbdkit-readahead-filter(1)>,
+L<nbdkit-retry-filter(1)>,
 L<nbdkit-ssh-plugin(1)>,
 L<nbdkit-plugin(3)>,
 L<http://curl.haxx.se>.
diff --git a/plugins/ssh/nbdkit-ssh-plugin.pod
b/plugins/ssh/nbdkit-ssh-plugin.pod
index 2010837..316e852 100644
--- a/plugins/ssh/nbdkit-ssh-plugin.pod
+++ b/plugins/ssh/nbdkit-ssh-plugin.pod
@@ -288,6 +288,7 @@ libssh itself.
 L<nbdkit(1)>,
 L<nbdkit-curl-plugin(1)>,
 L<nbdkit-readahead-filter(1)>,
+L<nbdkit-retry-filter(1)>,
 L<nbdkit-plugin(3)>,
 L<ssh(1)>,
 L<ssh-agent(1)>,
diff --git a/plugins/vddk/nbdkit-vddk-plugin.pod
b/plugins/vddk/nbdkit-vddk-plugin.pod
index 8820a50..9e451c6 100644
--- a/plugins/vddk/nbdkit-vddk-plugin.pod
+++ b/plugins/vddk/nbdkit-vddk-plugin.pod
@@ -431,6 +431,8 @@ writing.
 
 L<nbdkit(1)>,
 L<nbdkit-plugin(3)>,
+L<nbdkit-readahead-filter(1)>,
+L<nbdkit-retry-filter(1)>,
 L<virsh(1)>,
 L<https://www.vmware.com/support/developer/vddk/>
 
-- 
2.23.0

We use a custom sh plugin to test retries are working.
---
 tests/Makefile.am               |   8 +++
 tests/test-retry-reopen-fail.sh | 108 ++++++++++++++++++++++++++++++++
 tests/test-retry.sh             |  97 ++++++++++++++++++++++++++++
 3 files changed, 213 insertions(+)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 1b1e05b..af9b9d9 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -114,6 +114,8 @@ EXTRA_DIST = \
 	test-reflection-address.sh \
 	test-reflection-base64.sh \
 	test-reflection-raw.sh \
+	test-retry.sh \
+	test-retry-reopen-fail.sh \
 	test-shutdown.sh \
 	test-ssh.sh \
 	test.tcl \
@@ -1040,6 +1042,12 @@ test_readahead_SOURCES = test-readahead.c test.h
 test_readahead_CFLAGS = $(WARNINGS_CFLAGS) $(LIBGUESTFS_CFLAGS)
 test_readahead_LDADD = libtest.la $(LIBGUESTFS_LIBS)
 
+# retry filter test.
+TESTS += \
+	test-retry.sh \
+	test-retry-reopen-fail.sh \
+	$(NULL)
+
 # truncate filter tests.
 TESTS += \
 	test-truncate1.sh \
diff --git a/tests/test-retry-reopen-fail.sh b/tests/test-retry-reopen-fail.sh
new file mode 100755
index 0000000..a6279b8
--- /dev/null
+++ b/tests/test-retry-reopen-fail.sh
@@ -0,0 +1,108 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2018-2019 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+# This test is similar to test-retry.sh but it also tests the case
+# where the reopen operation fails.
+
+source ./functions.sh
+set -e
+set -x
+
+requires qemu-img --version
+requires stat --version
+
+files="retry-reopen-fail.img retry-reopen-fail-start
+       retry-reopen-fail-count retry-reopen-fail-open-count"
+rm -f $files
+cleanup_fn rm -f $files
+
+touch retry-reopen-fail-start
+
+# Create a custom plugin which will test retrying.
+nbdkit -v -U - --filter=retry \
+       sh - \
+       --run 'qemu-img convert $nbd retry-reopen-fail.img'
<<'EOF'
+case "$1" in
+    open)
+        # Count how many times the connection is (re-)opened.
+        i=`cat retry-reopen-fail-open-count`
+        ((i++))
+        echo $i > retry-reopen-fail-open-count
+        if [ $i -eq 2 ]; then
+            echo "EIO open failed" >&2
+            exit 1
+        fi
+        ;;
+    pread)
+        # Fail 3 times then succeed, and count how long it takes.
+        i=`cat retry-reopen-fail-count`
+        ((i++))
+        echo $i > retry-reopen-fail-count
+        if [ $i -le 3 ]; then
+            echo "EIO pread failed" >&2
+            exit 1
+        else
+            dd if=/dev/zero count=$3 iflag=count_bytes
+        fi
+        ;;
+
+    get_size) echo 512 ;;
+    *) exit 2 ;;
+esac
+EOF
+
+# In this test we should see 4 failures:
+# pread FAILS
+# retry and wait 2 seconds
+# open FAILS
+# retry and wait 4 seconds
+# open succeeds
+# pread FAILS
+# retry and wait 8 seconds
+# pread FAILS
+# retry and wait 16 seconds
+# pread succeeds
+
+# The minimum time for the test should be 2+4+8+16 = 30 seconds.
+start_t=`stat -c '%Z' retry-reopen-fail-start`
+end_t=`date +'%s'`
+if [ $((end_t - start_t)) -lt 30 ]; then
+    echo "$0: test ran too quickly"
+    exit 1
+fi
+
+# Check the handle was opened 5 times.
+retry_open_count=`cat retry-reopen-fail-open-count`
+if [ $retry_open_count -ne 5 ]; then
+    echo "$0: retry-reopen-fail-open-count ($retry_open_count) != 5"
+    exit 1
+fi
diff --git a/tests/test-retry.sh b/tests/test-retry.sh
new file mode 100755
index 0000000..47d6e23
--- /dev/null
+++ b/tests/test-retry.sh
@@ -0,0 +1,97 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2018-2019 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+source ./functions.sh
+set -e
+set -x
+
+requires qemu-img --version
+requires stat --version
+
+files="retry.img retry-start retry-count retry-open-count"
+rm -f $files
+cleanup_fn rm -f $files
+
+touch retry-start
+
+# Create a custom plugin which will test retrying.
+nbdkit -v -U - --filter=retry \
+       sh - \
+       --run 'qemu-img convert $nbd retry.img' <<'EOF'
+case "$1" in
+    open)
+        # Count how many times the connection is (re-)opened.
+        i=`cat retry-open-count`
+        echo $((i+1)) > retry-open-count
+        ;;
+    pread)
+        # Fail 3 times then succeed, and count how long it takes.
+        i=`cat retry-count`
+        ((i++))
+        echo $i > retry-count
+        if [ $i -le 3 ]; then
+            echo "EIO pread failed" >&2
+            exit 1
+        else
+            dd if=/dev/zero count=$3 iflag=count_bytes
+        fi
+        ;;
+
+    get_size) echo 512 ;;
+    *) exit 2 ;;
+esac
+EOF
+
+# In this test we should see 3 failures:
+# pread FAILS
+# retry and wait 2 seconds
+# pread FAILS
+# retry and wait 4 seconds
+# pread FAILS
+# retry and wait 8 seconds
+# pread succeeds
+
+# The minimum time for the test should be 2+4+8 = 14 seconds.
+start_t=`stat -c '%Z' retry-start`
+end_t=`date +'%s'`
+if [ $((end_t - start_t)) -lt 14 ]; then
+    echo "$0: test ran too quickly"
+    exit 1
+fi
+
+# Check the handle was opened 4 times (first open + one reopen for
+# each retry).
+retry_open_count=`cat retry-open-count`
+if [ $retry_open_count -ne 4 ]; then
+    echo "$0: retry-open-count ($retry_open_count) != 5"
+    exit 1
+fi
-- 
2.23.0

On 9/19/19 10:26 AM, Richard W.M. Jones wrote:
> This is intended for use by the forthcoming retry filter to close and
> reopen the backend chain.  It is handled entirely by server/backend.c
> as no cooperation is needed with the plugin.
> 
> Note the explicit readonly parameter: An alternative would be to store
> the previous readonly setting in the b_conn_handle struct.  However
> passing it explicitly allows the retry filter to retry as readonly,
> which might be useful.  This design does however require any filter
> which might call .reopen to save the original readonly parameter from
> the .open call.
> ---
>  include/nbdkit-filter.h |  6 ++++++
>  server/backend.c        | 12 ++++++++++++
>  server/filters.c        |  8 ++++++++
>  server/internal.h       |  4 ++++
>  4 files changed, 30 insertions(+)
> 
I made comments on v2 that still apply here (crossed emails again).
Another one I just thought of:

> +int
> +backend_reopen (struct backend *b, struct connection *conn, int readonly)
> +{
> +  struct b_conn_handle *h = &conn->handles[b->i];
> +
> +  debug ("%s: reopen", b->name);
> +
> +  if (h->handle != NULL)
> +    backend_close (b, conn);
> +  return backend_open (b, conn, readonly);
Do we want to grab a resource lock at this point?  Should we be trying
hard to prevent further client calls from going to the retry filter
while the retry filter is reopening the real plugin?  Or do those locks
belong in the retry filter itself?  If nothing else, we probably want
some sort of locking to ensure that two threads are not trying reopen at
the same time.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 9/19/19 10:26 AM, Richard W.M. Jones wrote:
> We use a custom sh plugin to test retries are working.
> ---
>  tests/Makefile.am               |   8 +++
>  tests/test-retry-reopen-fail.sh | 108 ++++++++++++++++++++++++++++++++
>  tests/test-retry.sh             |  97 ++++++++++++++++++++++++++++
>  3 files changed, 213 insertions(+)
> 
> +++ b/tests/test-retry-reopen-fail.sh
> +
> +# Create a custom plugin which will test retrying.
> +nbdkit -v -U - --filter=retry \
> +       sh - \
> +       --run 'qemu-img convert $nbd retry-reopen-fail.img'
<<'EOF'
> +case "$1" in
> +    open)
> +        # Count how many times the connection is (re-)opened.
> +        i=`cat retry-reopen-fail-open-count`
$() is nicer than ``, but even better is just:

read i retry-reopen-fail-open-count
> +        ((i++))
> +        echo $i > retry-reopen-fail-open-count
Potentially racy if more than one thread tries to do this - but sh
plugins don't default to parallel and you aren't requesting parallel
mode, so we are safe due to serialization. But maybe worth a comment?
> +        if [ $i -eq 2 ]; then
> +            echo "EIO open failed" >&2
> +            exit 1
> +        fi
> +        ;;
> +    pread)
> +        # Fail 3 times then succeed, and count how long it takes.
> +        i=`cat retry-reopen-fail-count`
same thing about using 'read'

> +
> +# In this test we should see 4 failures:
> +# pread FAILS
> +# retry and wait 2 seconds
> +# open FAILS
> +# retry and wait 4 seconds
> +# open succeeds
> +# pread FAILS
> +# retry and wait 8 seconds
> +# pread FAILS
> +# retry and wait 16 seconds
> +# pread succeeds
> +
> +# The minimum time for the test should be 2+4+8+16 = 30 seconds.
> +start_t=`stat -c '%Z' retry-reopen-fail-start`
> +end_t=`date +'%s'`
> +if [ $((end_t - start_t)) -lt 30 ]; then
> +    echo "$0: test ran too quickly"
> +    exit 1
> +fi
Slows down 'make check'; is there any way we can scale it to be slightly
faster, such as using a smaller retry interval than just 1 second as our
starting point?
> +
> +# Check the handle was opened 5 times.
> +retry_open_count=`cat retry-reopen-fail-open-count`
Another potential 'read' spot.
> +if [ $retry_open_count -ne 5 ]; then
> +    echo "$0: retry-reopen-fail-open-count ($retry_open_count) !=
5"
> +    exit 1
> +fi
> diff --git a/tests/test-retry.sh b/tests/test-retry.sh
> new file mode 100755
> index 0000000..47d6e23
> --- /dev/null
> +++ b/tests/test-retry.sh
> +touch retry-start
> +
> +# Create a custom plugin which will test retrying.
> +nbdkit -v -U - --filter=retry \
> +       sh - \
> +       --run 'qemu-img convert $nbd retry.img'
<<'EOF'
> +case "$1" in
> +    open)
> +        # Count how many times the connection is (re-)opened.
> +        i=`cat retry-open-count`
and more in this file
> +
> +# The minimum time for the test should be 2+4+8 = 14 seconds.
> +start_t=`stat -c '%Z' retry-start`
> +end_t=`date +'%s'`
I'm not sure how portable this will be to non-Linux, but we'll deal with
that when someone complains.  It may also be an issue if filesystem time
is skewed in relation to system date.

Bash includes $SECONDS which auto-increments at 1-second intervals,
would testing that be more reliable than stat/date?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 9/19/19 10:26 AM, Richard W.M. Jones wrote:
> This is intended for use by the forthcoming retry filter to close and
> reopen the backend chain.  It is handled entirely by server/backend.c
> as no cooperation is needed with the plugin.
> 
> Note the explicit readonly parameter: An alternative would be to store
> the previous readonly setting in the b_conn_handle struct.  However
> passing it explicitly allows the retry filter to retry as readonly,
> which might be useful.  This design does however require any filter
> which might call .reopen to save the original readonly parameter from
> the .open call.
> ---
>  include/nbdkit-filter.h |  6 ++++++
>  server/backend.c        | 12 ++++++++++++
>  server/filters.c        |  8 ++++++++
>  server/internal.h       |  4 ++++
>  4 files changed, 30 insertions(+)
docs/nbdkit-filter.pod should probably get an update as well.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 9/19/19 10:26 AM, Richard W.M. Jones wrote:
> This filter can be used to transparently reopen/retry when a plugin
> fails.  The connection is closed and reopened which for most plugins
> causes them to attempt to reconnect to their source.
> 
> For example if doing a long or slow SSH copy:
> 
>   nbdkit -U - ssh host=remote /var/tmp/test.iso \
>     --run 'qemu-img convert -p -f raw $nbd -O qcow2 test.qcow2'
> 
> if the SSH connection or network goes down in the middle then the
> whole operation will fail.
> 
> By adding the retry filter:
> 
>   nbdkit -U - ssh --filter=retry host=remote /var/tmp/test.iso \
>     --run 'qemu-img convert -p -f raw $nbd -O qcow2 test.qcow2'
> 
> this operation can recover from temporary failures in at least some
> circumstances.  The NBD connection (a local Unix domain socket in the
> example above) is not interrupted during retries, so NBD clients don't
> need to be taught how to retry - everything is handled internally by
> nbdkit.
> ---
Pretty cool how fast we've gone from idea to mostly-working code.
nbdkit is awesome for fast prototyping.
> +++ b/filters/retry/nbdkit-retry-filter.pod
> @@ -0,0 +1,103 @@
> +=head1 NAME
> +
> +nbdkit-retry-filter - reopen connection on error
> +
> +=head1 SYNOPSIS
> +
> + nbdkit --filter=retry PLUGIN
Worth adding [retry-parameter]... here?
> +
> +=head1 DESCRIPTION
> +
> +C<nbdkit-retry-filter> is a filter that transparently reopens the
> +plugin connection when an error is encountered.  It can be used to
> +make long-running copy operations reliable in the presence of
> +temporary network failures, without requiring any changes to the
> +plugin.
> +
> +A number of parameters are available to control:
> +
> +=over 4
> +
> +=item *
> +
> +How many times we retry.
> +
> +=item *
> +
> +The delay between retries, and whether we wait longer each time (known
> +as "exponential back-off").
> +
> +=item *
> +
> +If we reopen the plugin in read-only mode after the first failure.
> +
> +=back
> +
> +The default (no parameters) is designed to offer a happy medium
> +between recovering from short temporary failures but not doing
> +anything too bad when permanent or unrecoverable failures happen: We
> +retry 5 times with exponential back-off, waiting in total about 1
> +minute before we give up.
> +
> +=head1 EXAMPLE
> +
> +In this example we copy and convert a large file using
> +L<nbdkit-ssh-plugin(1)>, L<qemu-img(1)> and
L<nbdkit-captive(1)>.
> +
> + nbdkit -U - ssh --filter=retry host=remote /var/tmp/test.iso \
> +   --run 'qemu-img convert -p -f raw $nbd -O qcow2 test.qcow2'
> +
> +Without I<--filter=retry> a temporary failure would cause the copy
to
> +fail (for example, the remote host’s firewall is restarted causing the
> +SSH connection to be closed).  Adding this filter means that it may be
> +possible to transparently recover.
> +
> +=head1 PARAMETERS
EXAMPLE before PARAMETERS is unusual.  Not necessarily wrong, though.
> +
> +=over 4
> +
> +=item B<retries=>N
> +
> +The number of times any single operation will be retried before we
> +give up and fail the operation.  The default is 5.
> +
> +=item B<retry-delay=>N
> +
> +The number of seconds to wait before retrying.  The default is 2
> +seconds.
Worth allowing sub-second times?
> +
> +=item B<retry-exponential=yes>
> +
> +Use exponential back-off.  The retry delay is doubled between each
> +retry.  This is the default.
> +
> +=item B<retry-exponential=no>
> +
> +Do not use exponential back-off.  The retry delay is the same between
> +each retry.
> +
> +=item B<retry-readonly=yes>
> +
> +As soon as a failure occurs and we retry, switch the underlying plugin
> +permanently to read-only mode.
> +
> +=item B<retry-readonly=no>
> +
> +Do not change the read-write/read-only mode of the plugin when
> +retrying.  This is the default.
Worth an intermediate mode that only forces readonly if a write
operation fails, but tries to preserve read/write if a read operation fails?

> +
> +static int retries = 5;         /* 0 = filter is disabled */
> +static int initial_delay = 2;
Would need a different unit here if you allow sub-second retry.
> +static bool exponential_backoff = true;
> +static bool force_readonly = false;
Initializing a static variable to 0 is sometimes worth avoiding (as it
can force .data instead of .bss for a slightly larger binary), but here
it makes sense.
> +
> +static int
> +retry_config (nbdkit_next_config *next, void *nxdata,
> +              const char *key, const char *value)
> +{
> +  int r;
> +
> +  if (strcmp (key, "retries") == 0) {
> +    if (sscanf (value, "%d", &retries) != 1 || retries <
0) {
> +      nbdkit_error ("cannot parse retries: %s", value);
> +      return -1;
> +    }
> +  }
> +  else if (strcmp (key, "retry-delay") == 0) {
> +    if (sscanf (value, "%d", &initial_delay) != 1 ||
initial_delay <= 0) {
sscanf("%d") cannot detect overflow; should this use strtol with errno
checking instead?

> +#define retry_config_help \
> +  "retries=<N>              Number of retries (default:
5).\n" \
> +  "retry-delay=<N>          Second to wait before retry
(default: 2).\n" \
Seconds
> +  "retry-exponential=yes|no Exponential back-off (default:
yes).\n" \
> +  "retry-readonly=yes|no    Force read-only on failure (default:
no).\n"
> +
> +struct retry_handle {
> +  int readonly;                 /* Save original readonly setting. */
> +};
> +
> +static void *
> +retry_open (nbdkit_next_open *next, void *nxdata, int readonly)

Out of review time today, I'll get back to the rest of this file later...



-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org