[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.
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
On Fri, Oct 15, 2021 at 03:17:16PM +0100, Richard W.M. Jones wrote:
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:
+++ 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
to a web
+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. + +++ b/filters/retry-request/retry-request.c @@ -0,0 +1,278 @@
+ +/* 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; \
You documented 'r', but not 'err' needing to be in scope as well.
+ 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;
Most other calls into the plugin are obviously safe, but there are lots of asserts around calls into nbdkit_next_open obeying expectations - if you didn't trip any when .open happened to hit the bad mirror out of the round robin, then I think we're okay here on life-cycle.
+ } + 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;
The simple case is still legible,...
+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;
...and the complex case seems much nicer. Yeah, I like this layout better.
+ + 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;
This is another spot where failure is not due to a flaky plugin, but to internal errors (probably ENOMEM); I agree with your analysis that retrying the transfer is not worthwhile.
+++ b/tests/test-retry-request.sh @@ -0,0 +1,94 @@ +#!/usr/bin/env bash
+ +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
Looks okay. But we should also test a retry of .open failing, since that one goes through enough difference in code path to be worth checking closely. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Fri, Oct 15, 2021 at 10:12:26AM -0500, Eric Blake wrote:
On Fri, Oct 15, 2021 at 03:17:16PM +0100, Richard W.M. Jones wrote:
+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;
Most other calls into the plugin are obviously safe, but there are lots of asserts around calls into nbdkit_next_open obeying expectations - if you didn't trip any when .open happened to hit the bad mirror out of the round robin, then I think we're okay here on life-cycle.
I was apprehensive about this one too, but it seems to work. See proposed test & output attached. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On 10/15/21 16:17, Richard W.M. Jones wrote:
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 */
I wondered if these were safe (i.e., taking the address of errno, plus the resultant effective "errno=errno" assignment in RETRY_START). Apparently "errno" only needs to be a "modifiable lvalue" -- but that does suffice for the & operator, and the assignment too. Acked-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo
+ + 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
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
On 10/15/21 16:17, Richard W.M. Jones wrote:
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(-)
I've mostly only skimmed this, for it being a test; it looks OK to me. I have some superficial comments / questions:
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);
Is exit() safe here (with the web server running in another thread)? Furthermore, should we do anything for (e.g.) deleting the pathname of the unix domain socket? (I don't think it matters much; I'm asking this is mostly for my own education.) And then that would apply more or less to every error exit after this point.
+ } + 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); + }
Can we put this block of code at the very end of the function, and here, just say "goto nbd_error"? Because right now we have gotos jumping backward, which I find awkward even for (otherwise justified) "algorithmic reasons", and very awkward for a usual error path. ... Either way: Acked-by: Laszlo Ersek <lersek@redhat.com> Thanks! Laszlo
+ + 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
On Mon, Oct 18, 2021 at 08:27:36PM +0200, Laszlo Ersek wrote:
On 10/15/21 16:17, Richard W.M. Jones wrote:
+#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);
Is exit() safe here (with the web server running in another thread)?
I didn't know, but I temporarily modified the test changing #ifndef -> #ifdef above and it did exit (skip) fine.
Furthermore, should we do anything for (e.g.) deleting the pathname of the unix domain socket? (I don't think it matters much; I'm asking this is mostly for my own education.) And then that would apply more or less to every error exit after this point.
I had to go and check this, and it turns out we do unlink the socket in an at-exit handler (cleanup) in tests/web-server.c, so I think we're good even on error paths.
+ } + 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); + }
Can we put this block of code at the very end of the function, and here, just say "goto nbd_error"?
Because right now we have gotos jumping backward, which I find awkward even for (otherwise justified) "algorithmic reasons", and very awkward for a usual error path.
OK, adjusted it as you say. Thanks, Rich.
... Either way:
Acked-by: Laszlo Ersek <lersek@redhat.com>
Thanks! Laszlo
+ + 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
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On Mon, Oct 18, 2021 at 10:12:49PM +0100, Richard W.M. Jones wrote:
On Mon, Oct 18, 2021 at 08:27:36PM +0200, Laszlo Ersek wrote:
On 10/15/21 16:17, Richard W.M. Jones wrote:
+#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);
Is exit() safe here (with the web server running in another thread)?
I didn't know, but I temporarily modified the test changing #ifndef -> #ifdef above and it did exit (skip) fine.
Actually not a good test, because that happens before the web server launches. Inserting an exit later in the code works as expected. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On 10/18/21 23:30, Richard W.M. Jones wrote:
On Mon, Oct 18, 2021 at 10:12:49PM +0100, Richard W.M. Jones wrote:
On Mon, Oct 18, 2021 at 08:27:36PM +0200, Laszlo Ersek wrote:
On 10/15/21 16:17, Richard W.M. Jones wrote:
+#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);
Is exit() safe here (with the web server running in another thread)?
I didn't know, but I temporarily modified the test changing #ifndef -> #ifdef above and it did exit (skip) fine.
Actually not a good test, because that happens before the web server launches. Inserting an exit later in the code works as expected.
Awesome, thanks. It's really thoughtful that web_server() "owns" the unix domain socket, i.e., that it takes responsibility for removing the pathname. Laszlo
On Fri, Oct 15, 2021 at 9:17 AM Richard W.M. Jones <rjones@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?
On Mon, Oct 18, 2021 at 09:09:57AM -0500, Alexander Wels wrote:
On Fri, Oct 15, 2021 at 9:17 AM Richard W.M. Jones <rjones@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?
No, you'd have to either build it from source and use /path/to/nbdkit to run it from the build directory[1] or else add the patch to the Fedora package[2] and build an RPM from it. [1] https://gitlab.com/nbdkit/nbdkit/-/blob/master/README "Building" [2] https://src.fedoraproject.org/rpms/nbdkit/tree/rawhide Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
participants (4)
-
Alexander Wels -
Eric Blake -
Laszlo Ersek -
Richard W.M. Jones