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=MPsEubAq; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 5EE415A061F for ; Thu, 20 Feb 2025 11:18:22 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202502; t=1740046691; bh=AldDIRtOLsf8sWnA9+5UqF5YXxMa/J2Sw5wBB49ZcKA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=MPsEubAqiRTleDAw17IdjJ+Xo754LMwnwMFwwdgyHS8aH5xnFzCc8qoP3c6JOAMgI AZSxXffQE2BgWAlyVhhbWPqDiuyjHll6eQXhcVd3ca/MQZeOGD1i6XQMeGYDILjUVR SnASb/MBlpQKrTqE9GS9GtP7U9PCjkOFfqAnF+2M67QVRhRyRco2f2YThUB8Hs/9tl EWNs4KgQ1G1NpcNVCMtSKw6TAQRkYoxobvkDcdezTjEzBTx4MMOpmIzWWVdvQkylBr QL4uVStrkGFcJGiYwpnsiN1DEKPRIO8JPlHRt+tHGV3pB95nto3puLdjLbHMQuwg/5 hLYdjImTmfsxQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Yz8NM65kzz4x2c; Thu, 20 Feb 2025 21:18:11 +1100 (AEDT) Date: Thu, 20 Feb 2025 21:18:06 +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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="pq3++tnM23ysyxU+" Content-Disposition: inline In-Reply-To: <20250220090726.43432475@elisabeth> Message-ID-Hash: OIR65RE35QQQLYZIICUAPPTVGNJURFJK X-Message-ID-Hash: OIR65RE35QQQLYZIICUAPPTVGNJURFJK 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: --pq3++tnM23ysyxU+ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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_REPAIR. = 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 rep= air > > mode. In some cases we may not roll back these changes properly, meani= ng > > 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 we ha= ve > > no connection from passt-repair carry on with the migration, but don'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 > 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). Ah... true. Although it shouldn't make it any worse for that case, right, so that could be a separate fix. > 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. So, the issue is that leaving sockets in repair mode after we leave the migration path would be very bad. 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(). > 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. 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(). 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. 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 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 --pq3++tnM23ysyxU+ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAme3AV0ACgkQzQJF27ox 2Gcu/Q/9HxUYuCWj/ohwjxh8lXFznJnepQcOJ9/9VQY3sMheZLzG3I0RuQc22eun nglUxfYEem3SHBoHycA0usFr7jc5VA5YwTTrlU7nIjI+aKsjowoxIG5No29s1TTL octEfT7qXVWzBoj6aomRH8wV/z8zrDOW6lQZGlKXgAQGK0kSojamt5h5Ffuc3EmW 16pMHFAOGaNtm/2m7OIdw2LshYqKr8lhEbDaxDLxWtERVpdyR8dKjSESVc3O7XaR DmegZWEKh9tQfB3eK/IMjCXFFXPasjEnwtfMADPjNTdEutuKzFPsoU4+11sZ3w0k io4mlf9WkNbQFbFS3Erm6DFZK51MwNFNWSoO25QVEPbrmTqt4GK/AH94V/MullgH fOf0+Zlx9Q2mrxUhmM8tA7YH5zyK4HA3wc2t+NxqxTe2QPWWfndUsJ/XQ0Lme0z7 3qa9THHUN/dmTY4KjVUWBgNb84lsaZdFNH4I4zDAsWPgs0zAUGHYhkwuEIZ7may3 0mgGpJ8wLT5P587u3VD+nUn8jUwcW8pwHVc43bDn43Oe5zw01zrxi/KsBpvn7Vt/ /4kTUiWre0ge0Gvrji+dlcPVwVy4X7qlAjY2c0CAkUXTFImVc+J/82k4E/fimELa kEvT442iYhK4UR0k/hL1XzdPOH/DPRee14zU8O3Q29pJBTSpawQ= =RlxL -----END PGP SIGNATURE----- --pq3++tnM23ysyxU+--