-
Notifications
You must be signed in to change notification settings - Fork 32
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
Remove EBS Attach for RFS in GovCloud #1216
Merged
AndreKurait
merged 1 commit into
opensearch-project:main
from
AndreKurait:FixRFSEBSGovCloud
Jan 6, 2025
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ import { | |
createOpenSearchServerlessIAMAccessPolicy, | ||
getSecretAccessPolicy, | ||
getMigrationStringParameterValue, | ||
ClusterAuth, parseArgsToDict, appendArgIfNotInExtraArgs | ||
ClusterAuth, parseArgsToDict, appendArgIfNotInExtraArgs, isStackInGovCloud | ||
} from "../common-utilities"; | ||
import { RFSBackfillYaml, SnapshotYaml } from "../migration-services-yaml"; | ||
import { OtelCollectorSidecar } from "./migration-otel-collector-sidecar"; | ||
|
@@ -116,7 +116,9 @@ export class ReindexFromSnapshotStack extends MigrationServiceCore { | |
const extraArgsDict = parseArgsToDict(props.extraArgs) | ||
const storagePath = "/storage" | ||
const planningSize = props.maxShardSizeGiB ?? 80; | ||
const maxShardSizeBytes = planningSize * 1024 * 1024 * 1024 * 1.10 // Add 10% buffer | ||
const planningSizeBuffer = 1.10 | ||
const maxShardSizeGiB = planningSize * planningSizeBuffer | ||
const maxShardSizeBytes = maxShardSizeGiB * (1024 ** 3) | ||
command = appendArgIfNotInExtraArgs(command, extraArgsDict, "--s3-local-dir", `"${storagePath}/s3_files"`) | ||
command = appendArgIfNotInExtraArgs(command, extraArgsDict, "--s3-repo-uri", `"${s3Uri}"`) | ||
command = appendArgIfNotInExtraArgs(command, extraArgsDict, "--s3-region", this.region) | ||
|
@@ -170,40 +172,60 @@ export class ReindexFromSnapshotStack extends MigrationServiceCore { | |
|
||
// Calculate the volume size based on the max shard size | ||
// Have space for the snapshot and an unpacked copy, with buffer | ||
const volumeSizeGB = Math.max( | ||
Math.ceil(maxShardSizeBytes/(1000**3) * 2 * 1.15), | ||
const shardVolumeSizeGiBBufferMultiple = 1.10 | ||
const shardVolumeSizeGiB = Math.max( | ||
Math.ceil(maxShardSizeGiB * 2 * shardVolumeSizeGiBBufferMultiple), | ||
1 | ||
) | ||
|
||
if (volumeSizeGB > 16000) { | ||
if (shardVolumeSizeGiB > (16*1024)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for cleaning up this logic a bit |
||
// 16 TiB is the maximum volume size for GP3 | ||
throw new Error(`"Your max shard size of ${props.maxShardSizeGiB} GiB is too large to migrate."`) | ||
} | ||
|
||
// Volume we'll use to download and unpack the snapshot | ||
const snapshotVolume = new ServiceManagedVolume(this, 'SnapshotVolume', { | ||
name: 'snapshot-volume', | ||
managedEBSVolume: { | ||
size: Size.gibibytes(volumeSizeGB), | ||
volumeType: EbsDeviceVolumeType.GP3, | ||
fileSystemType: FileSystemType.XFS, | ||
throughput: props.reindexFromSnapshotWorkerSize === "maximum" ? 450 : 125, | ||
tagSpecifications: [{ | ||
tags: { | ||
Name: `rfs-snapshot-volume-${props.stage}`, | ||
}, | ||
propagateTags: EbsPropagatedTagSource.SERVICE, | ||
}], | ||
encrypted: true, | ||
}, | ||
}); | ||
// Reserve 5 GiB of storage for system | ||
const systemStorageGiB = 5 | ||
let ephemeralStorageGiB = systemStorageGiB | ||
if (isStackInGovCloud(this)) { | ||
// ECS EBS attachment is not supported in GovCloud | ||
// https://docs.aws.amazon.com/govcloud-us/latest/UserGuide/govcloud-ecs.html#govcloud-ecs-diffs | ||
// Use Ephemeral Storage instead, adding size for shard | ||
ephemeralStorageGiB = Math.ceil(shardVolumeSizeGiB + systemStorageGiB) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Lots of math, that logic is easier to test if it was refactored into a stand-alone function(s) |
||
const maxSupportedEphemeralStorageGiB = 200 | ||
if (ephemeralStorageGiB > maxSupportedEphemeralStorageGiB) { | ||
// Reverse calculations above for max size | ||
const maxGovCloudSupportedShardSizeGiB = Math.floor((maxSupportedEphemeralStorageGiB-systemStorageGiB) | ||
/2/shardVolumeSizeGiBBufferMultiple/planningSizeBuffer) | ||
throw new Error(`Your max shard size of ${props.maxShardSizeGiB} GiB is too large to migrate ` + | ||
`in GovCloud, the max supported is ${maxGovCloudSupportedShardSizeGiB} GiB.`) | ||
} | ||
} | ||
else { | ||
// Volume we'll use to download and unpack the snapshot | ||
const snapshotVolume = new ServiceManagedVolume(this, 'SnapshotVolume', { | ||
name: 'snapshot-volume', | ||
managedEBSVolume: { | ||
size: Size.gibibytes(shardVolumeSizeGiB), | ||
volumeType: EbsDeviceVolumeType.GP3, | ||
fileSystemType: FileSystemType.XFS, | ||
throughput: props.reindexFromSnapshotWorkerSize === "maximum" ? 450 : 125, | ||
tagSpecifications: [{ | ||
tags: { | ||
Name: `rfs-snapshot-volume-${props.stage}`, | ||
}, | ||
propagateTags: EbsPropagatedTagSource.SERVICE, | ||
}], | ||
encrypted: true, | ||
}, | ||
}); | ||
|
||
volumes.push(snapshotVolume); | ||
mountPoints.push({ | ||
containerPath: storagePath, | ||
readOnly: false, | ||
sourceVolume: snapshotVolume.name, | ||
}); | ||
volumes.push(snapshotVolume); | ||
mountPoints.push({ | ||
containerPath: storagePath, | ||
readOnly: false, | ||
sourceVolume: snapshotVolume.name, | ||
}); | ||
} | ||
|
||
this.createService({ | ||
serviceName: 'reindex-from-snapshot', | ||
|
@@ -217,6 +239,7 @@ export class ReindexFromSnapshotStack extends MigrationServiceCore { | |
cpuArchitecture: props.fargateCpuArch, | ||
taskCpuUnits: props.reindexFromSnapshotWorkerSize === "maximum" ? 16 * 1024 : 2 * 1024, | ||
taskMemoryLimitMiB: props.reindexFromSnapshotWorkerSize === "maximum" ? 32 * 1024 : 4 * 1024, | ||
ephemeralStorageGiB: ephemeralStorageGiB, | ||
environment: { | ||
"RFS_COMMAND": command, | ||
"RFS_TARGET_USER": targetUser, | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
How does this change in behavior affect the user experience? Can you briefly list all the scenarios and the before/after outcome? I ask because I'm not entirely sure it does change things.
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 also ensures for RFS GovCloud we don't attempt to set this less than 21GB which will otherwise result in an deployment error
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.
Got it - so we're preventing a deploy time error by overriding a too-small value with the actual minimum. Thanks!