-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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
kubectl attach test: wait for input before proceeding #79235
Conversation
/retest Flake: |
Output seems to show it passing, so ... /retest |
This way we know stdin is attached and we are in a known state.
@soltysh you previously asked for a simplification of this test, and you were right! This adds a little complexity to the command we run, so that we wait for an attach, but then the test logic is much simpler because there's only one path (cf #73099 which describes the various possible paths previously) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable, let's merge it and see how the flakiness changes.
/lgtm
/approve
/milestone v1.16
Thanks @justinsb and sorry for delays 😊 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…ore proceeding This way we know stdin is attached and we are in a known state. ref: kubernetes/kubernetes#79235 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
…ore proceeding This way we know stdin is attached and we are in a known state. ref: kubernetes/kubernetes#79235 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
…ore proceeding This way we know stdin is attached and we are in a known state. ref: kubernetes/kubernetes#79235 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
I think this breaks the intent of the test. Kubectl attach isn’t supposed to race here. |
Not quite, we've seen this being racy also in openshift, mostly problem was on the container engine side. |
Interesting - that certainly wasn't my intent @smarterclayton - sorry! I'm poking around, trying to understand what the previous race was. |
This way we know stdin is attached and we are in a known state.
/kind failing-test
Fixes #73099