mirror of
https://github.com/krkn-chaos/krkn.git
synced 2026-08-25 09:27:36 +00:00
fix: build egress tc netem tree with actual loss/delay values (#1501)
* fix: build tc netem tree with actual loss/delay values instead of changing after The egress path in node-network-chaos built the tc htb+netem tree with hardcoded loss 0% and delay 0ms, then tried to update via tc qdisc change. On some hardware (bare-metal with Mellanox NIC), the change command fails silently — no packets are ever dropped, producing false-negative tests. Build the tree with actual rate/delay/loss values in a single pass, matching how the ingress path already works. This eliminates the tc qdisc change step entirely. Also log stderr warnings from tc commands that were previously swallowed silently. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: ddjain <darjain@redhat.com> * refactor: remove unused get_egress_shaping_comand after single-pass tc tree build Signed-off-by: ddjain <darjain@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> --------- Signed-off-by: ddjain <darjain@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
co-authored by
Claude Opus 4.6
Cursor
parent
e584df28a0
commit
249b07b250
@@ -67,15 +67,20 @@ def tc_node(args: list[str]) -> subprocess.CompletedProcess:
|
||||
return run(["tc"] + args)
|
||||
|
||||
|
||||
def get_build_tc_tree_commands(devs: list[str]) -> list[str]:
|
||||
def get_build_tc_tree_commands(
|
||||
devs: list[str],
|
||||
rate: Optional[str] = None,
|
||||
delay: Optional[str] = None,
|
||||
loss: Optional[str] = None,
|
||||
) -> list[str]:
|
||||
tree = []
|
||||
for dev in devs:
|
||||
tree.append(f"tc qdisc add dev {dev} root handle {ROOT_HANDLE} htb default 1")
|
||||
tree.append(
|
||||
f"tc class add dev {dev} parent {ROOT_HANDLE} classid {CLASS_ID} htb rate 1gbit",
|
||||
f"tc class add dev {dev} parent {ROOT_HANDLE} classid {CLASS_ID} htb rate {_normalize_rate(rate)}",
|
||||
)
|
||||
tree.append(
|
||||
f"tc qdisc add dev {dev} parent {CLASS_ID} handle {NETEM_HANDLE} netem delay 0ms loss 0%",
|
||||
f"tc qdisc add dev {dev} parent {CLASS_ID} handle {NETEM_HANDLE} netem delay {_normalize_delay(delay)} loss {_normalize_loss(loss)}%",
|
||||
)
|
||||
|
||||
return tree
|
||||
@@ -87,27 +92,6 @@ def namespaced_tc_commands(pids: list[str], commands: list[str]) -> list[str]:
|
||||
]
|
||||
|
||||
|
||||
def get_egress_shaping_comand(
|
||||
devices: list[str],
|
||||
rate_mbit: Optional[str],
|
||||
delay_ms: Optional[str],
|
||||
loss_pct: Optional[str],
|
||||
) -> list[str]:
|
||||
|
||||
rate_commands = []
|
||||
rate = _normalize_rate(rate_mbit)
|
||||
d = _normalize_delay(delay_ms)
|
||||
l = _normalize_loss(loss_pct)
|
||||
for dev in devices:
|
||||
rate_commands.append(
|
||||
f"tc class change dev {dev} parent {ROOT_HANDLE} classid {CLASS_ID} htb rate {rate}"
|
||||
)
|
||||
rate_commands.append(
|
||||
f"tc qdisc change dev {dev} parent {CLASS_ID} handle {NETEM_HANDLE} netem delay {d} loss {l}%"
|
||||
)
|
||||
return rate_commands
|
||||
|
||||
|
||||
def get_clear_egress_shaping_commands(devices: list[str]) -> list[str]:
|
||||
return [f"tc qdisc del dev {dev} root handle {ROOT_HANDLE}" for dev in devices]
|
||||
|
||||
@@ -200,19 +184,11 @@ def common_set_limit_rules(
|
||||
pids: Optional[list[str]] = None,
|
||||
):
|
||||
if egress:
|
||||
build_tree_commands = get_build_tc_tree_commands(interfaces)
|
||||
if pids:
|
||||
build_tree_commands = namespaced_tc_commands(pids, build_tree_commands)
|
||||
egress_shaping_commands = get_egress_shaping_comand(
|
||||
interfaces,
|
||||
bandwidth,
|
||||
latency,
|
||||
loss,
|
||||
build_tree_commands = get_build_tc_tree_commands(
|
||||
interfaces, rate=bandwidth, delay=latency, loss=loss,
|
||||
)
|
||||
if pids:
|
||||
egress_shaping_commands = namespaced_tc_commands(
|
||||
pids, egress_shaping_commands
|
||||
)
|
||||
build_tree_commands = namespaced_tc_commands(pids, build_tree_commands)
|
||||
error_counter = 0
|
||||
for rule in build_tree_commands:
|
||||
result = kubecli.exec_cmd_in_pod([rule], network_chaos_pod_name, namespace)
|
||||
@@ -220,15 +196,12 @@ def common_set_limit_rules(
|
||||
log_info(f"created tc tree in pod: {rule}", parallel, target)
|
||||
else:
|
||||
error_counter += 1
|
||||
log_warning(f"tc command returned output (may be a warning): {rule} — {result}", parallel, target)
|
||||
if len(build_tree_commands) == error_counter:
|
||||
log_error(
|
||||
"failed to apply egress shaping rules on cluster", parallel, target
|
||||
)
|
||||
|
||||
for rule in egress_shaping_commands:
|
||||
result = kubecli.exec_cmd_in_pod([rule], network_chaos_pod_name, namespace)
|
||||
if not result:
|
||||
log_info(f"applied egress shaping rules: {rule}", parallel, target)
|
||||
if ingress:
|
||||
ingress_shaping_commands = get_ingress_shaping_commands(
|
||||
interfaces,
|
||||
|
||||
@@ -15,7 +15,6 @@ from unittest.mock import MagicMock, patch, call
|
||||
from krkn.scenario_plugins.network_chaos_ng.modules.utils_network_chaos import (
|
||||
get_build_tc_tree_commands,
|
||||
namespaced_tc_commands,
|
||||
get_egress_shaping_comand,
|
||||
get_clear_egress_shaping_commands,
|
||||
get_ingress_shaping_commands,
|
||||
get_clear_ingress_shaping_commands,
|
||||
@@ -74,7 +73,7 @@ class TestBuildTcTreeCommands(unittest.TestCase):
|
||||
|
||||
def test_build_tc_tree_single_interface(self):
|
||||
"""
|
||||
Test building tc tree commands for a single interface
|
||||
Test building tc tree commands for a single interface with default values
|
||||
"""
|
||||
devices = ["eth0"]
|
||||
result = get_build_tc_tree_commands(devices)
|
||||
@@ -89,6 +88,43 @@ class TestBuildTcTreeCommands(unittest.TestCase):
|
||||
result,
|
||||
)
|
||||
|
||||
def test_build_tc_tree_with_actual_values(self):
|
||||
"""
|
||||
Test building tc tree commands with explicit rate, delay, and loss values
|
||||
"""
|
||||
devices = ["eth0"]
|
||||
result = get_build_tc_tree_commands(
|
||||
devices, rate="100", delay="50", loss="10"
|
||||
)
|
||||
|
||||
self.assertEqual(len(result), 3)
|
||||
self.assertIn("tc qdisc add dev eth0 root handle 100: htb default 1", result)
|
||||
self.assertIn(
|
||||
"tc class add dev eth0 parent 100: classid 100:1 htb rate 100mbit", result
|
||||
)
|
||||
self.assertIn(
|
||||
"tc qdisc add dev eth0 parent 100:1 handle 101: netem delay 50ms loss 10%",
|
||||
result,
|
||||
)
|
||||
|
||||
def test_build_tc_tree_with_suffixed_values(self):
|
||||
"""
|
||||
Test building tc tree commands with pre-suffixed values
|
||||
"""
|
||||
devices = ["eth0"]
|
||||
result = get_build_tc_tree_commands(
|
||||
devices, rate="1gbit", delay="100ms", loss="30%"
|
||||
)
|
||||
|
||||
self.assertEqual(len(result), 3)
|
||||
self.assertIn(
|
||||
"tc class add dev eth0 parent 100: classid 100:1 htb rate 1gbit", result
|
||||
)
|
||||
self.assertIn(
|
||||
"tc qdisc add dev eth0 parent 100:1 handle 101: netem delay 100ms loss 30%",
|
||||
result,
|
||||
)
|
||||
|
||||
def test_build_tc_tree_multiple_interfaces(self):
|
||||
"""
|
||||
Test building tc tree commands for multiple interfaces
|
||||
@@ -157,67 +193,6 @@ class TestNamespacedTcCommands(unittest.TestCase):
|
||||
self.assertEqual(len(result), 4)
|
||||
|
||||
|
||||
class TestEgressShapingCommands(unittest.TestCase):
|
||||
|
||||
def test_egress_shaping_with_all_params(self):
|
||||
"""
|
||||
Test egress shaping commands with bandwidth, latency and loss
|
||||
"""
|
||||
devices = ["eth0"]
|
||||
result = get_egress_shaping_comand(devices, "100", "50", "10")
|
||||
|
||||
self.assertEqual(len(result), 2)
|
||||
self.assertIn(
|
||||
"tc class change dev eth0 parent 100: classid 100:1 htb rate 100mbit",
|
||||
result,
|
||||
)
|
||||
self.assertIn(
|
||||
"tc qdisc change dev eth0 parent 100:1 handle 101: netem delay 50ms loss 10%",
|
||||
result,
|
||||
)
|
||||
|
||||
def test_egress_shaping_with_defaults(self):
|
||||
"""
|
||||
Test egress shaping commands with None values defaults to 1gbit, 0ms, 0%
|
||||
"""
|
||||
devices = ["eth0"]
|
||||
result = get_egress_shaping_comand(devices, None, None, None)
|
||||
|
||||
self.assertEqual(len(result), 2)
|
||||
self.assertIn(
|
||||
"tc class change dev eth0 parent 100: classid 100:1 htb rate 1gbit", result
|
||||
)
|
||||
self.assertIn(
|
||||
"tc qdisc change dev eth0 parent 100:1 handle 101: netem delay 0ms loss 0%",
|
||||
result,
|
||||
)
|
||||
|
||||
def test_egress_shaping_with_suffixed_params(self):
|
||||
"""
|
||||
Test that pre-suffixed values (e.g. "100mbit", "50ms", "10%") are passed through unchanged.
|
||||
"""
|
||||
devices = ["eth0"]
|
||||
result = get_egress_shaping_comand(devices, "100mbit", "50ms", "10%")
|
||||
|
||||
self.assertIn(
|
||||
"tc class change dev eth0 parent 100: classid 100:1 htb rate 100mbit",
|
||||
result,
|
||||
)
|
||||
self.assertIn(
|
||||
"tc qdisc change dev eth0 parent 100:1 handle 101: netem delay 50ms loss 10%",
|
||||
result,
|
||||
)
|
||||
|
||||
def test_egress_shaping_multiple_interfaces(self):
|
||||
"""
|
||||
Test egress shaping for multiple interfaces
|
||||
"""
|
||||
devices = ["eth0", "eth1"]
|
||||
result = get_egress_shaping_comand(devices, "100", "50", "10")
|
||||
|
||||
self.assertEqual(len(result), 4)
|
||||
|
||||
|
||||
class TestClearEgressShapingCommands(unittest.TestCase):
|
||||
|
||||
def test_clear_egress_single_interface(self):
|
||||
@@ -433,8 +408,8 @@ class TestCommonSetLimitRules(unittest.TestCase):
|
||||
pids=None,
|
||||
)
|
||||
|
||||
# Should call exec_cmd_in_pod for egress rules (3 build + 2 shaping)
|
||||
self.assertGreaterEqual(self.mock_kubecli.exec_cmd_in_pod.call_count, 5)
|
||||
# Should call exec_cmd_in_pod for egress tree only (3 commands, no shaping step)
|
||||
self.assertEqual(self.mock_kubecli.exec_cmd_in_pod.call_count, 3)
|
||||
|
||||
@patch("krkn.scenario_plugins.network_chaos_ng.modules.utils_network_chaos.log_info")
|
||||
def test_set_ingress_only(self, mock_log_info):
|
||||
@@ -479,8 +454,9 @@ class TestCommonSetLimitRules(unittest.TestCase):
|
||||
pids=None,
|
||||
)
|
||||
|
||||
# Should call exec_cmd_in_pod for both egress and ingress
|
||||
self.assertGreater(self.mock_kubecli.exec_cmd_in_pod.call_count, 10)
|
||||
# 3 egress tree commands + ingress commands (modprobe, ip link add, ip link set,
|
||||
# tc qdisc ingress, tc filter, tc qdisc htb, tc class, tc netem = 8)
|
||||
self.assertGreater(self.mock_kubecli.exec_cmd_in_pod.call_count, 8)
|
||||
|
||||
@patch("krkn.scenario_plugins.network_chaos_ng.modules.utils_network_chaos.log_info")
|
||||
def test_set_with_pids(self, mock_log_info):
|
||||
@@ -509,8 +485,9 @@ class TestCommonSetLimitRules(unittest.TestCase):
|
||||
"Expected nsenter commands when pids are provided",
|
||||
)
|
||||
|
||||
@patch("krkn.scenario_plugins.network_chaos_ng.modules.utils_network_chaos.log_warning")
|
||||
@patch("krkn.scenario_plugins.network_chaos_ng.modules.utils_network_chaos.log_error")
|
||||
def test_set_with_command_failure(self, mock_log_error):
|
||||
def test_set_with_command_failure(self, mock_log_error, mock_log_warning):
|
||||
"""
|
||||
Test handling of command failures
|
||||
"""
|
||||
@@ -534,6 +511,8 @@ class TestCommonSetLimitRules(unittest.TestCase):
|
||||
|
||||
# Should log error when all commands fail
|
||||
mock_log_error.assert_called()
|
||||
# Should log warnings for each failed command
|
||||
self.assertEqual(mock_log_warning.call_count, 3)
|
||||
|
||||
|
||||
class TestCommonDeleteLimitRules(unittest.TestCase):
|
||||
|
||||
Reference in New Issue
Block a user