Parametrize reboot commands #297
Conversation
|
This is on top of #260 . We can either merge it with 2 prs or just one. |
|
I was going to suggest adding a make target that runs the tests, but this might be complicated since the binary won't compile on macos, the tests can't be run without some kind of linux environment. It would be great if the tests you've added here can be hooked up to run as part of the CI. |
|
@kingdonb IIRC, the |
|
The Dockerfile isn't running any test from what I can tell, and there is no sign of test in the Makefile at all. There are a few tests that look like they are meant to be run by hand, on kind, one that sets up the reboot sentinels and another that checks the reboots proceeding orderly. I like the unit tests I saw that you added, this is a great technique to make source code better. The linux binary is cross-compiled in the Makefile and copied into the docker image, it would be possible to run the test inside a build if there was a compiler/sdk added in the Dockerfile, or build from some SDK image. Perhaps this Dockerfile can be factored to support this as a multi-stage build? I found a Linux machine to run the tests on that you added: I am not sure if it's possible to redirect those logs or capture somehow, since they are good logs that indicate success of the test, (but outwardly giving the appearance of bad news during the test execution is less than helpful for anyone running the tests and reading along for success output.) I think this is a great PR and I am likely to give it my approval if some of this can be addressed |
evrardjp
force-pushed
the
refactor-reboot-command
branch
from
29a938d
to
824cf33
Compare
|
Rebased. |
|
New features in, need adapting. WIP. |
evrardjp
force-pushed
the
refactor-reboot-command
branch
from
824cf33
to
de19f05
Compare
|
This should do the trick. |
Without this patch, we rely on global state in many functions for which we check the reboot blockers. This is a problem, as it's harder to test. This patch fixes it by refactoring the reboot blockers. This also includes a first series of unit tests for our main.
Without this patch, you cannot configure the reboot command to use, or the use another command to trigger a reboot. This is a problem, as multiple users have asked for it in the past, and we are lacking flexibility. This fixes it by introducing two new parameters, - one to provide a custom reboot command. This should help people running kured on non systemd OS - one to provide a custom sentinel command. This should help people running non Ubuntu OS, as they can directly use their command instead of generating a file (useful for CentOS/SUSE) For this, several refactors had to be done, to remove global state in some functions. Making those functions closer to "pure functions" helps us increase our test coverage here and later. As commandReboot was very close to rebootCommand, the function to reboot the node has been renamed to invokeReboot.
Without this, go test will rightfully fail. This is a problem, as we don't have go test enabled, but we want to have this in the future. This should fix it.
evrardjp
force-pushed
the
refactor-reboot-command
branch
from
de19f05
to
885be7c
Compare
|
Rebased for 1.20. |
|
Well, the CI does a preliminary test and doesn't seem to complain too much :) |
|
I won't merge my own PR, so someone else should do ;) |
No description provided.