Skip to content

fix: if a watch closes and fails to reconnect, exit the operator #397

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

Merged

Conversation

secondsun
Copy link
Contributor

This is one of my proposed fixes for #395

@secondsun secondsun force-pushed the watch_close_on_reconnect_fail branch from 235fa24 to 5d49be2 Compare April 19, 2021 14:00
@secondsun
Copy link
Contributor Author

secondsun commented Apr 19, 2021

@metacosm I do get some different intermittent failures, but the failing test here is working locally. Thoughts?

@wtrocki
Copy link

wtrocki commented Apr 20, 2021

@secondsun we need this test to adjust to our solution:

DefaultEventHandlerTest.executesTheControllerInstantlyAfterErrorIfEventsBuffered:150 

After that build should pass and that would be easy merge

@metacosm metacosm self-assigned this Apr 20, 2021
@secondsun secondsun force-pushed the watch_close_on_reconnect_fail branch from a1eb58e to 6bd8f4f Compare April 20, 2021 16:27
Copy link
Collaborator

@metacosm metacosm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@adam-sandor adam-sandor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good solution for a rare event. I like that the framework gets tested on some buggy clusters so we can get these edge cases handled.

@metacosm
Copy link
Collaborator

Going ahead with the merge despite the tests failing. I've checked the PR locally on my system and everything works fine. Not sure why the tests are failing all the sudden.

@metacosm metacosm merged commit 937e274 into operator-framework:master Apr 20, 2021
@secondsun
Copy link
Contributor Author

The tests also passed before the rebase so they work sometime!

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