Skip to content

Conversation

pippolo84
Copy link
Member

Node linux tests have been updated in #38490 to remove all the assertions regarding the deprecated tunnel map. However:

  • the agent restart test was originally meant to check the update of the tunnel map entries after a node configuration change. In Remove insertions and deletions to deprecated tunnel map #38490 it has been changed to assert the update of the routes, but the linux node handler is not able (and never has been) to remove the stale routes when an address family gets disabled. Therefore, the test should be removed.
  • the direct routing test lacked the correct configuration of the dummy node, specifically the CiliumInternalI{Pv4,IPv6} needed to successfully install the fake routes
  • the mocked tunnel map is not useful anymore, since it has been removed altogether

All of these should have led to the tests failing in the CI, but that didn't happen because of #38172: the node_linux_test.go file has been tagged as unparallel but the tests-privileged target in the Makefile has not been updated to include the whole node package as part of the UNPARALLELTESTPKGS list.

The latter issue will be addressed in #39250, where a run of the updated tests completed successfully.

pippolo84 added 3 commits May 1, 2025 00:14
TestAgentRestartOptionChanges was meant to verify the correct insertion
and removal of the tunnel map entries following a change in the daemon
config options (specifically the enabling and disabling of IP addresses
families).

Since the tunnel map has been removed and the routes handling is tested
in the other linux node tests, TestAgentRestartOptionChanges can be
safely removed.

This fixes a previous incorrect change in the test that was undetected
by the CI because the entire node_linux_test.go file was not built as
part of the test suite due to the "unparallel" build tag.  The fixes
aimed to verify that the routes were updated after a node configuration
change, but the linux node handler does not have (and never has been)
that ability.

Fixes: ef893ad ("node: Rework the agent restart test")
Related: 91df013 ("pkg: Mark node_linux_test.go as unparallel")

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
The local node configuration should be set before inserting the fake
routes, otherwise the route addition fails with the following error:

The test failure went undetected in the CI because the entire
node_linux_test.go file was not built as part of the test suite due to
the "unparallel" build tag.

Related: 91df013 ("pkg: Mark node_linux_test.go as unparallel")

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
Tunnel map has been removed, thus the tests that checks the linux node
logic do not need to set a mock map anymore.

Fixes: 17bc606 ("bpf, pkg/datapath: Remove tunnel map")
Related: 91df013 ("pkg: Mark node_linux_test.go as unparallel")

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84 pippolo84 added kind/bug/CI This is a bug in the testing code. area/CI Continuous Integration testing issue or flake area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. release-note/ci This PR makes changes to the CI. labels Apr 30, 2025
@pippolo84 pippolo84 requested a review from a team as a code owner April 30, 2025 22:14
@pippolo84 pippolo84 requested a review from gentoo-root April 30, 2025 22:14
@pippolo84 pippolo84 added the affects/v1.17 This issue affects v1.17 branch label Apr 30, 2025
@pippolo84
Copy link
Member Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 5, 2025
@julianwiedmann julianwiedmann added this pull request to the merge queue May 5, 2025
Merged via the queue into cilium:main with commit bc45ec5 May 5, 2025
72 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/v1.17 This issue affects v1.17 branch area/CI Continuous Integration testing issue or flake area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. kind/bug/CI This is a bug in the testing code. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants