Skip to content
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

Watch keepalive not working anymore with 1.0.0 #2127

Open
Alabate opened this issue Dec 27, 2024 · 4 comments · May be fixed by #2131
Open

Watch keepalive not working anymore with 1.0.0 #2127

Alabate opened this issue Dec 27, 2024 · 4 comments · May be fixed by #2131

Comments

@Alabate
Copy link

Alabate commented Dec 27, 2024

Describe the bug
With the version 1.0.0, when a watch request is made and the connection drop, no more events are reported, but there is also no error that indicate that the connection was dropped. (I waited 10 minutes to be sure)

With version 0.22.2, under ~33s, I got the error read ETIMEDOUT, allowing me to retry the connection and restart the watch request.

This issue was already raised on #559 and fixed with PR #630. This PR add a config to configure keepalive packets in watch.ts, but I don't see this config on the version 1.0.0.

Client Version
1.0.0 (but working on 0.22.2)

Server Version
1.30.3

To Reproduce
In my case, I start the watch-example.ts and then cut the VPN connection to my cluster. As it's related to network perturbation, it's not easy to suggest an easy reproducible way to do it. I guess you can also use your firewall to drop packets in the middle of a watch request.

Expected behavior
I expect an error to be thrown when the Kubernetes api becomes unavailable while a watch request is running like it was the case for previous versions.

** Example Code**
I used the watch-example.ts

Environment (please complete the following information):

  • OS: Linux
  • NodeJS Version 20.16.0
@cjihrig
Copy link
Contributor

cjihrig commented Dec 27, 2024

My guess is that the fix from the v0.x branch just never made it to the v1.x branch. Does applying the fix from #630 fix the issue for you locally?

If it does fix the issue, I also wonder if we even need the net-keepalive dependency from #630. It looks like they have documented that newer versions of Node used by the v1.x branch may not need the dependency - hertzg/node-net-keepalive#318.

@jportner
Copy link

node-fetch doesn't support keepalive out of the box, you need to use an Agent, see node-fetch/node-fetch#423

@cjihrig
Copy link
Contributor

cjihrig commented Dec 27, 2024

Ah, ok. We do support Agents in the new version. This call returns an object containing an agent (created in src/config.ts). So maybe we need to update those relevant code paths.

@Alabate
Copy link
Author

Alabate commented Dec 27, 2024

Actually, in 0.22.2, net-keepalive is already removed in favor of socket.setKeepAlive(): https://github.com/kubernetes-client/javascript/blob/0.22.2/src/watch.ts#L120 (PR: #635)

So I guess, we need to access the socket and apply the same options, I will try to do a PR.

Alabate pushed a commit to Alabate/kubernetes-client-javascript that referenced this issue Dec 28, 2024
These features were present prior to the 1.0 refactor but were inadvertently removed.

See kubernetes-client#2127
Alabate pushed a commit to Alabate/kubernetes-client-javascript that referenced this issue Dec 28, 2024
This will send sends keep-alive probes to the server every 30 seconds.
These features were present prior to the 1.0 refactor but were inadvertently removed.

Fixes kubernetes-client#2127

Previous relevant issues:
- Initial issue: kubernetes-client#559
  - PR: kubernetes-client#630
- Improvement: kubernetes-client#632
  - PR: kubernetes-client#635
Alabate added a commit to Alabate/kubernetes-client-javascript that referenced this issue Dec 28, 2024
This will send sends keep-alive probes to the server every 30 seconds.
These features were present prior to the 1.0 refactor but were inadvertently removed.

Fixes kubernetes-client#2127

Previous relevant issues:
- Initial issue: kubernetes-client#559
  - PR: kubernetes-client#630
- Improvement: kubernetes-client#632
  - PR: kubernetes-client#635
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants