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

Use IO#readline_nonblock instead of IO#readline when reading output from CPI sub-processes. #2336

Merged

Conversation

yatzek
Copy link
Contributor

@yatzek yatzek commented Nov 2, 2021

IO.#readline, being a blocking method, can cause a dealock when the cpi
sends output without a line break. We had such issues with azure cpi.

What is this change about?

Describe the change and why it's needed.

Please provide contextual information.

Include any links to other PRs, stories, slack discussions, etc... that will help establish context.

What tests have you run against this PR?

Include a comprehensive list of all tests run successfully.

How should this change be described in bosh release notes?

Something brief that conveys the change and is written with the Operator audience in mind.
See previous release notes for examples.

Does this PR introduce a breaking change?

Does this introduce changes that would require operators to take care in order to upgrade without a failure?

Tag your pair, your PM, and/or team!

It's helpful to tag a few other folks on your team or your team alias in case we need to follow up later.

@yatzek yatzek force-pushed the fix_azure_cpi_incomplete_line_stdout branch from 1611fb7 to 68ff426 Compare November 3, 2021 18:35
@rkoster rkoster self-requested a review November 4, 2021 11:39
@rkoster rkoster self-assigned this Nov 4, 2021
@rkoster rkoster requested a review from beyhan November 4, 2021 11:40
Copy link
Contributor

@rkoster rkoster left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, seems like a great improvement. However, when testing test change locally I found that changing back to the original implementation did not make the test fail.

…rom CPI sub-processes.

IO.#readline, being a blocking method, can cause a dealock when the cpi
sends output without a line break. We had such issues with azure cpi.
@yatzek yatzek force-pushed the fix_azure_cpi_incomplete_line_stdout branch from 68ff426 to 3e8ad25 Compare November 4, 2021 13:34
Copy link
Contributor

@rkoster rkoster left a comment

Choose a reason for hiding this comment

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

With the latest changes I'm getting:

........................................................................................................................An exception occurred running RSpec::Core::Example:
#<NoMethodError: undefined method `asset' for #<RSpec::ExampleGroups::BoshCloudsExternalCpi::FixForADealockScenarioWhenACpiSubProcessSendsAnInclompleteLineMissingNToSTDOUT "does not deadlock" (./bosh-director/spec/unit/cloud/external_cpi_spec.rb:658)>>

Test directory: /Users/rubenk/workspace/bosh/src/tmp/integration-tests-workspace/pid-99152/spec-20211108-99152-ikaeaa

Sandbox directory: /Users/rubenk/workspace/bosh/src/tmp/integration-tests-workspace/pid-99152
F

Failures:

  1) Bosh::Clouds::ExternalCpi Fix for a dealock scenario when a cpi sub-process sends an inclomplete line (missing "\n") to STDOUT does not deadlock
     Failure/Error: before { allow(File).to receive(:executable?).with(asset("bin/dummy_cpi")).and_return(true) }

     NoMethodError:
       undefined method `asset' for #<RSpec::ExampleGroups::BoshCloudsExternalCpi::FixForADealockScenarioWhenACpiSubProcessSendsAnInclompleteLineMissingNToSTDOUT "does not deadlock" (./bosh-director/spec/unit/cloud/external_cpi_spec.rb:658)>
     # ./bosh-director/spec/unit/cloud/external_cpi_spec.rb:656:in `block (3 levels) in <top (required)>'

Finished in 3.13 seconds (files took 3.25 seconds to load)
841 examples, 1 failure

Failed examples:

rspec ./bosh-director/spec/unit/cloud/external_cpi_spec.rb:658 # Bosh::Clouds::ExternalCpi Fix for a dealock scenario when a cpi sub-process sends an inclomplete line (missing "\n") to STDOUT does not deadlock

Copy link
Member

@beyhan beyhan left a comment

Choose a reason for hiding this comment

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

My summary:

  • all unit tests ran successful on my MacOS.
  • leaving the new does not deadlock unit test and reverting to the old code failed the test

@rkoster is the failure with the unit test reproducible on your machine?

src/bosh-director/lib/cloud/external_cpi.rb Show resolved Hide resolved
@rkoster
Copy link
Contributor

rkoster commented Nov 11, 2021

Thanks @yatzek!

@rkoster rkoster merged commit bb3b174 into cloudfoundry:master Nov 11, 2021
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