Fix Cleanup Code for v2.x #1650
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aauren
force-pushed
the
fix_cleanup_code_for_v2.x
branch
from
237d11e
to
e488897
Compare
|
@aauren it works. Not without errors though |
|
@vladimirtiukhtin - Thanks for testing! Can you show me how you ran the cleanup? When I tested, I ran it from within the kube-router container and I didn't get any of the errors that you got. But we could probably make it a bit more robust. |
|
This is what I did on one of the nodes |
|
So on my cluster, I was able to run without issues if I add in a bind mount for the The ipset error are likely a symptom of the referencing iptables not being removed. Those errors are caused by this log line: I can't reproduce that on my node, but that is probably due to not having the same contention over iptables as your cluster likely has. You mentioned in another post that you have configured some jobs to run with a frequency of 1 minute. If those types of nodes are the same ones where you are testing this, then you likely have a lot more going on with iptables than I do in my cluster. My guess is that See if that works for you? |
Before the logic ran like the following in terms of preference: 1. Prefer environment var NODE_NAME 2. `Use os.Hostname()` 3. Fallback to `--hostname-override` passed by user This didn't make a whole lot of sense, as `--hostname-override` is directly, and supposedly intentionally set by the user, therefore it should be the MOST preferred, not the least preferred. Additionally, none of the errors encountered were passed back to the user so that future conditions could be considered, so if there was an error at the API level, that error was swallowed. Now the logic looks like: 1. Prefer `--hostname-override` if it is set. If it is set and we weren't able to resolve to a node object, return the error 2. Use environment var NODE_NAME if it is set. If it is set and we weren't able to resolve to a node object, return the error 3. Fallback to `os.Hostname()`. If we weren't able to resolve to a node object then return the error and give the user options
kube-router v2.X introduced the idea of iptables and ipset handlers that allow kube-router to be dual-stack capable. However, the cleanup logic for the various controllers was not properly ported when this happened. When the cleanup functions run, they often have not had their controllers fully initialized as cleanup should not be dependant on kube-router being able to reach a kube-apiserver. As such, they were missing these handlers. And as such they either silently ended up doing noops or worse, they would run into nil pointer failures. This corrects that, so that kube-router no longer fails this way and cleans up as it had in v1.X.
aauren
force-pushed
the
fix_cleanup_code_for_v2.x
branch
from
e488897
to
2f4fbf8
Compare
|
I have updated the user-guide to show a better version of the cleanup command which should resolve any errors for most users. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
kube-router v2.X introduced the idea of iptables and ipset handlers that allow kube-router to be dual-stack capable. However, the cleanup logic for the various controllers was not properly ported when this happened. When the cleanup functions run, they often have not had their controllers fully initialized as cleanup should not be dependant on kube-router being able to reach a kube-apiserver.
As such, they were missing these handlers. And as such they either silently ended up doing noops or worse, they would run into nil pointer failures.
This corrects that, so that kube-router no longer fails this way and cleans up as it had in v1.X.
@vladimirtiukhtin - I tested this in my environment and found it worked correctly, but if you wouldn't mind checking as well, that would be helpful to ensure that there isn't something I'm missing. There should be a kube-router docker container that is built as part of this PR that you can find in the GitHub actions section if you don't want to build kube-router yourself.
Fixes: #1649