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

Catch EBADF when watch_stream fails #280

Merged
merged 1 commit into from
Jan 7, 2018

Conversation

zeari
Copy link

@zeari zeari commented Dec 31, 2017

Solves https://bugzilla.redhat.com/show_bug.cgi?id=1507528
TBH I think we can catch any exception there and not IOError, EBADF specifically.

Explanation:
until finish is called, the code is looping in response.body.each do |chunk|.
finish closes @http_client while the other thread is reading from it so there could be any exception there depending where exactly the other thread is.

This isnt a great solution but the bug is just an error in the log and isnt an actual usability issue
@cben @agrare @moolitayer Please review

@moolitayer
Copy link
Collaborator

@zeari please add a test for the fix done here

@zeari
Copy link
Author

zeari commented Jan 1, 2018

@zeari please add a test for the fix done here

I have no clue how to test that

@agrare
Copy link
Member

agrare commented Jan 3, 2018

I have no clue how to test that

Could you add a test here https://github.com/abonas/kubeclient/blob/master/test/test_watch.rb which stubs the request and raises EBADF?

@agrare
Copy link
Member

agrare commented Jan 3, 2018

the bug is just an error in the log and isnt an actual usability issue

@zeari I thought you said this was preventing your event catcher worker from shutting down and causing it to get hung up?

@zeari
Copy link
Author

zeari commented Jan 4, 2018

@zeari I thought you said this was preventing your event catcher worker from shutting down and causing it to get hung up?

I thought so but now i see its just miq taking time shutting down workers.

@agrare
Copy link
Member

agrare commented Jan 4, 2018

Ah okay, still good to catch this cleanly 👍

@zeari
Copy link
Author

zeari commented Jan 4, 2018

@moolitayer @agrare Added specs

Copy link
Member

@agrare agrare 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

@moolitayer moolitayer left a comment

Choose a reason for hiding this comment

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

nice trick LGTM 👍

@moolitayer moolitayer requested a review from cben January 4, 2018 14:23
Copy link
Collaborator

@cben cben left a comment

Choose a reason for hiding this comment

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

LGTM

@moolitayer moolitayer merged commit e33da93 into ManageIQ:master Jan 7, 2018
cben pushed a commit to cben/kubeclient that referenced this pull request Jan 22, 2018
Catch EBADF when watch_stream fails
cben pushed a commit to cben/kubeclient that referenced this pull request Jan 25, 2018
Catch EBADF when watch_stream fails
moolitayer pushed a commit to moolitayer/kubeclient that referenced this pull request Feb 4, 2018
Catch EBADF when watch_stream fails

(cherry picked from commit e33da93)
moolitayer pushed a commit to moolitayer/kubeclient that referenced this pull request Feb 4, 2018
Catch EBADF when watch_stream fails

(cherry picked from commit e33da93)
@zeari
Copy link
Author

zeari commented Apr 9, 2018

TBH I think we can catch any exception there and not IOError, EBADF specifically.

This issue reared its ugly head again but this time as HTTP::ConnectionError instead of Errno::EBADF:

[----] E, [2018-04-09T14:52:33.790859 #21326:2245430] ERROR -- : MIQ(ManageIQ::Providers::Openshift::ContainerManager::EventCatcher::Runner#start_event_monitor) EMS [vm-48-38.eng.lab.tlv.redhat.com] as [] Event Monitor Thread aborted because [error reading from socket: Bad file descriptor]
[----] E, [2018-04-09T14:52:33.791242 #21326:2245430] ERROR -- : [HTTP::ConnectionError]: error reading from socket: Bad file descriptor  Method:[block in method_missing]
[----] E, [2018-04-09T14:52:33.791371 #21326:2245430] ERROR -- : .../.rvm/rubies/ruby-2.3.3/lib/ruby/2.3.0/openssl/buffering.rb:125:in `sysread'
.../.rvm/rubies/ruby-2.3.3/lib/ruby/2.3.0/openssl/buffering.rb:125:in `readpartial'
.../.rvm/gems/ruby-2.3.3/gems/http-2.2.2/lib/http/timeout/null.rb:44:in `readpartial'
.../.rvm/gems/ruby-2.3.3/gems/http-2.2.2/lib/http/connection.rb:212:in `read_more'
.../.rvm/gems/ruby-2.3.3/gems/http-2.2.2/lib/http/connection.rb:87:in `readpartial'
.../.rvm/gems/ruby-2.3.3/gems/http-2.2.2/lib/http/response/body.rb:29:in `readpartial'
.../.rvm/gems/ruby-2.3.3/gems/http-2.2.2/lib/http/response/body.rb:34:in `each'
.../.rvm/gems/ruby-2.3.3/gems/kubeclient-2.5.2/lib/kubeclient/watch_stream.rb:24:in `each'
.../.rvm/gems/ruby-2.3.3/bundler/gems/manageiq-providers-kubernetes-24f1e8999fa7/app/models/manageiq/providers/kubernetes/container_manager/kubernetes_event_monitor.rb:32:in `each'
.../.rvm/gems/ruby-2.3.3/bundler/gems/manageiq-providers-kubernetes-24f1e8999fa7/app/models/manageiq/providers/kubernetes/container_manager/event_catcher_mixin.rb:40:in `monitor_events'
.../Projects/manageiq/app/models/manageiq/providers/base_manager/event_catcher/runner.rb:161:in `block in start_event_monitor'

Can we just ignore exceptions when @finished == true ? @cben @moolitayer @agrare

@cben
Copy link
Collaborator

cben commented Apr 10, 2018

Yeah, I think

rescue StandardError
  raise unless @finished

would be good.
(all errors we saw till now fall under StandardError, and exceptions outside it are mostly there for good reason, e.g. Interrupt raised on Ctrl+C we shouldn't mask. see hierarchy: https://ruby-doc.org/core/Exception.html)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants