-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Properly handle filesizes larger than 8 Gb #76707
Conversation
Tagging subscribers to this area: @dotnet/area-system-io |
[InlineData(TarEntryFormat.Pax, LegacyMaxFileSize)] | ||
// Pax supports unlimited size files. | ||
[InlineData(TarEntryFormat.Pax, LegacyMaxFileSize + 1)] | ||
public void WriteEntry_LongFileSize(TarEntryFormat entryFormat, long size) |
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 new tests create ~8 Gb files, locally it took me 153s to run all tests now, when previously was taken ~5 secs. Any suggestions if this can be improved?
Of course, I will add [OuterLoop]
.
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.
And I think there is also another attribute that ensures the test does not run in parallel with others, which would help avoid running out of memory.
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.
avoid running out of
memory.
disk.
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.
I meant memory. I don't think we should be using disk at all.
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.
If memory then disable for 32 bit?
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.
I do think in memory is the way to go.
@MattGal remind us, in Helix, what's the minimum $TEMP disk space you can be sure is available? As I seem to recall that on some queues, it can be only a few GB.
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.
Helix machines will keep taking work as long as the drive their "work" directory is on has > 3172 MB of free space. Looking at this PR it sounds like you want to create 8+ GB files so there's no guarantee this test can work there; perhaps if you must do this you could check the free space first and nerf the test out.
Folks haven't directly linked the ones they're looking at, so grabbing one from RedHat 7 it's hitting this problem even in the case of running on machines that start with ~48 GB free space.
My guess here is that there are multiple tests running in parallel, that create > 8 GB files simultaneously and eat the entire disk quickly, given that the link above ran on a machine that definitely had ~46 GB free when the work item started.
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.
Yes, that's another thing to do if you're making huge files -- run them alone, serialized and not in parallel. We have prior art for using Xunit collections to do this in the runtime repo for certain tests. (example)
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.
I still lean to use a file for the 8Gbs of tar, but just curious, @MattGal does the helix machines could handle a test with 8Gb of memory consumption?
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.
Most Helix test VMs are of the Standard D2* variety (various intel versions and AMD EPYC used in different places for reasons). These machines have 8 GB total RAM, so it's probably a no, but more likely a "varies by OS" problem, since virtualization should allow this in some environments.
This comment was marked as outdated.
This comment was marked as outdated.
I think we should stick to MemoryStreams to avoid running out of disk space. |
src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.Tests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntryAsync.Tests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Read.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Outdated
Show resolved
Hide resolved
/azp list |
This comment was marked as off-topic.
This comment was marked as off-topic.
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
@dotnet/area-infrastructure-libraries all Unix outer loop legs exited with code 127 when running python (I think); some Windows legs exited with code 9009 for corerun.exe. Any ideas?
|
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.
For the outerloop tests that fail to find dotnet
, I created this arcade issue: dotnet/arcade#11185
From our chat conversation, you verified that some outerloop tests ran your newly added tar tests and they passed.
All other test failures are unrelated. So this LGTM now.
Will trigger outerloop once more just to be safe. |
This comment was marked as off-topic.
This comment was marked as off-topic.
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
As discussed in dotnet/arcade#11185, outerloop issues are unrelated. |
/backport to release/7.0 |
Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3207286202 |
FYI the arcade issue was closed. It was determined this was a runtime problem, so I opened this new issue in our repo: #76755 |
@rainersigwald @baronfel FYI we are backporting this to GA. |
Fixes #76563