Add scheduled EBS backups and a snapshot restore path to rvn-ec2-service - #115
Add scheduled EBS backups and a snapshot restore path to rvn-ec2-service#115devin-ai-integration[bot] wants to merge 10 commits into
Conversation
Co-Authored-By: brandon <brandon@flightcontrol.dev>
Co-Authored-By: brandon <brandon@flightcontrol.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: brandon <brandon@flightcontrol.dev>
Ravion Module Publish PlanDry run only. No Ravion API mutations were made.
Diffsrvn-ec2-service n/a -> 1.5.0--- remote
+++ compiled
-description: Runs supervised workloads on a stable EC2 Auto Scaling Group, with optional shared ALB routing and switchable container or manual in-place deploys.
+description: Runs supervised workloads on a stable EC2 Auto Scaling Group, with optional shared ALB routing, local EBS data, and switchable container or manual in-place deploys.
name: EC2 Service
type: rvn-ec2-servicervn-ec2-service 1.4.1 -> 1.5.0--- remote
+++ compiled
show_when:
data_volume_creation_enabled: true
type: string
+ - collapsible: true
+ description: Optional EBS snapshot ID to restore into the data volume when a replacement instance boots. Clear this after the restore is complete.
+ id: data_volume_snapshot_id
+ label: Data volume snapshot ID
+ patterns:
+ - message: Enter an EBS snapshot ID.
+ pattern: ^snap-[a-z0-9]+$
+ placeholder: snap-...
+ required: false
+ show_when:
+ data_volume_creation_enabled: true
+ type: string
+ - description: Scheduled EBS snapshots for on-disk application data.
+ id: section_backups
+ label: Backups
+ type: section
- default: false
+ description: Schedule Amazon Data Lifecycle Manager snapshots for this service's instances. Opt in for on-disk databases such as SQLite or Postgres.
+ id: backup_enabled
+ label: Enable EBS backups
+ type: boolean
+ - default: 24
+ description: Maximum age of a scheduled snapshot, and therefore the honest backup RPO.
+ id: backup_interval_hours
+ label: Backup interval (hours)
+ max: 24
+ min: 1
+ show_when:
+ backup_enabled: true
+ type: number
+ values:
+ - label: Every hour
+ value: 1
+ - label: Every 2 hours
+ value: 2
+ - label: Every 3 hours
+ value: 3
+ - label: Every 4 hours
+ value: 4
+ - label: Every 6 hours
+ value: 6
+ - label: Every 8 hours
+ value: 8
+ - label: Every 12 hours
+ value: 12
+ - label: Daily
+ value: 24
+ - default: 05:00
+ description: UTC time in HH:MM format when the daily schedule begins.
+ id: backup_start_time
+ label: Backup start time (UTC)
+ patterns:
+ - message: Use HH:MM in UTC.
+ pattern: ^(0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$
+ show_when:
+ backup_enabled: true
+ type: string
+ - default: 7
+ description: Number of snapshots retained for this schedule.
+ id: backup_retention_count
+ label: Snapshots to retain
+ min: 1
+ show_when:
+ backup_enabled: true
+ type: number
+ - default: false
+ description: Include the operating system volume in each snapshot set. It is included automatically when no data volume exists.
+ id: backup_root_volume_included
+ label: Include root volume
+ show_when:
+ backup_enabled: true
+ type: boolean
+ - default: filesystem_freeze
+ description: Freeze the data filesystem before snapshots, take crash-consistent snapshots, or provide database-specific pre/post commands.
+ id: backup_consistency_mode
+ label: Backup consistency mode
+ show_when:
+ backup_enabled: true
+ type: string
+ values:
+ - description: Sync and freeze the data volume without touching / or /boot.
+ label: Filesystem freeze
+ value: filesystem_freeze
+ - description: Snapshot without scripts; database engines must recover as after a power loss.
+ label: Crash consistent
+ value: crash_consistent
+ - description: Run your own database quiesce commands before and after the snapshot.
+ label: Custom
+ value: custom
+ - description: Command that quiesces the database before each snapshot. Required for Custom consistency mode.
+ id: backup_pre_script_command
+ label: Backup pre-script command
+ required: true
+ show_when:
+ backup_consistency_mode: custom
+ backup_enabled: true
+ type: text
+ - description: Command that resumes the database after each snapshot. Required for Custom consistency mode.
+ id: backup_post_script_command
+ label: Backup post-script command
+ required: true
+ show_when:
+ backup_consistency_mode: custom
+ backup_enabled: true
+ type: text
+ - collapsible: true
+ description: Optional AWS region for a second snapshot copy. Cross-region copies incur additional storage and transfer costs.
+ id: backup_cross_region_copy_destination
+ label: Cross-region copy destination
+ required: false
+ show_when:
+ backup_enabled: true
+ type: string
+ values: $values:aws/regions
+ - default: false
description: Mount a shared network filesystem that every instance can access. Use it when multiple instances need the same files, or when files must survive instance replacement without a restore step.
id: efs_enabled
label: EFS file system
@@
Instances are as stable as an EC2 instance you launch yourself in the AWS console. Deploys, app restarts, and stack updates do not replace them, so each instance keeps its root and optional data volume, and everything on those disks, for its whole life. Even changing the AMI leaves running instances alone: the change becomes a new launch template version that only applies to instances launched later, because the module does not run an instance refresh. An instance is replaced when you deliberately terminate or recycle it, for example to roll out that new AMI, when the group scales in, or when it fails its Auto Scaling health check (EC2 by default, or load balancer health when `health_check_type` is set to `ELB`). Replacement is what destroys the volumes, so take regular EBS snapshots or back up off-instance if critical data lives on the disk.
- Terraform source: [ravionhq/modules/compute/ec2_service](https://github.com/ravionhq/modules/tree/rvn-ec2-service@1.4.1/compute/ec2_service)
+ Terraform source: [ravionhq/modules/compute/ec2_service](https://github.com/ravionhq/modules/tree/rvn-ec2-service@1.5.0/compute/ec2_service)
## Use cases
@@
Root and Data volumes are encrypted gp3 EBS volumes attached to the instance. They are durable for the life of the instance: nothing in the normal deploy or update path replaces an instance, so files written to disk stay there, at local-disk latency. This is the same durability model as an EC2 instance you create yourself in the AWS console.
- Both volumes are deleted with the instance, which only happens for the three reasons in Instance replacement below. Keep backups of critical data on the disk — an EBS snapshot schedule, database dumps to S3, or a periodic sync — rather than avoiding local storage.
+ Both volumes are deleted with the instance, which only happens for the three reasons in Instance replacement below. On-disk databases such as SQLite and Postgres are supported when Backups are configured; without backups, a replacement loses the volume.
- Enable EFS and select a Ravion EFS module when several instances must share the same files, or when a replacement instance must find the data already in place. The service mounts the file system through its access point when present and attaches the EFS client security group to each instance. Container mode bind-mounts the Data and EFS host paths into the app container at the same paths. EFS is a network file system, so latency-sensitive local state such as an embedded database is usually better on the instance's own volume with backups.
+ Enable EFS and select a Ravion EFS module when several instances must share the same files, or when a replacement instance must find the data already in place. EFS is for shared files, not a live SQLite or Postgres data directory.
+ Shared or multi-attach EBS block storage is deliberately not supported. A block device is not a shared filesystem; concurrent read/write mounts of XFS or ext4 can corrupt it. Reliable multi-writer access needs a cluster filesystem and fencing, while single-writer databases gain nothing from multi-attach.
+
+ ## Backups and restore
+
+ Enable Backups to create a service-specific Amazon Data Lifecycle Manager schedule. The default daily snapshot uses `filesystem_freeze` when a Data volume exists, which runs `sync` and freezes only the data mount (never `/` or `/boot`) while the multi-volume snapshot set is taken. Use `crash_consistent` when filesystem freezing is unsuitable, or `custom` with both pre- and post-script commands for engine-specific quiescing. A safety timeout thaws a frozen filesystem if the post-script is delayed or lost.
+
+ Snapshots are incremental EBS snapshots, but the schedule still costs storage and cross-region copies cost transfer and destination-region storage. The honest RPO is the Backup interval: a failure immediately before a scheduled snapshot can lose up to that interval. Phase 1 has no automatic restore. The RTO requires a human operator to select a snapshot and recycle an instance; a replacement otherwise boots with an empty Data volume.
+
+ To restore:
+
+ 1. Find a snapshot using the `RavionBackup=<service name>` tag, or the `backup_snapshot_filter` output.
+ 2. Set Data volume snapshot ID to the selected `snap-...` ID while Data volume remains enabled. Keep Data volume size at least as large as the snapshot; the configured volume type and size are preserved, and AWS rejects an undersized restore.
+ 3. Apply the change and recycle the affected instance so it launches from the snapshot.
+ 4. Verify that the restored filesystem is mounted at the Data volume mount path and that the application sees the expected data.
+ 5. Clear Data volume snapshot ID and apply again, so future replacements do not keep booting from that pinned snapshot.
+
+ This phase intentionally does not provide logical dumps, S3 backups, termination hooks, automatic snapshot discovery, or continuous replication. Those mechanisms are for later phases and are needed for lower RPO, object-level recovery, or automatic replacement restoration.
+
## Instance replacement
Only three things replace a running instance. Everything else — deploys, app crashes and restarts, configuration changes, stack updates — leaves the instance and its disks in place.
@@
base_path: compute/ec2_service
branch: main
execution_environment_id: << module.input.execution_environment_id >>
- ref: rvn-ec2-service@1.4.1
+ ref: rvn-ec2-service@1.5.0
repo: https://github.com/ravionhq/modules
stack_id: <<stack.id>>
terraform_variables:
@@
additional_user_data: << module.input.additional_user_data || "" >>
ami_id: << module.input.ami_id || nil >>
app_port: "<< module.input.http_traffic_enabled ? module.input.app_port : nil >>"
+ backup_consistency_mode: '<< module.input.data_volume_creation_enabled ? (module.input.backup_consistency_mode || "filesystem_freeze") : "crash_consistent" >>'
+ backup_cross_region_copy_destination: << module.input.backup_cross_region_copy_destination || nil >>
+ backup_enabled: << module.input.backup_enabled >>
+ backup_interval_hours: << module.input.backup_interval_hours >>
+ backup_post_script_command: << module.input.backup_post_script_command || nil >>
+ backup_pre_script_command: << module.input.backup_pre_script_command || nil >>
+ backup_retention_count: << module.input.backup_retention_count >>
+ backup_root_volume_included: << module.input.backup_root_volume_included >>
+ backup_start_time: << module.input.backup_start_time >>
container_start_command: '<< module.input.deploy_type == "container" ? (module.input.container_start_command || nil) : nil >>'
cpu_autoscaling_enabled: << module.input.cpu_autoscaling_enabled >>
cpu_target_value: << module.input.cpu_target_value >>
data_volume_creation_enabled: << module.input.data_volume_creation_enabled >>
data_volume_mount_path: << module.input.data_volume_mount_path >>
data_volume_size: << module.input.data_volume_size >>
+ data_volume_snapshot_id: << module.input.data_volume_snapshot_id || nil >>
deploy_health_check_path: "<< module.input.http_traffic_enabled ? module.input.health_check_path : nil >>"
deploy_timeout_seconds: << module.input.deploy_timeout_seconds >>
desired_capac
... diff truncated ... |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| actions = [ | ||
| "ec2:CreateSnapshot", | ||
| "ec2:CreateSnapshots", | ||
| "ec2:DeleteSnapshot", | ||
| "ec2:DescribeInstances", | ||
| "ec2:DescribeVolumes", | ||
| "ec2:DescribeSnapshots", | ||
| ] |
There was a problem hiding this comment.
When backup_cross_region_copy_destination is set, DLM uses this role for the configured copy, but the policy omits ec2:CopySnapshot and ec2:ModifySnapshotAttribute, causing destination-region copies to fail authorization while source snapshots continue.
| actions = [ | |
| "ec2:CreateSnapshot", | |
| "ec2:CreateSnapshots", | |
| "ec2:DeleteSnapshot", | |
| "ec2:DescribeInstances", | |
| "ec2:DescribeVolumes", | |
| "ec2:DescribeSnapshots", | |
| ] | |
| actions = [ | |
| "ec2:CreateSnapshot", | |
| "ec2:CreateSnapshots", | |
| "ec2:CopySnapshot", | |
| "ec2:DeleteSnapshot", | |
| "ec2:DescribeInstances", | |
| "ec2:DescribeVolumes", | |
| "ec2:DescribeSnapshots", | |
| "ec2:ModifySnapshotAttribute", | |
| ] |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compute/ec2_service/iam_dlm.tf
Line: 22-29
Comment:
**Cross-region copies lack permissions**
When `backup_cross_region_copy_destination` is set, DLM uses this role for the configured copy, but the policy omits `ec2:CopySnapshot` and `ec2:ModifySnapshotAttribute`, causing destination-region copies to fail authorization while source snapshots continue.
```suggestion
actions = [
"ec2:CreateSnapshot",
"ec2:CreateSnapshots",
"ec2:CopySnapshot",
"ec2:DeleteSnapshot",
"ec2:DescribeInstances",
"ec2:DescribeVolumes",
"ec2:DescribeSnapshots",
"ec2:ModifySnapshotAttribute",
]
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Valid — will fix. ec2:CopySnapshot and ec2:ModifySnapshotAttribute are both in the AWS-managed AWSDataLifecycleManagerServiceRole, and without them backup_cross_region_copy_destination fails silently in the worst way: source snapshots keep succeeding, so the schedule looks healthy while the off-region copy never lands.
Adding KMS at the same time, since it has the same failure shape here — the module encrypts both volumes, so a cross-region copy of an encrypted snapshot needs grant/encrypt/decrypt/describe on the destination key, and without it the copy is authorized to start and then fails. Scoping those to the copy destination rather than adding them unconditionally.
There was a problem hiding this comment.
Good catch on KMS — that's the right call. The failure mode is identical: DLM gets far enough to start the cross-region copy, the snapshot encryption key isn't present in the destination region, and the copy quietly fails while the source schedule stays green.
One thing to confirm when you wire it up: the KMS permissions need to target the destination-region KMS key (or the replica key), not the source key. The source key already has the grants needed to create the snapshot; it's the destination CMK that needs kms:CreateGrant, kms:DescribeKey, kms:Encrypt, and kms:Decrypt granted to the DLM role so the copy job can re-encrypt on arrival. If the destination key ARN isn't known at plan time (e.g., it's a different account's managed key), scoping with a kms:ViaService condition for ec2.<destination-region>.amazonaws.com is the safe fallback rather than "*".
Rest of the fix looks solid. Landing ec2:CopySnapshot and ec2:ModifySnapshotAttribute unconditionally in the policy is fine — they're no-ops if cross-region copy is never configured and match what AWSDataLifecycleManagerServiceRole includes.
There was a problem hiding this comment.
Agreed on the ViaService scoping — the pushed version has the KMS statement gated on backup_cross_region_copy_destination != null but with resources = ["*"] and no condition, which is looser than it needs to be. Tightening it to kms:ViaService for both ec2.<source-region>.amazonaws.com and ec2.<destination-region>.amazonaws.com, since the copy needs decrypt against the source key as well as re-encrypt on arrival, plus kms:GrantIsForAWSResource on CreateGrant to match what AWSDataLifecycleManagerServiceRole does. The key ARNs genuinely aren't knowable at plan time here — the module takes no EBS CMK input and relies on the account default key — so a condition rather than a resource list is the right fallback.
On ec2:CopySnapshot / ec2:ModifySnapshotAttribute being unconditional: they're effectively no-ops without a copy configured, but I'd rather keep them gated on the copy destination, which is how they're pushed. Same reasoning as the KMS statement — a role that can copy and share snapshots is worth not granting to every service that merely enables local snapshots, and the gate costs nothing since enabling cross-region copy is already an apply.
There was a problem hiding this comment.
Both points are sound.
On kms:ViaService: the source region is always knowable at plan time (data.aws_region.current.name), and the destination comes from var.backup_cross_region_copy_destination, so you can interpolate both into the condition string without any "*" resource list. The two services to cover are ec2.${data.aws_region.current.name}.amazonaws.com and ec2.${var.backup_cross_region_copy_destination}.amazonaws.com. Adding kms:GrantIsForAWSResource as a condition on CreateGrant (not as a separate action — it's a condition key, not an action) is the right pattern; that's exactly what the managed role does to prevent the DLM role from issuing arbitrary grants.
On gating ec2:CopySnapshot / ec2:ModifySnapshotAttribute: the least-privilege argument holds. A role that can copy snapshots cross-region and modify snapshot attributes (including making them public) is meaningfully more powerful, and the conditional gate is cheap. Keeping them behind backup_cross_region_copy_destination != null is the correct call — my earlier note that they're "fine unconditionally" was prioritizing simplicity over principle, and you're right to push back.
One small thing to double-check in the pushed version: kms:Decrypt is needed on the source key (to read the encrypted snapshot data during transfer), and kms:Encrypt/kms:GenerateDataKey are needed on the destination key. If both ViaService conditions are in a single statement with StringEquals using a list, confirm the condition logic is OR (which it is by default for multi-value StringEquals on the same key) rather than accidentally requiring both to match simultaneously.
There was a problem hiding this comment.
Pushed in 4cec204, and your OR question is the right one to ask — it is OR. A multi-value StringEquals on a single condition key matches if any value matches, so one statement with both ec2.<source>.amazonaws.com and ec2.<destination>.amazonaws.com permits kms:Decrypt against the source key via source-region EC2 and kms:Encrypt/kms:GenerateDataKey against the destination key via destination-region EC2, without requiring an impossible simultaneous match. Splitting per region would only buy tighter per-action scoping, which isn't reachable anyway while the key ARNs are unknown.
CreateGrant is its own statement so kms:GrantIsForAWSResource (as a Bool condition, yes — not an action) applies only there rather than to the whole KMS block, and the source region comes from the module's existing local.region.
One correction: the conditions can't remove the "*" resource list. kms:ViaService constrains which service may call KMS on the role's behalf, but the statement still has to name key resources, and the module takes no CMK input — it relies on the account's default EBS key, whose ARN differs per region and isn't a plan-time value here. So it stays resources = ["*"] narrowed by ViaService in both regions, which is the same shape AWSDataLifecycleManagerServiceRole uses. If a CMK input is added later, that's when the wildcard can become a real resource list.
Co-Authored-By: brandon <brandon@flightcontrol.dev>
Co-Authored-By: brandon <brandon@flightcontrol.dev>
Co-Authored-By: brandon <brandon@flightcontrol.dev>
Co-Authored-By: brandon <brandon@flightcontrol.dev>
Co-Authored-By: brandon <brandon@flightcontrol.dev>
Co-Authored-By: brandon <brandon@flightcontrol.dev>
Co-Authored-By: brandon <brandon@flightcontrol.dev>
1 participant
Summary
Agents refuse to recommend
rvn-ec2-servicefor anything with a database on disk, because the docs said "take EBS snapshots or back up elsewhere" and the module offered no way to do it. This is phase 1 of the reviewed backup plan: the module now owns snapshot scheduling and a documented restore path, so on-disk SQLite/Postgres becomes a supported choice rather than a warning.backup_enabled(defaultfalse) creates a service-specific Amazon Data Lifecycle Manager policy targeting instances by a newRavionBackup = <name>tag, with interval/start-time/retention and optional cross-region copy. Instance targeting (not volume) is deliberate: it snapshots all of an instance's volumes as one consistent set and is the only DLM shape that supports pre/post scripts.Consistency defaults to
filesystem_freezewhen a data volume exists, which is what AWS's own PostgreSQL/MySQL template does —sync, thenfsfreeze -fon the data mount only, never/or/boot, thawed by the post script. Postgres treats the resulting image as a crash and replays WAL.crash_consistentandcustom(user quiesce commands) are also selectable. Because a lost post script would otherwise wedge writes indefinitely, the pre script arms its own thaw:The SSM document follows DLM's contract exactly (a
commandparameter allowingpre-script/post-script/dry-run, plusexecutionId) — miss it and DLM records every run as failed. Verified against the pinned AWS provider (v6.53.0) thatcreate_rule.scriptsexists; cross-regionretain_ruleaccepts only DAYS and coarser, so copy retention is derived from the real coverage window rather than reusing the snapshot count:Restore is
data_volume_snapshot_idon the data-volume mapping, and it exposed a latent bug that made it unusable:user_data.sh.tplpicked the data device by scanning for the disk without a filesystem, and wrote the fstab entry only inside that formatting branch. A snapshot-restored volume already has a filesystem, so it was skipped entirely — never fstab'd, never mounted, restored data silently absent while the app came up empty. Now the device is resolved explicitly (/dev/xvdf, then its symlink, then anebsnvme-idmatch tolerant of the label differences acrossamazon-ec2-utilsversions),mkfsruns only on a genuinely blank disk, and the fstab entry and mount happen either way.Honest limits, stated in the docs rather than glossed: RPO is
backup_interval_hours, and there is no automatic restore — a replacement instance still boots empty, and recovery is a deliberate operator action (pick snapshot → set input → recycle → verify → clear input). Docs also now say multi-attach/shared EBS is deliberately unsupported (block device ≠ shared filesystem; XFS/ext4 corrupt under concurrent RW mounts) and that EFS is for shared files, not a live database data directory. Logical dumps to S3, termination-time backups and continuous replication are phases 2/3 and are not in this PR.Preconditions reject the combinations that would otherwise fail silently:
filesystem_freezewithout a data volume (freezing a nonexistent mount fails the script, and DLM then skips the snapshot — a backup that reports enabled and produces nothing),customwithout both commands, and a snapshot ID without the data volume.tofu testcovers backups off/on, the no-data-volume shape, restore preserving the configured size and type, and that precondition.Not run:
make publish-local-dev MODULE=rvn-ec2-service, which needs the local API onlocalhost:8080and isn't reachable from this environment.Link to Devin session: https://app.devin.ai/sessions/2b20f3ce4e7744a69642a468e5b7d463
Requested by: @flybayer
Greptile Summary
This PR adds opt-in DLM snapshot scheduling, consistency hooks, cross-region copies, and a snapshot-based data-volume restore path to the EC2 service module.
Confidence Score: 4/5
The PR should not merge until the DLM role authorizes the advertised cross-region snapshot-copy path.
Setting a cross-region destination creates the DLM copy rule, but the associated execution role lacks the EC2 copy operations needed to produce the destination backups.
Files Needing Attention: compute/ec2_service/iam_dlm.tf and compute/ec2_service/backup.tf
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR UI["Ravion backup inputs"] --> TF["EC2 service Terraform module"] TF --> DLM["DLM lifecycle policy"] DLM --> SSM["Pre/post SSM consistency hooks"] SSM --> EC2["Tagged EC2 instance"] DLM --> SNAP["Regional EBS snapshot set"] SNAP --> COPY["Optional cross-region copy"] RESTORE["Selected snapshot ID"] --> LT["Launch-template data volume"] LT --> BOOT["Bootstrap resolves and mounts restored filesystem"]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Update module compiler test for backup s..." | Re-trigger Greptile
Context used (3)