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

GODRIVER-3513 Fix OIDC Evergreen task failures. #1995

Merged
merged 2 commits into from
Mar 25, 2025

Conversation

matthewdale
Copy link
Collaborator

@matthewdale matthewdale commented Mar 21, 2025

GODRIVER-3513

Summary

  • Mark the gotest.parse_files step as optional for tasks that still want to run handle-test-artifacts (e.g. to collect other app/server logs), but don't write any Go test output (e.g. the current OIDC tasks).
  • Comment out teardown_task_can_fail_task: true from testoidc_task_group until the bug DRIVERS-3141, which prevents successful Atlas cluster teardown in OIDC tests, is fixed.
  • Prevent Taskfile task evg-gather-test-suites from failing the Evergreen task if there are no *.suitefiles to archive.

Background & Motivation

Some OIDC tasks started failing after merging #1976. All of the failures were due to pre-existing problems in the test setup that surfaced after the config changes in that PR.

@mongodb-drivers-pr-bot mongodb-drivers-pr-bot bot added the priority-3-low Low Priority PR for Review label Mar 21, 2025
Copy link
Contributor

API Change Report

No changes found!

@matthewdale matthewdale marked this pull request as ready for review March 22, 2025 02:04
@matthewdale matthewdale added priority-2-medium Medium Priority PR for Review and removed priority-3-low Low Priority PR for Review labels Mar 22, 2025
Taskfile.yml Outdated
@@ -199,7 +199,7 @@ tasks:

evg-test-deployed-lambda-aws: bash ${DRIVERS_TOOLS}/.evergreen/aws_lambda/run-deployed-lambda-aws-tests.sh

evg-gather-test-suites: find . -name \*.suite | xargs tar czf test_suite.tgz
evg-gather-test-suites: find . -name \*.suite | xargs tar czf test_suite.tgz || true
Copy link
Collaborator

@prestonvasquez prestonvasquez Mar 24, 2025

Choose a reason for hiding this comment

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

Why would / should there not be *.suite files? Also, the current solution doesn't fail when I run it locally. Does it fail in a spawn host?

❯ task evg-gather-test-suites
task: [evg-gather-test-suites] find . -name \*.suite | xargs tar czf test_suite.tgz

If this is a legitimate use case, then suggest we prevent the tar command from running when no files are found rather than appending || true to mask errors. Can use --no-run-if-empty

find . -name \*.suite  | xargs --no-run-if-empty tar czf test_suite.tgz

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some tests, like the OIDC prose tests), aren't implemented as Go tests and don't write "*.suite" files. We still want to run handle-test-artifacts to collect logs, tho.

The more correct solution would be to re-implement the OIDC prose tests as Go tests, and possibly separate handle-test-artifacts into two functions: one that collects Go test artifacts, and another that collects all other logs. However, that's a much larger change, so I did the quick version here. I created GODRIVER-3517 for re-implementing the OIDC prose tests as Go tests.

The command does fail on spawn hosts with an error like this (see here):

task: [evg-gather-test-suites] find . -name \*.suite | xargs tar czf test_suite.tgz
tar: Cowardly refusing to create an empty archive
Try 'tar --help' or 'tar --usage' for more information.
task: Failed to run task "evg-gather-test-suites": exit status 123

I'm not sure why it doesn't fail on macOS. I'd guess it's a difference in the tar version (GNU tar vs bsdtar).

The --no-run-if-empty fix is much more elegant, I'll try that!

# TODO(DRIVERS-3141): Uncomment the following line once the teardown bug is
# fixed. See DRIVERS-3141 for more context.
#
# teardown_task_can_fail_task: true
Copy link
Collaborator

@prestonvasquez prestonvasquez Mar 24, 2025

Choose a reason for hiding this comment

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

Does just commenting out this line resolve the issues described in GODRIVER-3513? All three seem specific to the teardown behavior.

Can we just remove this line? With the changes in this PR it's unclear to me why would ever add it back: these tests no longer fail on teardown.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't fix the teardown step error, it just prevents the error from failing the whole task. The error is caused by a bug in drivers-evergreen-tools and must be fixed there (tracked by DRIVERS-3141). Since it's not something we can fix in the Go Driver repo, the best option seemed to be reverting to the previous behavior, which ignores the teardown errors.

We could technically resolve the task failures for both testoidc_task_group and testgcpoidc_task_group by uncommenting teardown_task_can_fail_task: true (the default is false), but I prefer to keep that enabled because it exposes teardown bugs.

@matthewdale matthewdale merged commit 927052b into mongodb:master Mar 25, 2025
34 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-2-medium Medium Priority PR for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants