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
Clean up Endpoints object #16
base: main
Are you sure you want to change the base?
Clean up Endpoints object #16
Changes from all commits
f50dda3
4335f94
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.
From where these 15s of potential termination grace timeout come from?
According to https://github.com/gardener/gardener-custom-metrics/blob/c43b2064794e5534f2a0d7a831285210620f9ed8/example/custom-metrics-deployment.yaml#L72 we should have 30s from the SIGTERM signal until the SIGKILL.
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.
15 is a nice, round number, and also half of the default 30. I'm speculating that upon a hypothetical future shortening of grace period, 15 will be a likely choice (the other obvious choice being 10, of course).
This is not a critical choice. I'm simply picking a value which is likely to work slightly better with potential future changes.
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.
There is an option for the manager which allows to specify the termination grace period for all runnables:
GracefulShutdownTimeout
and it is defaulted to 30s: https://github.com/kubernetes-sigs/controller-runtime/blob/76d3d0826fa9dca267c70c68c706f6de40084043/pkg/manager/internal.go#L55Not sure if it makes sense to use a (doubly) short time for this function.
Either way, if you have a strong reason to not specify the default or not make it configurable, and keep it
15s
can you please add the reason as a comment. Otherwise people will wonder where the magic number comes from.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.
Nit: I wouldn't call it a seedClient. In all other places we call it
client
. Tomorrow, we might need to support the runtime cluster to scale gardener-apiserver or virtual-kube-apiserver.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.
Agreed, but since this program is talking to multiple clusters, I don't want to use "client". I agree with your point that the name will need to change in the future, but at that time I'll also have the the context which will allow me come up with the right genralisation, without resorting to the excessively general (IMO) "client".
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.
Can we use some of the
Poll<...>
funcs in https://github.com/kubernetes/kubernetes/blob/v1.28.0/staging/src/k8s.io/apimachinery/pkg/util/wait/poll.go?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 admit that it may have been better to use a poll function, but I don't think this it's worth refactoring. Replacing a "for" with which everybody is familiar, with а callback-based function in a domain-specific library, is a matter of preference.
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'd personally prefer to use
Poll<...>
here unless there is a strong argument for the max number of attempts. Then I would stick with the for loop.Generally, one benefit is that
Poll<...>
should already be tested. Another is that when someone tries to do a similar wait and sees thefor{...}
loop he might decide to copy it and change it a bit, instead of simply reusing thePoll<...>
function. As for domain-specificity - I think both GCMx and the functions in the https://github.com/kubernetes/kubernetes/blob/v1.28.0/staging/src/k8s.io/apimachinery/pkg/util/wait/poll.go share the same domain.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.
Image that we have (or introduce) a bug in the func by forgetting to break/exit in 1 case. This
for {
would then results in endless loop. Why we should have a custom logic for retrying where there is a package that already does this?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.
@ialidzhikov @plkokanov
Poll() is a better fit. No good argument against it. It was just that I didn't know if refactoring is worth it.
But the fact is that I haven't used Poll() before, so I can't really judge. I expect you're probably right and the improved readability justifies the refactoring.
I'll use Poll().
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.
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'm intentionally keeping "delete with precondition" on a separate line here. It's an uncommon construct, which is likely to give the reader a pause, and I don't want to force other logic on the same line.
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.
What about using a time.NewTicker, or better yet
clock.RealClock.NewTicker
which allows you to use aClock
interface that can be mocked for tests.Tickers take into account the time that was actually spent while executing the
Get
/Update
calls. Additionally, since you will have to add aselect
statement for it, you can use theselect
to check if the context has expired meanwhile