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

CI: add support for writing to ptys #3591

Merged
merged 3 commits into from
Feb 22, 2025
Merged

Conversation

apostasie
Copy link
Contributor

This is the next step in removing unbuffer from our testing, adding the ability to write to stdin with ptys.

@apostasie
Copy link
Contributor Author

Failure is unrelated.

@apostasie apostasie marked this pull request as draft October 24, 2024 13:54
@apostasie apostasie changed the title Add support for writing to ptys [WIP] Add support for writing to ptys Oct 30, 2024
@apostasie
Copy link
Contributor Author

Ok, back on this...

Refresher: the motivation for this overall is to get rid of unbuffer, which has issues:

  • forces a third-party binary dependency on the host
  • merges together stdout and stderr (it's a pty...)
  • closes streams in unexpected ways, forcing us to introduce arbitrary waits
  • since it is not go, we cannot debug it as part of the normal testing process

The proposed solution introduces a pure go implementation leveraging ptmx for test tooling.
Right now, it is linux only.

The second commit uses the above to rewrite detach tests, removing unbuffer and focusing on avoiding flakyness and racy behaviors.
It also reverts #3558 since the new implementation addresses the underlying issue, and re-enables all tests for Docker as well.

The third separate commit fixes #3881 - as I tripped it while working on this.

Note that icmd is problematic (see #3883) and we might have to get rid of it eventually.
Right now, there is a very ugly hack in place, reading from the master and stuffing that instead of icmd.Stdout() in some cases.
This part of the code starts to smell really funny and might need some cleanup, but for now I think we should give this whole thing a spin and see how it goes before considering cleanup.

Also note that this PR removes entirely DelayOnceReader, which is harmful: its only motivation was to workaround unbuffer stream closing timing, but in the process it was hiding other sources of flakyness, specifically related to the way we proxy stdin to read detach keys which may race the container reading prior stdin content.

Finally, the rewritten tests should see a decent speed boost.

If this all checks-out with you folks, next PR will rewrite other tests using unbuffer so that we can close the chapter on it.

CI is pending, so there might be a few more minor adjustments (will mark "ready" when it is green), but otherwise, this is ready for review.

@AkihiroSuda @fahedouch

@apostasie apostasie changed the title [WIP] Add support for writing to ptys Add support for writing to ptys Feb 14, 2025
@apostasie apostasie marked this pull request as ready for review February 14, 2025 09:06
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 4beadf1 into containerd:main Feb 22, 2025
30 checks passed
@AkihiroSuda AkihiroSuda added the area/ci e.g., CI failure label Feb 22, 2025
@fahedouch
Copy link
Member

sorry for my lack of responsiveness, thank you for your efforts.

@apostasie
Copy link
Contributor Author

sorry for my lack of responsiveness, thank you for your efforts.

No problem at all @fahedouch
The fun is all mine - I get to hack on cool stuff and share it :-)

@apostasie apostasie changed the title Add support for writing to ptys CI: add support for writing to ptys Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci e.g., CI failure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants