Cleanup Part 1 - First refactors #990

evrardjp · 2024-10-12T11:46:23Z

This contains a few commits there were
historically in the MR 982 1, to refactor
kured for more maintainability.

This will be the first in a series of
refactors.

evrardjp · 2024-10-12T11:48:19Z

This COULD be merged after PR 989 if you want, but does not need to be.

It however MUST be merged after PR 982, as it contains a few of its commits. (needs rebase)

evrardjp · 2024-10-14T07:49:27Z

DCO issue on a commit, pushed it back.
I rebased on top of the changes asked in the PR for CI.

evrardjp · 2024-10-17T22:44:53Z

I don't like the idea to create so many small packages, with exported variables. Yet, I find that this is the best way to achieve the goals of cleanup. I intend to continue cleaning up with a part2, doing larger changes, then internalizing many packages.

Without this, the interface and the code to reboot is a bit more complex than it should be. We do not need setters and getters, as we are just instanciating a single instance of a rebooter interface. We create it based on user input, then pass the object around. This should cleanup the code. Signed-off-by: Jean-Philippe Evrard <[email protected]>

Without this, it makes the code a bit harder to read. This fixes it by extracting the method. Signed-off-by: Jean-Philippe Evrard <[email protected]>

This will be useful to refactor the checkers loop. Signed-off-by: Jean-Philippe Evrard <[email protected]>

This will make it easier to manipulate main in the future. Signed-off-by: Jean-Philippe Evrard <[email protected]>

Without this, validations are all over the place. This moves some validations directly into the function, to make the code simpler to read. Signed-off-by: Jean-Philippe Evrard <[email protected]>

Without this, the variable name is hard to follow. This fixes it by cleaning up the var name. Signed-off-by: Jean-Philippe Evrard <[email protected]>

pkg/reboot/command.go

jackfrancis · 2024-10-18T22:45:53Z

pkg/daemonsetlock/daemonsetlock.go

