[1.15] daemon: Cleanup after a failed start #36801
Conversation
[ upstream commit f21f303 ] Return the error from startDaemon through the start hook to abort shutdown cleanly rather than using log.Fatal. Signed-off-by: Jussi Maki <jussi@isovalent.com> Signed-off-by: Anton Ippolitov <anton.ippolitov@datadoghq.com>
[ upstream commit edc9cba ] If the daemon start failed due to e.g. invalid configuration the cleanup was not performed as it was only done from the stop function that was not invoked since the daemon start had failed. This in some situations lead to confusing crashes as hive did stop all the dependencies of the legacy daemon, for example this crash was reported: level=fatal msg="failed to start: daemon creation failed: failed to detect devices: ... ... bunch of "stopping" messages omitted ... panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1dfa041] goroutine 460 [running]: github.com/cilium/cilium/pkg/node.(*LocalNodeStore).Observe(0x3f4cfb0? ... <autogenerated>:1 +0x21 The crash was caused by the LocalNodeStore stopping and clearing the global variable in the node package (to make sure for example our tests don't leak data across them and to make sure we do cleanup correctly). To fix this, run the cleanup functions also when "newDaemon" or "startDaemon" fails with an error. Fixes: f21f303 ("daemon: bubble up error from startDaemon instead of fataling") Signed-off-by: Jussi Maki <jussi@isovalent.com> Signed-off-by: Anton Ippolitov <anton.ippolitov@datadoghq.com>
|
/test |
|
Could you speak a bit about whether you can reproduce the issue you see and how much testing you have performed with this change? The bug being fixed here does not seem to be catastrophic or security-relevant (criteria), so I'm not sure whether applying this patch to v1.15 is worth the risk. Startup/shutdown ordering issues can be racy and hard to follow, so I'd be concerned about solving one issue then creating another different issue. |
|
Yeah, tbh it's an intermittent issue that we've seen only a handful of times and I haven't managed to reliably reproduce it: Since I saw that the original PR had the "backport needed" label, I thought that I might as well do it but I agree with you that it's not a critical bug. So if you think it's not worth it, I am fine with closing the PR. |
@antonipp Given the backport criteria and your above statement, it seems like this PR doesn't meet the requirements. If you agree, feel free to close the PR. |
Backport #32673 to 1.15
This required backporting this commit as well: f21f303
Fixes: #32647