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: disable concurrency when downloading azure files #12532

Closed

Conversation

roelarents
Copy link
Contributor

workaround for Azure/azure-sdk-for-go#22156

Motivation

Recently bumped the azure sdk for go for blob storage. But it has an issue with concurrent downloads.

Modifications

Set concurrency when downloading artifacts/files/blobs from azure blob storage to 1 (instead of 5)

Verification

Tested in production with a patch on 3.5.4. We suffered from this issue before (hanging workflow steps) and now not anymore.

workaround for Azure/azure-sdk-for-go#22156

Signed-off-by: Roel Arents <roel.arents@kadaster.nl>
@roelarents roelarents marked this pull request as ready for review January 17, 2024 09:52
@agilgur5 agilgur5 added the area/artifacts S3/GCP/OSS/Git/HDFS etc label Jan 17, 2024
@agilgur5 agilgur5 added type/dependencies PRs and issues specific to updating dependencies go Pull requests that update Go dependencies labels Jan 17, 2024
@agilgur5
Copy link
Contributor

This sounds like it's going to be fixed soon upstream? So maybe we should just wait for the fix and update the dep rather than adding and removing a workaround. I would think the Azure SDK would have a release before Argo as well

Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
Signed-off-by: Roel Arents <2691308+roelarents@users.noreply.github.com>
@roelarents
Copy link
Contributor Author

True, if they fix it before the next argo release then this is obsolete. If not, I do suggest this temporary workaround. Because there is another issue (that we also encountered in production) in the azure blob sdk since 1.0 (which is in argo 3.5). That has been fixed upstream already. I bumped the sdk to 1.2.1 for it. But that introduced this issue. (Probably/possibly there is a version in between with the fix for the one and not yet introducing the next. But I think using the latest version is easier.)

@tanyasethi-msft
Copy link

tanyasethi-msft commented Jan 23, 2024

This fix constraints the concurrency to 1, which is not preferable.
We will be fixing the issue in our next release, Please use azblob v1.1.0 for temporary mitigation.

@roelarents
Copy link
Contributor Author

yep, they fixed it. this can be closed.

@roelarents roelarents closed this Feb 15, 2024
@agilgur5 agilgur5 added the area/upstream This is an issue with an upstream dependency, not Argo itself label Feb 15, 2024
@agilgur5
Copy link
Contributor

agilgur5 commented Feb 15, 2024

Looks like it was fixed in Azure/azure-sdk-for-go#22334 which was released in azblob v1.3.0.

This PR has been replaced by #12667 as such

@agilgur5 agilgur5 added the solution/superseded This PR or issue has been superseded by another one (slightly different from a duplicate) label Feb 17, 2024
@agilgur5 agilgur5 added the solution/workaround There's a workaround, might not be great, but exists label Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/artifacts S3/GCP/OSS/Git/HDFS etc area/upstream This is an issue with an upstream dependency, not Argo itself go Pull requests that update Go dependencies solution/superseded This PR or issue has been superseded by another one (slightly different from a duplicate) solution/workaround There's a workaround, might not be great, but exists type/dependencies PRs and issues specific to updating dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants