-
Notifications
You must be signed in to change notification settings - Fork 103
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
Store KinD logs artifact in E2E tests #1250
Conversation
019b4bf
to
9ed5a03
Compare
Oh, it isn't that easy, because these test are run in a separate container and we can't access its contents. Have to find a different approach to not use |
d08cb1b
to
827ed3c
Compare
7616e40
to
9a21216
Compare
9a21216
to
dd4880d
Compare
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.
lgtm
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.
The first question that springs to my mind is - why not just put the logs into a directory mounted into the container with -v
, all the more since we already use this mechanism to expose the reports
directory?
Your approach would also work but strikes me as a lot of complexity (random string generation, explicit container name, explicit imperative cleanup, potentially fragile manipulation of kudo-e2e-test.yaml
) that could be easily avoided.
test/run_tests.sh
Outdated
|
||
# "TARGET" is a Makefile target that runs tests | ||
TARGET=$1 | ||
|
||
INTEGRATION_OUTPUT_JUNIT=${INTEGRATION_OUTPUT_JUNIT:-false} | ||
|
||
CONTAINER_POSTFIX=$(< /dev/urandom base64 | tr -dc '[:alpha:]' | head -c 8 || true) |
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.
Hm, do we really want to continue if this is too short (possibly empty)?
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 is to avoid a SIGPIPE
.
@porridge Yeah, mounting a directory seems to be the easiest approach, I somehow didn't see that we already do this for the reports. |
726ce0d
to
d594436
Compare
Co-Authored-By: Marcin Owsiany <[email protected]>
Mounting directories didn't work due to file permission issues. I'm keeping the |
I ran some experiments, and it turns out that circleci runs the job as user A strategically placed recursive Could I convince you to go back to the path of exposing these with |
@porridge Thanks for looking into this! I'll change this to directory mounts & |
@porridge Log collection is now using a mounted folder, ptal. |
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.
Any particular reason you're telling CircleCi to store the tarball, rather than directly the directory with logs?
Co-Authored-By: Marcin Owsiany <[email protected]>
@porridge It's a tarball because when debugging you usually have to look at multiple files to figure out what's wrong. Better have them all at once. |
So you cannot browse their content within CircleCI UI? Download only? |
Co-Authored-By: Marcin Owsiany <[email protected]> Signed-off-by: Andreas Neumann <[email protected]>
Co-Authored-By: Marcin Owsiany <[email protected]> Signed-off-by: Thomas Runyon <[email protected]>
What this PR does / why we need it:
Updates the CI configuration to storage the cluster logs as tarball. This allows us to better debug CI failures like, e.g., #1235.