-
Notifications
You must be signed in to change notification settings - Fork 359
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
AN-300 Fall back to large Docker image size if we can't find a real one #7673
Conversation
val userCommandImageSizeRoundedUpInGB = userCommandImageSizeInGB.ceil.toInt | ||
.map(s => MemorySize(s.toDouble, MemoryUnit.Bytes).to(MemoryUnit.GB)) | ||
.getOrElse(MemorySize(30, MemoryUnit.GB)) | ||
val userCommandImageSizeRoundedUpInGB = userCommandImageSizeInGB.amount.ceil.toInt |
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.
Slight refactor because I didn't want to write .getOrElse(30000000000L)
val userCommandImageSizeInBytes = createPipelineParameters.jobDescriptor.dockerSize | ||
|
||
// Compute the decompressed size based on the information available. If we couldn't get the image size, | ||
// default to 30GB. Defaulting to 0 can cause task to run out of disk. (more in AN-300) |
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.
This is 30 GB extra for the image, right? Not a 30 GB disk overall.
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.
Correct!
.map(s => MemorySize(s.toDouble, MemoryUnit.Bytes).to(MemoryUnit.GB)) | ||
val (userCommandImageSizeInGB, userImageLogString) = maybeUserCommandImageSizeInGB match { | ||
case Some(imageSize) => (imageSize, "user command image") | ||
case None => (MemorySize(30, MemoryUnit.GB), "failed to obtain user command image size, using safe default") |
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 appreciate this specific message.
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.
LGTM, thanks!
Description
See discussion in AN-300. This should fix cases where we can't find the Docker image size, so we don't bump the boot disk size to account for it, and tasks fail with out-of-disk during
docker pull
.Batch will be handled in a different ticket.
Includes a revert of #7585, since this is a better-targeted fix for the same issue.
Release Notes Confirmation
CHANGELOG.md
CHANGELOG.md
in this PRCHANGELOG.md
because it doesn't impact community usersTerra Release Notes