openvidu-test-e2e: fix the two connection-quality flakes

openvidu-test-e2e/src/main/java/io/openvidu/test/e2e/NetworkConditioner.java

@@ -28,7 +28,8 @@ public class NetworkConditioner {
 
 	/**
 	 * UDP port of the SFU's embedded TURN server (livekit.yaml
-	 * {@code turn.udp_port}). Same reasoning as {@link #SFU_ICE_TCP_PORT}: it is the
+	 * {@code turn.udp_port}). Same reasoning as {@link #SFU_ICE_TCP_PORT}: it is
+	 * the
 	 * last route out of a media blackout, since a client whose host and srflx
 	 * candidates are all dead falls back to allocating a TURN relay. Blocking the
 	 * listener is enough; the relay range ({@code turn.relay_range_*}) is where the
@@ -58,6 +59,16 @@ public class NetworkConditioner {
 	// blackoutOutbound().
 	private static String blackoutContainer;
 
+	// Container Pumba is currently impairing. Tracked so that clear() can scrub its
+	// network namespace itself instead of trusting Pumba to have reverted.
+	private static String impairedContainer;
+
+	// SIGTERM-to-SIGKILL grace given to Pumba on clear(). Reverting is not just a
+	// syscall for Pumba: it starts ANOTHER container (the nettools sidecar) to run
+	// the tc/iptables delete, so on a loaded Docker daemon it can take much longer
+	// than the usual 1-2 s. Killed halfway, it leaves the impairment installed.
+	private static final int PUMBA_STOP_GRACE_SEC = 20;
+
 	public enum Direction {
 		OUTBOUND, INBOUND
 	}
@@ -225,6 +236,7 @@ public class NetworkConditioner {
 			cmd = pumbaRun() + " iptables " + opts + " loss --mode random --probability "
 					+ String.format(Locale.US, "%.2f", probability) + " " + targetContainer;
 		}
+		impairedContainer = targetContainer;
 		runPumba(cmd);
 	}
 
@@ -307,17 +319,25 @@ public class NetworkConditioner {
 		}
 		String cmd = pumbaRun() + " netem " + opts + " delay --time " + delayMs + " --jitter " + jitterMs + " "
 				+ targetContainer;
+		impairedContainer = targetContainer;
 		runPumba(cmd);
 	}
 
 	/**
-	 * Stop the current Pumba container (SIGTERM) so it reverts the netem qdisc /
+	 * Remove every impairment: stop the current Pumba container (SIGTERM, so it
-	 * iptables rule immediately.
+	 * reverts the netem qdisc / iptables rule itself) and then scrub the target's
+	 * network namespace anyway, so that the impairment is gone whether or not
+	 * Pumba got to revert it.
 	 */
 	public static void clear() {
+		final String target = impairedContainer;
+		impairedContainer = null;
 		if (currentPumbaContainerName != null) {
 			log.info("Clearing network impairment (stopping Pumba container {})", currentPumbaContainerName);
-			commandLine.executeCommand("docker stop -t 5 " + currentPumbaContainerName, 30);
+			String out = commandLine.executeCommand(
+					"docker stop -t " + PUMBA_STOP_GRACE_SEC + " " + currentPumbaContainerName + " 2>&1",
+					PUMBA_STOP_GRACE_SEC + 30);
+			log.info("docker stop {} result: {}", currentPumbaContainerName, out);
 			currentPumbaContainerName = null;
 		}
 		if (blackoutContainer != null) {
@@ -326,6 +346,20 @@ public class NetworkConditioner {
 			nettools(blackoutContainer, "iptables", "-F OUTPUT");
 			blackoutContainer = null;
 		}
+		if (target != null) {
+			scrubImpairments(target);
+		}
+	}
+
+	/**
+	 * Delete anything Pumba may have left behind in {@code targetContainer}'s
+	 * network namespace: its INPUT DROP rules (ingress loss) and the root qdisc
+	 * tree carrying its netem (egress loss/delay).
+	 */
+	private static void scrubImpairments(String targetContainer) {
+		String out = nettools(targetContainer, "sh", "-c \"iptables -F INPUT; tc qdisc del dev eth0 root\"");
+		log.info("Scrubbing leftover impairments on container {} (iptables -F INPUT; tc qdisc del dev eth0 root): {}",
+				targetContainer, out.isBlank() ? "removed" : out.trim());
 	}
 
 	/**

openvidu-test-e2e/src/test/java/io/openvidu/test/e2e/OpenViduTestAppE2eTest.java

@@ -437,6 +437,46 @@ public class OpenViduTestAppE2eTest extends AbstractOpenViduTestappE2eTest {
 		return null;
 	}
 
+	private static int qualityRank(ConnectionQuality quality) {
+		if (quality == null) {
+			return Integer.MAX_VALUE;
+		}
+		switch (quality) {
+			case LOST:
+				return 0;
+			case POOR:
+				return 1;
+			case GOOD:
+				return 2;
+			case EXCELLENT:
+				return 3;
+			default:
+				return Integer.MAX_VALUE;
+		}
+	}
+
+	private static ConnectionQuality worstQuality(ConnectionQuality a, ConnectionQuality b) {
+		return qualityRank(b) < qualityRank(a) ? b : a;
+	}
+
+	// Holds the current impairment for `holdMillis` and records, for that loss
+	// step, the WORST quality each participant reported at any point during it.
+	private void holdAndRecordWorstQuality(OpenViduTestappUser publisher, OpenViduTestappUser subscriber,
+			long holdMillis, int lossPct, Map<Integer, ConnectionQuality> publisherQuality,
+			Map<Integer, ConnectionQuality> subscriberQuality) throws InterruptedException {
+		final long sampleIntervalMillis = 1000;
+		final long deadline = System.currentTimeMillis() + holdMillis;
+		ConnectionQuality worstPublisher = null;
+		ConnectionQuality worstSubscriber = null;
+		do {
+			worstPublisher = worstQuality(worstPublisher, latestConnectionQuality(publisher, 0, "PunchbagUser"));
+			worstSubscriber = worstQuality(worstSubscriber, latestConnectionQuality(subscriber, 0, "RegularUser"));
+			Thread.sleep(sampleIntervalMillis);
+		} while (System.currentTimeMillis() < deadline);
+		publisherQuality.put(lossPct, worstPublisher);
+		subscriberQuality.put(lossPct, worstSubscriber);
+	}
+
 	// First loss% (ascending) at which the recorded quality equals `level`, or -1
 	// if never reached.
 	private static int firstLossReaching(Map<Integer, ConnectionQuality> observed, ConnectionQuality level) {
@@ -1237,18 +1277,17 @@ public class OpenViduTestAppE2eTest extends AbstractOpenViduTestappE2eTest {
 		final int STEP_PCT = 10;
 		final int HOLD_SECONDS = 16;
 
-		// Ramp the PUBLISHER's uplink loss and record, at each step, the settled
+		// Ramp the PUBLISHER's uplink loss and record, at each step, the WORST
-		// quality of both the impaired publisher (PunchbagUser) and the untouched
+		// quality seen during the whole hold, for both the impaired publisher
-		// subscriber's OWN quality (RegularUser).
+		// (PunchbagUser) and the untouched subscriber's OWN quality (RegularUser).
 		Map<Integer, ConnectionQuality> publisherQuality = new LinkedHashMap<>();
 		Map<Integer, ConnectionQuality> subscriberQuality = new LinkedHashMap<>();
 		try {
 			for (int pct = STEP_PCT; pct <= 90; pct += STEP_PCT) {
 				log.info("Packet loss to " + pct + "%");
 				NetworkConditioner.updateOutboundLossPercent(container, pct);
-				Thread.sleep(HOLD_SECONDS * 1000L);
+				holdAndRecordWorstQuality(punchbagUser, regularUser, HOLD_SECONDS * 1000L, pct, publisherQuality,
-				publisherQuality.put(pct, latestConnectionQuality(punchbagUser, 0, "PunchbagUser"));
+						subscriberQuality);
-				subscriberQuality.put(pct, latestConnectionQuality(regularUser, 0, "RegularUser"));
 			}
 
 			// Final step: a TOTAL blackout (100% loss) across the WHOLE SFU media port
@@ -1264,24 +1303,25 @@ public class OpenViduTestAppE2eTest extends AbstractOpenViduTestappE2eTest {
 			publisherQuality.put(BLACKOUT_PCT, pubBlackout);
 			subscriberQuality.put(BLACKOUT_PCT, latestConnectionQuality(regularUser, 0, "RegularUser"));
 
-			log.info(buildRampResultTable(publisherQuality, subscriberQuality));
+			final String table = buildRampResultTable(publisherQuality, subscriberQuality);
+			log.info(table);
 
 			int firstGood = firstLossReaching(publisherQuality, ConnectionQuality.GOOD);
 			int firstPoor = firstLossReaching(publisherQuality, ConnectionQuality.POOR);
 			int firstLost = firstLossReaching(publisherQuality, ConnectionQuality.LOST);
 			Assertions.assertTrue(firstGood >= 10 && firstGood <= 20,
 					"EXCELLENT->GOOD transition expected between 10% and 20% loss, but first GOOD was at " + firstGood
-							+ "%");
+							+ "%" + table);
 			Assertions.assertTrue(firstPoor >= 20 && firstPoor <= 50,
 					"GOOD->POOR transition expected between 20% and 50% loss, but first POOR was at " + firstPoor
-							+ "%");
+							+ "%" + table);
 			Assertions.assertTrue(firstPoor > firstGood,
-					"POOR must appear after GOOD (firstGood=" + firstGood + "%, firstPoor=" + firstPoor + "%)");
+					"POOR must appear after GOOD (firstGood=" + firstGood + "%, firstPoor=" + firstPoor + "%)" + table);
 			Assertions.assertTrue(firstLost >= 50,
 					"POOR->LOST transition expected ONLY at severe loss (>=50%), but first LOST was at " + firstLost
-							+ "%");
+							+ "%" + table);
 			Assertions.assertTrue(firstLost > firstPoor,
-					"LOST must appear after POOR (firstPoor=" + firstPoor + "%, firstLost=" + firstLost + "%)");
+					"LOST must appear after POOR (firstPoor=" + firstPoor + "%, firstLost=" + firstLost + "%)" + table);
 
 			// Subscriber's own network is always EXCELLENT
 			for (Entry<Integer, ConnectionQuality> e : subscriberQuality.entrySet()) {
@@ -1289,7 +1329,7 @@ public class OpenViduTestAppE2eTest extends AbstractOpenViduTestappE2eTest {
 				Assertions.assertFalse(
 						sq == ConnectionQuality.GOOD || sq == ConnectionQuality.POOR || sq == ConnectionQuality.LOST,
 						"RegularUser (subscriber) network is NOT impaired, so its own connection quality must stay "
-								+ "EXCELLENT, but was " + sq + " at " + e.getKey() + "% publisher loss");
+								+ "EXCELLENT, but was " + sq + " at " + e.getKey() + "% publisher loss" + table);
 			}
 		} finally {
 			NetworkConditioner.clear();