From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202502 header.b=gM0DSa3g; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id AA2535A0008 for ; Fri, 21 Feb 2025 07:28:58 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202502; t=1740106236; bh=FgFBcKFPyK3UX7Gs0csrQ1YFrJm/8up/xPJTenPAXII=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gM0DSa3gwcE8EwkHrCsIK1yylSqL94y4O8nmQ2oku0XzBPtDWpoWgE4ZBcO0kvJ1E isa6kCkzzs5nDd0DHFnxiMHdgzuc42YmXJME9yPkFUvTGoN+RP+KKr5zngzHhtxe6s IMEjTFw1RVUkzUQ0D1JlMXdGmO8VZveJLMPIS1xKdVv8AViY+KhbpSoiqsK9fsElu3 E+N7nA04Q8mBCLHtA9RZSAmVS/iDd4s7wj902Rzyb2dQ/K8ZwzsimBGlyjql8wJBvL HMAU4QEAWartcQeQj1DVnH2YSATnu3EwpKamPzMWJmpuury1rC0ut7JBoRlO/YRBAS czfa9mgxbjxsQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4YzZPS4Btnz4x5f; Fri, 21 Feb 2025 13:50:36 +1100 (AEDT) Date: Fri, 21 Feb 2025 13:40:12 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 2/2] migrate, flow: Don't attempt to migrate TCP flows without passt-repair Message-ID: References: <20250220060318.1796504-1-david@gibson.dropbear.id.au> <20250220060318.1796504-3-david@gibson.dropbear.id.au> <20250220090726.43432475@elisabeth> <20250220113800.05be8f5f@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="zk+x4U7X96IR+heo" Content-Disposition: inline In-Reply-To: <20250220113800.05be8f5f@elisabeth> Message-ID-Hash: 7N2ODLMIFPLZDCMWLIRKOID5WEJNMEIS X-Message-ID-Hash: 7N2ODLMIFPLZDCMWLIRKOID5WEJNMEIS X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --zk+x4U7X96IR+heo Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Feb 20, 2025 at 11:38:00AM +0100, Stefano Brivio wrote: > On Thu, 20 Feb 2025 21:18:06 +1100 > David Gibson wrote: >=20 > > On Thu, Feb 20, 2025 at 09:07:26AM +0100, Stefano Brivio wrote: > > > On Thu, 20 Feb 2025 17:03:18 +1100 > > > David Gibson wrote: > > > =20 > > > > Migrating TCP flows requires passt-repair in order to use TCP_REPAI= R. If > > > > passt-repair is not started, our failure mode is pretty ugly though= : we'll > > > > attempt the migration, hitting various problems when we can't enter= repair > > > > mode. In some cases we may not roll back these changes properly, m= eaning > > > > we break network connections on the source. > > > >=20 > > > > Our general approach is not to completely block migration if there = are > > > > problems, but simply to break any flows we can't migrate. So, if w= e have > > > > no connection from passt-repair carry on with the migration, but do= n't > > > > attempt to migrate any TCP connections. > > > >=20 > > > > Signed-off-by: David Gibson > > > > --- > > > > flow.c | 11 +++++++++-- > > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > >=20 > > > > diff --git a/flow.c b/flow.c > > > > index 6cf96c26..749c4984 100644 > > > > --- a/flow.c > > > > +++ b/flow.c > > > > @@ -923,6 +923,10 @@ static int flow_migrate_repair_all(struct ctx = *c, bool enable) > > > > union flow *flow; > > > > int rc; > > > > =20 > > > > + /* If we don't have a repair helper, there's nothing we can do */ > > > > + if (c->fd_repair < 0) > > > > + return 0; > > > > + =20 > > >=20 > > > This doesn't fix the behaviour in a relatively likely failure mode: > > > passt-repair is there, but we can't communicate to it (LSM policy > > > issues or similar). =20 > >=20 > > Ah... true. Although it shouldn't make it any worse for that case, > > right, so that could be a separate fix. >=20 > Sure. >=20 > > > In that case, unconditionally terminating on failure in the rollback > > > function: > > >=20 > > > if (tcp_flow_repair_off(c, &flow->tcp)) > > > die("Failed to roll back TCP_REPAIR mode"); > > >=20 > > > if (repair_flush(c)) > > > die("Failed to roll back TCP_REPAIR mode"); > > >=20 > > > isn't a very productive thing to do: we go from an uneventful failure > > > where flows were not affected at all to a guest left without > > > connectivity. =20 > >=20 > > So, the issue is that leaving sockets in repair mode after we leave > > the migration path would be very bad. >=20 > Why? I really can't see anything catastrophic happening as a result of > that (hence my v12 version of this). Surely not as bad as the guest > losing connectivity without any possible recovery. I was meaning specifically trying to carry on using a repair mode socket as if it wasn't in repair mode, not closing it out. > > We can't easily close > > sockets/flows for which that's the case, because the batching means if > > there's a failure we don't actually know which sockets are in which > > mode, hence the die(). >=20 > They can be closed (via tcp_rst()) anyway. If they're in repair mode, > no RST will reach the peer, and if they aren't, it will. Ah, yes, I guess we can just close anything that might be affected. Brutal, but effective. It will disrupt connectivity, of course, but not as badly as dying completely. > > > That starts looking less robust than the alternative (e.g. what I > > > implemented in v12: silently fail and continue) at least without > > > https://patchew.org/QEMU/20250217092550.1172055-1-lvivier@redhat.com/ > > > in a general case as well: if we continue, we'll have hanging flows > > > that will expire on timeout, but if we don't, again, we'll have a > > > guest without connectivity. > > >=20 > > > I understand that leaving flows around for that long might present a > > > relevant inconsistency, though. > > >=20 > > > So I'm wondering about some alternatives: actually, the rollback > > > function shouldn't be called at all in this case. Or it could just > > > (indirectly) call tcp_rst() on all the flows that were possibly > > > affected. =20 > >=20 > > Making it be a safe no-op if we never managed to turn repair on for > > anything would make sense to me. Unfortunately, in this situation we > > won't see an error until we do a repair_flush() which means we now > > don't know the state of any sockets we already passed to > > repair_set(). > >=20 > > We could, I suppose, close all flows that we passed to repair_set() by > > the time we see the error. If we have < one batch's worth of > > connections that will kill connectivity almost as much as die()ing, > > though. I guess it will come back without needing qemu to restart us, > > though, so that's something. >=20 > Right, yes, that's what I'm suggesting. Ok. I'll work on something to do that. > > This sort of thing is, incidentally, why I did way back suggest the > > possibility of passt-repair reporting failures per-fd, rather than > > just per-batch. >=20 > Sorry, I somehow missed that proposal, and I can't find any trace of > it. It may have just been on IRC somewhere. > But anyway, the problem is that if we fail to read a batch for any > reason (invalid ancillary data... maybe always implying a kernel issue, > but I'm not sure), you can't _reliably_ report per-fd failures. > *Usually*, you can. Worth it? Ah, I see. We could handle that by being able to report both per-fd and "whole batch" failure (equivalent to failure on every fd), but that would complexify the protocol, of course. > In any case, if it's simple, we can still do it, because passt and > passt-repair are distributed together. You can't pass back the file > descriptors via SCM_RIGHTS though, because we want to close() them > before we reply. >=20 > Another alternative could be that passt-repair reverts back the state > of the file descriptors that were already switched, on failure. That might help a bit, we'd still need to rework the passt-side interface to know what needs reverting at the right stage. I'll take those ideas and see what I can come up with today. --=20 David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson --zk+x4U7X96IR+heo Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAme354sACgkQzQJF27ox 2GeLORAAl5R/eeDmxcGvp2hexVsD7nvTI9eB58ydDr5x+wxIZSjRAe+2WSsnF53Q Na0qPNDfqKtQMGpevbXogefRCCvfWOSp7Eab1RLI6DnTAARTgrbF+liRd8dIBcFA hXk5mcx4KEID41M/BhKPhipjfGH/BBxa70jCa87t3S6HXR6Ba402lL+PrYSHCl0q IC8fIlGUJCgKt5rCE963X1MdV8c/ddyls2CmVr0yBriSfyrWNFSinE7sM0l2RVd9 52/2TZWuZgWIAUTNKjBWudTQ94dxr74mtRte2uPydT9Mdid+VVMbgdqjvgcTOCkJ gXMyRflyZv6lkuXpCzQ26u2JDnUXpkYuVw4A3+b9xLbR+U3orvZVo6NABO7N2wKX 7nxfFKYERbdkyPO6XLquxlcMiJdidTNS2XpBk7O0fGOFnaNyNNcvvM0x5CKz2S7L zC/XGpO+IoHj6CWp/nzaQtna63FEbVTT1vVF2fS5pyuTXZgUwuH1jBDS5LJuqjPN r0Xxssc/J7QmGTvZtY7S+3ottJQgEIyKuEbOeFMb1c2H5Z0pFyakk9ITnyNC55xs /WSE1XrXnN03Sn22GRzhB9+x9ntGqSKcrdSOSTG2voxvMFVL7UvH4a9Ij7oHV74Z YK4GjJV71BctwTJO0WWCBN3dJJx5pmHuQeP3vGdvvfgbDrd1CXs= =FEOl -----END PGP SIGNATURE----- --zk+x4U7X96IR+heo--