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 for File Share Limit Reached Error #2895

Merged
merged 16 commits into from
Jan 17, 2025
Merged

Conversation

wonwuakpa-msft
Copy link
Collaborator

@wonwuakpa-msft wonwuakpa-msft commented Dec 17, 2024

Description

  • Changes:

    • Added nil and closure checks to ensure there's no panic from closing channels.
    • Added a description for the ShareSizeLimitReached error to guide users on how to handle the error
    • Added test to catch ShareSizeLimitReached error
  • Feature / Bug Fix: (Brief description of the feature or issue being addressed)
    User reported this AzCopy behavior when uploading data to a file share that exceeds the quota (i.e uploading 4GB to a 1GB file share). When the quota is hit, the ongoing job is stopped and the already uploaded files are not deleted. AzCopy team concluded this is not a bug. There is a high chance of unnecessary failed API calls after the ShareSizeLimitReached error is hit. Thus, the current behavior of stopping the job is valid.

  • Related Links:

  • Issues

  • Email: Expected AzCopy Behavior When Exceeding Quotas

Type of Change

  • Bug fix
  • New feature
  • Documentation update required
  • Code quality improvement
  • Other (describe):

How Has This Been Tested?

Existing e2e tests and new Scenario_UploadFileQuota test
AzCopy randomly uploads files to the file share until the quota is hit. Wrote a script to validate that these uploaded files are non-empty. If empty files are uploaded, they are correctly deleted in the cleanup handler in the codebase.

Thank you for your contribution to AzCopy!

cmd/make.go Outdated Show resolved Hide resolved
Copy link
Member

@adreed-msft adreed-msft left a comment

Choose a reason for hiding this comment

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

Test doesn't adequately test the change in question

e2etest/zt_newe2e_file_test.go Outdated Show resolved Hide resolved
e2etest/zt_newe2e_file_test.go Outdated Show resolved Hide resolved
e2etest/zt_newe2e_file_test.go Outdated Show resolved Hide resolved
e2etest/zt_newe2e_file_test.go Outdated Show resolved Hide resolved
e2etest/zt_newe2e_file_test.go Outdated Show resolved Hide resolved
e2etest/zt_newe2e_file_test.go Outdated Show resolved Hide resolved
Copy link
Member

@adreed-msft adreed-msft left a comment

Choose a reason for hiding this comment

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

Looks good-- One concern about test efficiency that I'd like you to take a proper look at, but worst case it can be a remediation item next release.

e2etest/zt_newe2e_file_test.go Outdated Show resolved Hide resolved
e2etest/zt_newe2e_file_test.go Show resolved Hide resolved
@wonwuakpa-msft wonwuakpa-msft merged commit f0cf11a into main Jan 17, 2025
23 checks passed
@vibhansa-msft vibhansa-msft added this to the 10.28.0 milestone Jan 20, 2025
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.

5 participants