make stream shutdown if self-node has been removed #2125
Conversation
Signed-off-by: Kristoffer Dalby <[email protected]>
WalkthroughThe changes in this pull request enhance the integration testing and error handling of the Headscale application. A new test case is introduced to ensure that deleting an online node does not cause a panic. Modifications to several methods improve logging capabilities by returning additional information, such as log file paths, alongside error statuses. These updates focus on increasing the robustness and reliability of the application during node management operations. Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Signed-off-by: Kristoffer Dalby <[email protected]>
Currently we will read the node from database, and since it is deleted, the id might be set to nil. Keep the node around and just shutdown, so it is cleanly removed from notifier. Fixes juanfont#2118 Signed-off-by: Kristoffer Dalby <[email protected]>
kradalby
force-pushed
the
kradalby/2118-panic-online-delete
branch
from
4d32d47 to
eb71f8a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
integration/scenario.go (1)
192-211: LGTM, with a minor suggestion.The new
ShutdownAssertNoPanicsfunction looks good. It effectively shuts down the control servers and asserts that no panics occurred in the logs when a testing context is provided.Suggestion:
Consider improving the error handling by checking the error returned fromcontrol.Shutdown()before proceeding to read the log files. You could log the error or perform some other action if the shutdown fails.integration/tsic/tsic.go (1)
1001-1003: LGTM, but consider converting the TODO comment to a GitHub issue.The changes to propagate the error from
SaveLogfunction call are approved.However, to better track the future enhancement of asserting panic logs from Tailscale, consider converting the TODO comment to a GitHub issue.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (8)
- .github/workflows/test-integration.yaml (1 hunks)
- hscontrol/poll.go (2 hunks)
- integration/control.go (1 hunks)
- integration/dockertestutil/logs.go (2 hunks)
- integration/general_test.go (1 hunks)
- integration/hsic/hsic.go (2 hunks)
- integration/scenario.go (4 hunks)
- integration/tsic/tsic.go (1 hunks)
Additional comments not posted (16)
integration/dockertestutil/logs.go (8)
20-20: LGTM!The change to the return signature of the
SaveLogfunction is approved. Returning the paths to the saved log files provides more information to the caller, which is beneficial.
23-23: LGTM!The change to the error return statement is approved. Returning empty strings for the log file paths along with the error maintains a consistent return pattern and allows the caller to handle errors effectively.
44-44: LGTM!The change to the error return statement is approved. Returning empty strings for the log file paths along with the error maintains a consistent return pattern and allows the caller to handle errors effectively.
49-49: LGTM!Using
path.Jointo construct the file path is a good practice as it ensures cross-platform compatibility.
51-56: LGTM!
- Using
os.WriteFileto write the stdout log file is a good practice as it provides a simple and efficient way to write data to a file.- The change to the error return statement is approved. Returning empty strings for the log file paths along with the error maintains a consistent return pattern and allows the caller to handle errors effectively.
59-59: LGTM!Using
path.Jointo construct the file path is a good practice as it ensures cross-platform compatibility.
61-66: LGTM!
- Using
os.WriteFileto write the stderr log file is a good practice as it provides a simple and efficient way to write data to a file.- The change to the error return statement is approved. Returning empty strings for the log file paths along with the error maintains a consistent return pattern and allows the caller to handle errors effectively.
69-69: LGTM!Returning the paths to the stdout and stderr log files along with a nil error is approved. This change provides more information to the caller, which is beneficial.
.github/workflows/test-integration.yaml (1)
55-55: Brilliant addition to the test suite!The inclusion of the
Test2118DeletingOnlineNodePanicstest case is a fantastic enhancement to the integration testing workflow. It directly addresses the critical issue mentioned in the PR objectives, where deleting an online node causes a panic in the Headscale application.By adding this specific test case, you are ensuring that the scenario of deleting an online node is thoroughly validated, and any potential panics or crashes are caught during the testing phase. This proactive approach will greatly improve the robustness and reliability of the application.
Furthermore, the naming convention of the test case follows the established pattern in the test suite, making it consistent and easily understandable.
Excellent work on enhancing the test coverage and addressing a critical issue!
integration/scenario.go (1)
235-241: Looks good!The modification to the
Shutdownfunction to callShutdownAssertNoPanics(nil)is a clean way to integrate the new panic assertions into the existing shutdown process for non-test scenarios.integration/hsic/hsic.go (2)
401-402: LGTM! The changes to theShutdownmethod signature and logic are approved.The updated
Shutdownmethod now returns stdout and stderr paths in addition to the error, which will be helpful for diagnostics by providing direct access to the log file paths.
466-466: LGTM! The changes to theSaveLogmethod signature are approved.The updated
SaveLogmethod now returns stdout and stderr paths in addition to the error, which aligns with the changes made to theShutdownmethod. This ensures that both methods can effectively communicate the results of their operations.hscontrol/poll.go (2)
8-8: LGTM!The import statement for the
slicespackage is correctly added.
277-281: Looks good!The added code correctly handles the case when the current node has been removed from Headscale. It checks if the node ID is present in the
update.Removedslice usingslices.Containsand closes the stream by returning from the method if the node is found. The tracing and stream closure logic is properly implemented.integration/general_test.go (2)
958-1054: LGTM!The test is well-structured, follows clear steps, and uses appropriate assertions to verify the expected behaviour. The test ensures that deleting an online node does not cause a panic and that the node is actually deleted.
1003-1013: Skipped reviewingexecuteAndUnmarshalfunction.The
executeAndUnmarshalfunction is not defined in the provided code snippet. Its usage in the test seems appropriate for executing Headscale CLI commands and parsing the JSON output. If the function definition is added in the future, it should be reviewed separately.Also applies to: 1039-1048
* add shutdown that asserts if headscale had panics Signed-off-by: Kristoffer Dalby <[email protected]> * add test case producing 2118 panic Signed-off-by: Kristoffer Dalby <[email protected]> * make stream shutdown if self-node has been removed Currently we will read the node from database, and since it is deleted, the id might be set to nil. Keep the node around and just shutdown, so it is cleanly removed from notifier. Fixes juanfont#2118 Signed-off-by: Kristoffer Dalby <[email protected]> --------- Signed-off-by: Kristoffer Dalby <[email protected]>
Currently we will read the node from database, and since it is
deleted, the id might be set to nil. Keep the node around and
just shutdown, so it is cleanly removed from notifier.
Fixes #2118
Also adds the ability to check integration test headscale for panics
and a test case producing the error, which is then fixed.
Signed-off-by: Kristoffer Dalby [email protected]
Summary by CodeRabbit
New Features
Bug Fixes
Documentation