Skip to content

Conversation

@rakshit-souvik
Copy link

No description provided.

@Copilot Copilot AI review requested due to automatic review settings July 4, 2025 11:48
@rakshit-souvik rakshit-souvik requested review from a team as code owners July 4, 2025 11:48
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR migrates the existing Vagrant-based Kafka worker AMI builds and test cluster bring-up to Terraform and Packer, consolidating infrastructure-as-code and automating image creation and provisioning.

  • Adds Packer templates (worker-ami.json, aws-packer.json) and related provisioning scripts for base and worker AMIs
  • Introduces Terraform configurations (variable.tf, main.tf and supporting scripts) to deploy Kafka worker instances
  • Updates helper shell scripts and Python orchestration (kafka_runner) to integrate with the new Terraform workflow

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file
File Description
vagrant/worker-ami.json Packer template for building Kafka worker AMI
vagrant/base.sh Base provisioning script (export DEBIAN_FRONTEND)
vagrant/aws-packer.json Packer template for base AMI
terraform/variable.tf Defines Terraform input variables
terraform/main.tf Core Terraform AWS instance resource definitions
terraform/resources/terraform-worker-config.sh Helper script to set AMI_ID and IPv6 support
terraform/resources/requirements.txt Python requirements for orchestration scripts
terraform/resources/requirement_override.txt Override requirements for ducktape
terraform/cloudinit.yml Cloud-init user-data for instance provisioning
terraform/kafka_runner/util.py Python utility functions (SSH, subprocess helpers)
terraform/kafka_runner/run-test.py Main test runner orchestrating Terraform and ducktape
terraform/kafka_runner/package_ami.py Logic to package base and worker AMIs via Packer
ssh_checkers/aws_checker.py SSH retry logic using AWS CLI to verify instance state
Makefile Updates for mk-include dependency management
Comments suppressed due to low confidence (5)

vagrant/worker-ami.json:34

  • The builder references security_group_id, but this variable isn't declared under variables. Add a security_group_id entry to the variables block or remove the reference.
    "security_group_id": "{{user `security_group_id`}}",

vagrant/base.sh:41

  • JDK_ARCH is echoed but never defined in this script (only JDK_FULL exists). Either export JDK_ARCH earlier or change the echo to use the intended variable.
echo "JDK_MAJOR=$JDK_MAJOR JDK_ARCH=$JDK_ARCH"

terraform/variable.tf:7

  • You've declared num_workers as a string, but aws_instance.worker.count expects a number. Change the type to number for proper interpolation.
variable "num_workers" {

terraform/main.tf:74

  • [nitpick] You define two outputs (cloudinit_content-spot and later cloudinit_content) that both render the same data. Consider removing or renaming the duplicate to avoid confusion.
output "cloudinit_content-spot" { value= data.cloudinit_config.user_data.rendered}

terraform/resources/terraform-worker-config.sh:6

  • AMI_ID is defined as a function above, not a variable, so this test will always be false. You may want to check if the environment variable is empty ([ -z "$AMI_ID_VAR" ]) or call the function directly.
if [ -z "${AMI_ID}" ]; then

@airlock-confluentinc airlock-confluentinc bot force-pushed the master_terraform_migrate branch from 46bc63e to ece8a46 Compare July 9, 2025 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant