mirror of
https://github.com/GoodOlClint/PSProxmoxVE.git
synced 2026-09-04 03:05:32 +00:00
fix: report each flock reissue, and scale the gap to the retry budget
Two non-blocking review observations. A 45s retry is indistinguishable from a hang with nothing on the wire, so GuestLockRetry.Execute takes an onRetry hook and InvokeGuestTask reports each reissue through WriteVerbose. The gap between attempts now scales with the budget, capped at the 2s production value. A caller passing a short window wants a fast answer rather than one long sleep, which also takes the retrying unit tests off a real 2s sleep each: the xUnit run drops from 8s to 4s. PveHttpClient's window becomes a field so those tests can shorten it too.
This commit is contained in:
@@ -30,6 +30,8 @@ namespace PSProxmoxVE.Core.Client
|
|||||||
private readonly HttpClient _httpClient;
|
private readonly HttpClient _httpClient;
|
||||||
private bool _disposed;
|
private bool _disposed;
|
||||||
|
|
||||||
|
private TimeSpan _guestLockRetryWindow = GuestLockRetry.DefaultWindow;
|
||||||
|
|
||||||
private const string ApiTokenPrefix = "PVEAPIToken=";
|
private const string ApiTokenPrefix = "PVEAPIToken=";
|
||||||
private const string AuthCookieName = "PVEAuthCookie=";
|
private const string AuthCookieName = "PVEAuthCookie=";
|
||||||
private const string CsrfHeaderName = "CSRFPreventionToken";
|
private const string CsrfHeaderName = "CSRFPreventionToken";
|
||||||
@@ -382,7 +384,8 @@ namespace PSProxmoxVE.Core.Client
|
|||||||
/// cannot be resent, which is why this takes a factory rather than a request.
|
/// cannot be resent, which is why this takes a factory rather than a request.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
private Task<string> SendAsync(Func<HttpRequestMessage> buildRequest, string resource, string httpMethod) =>
|
private Task<string> SendAsync(Func<HttpRequestMessage> buildRequest, string resource, string httpMethod) =>
|
||||||
GuestLockRetry.ExecuteAsync(() => SendOnceAsync(buildRequest(), resource, httpMethod));
|
GuestLockRetry.ExecuteAsync(
|
||||||
|
() => SendOnceAsync(buildRequest(), resource, httpMethod), _guestLockRetryWindow);
|
||||||
|
|
||||||
private async Task<string> SendOnceAsync(HttpRequestMessage request, string resource, string httpMethod)
|
private async Task<string> SendOnceAsync(HttpRequestMessage request, string resource, string httpMethod)
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -24,7 +24,15 @@ namespace PSProxmoxVE.Core.Utilities
|
|||||||
/// </summary>
|
/// </summary>
|
||||||
public static readonly TimeSpan DefaultWindow = TimeSpan.FromSeconds(45);
|
public static readonly TimeSpan DefaultWindow = TimeSpan.FromSeconds(45);
|
||||||
|
|
||||||
private static readonly TimeSpan RetryInterval = TimeSpan.FromSeconds(2);
|
private static readonly TimeSpan MaxRetryInterval = TimeSpan.FromSeconds(2);
|
||||||
|
|
||||||
|
// The gap must never eat a meaningful share of a short budget: a caller passing a
|
||||||
|
// small window wants a fast answer, not one long sleep.
|
||||||
|
private static TimeSpan RetryInterval(TimeSpan budget)
|
||||||
|
{
|
||||||
|
var quarter = TimeSpan.FromMilliseconds(budget.TotalMilliseconds / 4);
|
||||||
|
return quarter < MaxRetryInterval ? quarter : MaxRetryInterval;
|
||||||
|
}
|
||||||
|
|
||||||
// Anchored, and specific to the two guest lock paths. `PVE::Tools::lock_file` emits this
|
// Anchored, and specific to the two guest lock paths. `PVE::Tools::lock_file` emits this
|
||||||
// same wording for storage, LVM, HA and firewall locks, none of which carry the
|
// same wording for storage, LVM, HA and firewall locks, none of which carry the
|
||||||
@@ -59,7 +67,11 @@ namespace PSProxmoxVE.Core.Utilities
|
|||||||
/// </summary>
|
/// </summary>
|
||||||
/// <param name="operation">The operation to run.</param>
|
/// <param name="operation">The operation to run.</param>
|
||||||
/// <param name="window">Retry budget. Defaults to <see cref="DefaultWindow"/>.</param>
|
/// <param name="window">Retry budget. Defaults to <see cref="DefaultWindow"/>.</param>
|
||||||
public static T Execute<T>(Func<T> operation, TimeSpan? window = null)
|
/// <param name="onRetry">
|
||||||
|
/// Invoked with the rejection before each reissue. A caller with somewhere to report
|
||||||
|
/// progress should pass one — a wait this long is otherwise indistinguishable from a hang.
|
||||||
|
/// </param>
|
||||||
|
public static T Execute<T>(Func<T> operation, TimeSpan? window = null, Action<Exception>? onRetry = null)
|
||||||
{
|
{
|
||||||
if (operation == null) throw new ArgumentNullException(nameof(operation));
|
if (operation == null) throw new ArgumentNullException(nameof(operation));
|
||||||
|
|
||||||
@@ -73,7 +85,8 @@ namespace PSProxmoxVE.Core.Utilities
|
|||||||
}
|
}
|
||||||
catch (Exception ex) when (IsLockTimeout(ex) && elapsed.Elapsed < budget)
|
catch (Exception ex) when (IsLockTimeout(ex) && elapsed.Elapsed < budget)
|
||||||
{
|
{
|
||||||
Thread.Sleep(RetryInterval);
|
onRetry?.Invoke(ex);
|
||||||
|
Thread.Sleep(RetryInterval(budget));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -95,7 +108,7 @@ namespace PSProxmoxVE.Core.Utilities
|
|||||||
}
|
}
|
||||||
catch (Exception ex) when (IsLockTimeout(ex) && elapsed.Elapsed < budget)
|
catch (Exception ex) when (IsLockTimeout(ex) && elapsed.Elapsed < budget)
|
||||||
{
|
{
|
||||||
await Task.Delay(RetryInterval).ConfigureAwait(false);
|
await Task.Delay(RetryInterval(budget)).ConfigureAwait(false);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -180,13 +180,15 @@ namespace PSProxmoxVE.Cmdlets
|
|||||||
if (issueOperation == null) throw new ArgumentNullException(nameof(issueOperation));
|
if (issueOperation == null) throw new ArgumentNullException(nameof(issueOperation));
|
||||||
|
|
||||||
var taskService = new TaskService();
|
var taskService = new TaskService();
|
||||||
return GuestLockRetry.Execute(() =>
|
return GuestLockRetry.Execute(
|
||||||
{
|
() =>
|
||||||
var task = issueOperation();
|
{
|
||||||
return string.IsNullOrEmpty(task.Upid)
|
var task = issueOperation();
|
||||||
? task
|
return string.IsNullOrEmpty(task.Upid)
|
||||||
: taskService.WaitForTask(session, node, task.Upid, null, null, null);
|
? task
|
||||||
});
|
: taskService.WaitForTask(session, node, task.Upid, null, null, null);
|
||||||
|
},
|
||||||
|
onRetry: ex => WriteVerbose($"Guest is locked, retrying: {ex.Message}"));
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
|
|||||||
@@ -25,6 +25,15 @@ namespace PSProxmoxVE.Core.Tests.Client
|
|||||||
field.SetValue(client, newInner);
|
field.SetValue(client, newInner);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// The gap between attempts scales with the budget, so a short window keeps these
|
||||||
|
// tests off the 2s production sleep.
|
||||||
|
private static void SetRetryWindow(PveHttpClient client, TimeSpan window)
|
||||||
|
{
|
||||||
|
var field = typeof(PveHttpClient).GetField("_guestLockRetryWindow",
|
||||||
|
BindingFlags.Instance | BindingFlags.NonPublic)!;
|
||||||
|
field.SetValue(client, window);
|
||||||
|
}
|
||||||
|
|
||||||
private static (PveHttpClient client, ScriptedHandler handler) NewClient(
|
private static (PveHttpClient client, ScriptedHandler handler) NewClient(
|
||||||
params (HttpStatusCode status, string body)[] responses)
|
params (HttpStatusCode status, string body)[] responses)
|
||||||
{
|
{
|
||||||
@@ -33,6 +42,7 @@ namespace PSProxmoxVE.Core.Tests.Client
|
|||||||
var client = new PveHttpClient(session);
|
var client = new PveHttpClient(session);
|
||||||
var handler = new ScriptedHandler(responses);
|
var handler = new ScriptedHandler(responses);
|
||||||
SetInnerHttpClient(client, new HttpClient(handler));
|
SetInnerHttpClient(client, new HttpClient(handler));
|
||||||
|
SetRetryWindow(client, TimeSpan.FromMilliseconds(400));
|
||||||
return (client, handler);
|
return (client, handler);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -1,4 +1,5 @@
|
|||||||
using System;
|
using System;
|
||||||
|
using System.Collections.Generic;
|
||||||
using System.Net;
|
using System.Net;
|
||||||
using System.Threading.Tasks;
|
using System.Threading.Tasks;
|
||||||
using PSProxmoxVE.Core.Exceptions;
|
using PSProxmoxVE.Core.Exceptions;
|
||||||
@@ -15,6 +16,10 @@ namespace PSProxmoxVE.Core.Tests.Utilities
|
|||||||
private const string LxcLockError =
|
private const string LxcLockError =
|
||||||
"can't lock file '/run/lock/lxc/pve-config-100.lock' - got timeout";
|
"can't lock file '/run/lock/lxc/pve-config-100.lock' - got timeout";
|
||||||
|
|
||||||
|
// The gap between attempts scales with the budget, so a short window keeps the
|
||||||
|
// retrying tests off a 2s production sleep.
|
||||||
|
private static readonly TimeSpan ShortWindow = TimeSpan.FromMilliseconds(400);
|
||||||
|
|
||||||
private static PveApiException ApiError(string message) =>
|
private static PveApiException ApiError(string message) =>
|
||||||
new PveApiException(HttpStatusCode.InternalServerError, message, "nodes/pve9a/qemu/100/config", "PUT");
|
new PveApiException(HttpStatusCode.InternalServerError, message, "nodes/pve9a/qemu/100/config", "PUT");
|
||||||
|
|
||||||
@@ -73,6 +78,23 @@ namespace PSProxmoxVE.Core.Tests.Utilities
|
|||||||
Assert.False(GuestLockRetry.IsLockTimeout(new InvalidOperationException(VmLockError)));
|
Assert.False(GuestLockRetry.IsLockTimeout(new InvalidOperationException(VmLockError)));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void Execute_ReportsEachReissueToTheOnRetryHook()
|
||||||
|
{
|
||||||
|
var attempts = 0;
|
||||||
|
var reported = new List<Exception>();
|
||||||
|
|
||||||
|
GuestLockRetry.Execute(() =>
|
||||||
|
{
|
||||||
|
attempts++;
|
||||||
|
if (attempts < 3) throw TaskError(VmLockError);
|
||||||
|
return 0;
|
||||||
|
}, ShortWindow, onRetry: reported.Add);
|
||||||
|
|
||||||
|
Assert.Equal(2, reported.Count);
|
||||||
|
Assert.All(reported, e => Assert.IsType<PveTaskFailedException>(e));
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public void Execute_ReturnsWithoutRetryingWhenTheOperationSucceeds()
|
public void Execute_ReturnsWithoutRetryingWhenTheOperationSucceeds()
|
||||||
{
|
{
|
||||||
@@ -94,7 +116,7 @@ namespace PSProxmoxVE.Core.Tests.Utilities
|
|||||||
attempts++;
|
attempts++;
|
||||||
if (attempts < 2) throw TaskError(VmLockError);
|
if (attempts < 2) throw TaskError(VmLockError);
|
||||||
return "cloned";
|
return "cloned";
|
||||||
});
|
}, ShortWindow);
|
||||||
|
|
||||||
Assert.Equal("cloned", result);
|
Assert.Equal("cloned", result);
|
||||||
Assert.Equal(2, attempts);
|
Assert.Equal(2, attempts);
|
||||||
@@ -124,7 +146,7 @@ namespace PSProxmoxVE.Core.Tests.Utilities
|
|||||||
attempts++;
|
attempts++;
|
||||||
if (attempts < 2) throw ApiError(VmLockError);
|
if (attempts < 2) throw ApiError(VmLockError);
|
||||||
return Task.FromResult("written");
|
return Task.FromResult("written");
|
||||||
});
|
}, ShortWindow);
|
||||||
|
|
||||||
Assert.Equal("written", result);
|
Assert.Equal("written", result);
|
||||||
Assert.Equal(2, attempts);
|
Assert.Equal(2, attempts);
|
||||||
|
|||||||
Reference in New Issue
Block a user