[MOSIP-44608] updated terraform script to support ebs volume for acti…#177
[MOSIP-44608] updated terraform script to support ebs volume for acti…#177bhumi46 merged 4 commits intomosip:developfrom
Conversation
…vemq as optional Signed-off-by: abhishek-1809 <abhisahu1920@gmail.com>
WalkthroughAdds an optional ActiveMQ persistent-storage flow: new Terraform variables and passthroughs, a conditional Changes
Sequence Diagram(s)sequenceDiagram
participant TR as Terraform Runner
participant SH as activemq-setup.sh (local)
participant ANS as Ansible (on Runner)
participant NGINX as NGINX Node
participant CP as K8s Control Plane
TR->>SH: invoke with env (NGINX_PRIVATE_IP, device, mount, SSH key, WORK_DIR)
SH->>ANS: execute ansible-playbook against NGINX
ANS->>NGINX: SSH/ping, install packages, wait for device
ANS->>NGINX: mkfs XFS if needed, mount, update /etc/fstab, set perms
ANS->>NGINX: configure NFS export, restart nfs-server
ANS->>TR: delegate_to localhost -> write /tmp/activemq-storageclass.yaml
TR->>CP: scp StorageClass YAML to control plane
TR->>CP: remote kubectl apply -f StorageClass
CP-->>TR: verify StorageClass created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
terraform/modules/aws/aws-main.tf (1)
552-553: Map iteration order is non-deterministic.The expression
[for instance in module.aws-resource-creation.K8S_CLUSTER_PRIVATE_IPS : instance][0]iterates over a map and takes the first element. Map iteration order in Terraform is not guaranteed to be consistent across runs.This is consistent with the existing pattern in
postgresql-setup(line 535), so it's an accepted approach in this codebase. However, if deterministic control-plane selection is required (e.g., always picking a specific node), consider usingvalues()with explicit sorting or referencing a specific map key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terraform/modules/aws/aws-main.tf` around lines 552 - 553, The current CONTROL_PLANE_HOST assignment uses an unordered map iteration ([for instance in module.aws-resource-creation.K8S_CLUSTER_PRIVATE_IPS : instance][0]) which yields non-deterministic results; change it to deterministically select a control plane IP by either (a) using values(module.aws-resource-creation.K8S_CLUSTER_PRIVATE_IPS) combined with a stable sort (e.g., sort by keys or values) before indexing, or (b) referencing a specific known map key if you want a particular node, and update CONTROL_PLANE_HOST to use that deterministic expression (see module.aws-resource-creation.K8S_CLUSTER_PRIVATE_IPS and CONTROL_PLANE_HOST for where to apply the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terraform/modules/aws/activemq-setup/activemq-setup.yml`:
- Around line 32-35: The playbook currently waits for and persists
activemq_storage_device (used in the wait_for task) as a raw device path (e.g.,
/dev/nvme3n1), which is unstable on Nitro instances; change the logic that sets
and uses activemq_storage_device so it resolves and persists a stable identifier
(UUID or /dev/disk/by-id/* or via ebsnvme-id) and update both the wait_for task
and the mount/fstab entries to use that stable identifier instead of the raw
device path; locate references to activemq_storage_device, the wait_for task,
and any mount/fstab tasks in the role/playbook and replace device-path usage
with a lookup step (blkid/lsblk or ebsnvme-id) that returns a filesystem UUID or
by-id path, then write that stable identifier into fstab/mount configuration.
In `@terraform/modules/aws/activemq-setup/main.tf`:
- Around line 42-44: The two null_resource blocks need their triggers expanded
so reprovisioning happens when node IPs or setup scripts change: update resource
"null_resource" "activemq-ebs-nfs-setup" and resource "null_resource"
"activemq-k8s-storageclass" to include triggers for the NGINX and control-plane
node IPs and content hashes of activemq-setup.yml and activemq-setup.sh (use
md5(file(...)) or md5 of concatenated vars like the pattern in nginx-setup.tf
lines 79–87); don’t rely on depends_on for reprovisioning—add a triggers map
that includes device/mount hashes plus the node IPs and script file hashes so
changes force re-run.
- Around line 115-118: The inline script currently hardcodes /home/ubuntu/.kube/
and uses find on '*.yaml'; change it to build the kubeconfig path from the
control plane user and use the standard kubeconfig location: export
KUBECONFIG=/home/${var.CONTROL_PLANE_USER}/.kube/config (or use
$HOME/.kube/config when running as that user), and fail or error-log if that
file does not exist before running kubectl; update the inline array (the inline
script block) that contains the KUBECONFIG export and kubectl commands so it
references var.CONTROL_PLANE_USER (or $HOME) and validates the config file
instead of searching for arbitrary *.yaml files.
- Around line 55-80: The comment is incorrect because no EXIT trap exists—add a
cleanup trap immediately after creating KEY_FILE and WORK_DIR that removes both
"$KEY_FILE" and "$WORK_DIR" on EXIT (and on ERR) to guarantee removal on success
or failure, ensure the trap runs before calling bash
"$WORK_DIR/activemq-setup.sh", and update/remove the misleading "also runs on
failure via trap" comment; reference the KEY_FILE and WORK_DIR variables and the
bash invocation of activemq-setup.sh when making the change.
---
Nitpick comments:
In `@terraform/modules/aws/aws-main.tf`:
- Around line 552-553: The current CONTROL_PLANE_HOST assignment uses an
unordered map iteration ([for instance in
module.aws-resource-creation.K8S_CLUSTER_PRIVATE_IPS : instance][0]) which
yields non-deterministic results; change it to deterministically select a
control plane IP by either (a) using
values(module.aws-resource-creation.K8S_CLUSTER_PRIVATE_IPS) combined with a
stable sort (e.g., sort by keys or values) before indexing, or (b) referencing a
specific known map key if you want a particular node, and update
CONTROL_PLANE_HOST to use that deterministic expression (see
module.aws-resource-creation.K8S_CLUSTER_PRIVATE_IPS and CONTROL_PLANE_HOST for
where to apply the change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bc36f586-1031-43b4-bfd7-bbe4755ac5ab
📒 Files selected for processing (12)
terraform/implementations/aws/infra/aws.tfvarsterraform/implementations/aws/infra/main.tfterraform/implementations/aws/infra/variables.tfterraform/infra/aws/main.tfterraform/infra/aws/variables.tfterraform/infra/main.tfterraform/infra/variables.tfterraform/modules/aws/activemq-setup/activemq-setup.shterraform/modules/aws/activemq-setup/activemq-setup.ymlterraform/modules/aws/activemq-setup/main.tfterraform/modules/aws/aws-main.tfterraform/modules/aws/variables.tf
| - name: Wait for ActiveMQ storage device to appear | ||
| wait_for: | ||
| path: "{{ activemq_storage_device }}" | ||
| timeout: 120 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n terraform/modules/aws/activemq-setup/activemq-setup.ymlRepository: mosip/infra
Length of output: 6859
🏁 Script executed:
rg "activemq_storage_device" --type yaml --type json -B 2 -A 2Repository: mosip/infra
Length of output: 2599
🏁 Script executed:
find terraform/modules/aws/activemq-setup -type f \( -name "*.tf" -o -name "*.json" -o -name "defaults" -o -name "vars" \) | head -20Repository: mosip/infra
Length of output: 99
🏁 Script executed:
rg "activemq_storage_device" --type-listRepository: mosip/infra
Length of output: 444
🏁 Script executed:
rg "activemq_storage_device" -lRepository: mosip/infra
Length of output: 488
🏁 Script executed:
cat -n terraform/modules/aws/activemq-setup/main.tfRepository: mosip/infra
Length of output: 5699
🌐 Web query:
AWS EC2 Nitro NVMe device naming stability reboot
💡 Result:
No, AWS EC2 Nitro-based instances do not guarantee NVMe device name stability across reboots. NVMe device names like /dev/nvme0n1 are assigned dynamically by the Linux kernel based on device enumeration order during boot, which can vary due to timing, attached volumes, or cloud-init. This can cause names to change (e.g., nvme0n1 to nvme1n1) even on the same instance after reboot. AWS official documentation explicitly states: "Occasionally, devices can respond to discovery in a different order in subsequent instance starts, which causes the device name to change." The block device driver may assign names differently from block device mappings. Best practices to handle this: - Use filesystem UUIDs or labels in /etc/fstab (find with blkid). - Use EBS volume ID (serial number, via lsblk -o +SERIAL or nvme id-ctrl). - On Amazon Linux 2017.09.01+, use ebsnvme-id /dev/nvmeXn1 to map to stable /dev/sdX name and volume ID. Udev rules create symlinks like /dev/xvdf -> nvmeXn1. - For monitoring/scripts, query by mountpoint (findmnt, lsblk) instead of device path. This applies to EBS volumes on Nitro instances; instance store NVMe follows similar dynamic enumeration.
Citations:
- 1: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/device_naming.html
- 2: https://docs.aws.amazon.com/ebs/latest/userguide/identify-nvme-ebs-device.html
- 3: https://docs.aws.amazon.com/ebs/latest/userguide/nvme-ebs-volumes.html
- 4: https://repost.aws/questions/QU7Tdm9m0-Qrmky0fzdwaPow/nvme-device-name-inconsistency-across-ec2-instances-root-disk-switching-from-nvme0n1p1-to-nvme1n1p1
- 5: https://stackoverflow.com/questions/64568780/aws-nvme-mounted-to-different-directory-after-reboot
- 6: https://stackoverflow.com/questions/70205661/correctly-specifying-device-name-for-ebs-volume-while-attaching-to-an-ec2-instan
- 7: https://www.ibm.com/support/pages/node/7265311
- 8: https://serverfault.com/questions/982570/how-to-preserve-drive-device-paths-for-aws-ec2-instance-between-reboots
Use a stable block device identifier instead of /dev/nvme3n1 in fstab.
The playbook waits for and persists activemq_storage_device directly (currently defaulting to /dev/nvme3n1). On EC2 Nitro instances, NVMe device enumeration is not stable across reboot or volume reattach, causing device names to reorder dynamically (e.g., /dev/nvme0n1 → /dev/nvme1n1). This causes the wrong volume to be remounted or the mount to fail silently after restart, risking data loss or cluster failure.
Replace the raw device path with a stable identifier in both the wait_for task and the mount task:
- Use the filesystem UUID (via
blkidorlsblk -o +UUID) in fstab - Or use
/dev/disk/by-id/nvme-<volume_id>for EBS-backed volumes (derived fromnvme id-ctrl) - Or use
ebsnvme-idto reliably map EBS volumes to their IDs
This aligns with AWS best practices for Nitro instance storage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@terraform/modules/aws/activemq-setup/activemq-setup.yml` around lines 32 -
35, The playbook currently waits for and persists activemq_storage_device (used
in the wait_for task) as a raw device path (e.g., /dev/nvme3n1), which is
unstable on Nitro instances; change the logic that sets and uses
activemq_storage_device so it resolves and persists a stable identifier (UUID or
/dev/disk/by-id/* or via ebsnvme-id) and update both the wait_for task and the
mount/fstab entries to use that stable identifier instead of the raw device
path; locate references to activemq_storage_device, the wait_for task, and any
mount/fstab tasks in the role/playbook and replace device-path usage with a
lookup step (blkid/lsblk or ebsnvme-id) that returns a filesystem UUID or by-id
path, then write that stable identifier into fstab/mount configuration.
| triggers = { | ||
| device_mount_hash = md5("${var.ACTIVEMQ_STORAGE_DEVICE}${var.ACTIVEMQ_MOUNT_POINT}") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the activemq-setup main.tf file
find . -path "*/terraform/modules/aws/activemq-setup/main.tf" -type fRepository: mosip/infra
Length of output: 101
🏁 Script executed:
# Also look for the nginx-setup comparison file
find . -path "*/terraform/modules/aws/nginx-setup/*main.tf" -type fRepository: mosip/infra
Length of output: 110
🏁 Script executed:
cat -n ./terraform/modules/aws/activemq-setup/main.tfRepository: mosip/infra
Length of output: 5699
🏁 Script executed:
cat -n ./terraform/modules/aws/nginx-setup/nginx-setup-main.tf | head -130Repository: mosip/infra
Length of output: 6262
Both null_resources need triggers for changes that require reprovisioning.
activemq-ebs-nfs-setup (lines 42-44) only triggers on device and mount path variables, missing NGINX node IP changes and script updates (activemq-setup.yml, activemq-setup.sh). activemq-k8s-storageclass (lines 94-96) has no triggers at all—depends_on only controls execution order, not reprovisioning. If the NGINX or control-plane node is replaced or scripts are edited, provisioning will not rerun on the new hosts. Include node IPs and script content hashes in triggers, per the pattern in nginx-setup.tf lines 79–87.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@terraform/modules/aws/activemq-setup/main.tf` around lines 42 - 44, The two
null_resource blocks need their triggers expanded so reprovisioning happens when
node IPs or setup scripts change: update resource "null_resource"
"activemq-ebs-nfs-setup" and resource "null_resource"
"activemq-k8s-storageclass" to include triggers for the NGINX and control-plane
node IPs and content hashes of activemq-setup.yml and activemq-setup.sh (use
md5(file(...)) or md5 of concatenated vars like the pattern in nginx-setup.tf
lines 79–87); don’t rely on depends_on for reprovisioning—add a triggers map
that includes device/mount hashes plus the node IPs and script file hashes so
changes force re-run.
…vemq as optional Signed-off-by: abhishek-1809 <abhisahu1920@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terraform/modules/aws/aws-main.tf`:
- Around line 541-550: Add fail-fast validation for activemq_storage_device and
activemq_mount_point by adding a Terraform precondition on the activemq-setup
module invocation or by adding validation blocks to the variables
activemq_storage_device and activemq_mount_point in
terraform/modules/aws/variables.tf; ensure each value is non-empty and matches
sensible patterns (e.g., device path regex like ^/dev/.+ and mount point like
^/([A-Za-z0-9._-]+)(/.*)?$) so Terraform will error before apply if inputs are
invalid, referencing the module call named activemq-setup and the variables
activemq_storage_device and activemq_mount_point.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 814ac392-71ac-4fc4-a0d3-9f23b45ffc94
📒 Files selected for processing (1)
terraform/modules/aws/aws-main.tf
…vemq as optional Signed-off-by: abhishek-1809 <abhisahu1920@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
terraform/modules/aws/aws-resource-creation/variables.tf (1)
49-53: Consider adding validation for minimum volume size.The variable could benefit from validation to ensure a minimum usable size when enabled:
validation { condition = var.NGINX_NODE_EBS_VOLUME_SIZE_3 == 0 || var.NGINX_NODE_EBS_VOLUME_SIZE_3 >= 10 error_message = "NGINX_NODE_EBS_VOLUME_SIZE_3 must be 0 (disabled) or at least 10 GB." }This prevents accidentally provisioning an unusably small volume.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terraform/modules/aws/aws-resource-creation/variables.tf` around lines 49 - 53, The variable NGINX_NODE_EBS_VOLUME_SIZE_3 lacks validation and can be set to unusably small sizes; add a Terraform validation block to variable "NGINX_NODE_EBS_VOLUME_SIZE_3" that enforces the value is either 0 (disabled) or >= 10 and provide a clear error_message like "NGINX_NODE_EBS_VOLUME_SIZE_3 must be 0 (disabled) or at least 10 GB." so accidental small volumes are rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terraform/modules/aws/aws-resource-creation/variables.tf`:
- Line 182: The ActiveMQ storage volume is currently created with encrypted =
false; change this to encrypted = true in the EBS volume/block device resource
(the block containing the encrypted attribute) and, if required, expose or set a
KMS key via kms_key_id (e.g., add/update a variable like kms_key_id and
reference it in the same resource) so encryption is enforced at rest; if you
prefer AWS-managed keys you can set encrypted = true without a kms_key_id,
otherwise wire up var.kms_key_id to the resource.
- Around line 177-188: The EBS volume creation is only gated by
NGINX_NODE_EBS_VOLUME_SIZE_3 > 0 which can attach a disk when the activemq-setup
module (which formats/mounts it) is disabled; update the condition so the block
that creates the volume also requires enable_activemq_setup (i.e. change the
conditional on the EBS block to check both var.enable_activemq_setup and
var.NGINX_NODE_EBS_VOLUME_SIZE_3 > 0) or, alternatively, add a clear documented
invariant that nginx_node_ebs_volume_size_3 > 0 implies enable_activemq_setup =
true and enforce that via validation; refer to the variables
NGINX_NODE_EBS_VOLUME_SIZE_3, enable_activemq_setup and the activemq-setup
module when making the change.
---
Nitpick comments:
In `@terraform/modules/aws/aws-resource-creation/variables.tf`:
- Around line 49-53: The variable NGINX_NODE_EBS_VOLUME_SIZE_3 lacks validation
and can be set to unusably small sizes; add a Terraform validation block to
variable "NGINX_NODE_EBS_VOLUME_SIZE_3" that enforces the value is either 0
(disabled) or >= 10 and provide a clear error_message like
"NGINX_NODE_EBS_VOLUME_SIZE_3 must be 0 (disabled) or at least 10 GB." so
accidental small volumes are rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 096063a9-e3fb-42fd-a328-788c16a8c036
📒 Files selected for processing (1)
terraform/modules/aws/aws-resource-creation/variables.tf
…vemq as optional Signed-off-by: abhishek-1809 <abhisahu1920@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
terraform/modules/aws/activemq-setup/activemq-setup.yml (1)
99-104: Review NFS export security options.The export line uses
no_root_squashandinsecure, which are permissive:
no_root_squash: Allows root on NFS clients to have root privileges on the shareinsecure: Allows connections from ports > 1024While these may be necessary for Kubernetes pod access (containers often run as root and use high ports), consider whether stricter options are feasible:
# More restrictive alternative (if compatible with your K8s setup): "{{ activemq_mount_point }} *(rw,sync,root_squash,all_squash,anonuid=1000,anongid=1000,subtree_check)"If the current options are intentional for compatibility, consider adding a comment explaining why.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terraform/modules/aws/activemq-setup/activemq-setup.yml` around lines 99 - 104, The NFS export added by the "Add ActiveMQ NFS export entry" task currently uses permissive options (no_root_squash, insecure); update the export line referenced by the variable activemq_mount_point to use stricter options (e.g., root_squash or all_squash with anonuid/anongid and remove insecure) if compatible with your Kubernetes pods, or if those permissive settings are required for pod access, add an inline comment next to the line explaining why no_root_squash and insecure are necessary for your K8s setup so the intent is documented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@terraform/modules/aws/activemq-setup/activemq-setup.yml`:
- Around line 99-104: The NFS export added by the "Add ActiveMQ NFS export
entry" task currently uses permissive options (no_root_squash, insecure); update
the export line referenced by the variable activemq_mount_point to use stricter
options (e.g., root_squash or all_squash with anonuid/anongid and remove
insecure) if compatible with your Kubernetes pods, or if those permissive
settings are required for pod access, add an inline comment next to the line
explaining why no_root_squash and insecure are necessary for your K8s setup so
the intent is documented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aab64b73-254c-446e-b39b-4e8d50b94820
📒 Files selected for processing (8)
terraform/implementations/aws/infra/aws.tfvarsterraform/implementations/aws/infra/profiles/esignet/aws.tfvarsterraform/implementations/aws/infra/profiles/mosip/aws.tfvarsterraform/modules/aws/activemq-setup/activemq-setup.ymlterraform/modules/aws/activemq-setup/main.tfterraform/modules/aws/aws-main.tfterraform/modules/aws/aws-resource-creation/variables.tfterraform/modules/aws/variables.tf
💤 Files with no reviewable changes (1)
- terraform/implementations/aws/infra/aws.tfvars
🚧 Files skipped from review as they are similar to previous changes (2)
- terraform/modules/aws/variables.tf
- terraform/modules/aws/aws-main.tf
2 participants
Summary by CodeRabbit
New Features
Chores