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

Fix path handling in download-mongodb #498

Merged
merged 17 commits into from
Sep 17, 2024

Conversation

blink1073
Copy link
Member

I tested this by cloning the c driver and invoking the integration script. The way that script gets invoked is pretty gnarly and unexpected. The path ends up being .evergreen/scripts/../../drivers-evergreen-tools/.evergreen/download-mongodb.sh, and I'm not sure how to get that into a suitable path. I'll have to follow up with a more robust solution.

@blink1073
Copy link
Member Author

blink1073 commented Sep 17, 2024

Worst case I'll follow up with an inline retry function because we're already seeing a failure with:

curl: (92) HTTP/2 stream 1 was not closed cleanly: INTERNAL_ERROR (err 2)
Error: Process completed with exit code 92.

@blink1073
Copy link
Member Author

test-oidc-k8s-local failure is expected, I had to tear down the cluster today

if [ -n "${MONGODB_BINARIES:-}" ]; then
cd "$(dirname "$(dirname "${MONGODB_BINARIES:?}")")"
else
cd $DRIVERS_TOOLS
fi

echo "Installing server binaries..."
"$SCRIPT_DIR/retry-with-backoff.sh" curl $MONGODB_DOWNLOAD_URL --output mongodb-binaries.tgz
curl $MONGODB_DOWNLOAD_URL --output mongodb-binaries.tgz
Copy link
Contributor

@ShaneHarvey ShaneHarvey Sep 17, 2024

Choose a reason for hiding this comment

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

We should be using curl_retry here. curl_retry is defined in this file. Same for all the other curl calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh you removed curl_retry in PYTHON-4734. How about we just revert that commit entirely (again)?

Copy link
Contributor

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

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

retry-with-backoff.sh

Has curl's native command line flags been considered?

--retry <num>

If a transient error is returned when curl tries to perform a transfer, it will retry this number of times before giving up. Setting the number to 0 makes curl do no retries (which is the default). Transient error means either: a timeout, an FTP 4xx response code or an HTTP 408, 429, 500, 502, 503 or 504 response code.

When curl is about to retry a transfer, it will first wait one second and then for all forthcoming retries it will double the waiting time until it reaches 10 minutes which then will be the delay between the rest of the retries.

This behavior seems completely equivalent to the emulated behavior of the retry-with-backoff.sh script. See also --max-time, --retry-max-time, --retry-connrefused, and --retry-all-errors.

@ShaneHarvey
Copy link
Contributor

We previously used curl's built in retries but not all curls we use are new enough because we test on so many different/old hosts. See PYTHON-3721 and DRIVERS-2625.

@blink1073
Copy link
Member Author

Updated to restore curl_retry. I'll make sure to test the c driver if changing this file in the future.

Copy link
Contributor

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

LGTM assuming the tests pass.

Copy link
Contributor

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

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

Minor feedback regarding curl_retry. Otherwise, as far as the C Driver is concerned, latest changes are LGTM.

.evergreen/download-mongodb.sh Outdated Show resolved Hide resolved
@eramongodb eramongodb merged commit 4327b26 into mongodb-labs:master Sep 17, 2024
42 of 43 checks passed
adriandole pushed a commit to adriandole/drivers-evergreen-tools that referenced this pull request Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants