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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Promote feature gate NodePortLocal to GA #5491
Promote feature gate NodePortLocal to GA #5491
Changes from all commits
27de96a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's GA now, I think the default should be
true
? @tnqn @antoninbasThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the default value to
true
now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, we introduced the toggle intentionally to avoid unnecessary overhead added by the module as NPL would only be useful when users are using particular external LoadBalancers, which doesn't seem the majority case. Otherwise the feature gate alone can achieve what we need.
I took a look at the current code, even though the allocation of the node port is only triggered by the existence of the annotation, the code still needs to do some jobs to figure out the expectation first, which indicates some costs. And there could be some confusing logs generated as far as I can tell, for example, the following will be logged unconditionally as long as a Pod is added/updated even though there is no Service selecting it.
At the moment, I still prefer to keep it disabled by default and let users enable it on demand. If one day the feature becomes more common or its execution cost (when user doesn't annoate any Service) is proved to be trival, it will make sense to me to enable it by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for @tnqn 's comments. I set the default value to
false
now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should define
o.enableNodePortLocal = *o.config.NodePortLocal.Enable && features.DefaultFeatureGate.Enabled(features.NodePortLocal)
, instead of mutatingo.config.NodePortLocal.Enable
. See #5401There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, Add a new variable
o.enableNodePortLocal