On Thu, Oct 14, 2021 at 09:58:40PM +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:You'll want to fix the subject line: retry-request, not request-request.> https://bugzilla.redhat.com/show_bug.cgi?id=2013000 > ---> +++ b/filters/retry-request/nbdkit-retry-request-filter.pod > @@ -0,0 +1,67 @@ > +=head1 NAME > + > +nbdkit-retry-request-filter - retry single requests on error > + > +=head1 SYNOPSIS > + > + nbdkit --filter=retry-request PLUGIN [retry-request-retries=N]Should this mention retry-request-delay?> + > +=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 areeither 'servers ... are' or 'server ... 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.Do we want this to support both '1' (seconds implied) and '1ms' (milliseconds), similar to the delay filter, for finer granularity? Do we want any sort of exponential backoff knob?> +++ b/filters/retry-request/retry-request.c > + > +#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" > + > +/* This macro encapsulates the logic of retrying. */ > +#define RETRY(code) \ > + do { \ > + unsigned i; \ > + r = -1; \Assumes 'r' is already declared and in scope, but matches use below.> + for (i = 0; r == -1 && i <= retries; ++i) { \ > + if (i > 0) { \ > + nbdkit_debug ("retry %d: waiting %d seconds before retrying", \ > + i, delay); \ > + if (nbdkit_nanosleep (delay, 0) == -1) { \ > + if (*err == 0) *err = errno; \ > + return -1; \ > + } \ > + } \ > + code; \Assumes that 'code' expands to something that sets 'r'; worth documenting?> + } \ > + } while (0) > + > +/* XXX This doesn't attempt to retry the initial open call. Should it? > + * Argument against: You probably want the initial open to fail > + * quickly. Argument for: curl's open call makes a request (to fetch > + * the size) so retrying could be useful. > + */Both good arguments. Maybe worth a separate knob?> + > +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 (r = next->pread (next, buf, count, offset, flags, err)); > + return r; > +} > +... Fairly straightforward.> +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 ( > + /* 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); > + );Wow - quite the large amount of code crammed in that macro call. All of the ',' in that code occur inside another set of (), so you do end up with only one argument to the macro proper. Would it read any cleaner if we had two macros, RETRY_START and RETRY_END, to write things like: RETRY_START /* Each retry ... */ nbdkit_extents_free (extents2); ... r = next->extents (...); 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; > +}Retrying extents is complicated, but your code looks correct. I know this is just an RFC, but the idea looks like it may have merit. Would we want to merge this functionality into the 'retry' filter (a single filter, with a knob on whether to retry single requests vs. full connection), or would that be too heavyweight for one filter? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2021-Oct-14 21:28 UTC
[Libguestfs] [PATCH nbdkit] New filter: request-request
On Thu, Oct 14, 2021 at 04:22:27PM -0500, Eric Blake wrote:> I know this is just an RFC, but the idea looks like it may have merit. > Would we want to merge this functionality into the 'retry' filter (a > single filter, with a knob on whether to retry single requests > vs. full connection), or would that be too heavyweight for one filter?I did look at that, but the current retry filter is complicated enough and this would make it quite a lot more complicated (unless you can see some easy way to do it). Also the two filters do have somewhat different behaviour and perhaps use cases. I really need a way to test this with the curl plugin to check that it really does what I think it does. In particular I'm not convinced curl will actually go through the whole redirect phase in this case, which would mean it doesn't actually solve Alexander's problem. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/