+func (dsl *DaemonSetLock) GetDaemonSet(sleep, timeout time.Duration) (*v1.DaemonSet, error) {
+	var ds *v1.DaemonSet
+	var lastError error
+	err := wait.PollImmediate(sleep, timeout, func() (bool, error) {


PollUntilContextTimeout might be cleaner here

https://pkg.go.dev/k8s.io/apimachinery/pkg/util/wait#PollUntilContextTimeout

Will do on follow up commit.

Done in a new commit, but not really confident here, as it has no test coverage. Can you double check on your side?

looks sane to me

pkg/checkers/checker.go

cmd/kured/pflags.go

cmd/kured/main.go

evrardjp · 2024-10-19T08:55:03Z

@jackfrancis can you please not squash merge this time? If necessary, I will adapt the commits to make it look cleaner, but I really dislike to lose history, especially when those commits are mostly atomic.

evrardjp · 2024-10-19T13:42:14Z

I added a few extra commits, to merely manage the API surface, and remove deps. I will stop here, it's large enough.

Everything is repushed, ready for your review again. It answers all the questions asked.

Without this, the checkers are only shell calls: test -f sentinelFile, or sentinelCommand. This changes the behaviour of existing code to test file for sentinelFile checker, and to keep the sentinel command as a command. However, to avoid having validation in the root loop, it moves to use a constructor to cleanup the code. Signed-off-by: Jean-Philippe Evrard <[email protected]>

Without this patch, the rebooter interface has data which is not related to the rebooter interface. This should get removed to make it easier to maintain. The loss comes from the logging, which mentioned the node. In order to not have a regression compared to [1], this ensures that at least the node to be rebooted appears in the main. [1]: kubereboot#134 Signed-off-by: Jean-Philippe Evrard <[email protected]>

Without this, we have no validation of the data in command/signal reboot. This was not a problem in the first refactor, as the constructor was a dummy one, without validation. However, as we refactoed, we now have code in the root method that is validation for the reboot command. This can now be encompassed in the constructor. Signed-off-by: Jean-Philippe Evrard <[email protected]>

Without this, the main loop is in need of 3 functions to simply parse flags and env variables (excluding input validation). This is a bit more complex than it should, especially since we only need to parse command line flags and env vars. This fixes it by simply using pflags (which we were already using) instead of pflags + viper + cobra (for which we do not have any benefit), and removing all the methods outside the mapping of env var with cli flag. The main code is now far simpler: It handles the reading, parsing, and returning in case of error. As we do not bubble up errors from rebootasRequired yet, this is good enough at this moment. Signed-off-by: Jean-Philippe Evrard <[email protected]>

If the notification url configuration is known to be not working, this should be raised as an error, not a warning. Without this, it would be easy to miss a misconfiguration. Signed-off-by: Jean-Philippe Evrard <[email protected]>

Implementation details of lock should not leak into the calling methods. Without this path, calls are a bit more complex and error handling is harder to find. This is a problem for long term maintenance, as it is tougher to refactor the locks without impacting the main. Decoupling the two (main usage of the lock, and the lock themselves) will allow us to introduce other kinds of locks easily. I solve this by inlining into the daemonsetlock package: - including all the methods for managing locks from the main.go functions. Those were mostly doing error handling where code became no-op by introducing multiple daemonsetlock types - adding the lock release delay part of lock info I also did not like the pattern include in Test method, which added a reference to nodeMeta: It was not very clear that Test was storing the current metadata of the node, or was returning the current state. (Metadata here only means unschedulable). The problem I saw was that the metadata was silently mutated from a lock Test method, which was very not obvious. Instead, I picked to explicitly return the lock data instead. I also made it explicit that the Acquire lock method is passing the node metadata as structured information, rather than an interface{}. This is a bit more fragile at runtime, but I prefer having very explicit errors if the locks are incorrect, rather than having to deal with unvalidated data. For the lock release delay, it was part of the rebootasrequired loop, where I believe it makes more sense to be part of the Release method itself, for readability. Yet, it hides the delay into the implementation detail, but it keeps the reboot as required goroutine more readable. Instead of passing the argument rebootDelay as parameter of the rebootasrequired method, this refactor took creation of the lock object in the main loop, close to all the variables, and then pass the lock object to the rebootasrequired. This makes the call for rebootasrequired more clear, and lock is now encompassing everything needed to acquire, release, or get info about the lock. Signed-off-by: Jean-Philippe Evrard <[email protected]>

Without this, a bit of the validation is done in main, while the rest is done in each constructor. This fixes it by create a new global constructor in checkers/reboot to solve all the cases and bubble up the errors. I prefered keeping the old constructors, and calling them, this way someone wanting to have a fork of the code could still create directly the good checker/rebooter, without the arbitrary decisions taken by the generic constructor. However, kured is not a library, and was never intended to be usable in forks, so we might want to reconsider is part 2 of the refactor. Signed-off-by: Jean-Philippe Evrard <[email protected]>

This will remove double pointers, and be explicit about the type we are using. Signed-off-by: Jean-Philippe Evrard <[email protected]>

Replaced with PollUntilContextTimeout. Signed-off-by: Jean-Philippe Evrard <[email protected]>

Without this, impossible to bubble up errors to main Signed-off-by: Jean-Philippe Evrard <[email protected]>

The main is doing flag validation through pflags, then did further validation by involving the constructors. With the recent refactor on the commit "Refactor constructors" in this branch, we moved away from that pattern. However, it means we reintroduced a log dependency into our external API, and the external API now had extra validations regardless of the type. This is unnecessary, so I moved away from that pattern, and moved back all the validation into a central place, internal, which is only doing what kured would desire, without exposing it to users. The users could still theoretically use the proper constructors for each type, as they would validate just fine. The only thing they would lose is the kured internal decision of validation/precedence. Signed-off-by: Jean-Philippe Evrard <[email protected]>

This at the same time, removes the alert public package. Alert was only used inside prometheus blocker, so it allows to simplify the code. Signed-off-by: Jean-Philippe Evrard <[email protected]>

evrardjp · 2024-10-19T14:01:47Z

@ckotzbauer I know you're quite busy nowadays... Could you tell us if you have time to review this/give your opinion?

I am okay to merge it with only @jackfrancis approval... but in case you want to have a look, don't hesitate to tell.

evrardjp · 2024-10-24T21:03:44Z

@jackfrancis should we get the ball rolling?

jackfrancis

lgtm

jackfrancis · 2024-10-24T21:30:55Z

@evrardjp feel free to merge w/ your preferred commit preservation strategy

evrardjp · 2024-10-27T20:45:13Z

Thanks @jackfrancis for the reviews.

evrardjp force-pushed the first_refactors branch 2 times, most recently from cad319d to 41ef215 Compare October 14, 2024 07:48

evrardjp changed the title ~~Add refactors~~ Add refactors (part 1) Oct 15, 2024

evrardjp force-pushed the first_refactors branch from 41ef215 to d5d9374 Compare October 17, 2024 21:16

evrardjp changed the title ~~Add refactors (part 1)~~ Cleanup Part 1 - First refactors Oct 17, 2024

evrardjp added 6 commits October 18, 2024 00:53

Extract privileged command wrapper into util

3bfdd76

Without this, it makes the code a bit harder to read. This fixes it by extracting the method. Signed-off-by: Jean-Philippe Evrard <[email protected]>

Add checker interface

574065f

This will be useful to refactor the checkers loop. Signed-off-by: Jean-Philippe Evrard <[email protected]>

Extract blockers

eeedf20

This will make it easier to manipulate main in the future. Signed-off-by: Jean-Philippe Evrard <[email protected]>

Move command line validations in pre function

00d8a52

Without this, validations are all over the place. This moves some validations directly into the function, to make the code simpler to read. Signed-off-by: Jean-Philippe Evrard <[email protected]>

Rename variable

36e6c8b

Without this, the variable name is hard to follow. This fixes it by cleaning up the var name. Signed-off-by: Jean-Philippe Evrard <[email protected]>

evrardjp force-pushed the first_refactors branch from d5d9374 to 1e8b592 Compare October 17, 2024 22:54

evrardjp mentioned this pull request Oct 18, 2024

[WIP] Cleanup Part 2 - Rewrite #1000

Open