Automatically reconnect Kubernetes watcher when closed exceptionally #6023
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
Motivation:
A watcher in `KubernetesEndpointGroup` automatically reconnects when it
fails to connect to the remote peer. However, it does not reconnect
when a `WatcherException` is raised.
```
io.fabric8.kubernetes.client.WatcherException: too old resource version: 573375490 (573377297)
at io.fabric8.kubernetes.client.dsl.internal.AbstractWatchManager.onStatus(AbstractWatchManager.java:401)
at io.fabric8.kubernetes.client.dsl.internal.AbstractWatchManager.onMessage(AbstractWatchManager.java:369)
at io.fabric8.kubernetes.client.dsl.internal.WatcherWebSocketListener.onMessage(WatcherWebSocketListener.java:52)
at com.linecorp.armeria.client.kubernetes.ArmeriaWebSocket.onNext(ArmeriaWebSocket.java:106)
at com.linecorp.armeria.client.kubernetes.ArmeriaWebSocket.onNext(ArmeriaWebSocket.java:37)
at com.linecorp.armeria.common.stream.DefaultStreamMessage.notifySubscriberWithElements(DefaultStreamMessage.java:412)
...
Caused by: io.fabric8.kubernetes.client.KubernetesClientException: too old resource version: 573375490 (573377297)
... 62 common frames omitted
```
I don't know why `too old resource version` was raised but watchers
should not be stopped until `KubernetesEndpointGroup` is closed
Modifications:
- Refactor `KuberntesEndpointGroup` to start watchers asynchronously.
- Automatically restart `Watcher`s when `onClose(WatcherException)` is
invoked.
- Add more logs that I think might be useful.
- Also make the log formats consistent
- Debounce the update of endpoints to prevent
`EndpointGroup.whenReady()` from completing with a small number of
endpoints are recevied.
- The purpose is to avoid a small number of endpoints from receiving too much
traffic when a watcher is newly created.
Result:
`KubernetesEndpointGroup` now automatically reconnects a `Watcher` when
`WatcherException` is raised.
jrhee17
approved these changes
Dec 9, 2024
| } else { | ||
| podWatch = podWatch0; | ||
| } | ||
| watchPodAsync(service0.getSpec().getSelector()); |
There was a problem hiding this comment.
Question) What do you think of keeping the selector as a volatile field in KubernetesEndpointGroup, and have the pod thread continuously watching this field?
I'm imagining the case where
watchPodAsync(newSelector)is called- slightly later, an exception is thrown and
watchPodAsync(oldSelector)is called - the new selector is not used
If the above case is not possible, the current implementation seems fine.
minwoox
reviewed
Dec 9, 2024
.../linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroupFaultToleranceTest.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroup.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroup.java
Outdated
Show resolved
Hide resolved
| * The debounce millis for the update of the endpoints. | ||
| * A short delay would be enough because the initial events are delivered sequentially. | ||
| */ | ||
| private static final int DEBOUNCE_MILLIS = 10; |
There was a problem hiding this comment.
Isn't it too short? maybe 100 millis? (I'm worried about making another flaky test. 😉 )
.../src/main/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroup.java
Outdated
Show resolved
Hide resolved
minwoox
reviewed
Dec 9, 2024
| logger.warn("Pod watcher for {}/{} is closed.", namespace, serviceName, cause); | ||
| logger.warn("[{}/{}] Pod watcher is closed.", namespace, serviceName, cause); | ||
| logger.info("[{}/{}] Reconnecting the pod watcher...", namespace, serviceName); | ||
| // TODO(ikhoon): Add a backoff strategy to prevent rapid reconnections when the pod watcher |
There was a problem hiding this comment.
Can we at least add a delay (e.g. 3 seconds) to prevent the situation?
There was a problem hiding this comment.
Refactored to count errors that occur continuously.
- Immediately retry for the first failure.
- Delay with a backoff from the second failure.
minwoox
reviewed
Dec 10, 2024
.../src/main/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroup.java
Show resolved
Hide resolved
ikhoon
added a commit
to ikhoon/armeria
that referenced
this pull request
Dec 11, 2024
…ine#6023) Motivation: A watcher in `KubernetesEndpointGroup` automatically reconnects when it fails to connect to the remote peer. However, it does not reconnect when a `WatcherException` is raised. ``` io.fabric8.kubernetes.client.WatcherException: too old resource version: 573375490 (573377297) at io.fabric8.kubernetes.client.dsl.internal.AbstractWatchManager.onStatus(AbstractWatchManager.java:401) at io.fabric8.kubernetes.client.dsl.internal.AbstractWatchManager.onMessage(AbstractWatchManager.java:369) at io.fabric8.kubernetes.client.dsl.internal.WatcherWebSocketListener.onMessage(WatcherWebSocketListener.java:52) at com.linecorp.armeria.client.kubernetes.ArmeriaWebSocket.onNext(ArmeriaWebSocket.java:106) at com.linecorp.armeria.client.kubernetes.ArmeriaWebSocket.onNext(ArmeriaWebSocket.java:37) at com.linecorp.armeria.common.stream.DefaultStreamMessage.notifySubscriberWithElements(DefaultStreamMessage.java:412) ... Caused by: io.fabric8.kubernetes.client.KubernetesClientException: too old resource version: 573375490 (573377297) ... 62 common frames omitted ``` I don't know why `too old resource version` was raised but an important thing is that watchers should not be stopped until `KubernetesEndpointGroup` is closed. Modifications: - Refactor `KuberntesEndpointGroup` to start watchers asynchronously. - Automatically restart `Watcher`s when `onClose(WatcherException)` is invoked. - Add more logs that I think might be useful. - Also make the log formats consistent - Debounce the update of endpoints to prevent `EndpointGroup.whenReady()` from completing with a small number of endpoints. - The purpose is to prevent a few endpoints from receiving too much traffic when a watcher is newly created. Result: `KubernetesEndpointGroup` automatically reconnects a `Watcher` when `WatcherException` is raised.
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.
Motivation:
A watcher in
KubernetesEndpointGroupautomatically reconnects when it fails to connect to the remote peer. However, it does not reconnect when aWatcherExceptionis raised.I don't know why
too old resource versionwas raised but an important thing is that watchers should not be stopped untilKubernetesEndpointGroupis closed.Modifications:
KuberntesEndpointGroupto start watchers asynchronously.Watchers whenonClose(WatcherException)is invoked.EndpointGroup.whenReady()from completing with a small number of endpoints.Result:
KubernetesEndpointGroupautomatically reconnects aWatcherwhenWatcherExceptionis raised.