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

Add support for WatchProgressRequest #268

Merged
merged 4 commits into from
Jan 30, 2024

Conversation

brandond
Copy link
Member

@brandond brandond commented Jan 16, 2024

@brandond brandond requested a review from a team as a code owner January 16, 2024 00:39
@brandond
Copy link
Member Author

@qwtsc @onprem are you able to test this?

@qwtsc
Copy link

qwtsc commented Jan 16, 2024

@qwtsc @onprem are you able to test this?

good! let me have a try!

@qwtsc
Copy link

qwtsc commented Jan 16, 2024

@brandond Bad news, it does not work in my e2e test environment.

@brandond
Copy link
Member Author

Are you able to run Kine in debug mode, and share the results?

@qwtsc
Copy link

qwtsc commented Jan 16, 2024

My e2e test doesn't work with real etcd as backend storage either. I am so confused now. @onprem can you share the settings of apiserver?

@qwtsc
Copy link

qwtsc commented Jan 16, 2024

FYI, the related issue has been reported in k&k.

@brandond
Copy link
Member Author

I commented on the Kubernetes issue you linked, but I think the behavior you're expecting was incorrect and is no longer observed by v1.27+

@brandond
Copy link
Member Author

Hmm. If I add info-level logging on progress requests in kine, I see that they are CONSTANTLY being spammed by the apiserver. I also note that things seem to take way longer to start up when the featuregate is enabled. This feels pretty broken at the moment, and I don't think it is etcd or kine - I think the feature does not work properly.

Also fixes progress notifications to be more reliable

Signed-off-by: Brad Davidson <[email protected]>
@brandond brandond force-pushed the enable_progressrequest branch from 5cab6d2 to 04e0f5a Compare January 18, 2024 10:45
@brandond
Copy link
Member Author

@qwtsc @onprem are you able to test with the commit I just pushed? I am still seeing some weirdness from the apiserver, but I believe the implementation is correct.

@qwtsc
Copy link

qwtsc commented Jan 18, 2024

@qwtsc @onprem are you able to test with the commit I just pushed? I am still seeing some weirdness from the apiserver, but I believe the implementation is correct.

Ok, Let's see if it works or not.

@qwtsc
Copy link

qwtsc commented Jan 18, 2024

@brandond unfortunately, it doesn't work, but same test case works for etcd with special flag experimental-watch-progress-notify-interval open. Can you find out what happened in etcd when running with this "experimental" flag?

@brandond
Copy link
Member Author

experimental-watch-progress-notify-interval is not a Boolean flag; it requires a value and overrides the default 10 minute watch progress interval. What are you setting it to?

Is your test case just the previously discussed curl request, or do you have something else that you can share?

@brandond
Copy link
Member Author

brandond commented Jan 18, 2024

Oh, I see from the upstream k/k issue that it must be set to 5 seconds instead of the default 10 minutes, for the Kubernetes feature to work. I didn't see that documented anywhere; let me take a look at making that configurable in kine.

@brandond
Copy link
Member Author

brandond commented Jan 18, 2024

@qwtsc please try with the most recent commit, using the new CLI flag --watch-progress-notify-interval=5s

@qwtsc
Copy link

qwtsc commented Jan 19, 2024

experimental-watch-progress-notify-interval is not a Boolean flag; it requires a value and overrides the default 10 minute watch progress interval. What are you setting it to?

Is your test case just the previously discussed curl request, or do you have something else that you can share?

Something like curl, but it's written in golang.

try with the most recent commit, using the new CLI flag --watch-progress-notify-interval=5s

let's try. BTW, you're so hard working, I've noticed progress in this PR almost every six hours. Do you ever take time to rest?:-)

@brandond
Copy link
Member Author

Do you ever take time to rest? :-)

We have had a weeklong ice storm and my kids are out of school, so I don't have much else to do lol

@qwtsc
Copy link

qwtsc commented Jan 19, 2024

Cheers, now It works smoothly. Thanks for your impressive contribution. @brandond

@brandond brandond force-pushed the enable_progressrequest branch from 2d52922 to bffa8c9 Compare January 19, 2024 09:08
Signed-off-by: Brad Davidson <[email protected]>
@brandond brandond merged commit 9b7a91a into k3s-io:master Jan 30, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants