-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[WIP] [DO NOT MERGE] Only run fan-in fan-out test #377
Conversation
Trying to gather more info for tektoncd#375 - does the test fail when it's run on it's own? (not 100% sure i got the syntax right haha just winging it)
Boo that didn't work:
I'll try this locally before pushing again XD |
the integration tests job looks like it passed, but it didn't actually run any tests |
Looks like it fails when it's run on it's own and the error I thought was ominous doesn't seem to be there: This looks interesting tho:
|
@bobcatfish : @pivotal-nader-ziada and I were suspecting if
|
I'll try reducing the test to only one task just to see if the failure still occurs! |
It looks like the test fails during the first Task so I want to try simplifying the test to see if it will fail with that Task only, or if it's the interaction between tasks that causes it.
oooo looks like it still fails, which is interesting:
|
I noticed this changed in tektoncd@5f28380 and the error we are getting is related to zones so just gonna wing it and see what happens. There was some other interesting stuff that changed too about service accounts that might be interesting to look into (or might have to change with this)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bobcatfish 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 |
curses, hacks foiled |
Since I'm intentionally trying to mess with this code manually, I need to disable the check that makes sure I haven't messed with it manually XD
hm weird. well i think just changing it in one place probably isnt enough after looking at knative/test-infra@c237e79 |
…al1-a`" That didn't work at all - I think I have to change a bunch of things if I want to go back to how it was (after looking at knative/test-infra@c237e79). This reverts commit daba8cf.
I want to try to simplify the repro case as much as I can, I'm guessing the problem is related to the output.
lol well that wasn't nearly clever enough:
|
lol jokes on me I wonder though about the fact that we're still binding all kinds of Tasks that aren't in teh Pipeline - maybe tektoncd#341 will catch that now that it's merged? maybe not tho!
Okay looks like it passed without the output! So that (i think) should imply that i can simplify this test down to only having the output and it should fail... |
The output is linked to the failure
Looks like we can repro this by just having an output (verified locally with us-central1 zone cluster instead of us-central-1a region cluster)
The pod gets into the bad state immediately, so we should see that right away in the taskrun status. We could make this even shorter, however the presubmit tests take like 10 min on their own anyway so *shrug* XD
@bobcatfish: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Okay I think I figured out what's going on, closing this PR and will update #375. |
Trying to gather more info for #375 - does the test fail when it's run
on it's own? (not 100% sure i got the syntax right haha just winging it)