Clean up Endpoints object #16
Conversation
gardener-robot
added
needs/review
gardener-robot-ci-1
added
the
reviewed/ok-to-test
gardener-robot-ci-2
added
needs/ok-to-test
|
/assign |
| // Also, try to finish before a potential 15 seconds termination grace timeout. | ||
| ctx, cancel := context.WithTimeout(context.Background(), 14*time.Second) | ||
| defer cancel() | ||
| seedClient := ha.manager.GetClient() |
There was a problem hiding this comment.
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.
| seedClient := ha.manager.GetClient() | |
| client := ha.manager.GetClient() |
There was a problem hiding this comment.
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".
pkg/ha/ha_service.go
Outdated
| return err | ||
| } | ||
|
|
||
| // cleanUp is executed upon ending leadership. Its purpose is to remove the Endpoints object created upon acquiring |
There was a problem hiding this comment.
AFAIS, the func is called also on start up so it is not completely correct to state that it is only executed upon ending leadership.
There was a problem hiding this comment.
In all places where it is executed, a leader position is being held and the process is about to terminate (or I have a bug I'm missing).
| err = seedClient.Delete(ctx, &endpoints, deletionPrecondition) | ||
| if client.IgnoreNotFound(err) == nil { |
There was a problem hiding this comment.
| err = seedClient.Delete(ctx, &endpoints, deletionPrecondition) | |
| if client.IgnoreNotFound(err) == nil { | |
| deletionPrecondition := client.Preconditions{UID: &endpoints.UID, ResourceVersion: &endpoints.ResourceVersion} | |
| if err = seedClient.Delete(ctx, endpoints, deletionPrecondition); client.IgnoreNotFound(err) == nil { |
There was a problem hiding this comment.
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.
| // leadership. | ||
| func (ha *HAService) cleanUp() error { | ||
| // Use our own context. This function executes when the main application context is closed. | ||
| // Also, try to finish before a potential 15 seconds termination grace timeout. |
There was a problem hiding this comment.
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.
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.
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#L55
Not 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.
On a first read-through I am a bit concerned about race conditions between multiple replicas if HA is used.
IIRC the motivation to add the cleanup here was so that we don't add a new rbac for gardenlet to access endpoints.
However, couldn't we let the gardener-resource-manager simply deploy an empty endpoint together with the service as part of the ManagedResource that will deploy GCMx (GRM has access to all resource kinds). The mr would then delete the endpoint when GCMx is uninstalled.
andrerun
force-pushed
the
endpoints-cleanup
branch
from
8326c95 to
2faea7b
Compare
gardener-robot-ci-1
added
reviewed/ok-to-test
andrerun
force-pushed
the
endpoints-cleanup
branch
from
2faea7b to
4335f94
Compare
gardener-robot-ci-3
added
reviewed/ok-to-test
| // leadership. | ||
| func (ha *HAService) cleanUp() error { | ||
| // Use our own context. This function executes when the main application context is closed. | ||
| // Also, try to finish before a potential 15 seconds termination grace timeout. |
There was a problem hiding this comment.
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#L55
Not 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.
|
|
||
| attempt := 0 | ||
| var err error | ||
| for { |
There was a problem hiding this comment.
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 the for{...} loop he might decide to copy it and change it a bit, instead of simply reusing the Poll<...> 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.
| if attempt >= 10 { | ||
| break | ||
| } | ||
| time.Sleep(1 * time.Second) |
There was a problem hiding this comment.
What about using a time.NewTicker, or better yet clock.RealClock.NewTicker which allows you to use a Clock 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 a select statement for it, you can use the select to check if the context has expired meanwhile
Change description:
GCMx, upon exiting leader role, now opportunistically deletes the service Endpoints object it configured for itself when it entered leadership role.
Notes:
If you want to test this function on a GCMx which was launched via scaffold debug, deleting the pod won't work, because PID=1 would be DLV, not GCMx. You'd need to either terminate leadership, or directly send SIG_TERM to the GCMx process. With a pod which was not launched via skaffold/DLV, you can just delete the pod.
Before this change, HAService.Run() used to exit quickly - it only needed to arrange the Endpoints object. After the change, HAService.Run() blocks for the duration of the program execution, because it is not responsible for the cleanup upon program termination. This is reflected by a change in the unit tests.