Richard W.M. Jones
2021-Oct-15 14:17 UTC
[Libguestfs] [PATCH nbdkit v2 0/2] New filter: nbdkit-retry-request-filter
I believe this addresses everything mentioned in the previous reviews (sorry if I missed anything). In addition I have extended web-server.c to implement a fairly realistic test scenario. Alexander: It would be useful if you could test this filter in your code as well so we can be sure it really fixes your problem. Rich.
Richard W.M. Jones
2021-Oct-15 14:17 UTC
[Libguestfs] [PATCH nbdkit v2 1/2] New filter: nbdkit-retry-request-filter
Similar to nbdkit-retry-filter, but this only retries single failing
requests rather than reopening the whole plugin. This is intended to
be used with the curl filter to solve:
https://bugzilla.redhat.com/show_bug.cgi?id=2013000
---
docs/nbdkit-captive.pod | 3 +-
.../nbdkit-retry-request-filter.pod | 74 +++++
filters/retry/nbdkit-retry-filter.pod | 7 +-
plugins/curl/nbdkit-curl-plugin.pod | 1 +
configure.ac | 2 +
filters/retry-request/Makefile.am | 68 +++++
tests/Makefile.am | 8 +
filters/retry-request/retry-request.c | 278 ++++++++++++++++++
tests/test-retry-request.sh | 94 ++++++
9 files changed, 533 insertions(+), 2 deletions(-)
diff --git a/docs/nbdkit-captive.pod b/docs/nbdkit-captive.pod
index d340b8ae0..eafe36d8b 100644
--- a/docs/nbdkit-captive.pod
+++ b/docs/nbdkit-captive.pod
@@ -118,7 +118,8 @@ help performance:
--run 'qemu-img convert $nbd disk.img'
If the source suffers from temporary network failures
-L<nbdkit-retry-filter(1)> may help.
+L<nbdkit-retry-filter(1)> or L<nbdkit-retry-request-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-filter(1)> like
diff --git a/filters/retry-request/nbdkit-retry-request-filter.pod
b/filters/retry-request/nbdkit-retry-request-filter.pod
new file mode 100644
index 000000000..fbb467ca9
--- /dev/null
+++ b/filters/retry-request/nbdkit-retry-request-filter.pod
@@ -0,0 +1,74 @@
+=head1 NAME
+
+nbdkit-retry-request-filter - retry single requests on error
+
+=head1 SYNOPSIS
+
+ nbdkit --filter=retry-request PLUGIN
+ [retry-request-retries=N] [retry-request-delay=N]
+ [retry-request-open=false]
+
+=head1 DESCRIPTION
+
+C<nbdkit-retry-request-filter> is a filter for nbdkit that
+transparently retries single requests if they fail. This is useful
+for plugins that are not completely reliable or have random behaviour.
+For example L<nbdkit-curl-plugin(1)> might behave this way if pointed
+at a load balancer which sometimes redirects to web server that is not
+responsive.
+
+An alternative filter with different trade-offs is
+L<nbdkit-retry-filter(1)>. That filter is more heavyweight because it
+always reopens the whole plugin connection on failure.
+
+=head1 PARAMETERS
+
+=over 4
+
+=item B<retry-request-retries=>N
+
+The number of times any single request will be retried before we give
+up and fail the operation. The default is 2.
+
+=item B<retry-request-delay=>N
+
+The number of seconds to wait before retrying. The default is 2
+seconds.
+
+=item B<retry-request-open=false>
+
+If set to false, do not retry opening the plugin. The default is to
+treat plugin open in the same way as other requests.
+
+=back
+
+=head1 FILES
+
+=over 4
+
+=item F<$filterdir/nbdkit-retry-request-filter.so>
+
+The filter.
+
+Use C<nbdkit --dump-config> to find the location of C<$filterdir>.
+
+=back
+
+=head1 VERSION
+
+C<nbdkit-retry-filter> first appeared in nbdkit 1.30.
+
+=head1 SEE ALSO
+
+L<nbdkit(1)>,
+L<nbdkit-filter(3)>,
+L<nbdkit-readahead-filter(1)>,
+L<nbdkit-retry-filter(1)>.
+
+=head1 AUTHORS
+
+Richard W.M. Jones
+
+=head1 COPYRIGHT
+
+Copyright (C) 2019-2021 Red Hat Inc.
diff --git a/filters/retry/nbdkit-retry-filter.pod
b/filters/retry/nbdkit-retry-filter.pod
index fee4b7e64..babd82bbd 100644
--- a/filters/retry/nbdkit-retry-filter.pod
+++ b/filters/retry/nbdkit-retry-filter.pod
@@ -16,6 +16,10 @@ make long-running copy operations reliable in the presence of
temporary network failures, without requiring any changes to the
plugin or the NBD client.
+An alternative and more fine-grained filter is
+L<nbdkit-retry-request-filter(1)> which will retry only single
+requests that fail.
+
Several optional parameters are available to control:
=over 4
@@ -113,7 +117,8 @@ C<nbdkit-retry-filter> first appeared in nbdkit 1.16.
L<nbdkit(1)>,
L<nbdkit-filter(3)>,
-L<nbdkit-readahead-filter(1)>.
+L<nbdkit-readahead-filter(1)>,
+L<nbdkit-retry-request-filter(1)>.
=head1 AUTHORS
diff --git a/plugins/curl/nbdkit-curl-plugin.pod
b/plugins/curl/nbdkit-curl-plugin.pod
index c7acf6225..07f71b389 100644
--- a/plugins/curl/nbdkit-curl-plugin.pod
+++ b/plugins/curl/nbdkit-curl-plugin.pod
@@ -503,6 +503,7 @@ L<nbdkit-extentlist-filter(1)>,
L<nbdkit-file-plugin(1)>,
L<nbdkit-readahead-filter(1)>,
L<nbdkit-retry-filter(1)>,
+L<nbdkit-retry-request-filter(1)>,
L<nbdkit-ssh-plugin(1)>,
L<nbdkit-torrent-plugin(1)>,
L<nbdkit-plugin(3)>,
diff --git a/configure.ac b/configure.ac
index fd15e2960..3fd8a397e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -139,6 +139,7 @@ filters="\
rate \
readahead \
retry \
+ retry-request \
stats \
swab \
tar \
@@ -1342,6 +1343,7 @@ AC_CONFIG_FILES([Makefile
filters/rate/Makefile
filters/readahead/Makefile
filters/retry/Makefile
+ filters/retry-request/Makefile
filters/stats/Makefile
filters/swab/Makefile
filters/tar/Makefile
diff --git a/filters/retry-request/Makefile.am
b/filters/retry-request/Makefile.am
new file mode 100644
index 000000000..95fdafdcd
--- /dev/null
+++ b/filters/retry-request/Makefile.am
@@ -0,0 +1,68 @@
+# nbdkit
+# Copyright (C) 2019-2021 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-request-filter.pod
+
+filter_LTLIBRARIES = nbdkit-retry-request-filter.la
+
+nbdkit_retry_request_filter_la_SOURCES = \
+ retry-request.c \
+ $(top_srcdir)/include/nbdkit-filter.h \
+ $(NULL)
+
+nbdkit_retry_request_filter_la_CPPFLAGS = \
+ -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/include \
+ -I$(top_srcdir)/common/utils \
+ $(NULL)
+nbdkit_retry_request_filter_la_CFLAGS = $(WARNINGS_CFLAGS)
+nbdkit_retry_request_filter_la_LDFLAGS = \
+ -module -avoid-version -shared $(NO_UNDEFINED_ON_WINDOWS) \
+ -Wl,--version-script=$(top_srcdir)/filters/filters.syms \
+ $(NULL)
+nbdkit_retry_request_filter_la_LIBADD = \
+ $(top_builddir)/common/utils/libutils.la \
+ $(IMPORT_LIBRARY_ON_WINDOWS) \
+ $(NULL)
+
+if HAVE_POD
+
+man_MANS = nbdkit-retry-request-filter.1
+CLEANFILES += $(man_MANS)
+
+nbdkit-retry-request-filter.1: nbdkit-retry-request-filter.pod
+ $(PODWRAPPER) --section=1 --man $@ \
+ --html $(top_builddir)/html/$@.html \
+ $<
+
+endif HAVE_POD
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 2fb39cf09..9a1485a6c 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1693,6 +1693,14 @@ EXTRA_DIST += \
test-retry-zero-flags.sh \
$(NULL)
+# retry-request filter test.
+TESTS += \
+ test-retry-request.sh \
+ $(NULL)
+EXTRA_DIST += \
+ test-retry-request.sh \
+ $(NULL)
+
# swab filter test.
TESTS += \
test-swab-8.sh \
diff --git a/filters/retry-request/retry-request.c
b/filters/retry-request/retry-request.c
new file mode 100644
index 000000000..ac7eb017f
--- /dev/null
+++ b/filters/retry-request/retry-request.c
@@ -0,0 +1,278 @@
+/* nbdkit
+ * Copyright (C) 2019-2021 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"
+#include "windows-compat.h"
+
+static unsigned retries = 2; /* 0 = filter is disabled */
+static unsigned delay = 2;
+static bool retry_open_call = true;
+
+static int
+retry_request_thread_model (void)
+{
+ return NBDKIT_THREAD_MODEL_PARALLEL;
+}
+
+static int
+retry_request_config (nbdkit_next_config *next, nbdkit_backend *nxdata,
+ const char *key, const char *value)
+{
+ int r;
+
+ if (strcmp (key, "retry-request-retries") == 0) {
+ if (nbdkit_parse_unsigned ("retry-request-retries", value,
&retries) == -1)
+ return -1;
+ if (retries > 1000) {
+ nbdkit_error ("retry-request-retries: value too large");
+ return -1;
+ }
+ return 0;
+ }
+ else if (strcmp (key, "retry-request-delay") == 0) {
+ if (nbdkit_parse_unsigned ("retry-request-delay", value,
&delay) == -1)
+ return -1;
+ if (delay == 0) {
+ nbdkit_error ("retry-request-delay cannot be 0");
+ return -1;
+ }
+ return 0;
+ }
+ else if (strcmp (key, "retry-request-open") == 0) {
+ r = nbdkit_parse_bool (value);
+ if (r == -1)
+ return -1;
+ retry_open_call = r;
+ return 0;
+ }
+
+ return next (nxdata, key, value);
+}
+
+#define retry_request_config_help \
+ "retry-request-retries=<N> Number of retries (default: 2).\n"
\
+ "retry-request-delay=<N> Seconds to wait before retry (default:
2).\n" \
+ "retry-request-open=false Do not retry opening the plugin (default:
true).\n"
+
+/* These macros encapsulate the logic of retrying. The code between
+ * RETRY_START...RETRY_END must set r to 0 or -1 on success or
+ * failure.
+ */
+#define RETRY_START \
+ { \
+ unsigned i; \
+ \
+ r = -1; \
+ for (i = 0; r == -1 && i <= retries; ++i) {
\
+ if (i > 0) { \
+ nbdkit_debug ("retry %u: waiting %u seconds before retrying",
\
+ i, delay); \
+ if (nbdkit_nanosleep (delay, 0) == -1) { \
+ if (*err == 0) \
+ *err = errno; \
+ break; \
+ } \
+ } \
+ do
+#define RETRY_END \
+ while (0); \
+ } \
+ }
+
+static void *
+retry_request_open (nbdkit_next_open *next, nbdkit_context *nxdata,
+ int readonly, const char *exportname, int is_tls)
+{
+ int r;
+
+ if (retry_open_call) {
+ int *err = &errno; /* used by the RETRY* macros */
+
+ RETRY_START
+ r = next (nxdata, readonly, exportname);
+ RETRY_END;
+ }
+ else {
+ r = next (nxdata, readonly, exportname);
+ }
+
+ return r == 0 ? NBDKIT_HANDLE_NOT_NEEDED : NULL;
+}
+
+static int
+retry_request_pread (nbdkit_next *next,
+ void *handle, void *buf, uint32_t count, uint64_t offset,
+ uint32_t flags, int *err)
+{
+ int r;
+
+ RETRY_START
+ r = next->pread (next, buf, count, offset, flags, err);
+ RETRY_END;
+ return r;
+}
+
+static int
+retry_request_pwrite (nbdkit_next *next,
+ void *handle,
+ const void *buf, uint32_t count, uint64_t offset,
+ uint32_t flags, int *err)
+{
+ int r;
+
+ RETRY_START
+ r = next->pwrite (next, buf, count, offset, flags, err);
+ RETRY_END;
+ return r;
+}
+
+static int
+retry_request_trim (nbdkit_next *next,
+ void *handle,
+ uint32_t count, uint64_t offset, uint32_t flags,
+ int *err)
+{
+ int r;
+
+ RETRY_START
+ r = next->trim (next, count, offset, flags, err);
+ RETRY_END;
+ return r;
+}
+
+static int
+retry_request_flush (nbdkit_next *next,
+ void *handle, uint32_t flags,
+ int *err)
+{
+ int r;
+
+ RETRY_START
+ r = next->flush (next, flags, err);
+ RETRY_END;
+ return r;
+}
+
+static int
+retry_request_zero (nbdkit_next *next,
+ void *handle,
+ uint32_t count, uint64_t offset, uint32_t flags,
+ int *err)
+{
+ int r;
+
+ RETRY_START
+ r = next->zero (next, count, offset, flags, err);
+ RETRY_END;
+ return r;
+}
+
+static int
+retry_request_extents (nbdkit_next *next,
+ void *handle,
+ uint32_t count, uint64_t offset, uint32_t flags,
+ struct nbdkit_extents *extents, int *err)
+{
+ CLEANUP_EXTENTS_FREE struct nbdkit_extents *extents2 = NULL;
+ int r;
+
+ RETRY_START {
+ /* Each retry must begin with extents reset to the right beginning. */
+ nbdkit_extents_free (extents2);
+ extents2 = nbdkit_extents_new (offset, next->get_size (next));
+ if (extents2 == NULL) {
+ *err = errno;
+ return -1; /* Not worth a retry after ENOMEM. */
+ }
+ r = next->extents (next, count, offset, flags, extents2, err);
+ } RETRY_END;
+
+ if (r == 0) {
+ size_t i;
+
+ /* Transfer the successful extents back to the caller. */
+ for (i = 0; i < nbdkit_extents_count (extents2); ++i) {
+ struct nbdkit_extent e = nbdkit_get_extent (extents2, i);
+
+ if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) {
+ *err = errno;
+ return -1;
+ }
+ }
+ }
+
+ return r;
+}
+
+static int
+retry_request_cache (nbdkit_next *next,
+ void *handle,
+ uint32_t count, uint64_t offset, uint32_t flags,
+ int *err)
+{
+ int r;
+
+ RETRY_START
+ r = next->cache (next, count, offset, flags, err);
+ RETRY_END;
+ return r;
+}
+
+static struct nbdkit_filter filter = {
+ .name = "retry-request",
+ .longname = "nbdkit retry request filter",
+ .thread_model = retry_request_thread_model,
+ .config = retry_request_config,
+ .config_help = retry_request_config_help,
+ .open = retry_request_open,
+ .pread = retry_request_pread,
+ .pwrite = retry_request_pwrite,
+ .trim = retry_request_trim,
+ .flush = retry_request_flush,
+ .zero = retry_request_zero,
+ .extents = retry_request_extents,
+ .cache = retry_request_cache,
+};
+
+NBDKIT_REGISTER_FILTER(filter)
diff --git a/tests/test-retry-request.sh b/tests/test-retry-request.sh
new file mode 100755
index 000000000..96c6a1c15
--- /dev/null
+++ b/tests/test-retry-request.sh
@@ -0,0 +1,94 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2018-2021 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_plugin sh
+requires nbdcopy --version
+requires dd iflag=count_bytes </dev/null
+
+files="retry-request.img retry-request-count"
+rm -f $files
+cleanup_fn rm -f $files
+
+touch retry-request-count
+start_t=$SECONDS
+
+# Create a custom plugin which will test retrying requests.
+nbdkit -v -U - \
+ sh - \
+ --filter=retry-request retry-request-retries=3 retry-request-delay=1 \
+ --run 'nbdcopy --synchronous "$uri" retry-request.img'
<<'EOF'
+#!/usr/bin/env bash
+case "$1" in
+ get_size) echo 512 ;;
+ pread)
+ # Fail 3 times then succeed.
+ read i < retry-request-count
+ ((i++))
+ echo $i > retry-request-count
+ if [ $i -le 3 ]; then
+ echo "EIO pread failed" >&2
+ exit 1
+ else
+ dd if=/dev/zero count=$3 iflag=count_bytes
+ fi
+ ;;
+
+ *) exit 2 ;;
+esac
+EOF
+
+# In this test we should see 3 failures:
+# pread FAILS
+# retry and wait 1 second
+# pread FAILS
+# retry and wait 1 second
+# pread FAILS
+# retry and wait 1 second
+# pread succeeds
+
+# The minimum time for the test should be 3 seconds.
+end_t=$SECONDS
+if [ $((end_t - start_t)) -lt 3 ]; then
+ echo "$0: test ran too quickly"
+ exit 1
+fi
+
+# Check retry-request-count = 4 (because we write #retries+1 to the file)
+read retry_count < retry-request-count
+if [ $retry_count -ne 4 ]; then
+ echo "$0: retry-request-count ($retry_count) != 4"
+ exit 1
+fi
--
2.32.0
Richard W.M. Jones
2021-Oct-15 14:17 UTC
[Libguestfs] [PATCH nbdkit v2 2/2] tests: Test retry-request + curl in a realistic mirroring situation
Test the new nbdkit-retry-request-filter with the curl plugin. I have
modified our test webserver so it provides a magical "/mirror" path
which acts somewhat like a download mirror with occasional broken
redirects.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2013000
---
tests/Makefile.am | 24 ++++
tests/test-retry-request-mirror.c | 130 ++++++++++++++++++++
tests/web-server.c | 197 +++++++++++++++++++++++++++---
.gitignore | 1 +
4 files changed, 337 insertions(+), 15 deletions(-)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 9a1485a6c..b8a3a654c 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1701,6 +1701,30 @@ EXTRA_DIST += \
test-retry-request.sh \
$(NULL)
+LIBNBD_TESTS += test-retry-request-mirror
+
+test_retry_request_mirror_SOURCES = \
+ test-retry-request-mirror.c \
+ web-server.c \
+ web-server.h \
+ test.h \
+ $(NULL)
+test_retry_request_mirror_CPPFLAGS = \
+ -I$(top_srcdir)/common/utils
+test_retry_request_mirror_CFLAGS = \
+ $(WARNINGS_CFLAGS) \
+ $(PTHREAD_CFLAGS) \
+ $(LIBNBD_CFLAGS) \
+ $(NULL)
+test_retry_request_mirror_LDFLAGS = \
+ $(top_builddir)/common/utils/libutils.la \
+ $(PTHREAD_LIBS) \
+ $(NULL)
+test_retry_request_mirror_LDADD = \
+ libtest.la \
+ $(LIBNBD_LIBS) \
+ $(NULL)
+
# swab filter test.
TESTS += \
test-swab-8.sh \
diff --git a/tests/test-retry-request-mirror.c
b/tests/test-retry-request-mirror.c
new file mode 100644
index 000000000..861e2e0e5
--- /dev/null
+++ b/tests/test-retry-request-mirror.c
@@ -0,0 +1,130 @@
+/* nbdkit
+ * Copyright (C) 2013-2021 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 is a test of recovering from broken redirects to a mirror
+ * service. See the following bug for background:
+ * https://bugzilla.redhat.com/show_bug.cgi?id=2013000
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <inttypes.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <libnbd.h>
+
+#include "cleanup.h"
+#include "web-server.h"
+
+#include "test.h"
+
+int
+main (int argc, char *argv[])
+{
+ const char *sockpath;
+ struct nbd_handle *nbd;
+ CLEANUP_FREE char *usp_param = NULL;
+ char buf[512];
+ int state, i;
+
+#ifndef HAVE_CURLOPT_UNIX_SOCKET_PATH
+ fprintf (stderr, "%s: curl does not support
CURLOPT_UNIX_SOCKET_PATH\n",
+ argv[0]);
+ exit (77);
+#endif
+
+ sockpath = web_server ("disk", NULL);
+ if (sockpath == NULL) {
+ fprintf (stderr, "%s: could not start web server thread\n",
argv[0]);
+ exit (EXIT_FAILURE);
+ }
+
+ /* Start nbdkit. */
+ if (asprintf (&usp_param, "unix-socket-path=%s", sockpath) ==
-1) {
+ perror ("asprintf");
+ exit (EXIT_FAILURE);
+ }
+ if (test_start_nbdkit ("--filter=retry-request",
+ "curl", usp_param,
"http://localhost/mirror",
+ "retry-request-delay=1",
+ NULL) == -1)
+ exit (EXIT_FAILURE);
+
+ nbd = nbd_create ();
+ if (nbd == NULL) {
+ nbd_error:
+ fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
+
+ if (nbd_connect_unix (nbd, sock /* NBD socket */) == -1)
+ goto nbd_error;
+
+ /* The way the test works is we fetch the magic "/mirror" path (see
+ * web-server.c). This redirects to /mirror1, /mirror2, /mirror3
+ * round robin on each request. /mirror1 returns all 1's, /mirror2
+ * returns all 2's, and /mirror3 returns a 404 error. The 404 error
+ * should be transparently skipped by the filter, so we should see
+ * alternating 1's and 2's buffers.
+ */
+ if (nbd_pread (nbd, buf, sizeof buf, 0, 0) == -1)
+ goto nbd_error;
+
+ if (buf[0] == 1 && buf[1] == 1 && buf[511] == 1)
+ state = 2;
+ else if (buf[0] == 2 && buf[1] == 2 && buf[511] == 2)
+ state = 1;
+ else {
+ fprintf (stderr, "%s: unexpected start state\n", argv[0]);
+ exit (EXIT_FAILURE);
+ }
+
+ for (i = 0; i < 10; ++i) {
+ if (nbd_pread (nbd, buf, sizeof buf, 0, 0) == -1)
+ goto nbd_error;
+ if (buf[0] != state || buf[1] != state || buf[511] != state) {
+ fprintf (stderr, "%s: unexpected state\n", argv[0]);
+ exit (EXIT_FAILURE);
+ }
+ state++;
+ if (state == 3)
+ state = 1;
+ }
+
+ nbd_close (nbd);
+ exit (EXIT_SUCCESS);
+}
diff --git a/tests/web-server.c b/tests/web-server.c
index ac44d16b2..07a2ec398 100644
--- a/tests/web-server.c
+++ b/tests/web-server.c
@@ -57,6 +57,8 @@
#define SOCK_CLOEXEC 0
#endif
+enum method { HEAD, GET };
+
static char tmpdir[] = "/tmp/wsXXXXXX";
static char sockpath[] = "............./sock";
static int listen_sock = -1;
@@ -67,7 +69,12 @@ static check_request_t check_request;
static void *start_web_server (void *arg);
static void handle_requests (int s);
-static void handle_request (int s, bool headers_only);
+static void handle_file_request (int s, enum method method);
+static void handle_mirror_redirect_request (int s);
+static void handle_mirror_data_request (int s, enum method method, char byte);
+static void send_404_not_found (int s);
+static void send_405_method_not_allowed (int s);
+static void send_500_internal_server_error (int s);
static void xwrite (int s, const char *buf, size_t len);
static void xpread (char *buf, size_t count, off_t offset);
@@ -177,12 +184,15 @@ start_web_server (void *arg)
static void
handle_requests (int s)
{
- size_t r, n, sz;
bool eof = false;
fprintf (stderr, "web server: accepted connection\n");
while (!eof) {
+ size_t r, n, sz;
+ enum method method;
+ char path[128];
+
/* Read request until we see "\r\n\r\n" (end of headers) or EOF.
*/
n = 0;
for (;;) {
@@ -214,30 +224,73 @@ handle_requests (int s)
/* Call the optional user function to check the request. */
if (check_request) check_request (request);
- /* HEAD or GET request? */
- if (strncmp (request, "HEAD ", 5) == 0)
- handle_request (s, true);
- else if (strncmp (request, "GET ", 4) == 0)
- handle_request (s, false);
+ /* Get the method and path fields from the first line. */
+ if (strncmp (request, "HEAD ", 5) == 0) {
+ method = HEAD;
+ n = strcspn (&request[5], " \n\t");
+ if (n >= sizeof path) {
+ send_500_internal_server_error (s);
+ eof = true;
+ break;
+ }
+ memcpy (path, &request[5], n);
+ path[n] = '\0';
+ }
+ else if (strncmp (request, "GET ", 4) == 0) {
+ method = GET;
+ n = strcspn (&request[4], " \n\t");
+ if (n >= sizeof path) {
+ send_500_internal_server_error (s);
+ eof = true;
+ break;
+ }
+ memcpy (path, &request[4], n);
+ path[n] = '\0';
+ }
else {
- /* Return 405 Method Not Allowed. */
- const char response[] - "HTTP/1.1 405 Method Not
Allowed\r\n"
- "Content-Length: 0\r\n"
- "Connection: close\r\n"
- "\r\n";
- xwrite (s, response, strlen (response));
+ send_405_method_not_allowed (s);
eof = true;
break;
}
+
+ fprintf (stderr, "web server: requested path: %s\n", path);
+
+ /* For testing retry-request + curl:
+ * /mirror redirects round-robin to /mirror1, /mirror2, /mirror3
+ * /mirror1 returns a file of \x01 bytes
+ * /mirror2 returns a file of \x02 bytes
+ * /mirror3 returns 404 errors
+ * Anything else returns a 500 error
+ */
+ if (strcmp (path, "/mirror") == 0)
+ handle_mirror_redirect_request (s);
+ else if (strcmp (path, "/mirror1") == 0)
+ handle_mirror_data_request (s, method, 1);
+ else if (strcmp (path, "/mirror2") == 0)
+ handle_mirror_data_request (s, method, 2);
+ else if (strcmp (path, "/mirror3") == 0) {
+ send_404_not_found (s);
+ eof = true;
+ }
+ else if (strncmp (path, "/mirror", 7) == 0) {
+ send_500_internal_server_error (s);
+ eof = true;
+ }
+
+ /* Otherwise it's a regular file request. 'path' is ignored,
we
+ * only serve a single file passed to web_server().
+ */
+ else
+ handle_file_request (s, method);
}
close (s);
}
static void
-handle_request (int s, bool headers_only)
+handle_file_request (int s, enum method method)
{
+ const bool headers_only = method == HEAD;
uint64_t offset, length, end;
const char *p;
const char response1_ok[] = "HTTP/1.1 200 OK\r\n";
@@ -295,6 +348,120 @@ handle_request (int s, bool headers_only)
free (data);
}
+/* Request for /mirror */
+static void
+handle_mirror_redirect_request (int s)
+{
+ static char rr = '1'; /* round robin '1',
'2', '3' */
+ /* Note we send 302 (temporary redirect), same as Fedora's mirrorservice.
*/
+ const char found[] = "HTTP/1.1 302 Found\r\nContent-Length: 0\r\n";
+ char location[] = "Location: /mirrorX\r\n";
+ const char eol[] = "\r\n";
+
+ location[17] = rr;
+ rr++;
+ if (rr == '4')
+ rr = '1';
+
+ xwrite (s, found, strlen (found));
+ xwrite (s, location, strlen (location));
+ xwrite (s, eol, strlen (eol));
+}
+
+static void
+handle_mirror_data_request (int s, enum method method, char byte)
+{
+ const bool headers_only = method == HEAD;
+ uint64_t offset, length, end;
+ const char *p;
+ const char response1_ok[] = "HTTP/1.1 200 OK\r\n";
+ const char response1_partial[] = "HTTP/1.1 206 Partial
Content\r\n";
+ const char response2[] + "Accept-rANGES: bytes\r\n" /* See
RHBZ#1837337 */
+ "Connection: keep-alive\r\n"
+ "Content-Type: application/octet-stream\r\n";
+ char response3[64];
+ const char response4[] = "\r\n";
+ char *data;
+
+ /* If there's no Range request header then send the full size as the
+ * content-length.
+ */
+ p = strcasestr (request, "\r\nRange: bytes=");
+ if (p == NULL) {
+ offset = 0;
+ length = statbuf.st_size;
+ xwrite (s, response1_ok, strlen (response1_ok));
+ }
+ else {
+ p += 15;
+ if (sscanf (p, "%" SCNu64 "-%" SCNu64, &offset,
&end) != 2) {
+ fprintf (stderr, "web server: could not parse "
+ "range request from curl client\n");
+ exit (EXIT_FAILURE);
+ }
+ /* Unclear but "Range: bytes=0-4" means bytes 0-3. '4'
is the
+ * byte beyond the end of the range.
+ */
+ length = end - offset;
+ xwrite (s, response1_partial, strlen (response1_partial));
+ }
+
+ xwrite (s, response2, strlen (response2));
+ snprintf (response3, sizeof response3,
+ "Content-Length: %" PRIu64 "\r\n", length);
+ xwrite (s, response3, strlen (response3));
+ xwrite (s, response4, strlen (response4));
+
+ if (headers_only)
+ return;
+
+ /* Send the file content. */
+ data = malloc (length);
+ if (data == NULL) {
+ perror ("malloc");
+ exit (EXIT_FAILURE);
+ }
+
+ memset (data, byte, length);
+ xwrite (s, data, length);
+
+ free (data);
+}
+
+static void
+send_404_not_found (int s)
+{
+ const char response[] + "HTTP/1.1 404 Not Found\r\n"
+ "Content-Length: 0\r\n"
+ "Connection: close\r\n"
+ "\r\n";
+ xwrite (s, response, strlen (response));
+}
+
+static void
+send_405_method_not_allowed (int s)
+{
+ const char response[] + "HTTP/1.1 405 Method Not Allowed\r\n"
+ "Content-Length: 0\r\n"
+ "Connection: close\r\n"
+ "\r\n";
+ xwrite (s, response, strlen (response));
+}
+
+static void
+send_500_internal_server_error (int s)
+{
+ const char response[] + "HTTP/1.1 500 Internal Server Error\r\n"
+ "Content-Length: 0\r\n"
+ "Connection: close\r\n"
+ "\r\n";
+ xwrite (s, response, strlen (response));
+}
+
static void
xwrite (int s, const char *buf, size_t len)
{
diff --git a/.gitignore b/.gitignore
index 847b72dd6..e7b07d510 100644
--- a/.gitignore
+++ b/.gitignore
@@ -155,6 +155,7 @@ plugins/*/*.3
/tests/test-python
/tests/test-random
/tests/test-readahead
+/tests/test-retry-request-mirror
/tests/test-ruby
/tests/test-S3/boto3/__pycache__/
/tests/test-shell
--
2.32.0
Alexander Wels
2021-Oct-18 14:09 UTC
[Libguestfs] [PATCH nbdkit v2 0/2] New filter: nbdkit-retry-request-filter
On Fri, Oct 15, 2021 at 9:17 AM Richard W.M. Jones <rjones at redhat.com> wrote:> I believe this addresses everything mentioned in the previous reviews > (sorry if I missed anything). In addition I have extended > web-server.c to implement a fairly realistic test scenario. > > Alexander: It would be useful if you could test this filter in your > code as well so we can be sure it really fixes your problem. > > Rich. > >Sure is there an rpm or something I can pull into my container? -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://listman.redhat.com/archives/libguestfs/attachments/20211018/e778e90e/attachment.htm>