Streamline VM bootstrap. #11462
Streamline VM bootstrap. #11462
Conversation
marvin-tigera
added
release-note-required
There was a problem hiding this comment.
Pull request overview
This PR streamlines the VM bootstrap process for Calico's CI test infrastructure by consolidating operations into the VM startup script, eliminating the need for post-creation SSH/SCP commands.
- Moves Docker Hub authentication and test artifact loading from
configure-test-vminto the VM's startup script - Replaces credential file copying with Google Cloud service account authentication
- Retrieves Docker Hub password from Google Secret Manager instead of environment variables
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.semaphore/vms/vm-bootstrap.sh |
Adds Docker Hub login using Secret Manager and GCS artifact download/extraction at VM startup |
.semaphore/vms/create-test-vms |
Adds variable substitution for the bootstrap script template and configures VMs with service account |
.semaphore/vms/configure-test-vm |
Removes post-creation SSH commands for Docker login and artifact transfer, now handled by startup script |
.semaphore/vms/create-test-vms
Outdated
| cat $my_dir/vm-bootstrap.sh | | ||
| sed "s|__GCS_WORKFLOW_DIR__|${GCS_WORKFLOW_DIR}|g" | | ||
| sed "s|__DOCKERHUB_USERNAME__|${DOCKERHUB_USERNAME}|g" | | ||
| sed "s|__CALICO_DIR_NAME__|${CALICO_DIR_NAME}|g" |
There was a problem hiding this comment.
Missing pipe operator in the sed chain. Line 46 should end with a pipe | to connect to the next sed command on line 47. Without it, the output from line 46's sed will be discarded and only line 47's sed will write to the output file.
| sed "s|__CALICO_DIR_NAME__|${CALICO_DIR_NAME}|g" | |
| sed "s|__CALICO_DIR_NAME__|${CALICO_DIR_NAME}|g" | |
|
|
||
| set +x | ||
| DOCKERHUB_USERNAME=__DOCKERHUB_USERNAME__ | ||
| DOCKERHUB_PASSWORD=$(gcloud secrets versions access latest --project=unique-caldron-775 --secret=ci-dockerhub-password) |
There was a problem hiding this comment.
[nitpick] Hardcoded project ID. The project ID unique-caldron-775 is hardcoded in both the secret access (line 85) and the service account (line 61 in create-test-vms). Consider using a variable like ${GCP_PROJECT} (already used in configure-test-vm line 22) for consistency and easier configuration management.
| DOCKERHUB_PASSWORD=$(gcloud secrets versions access latest --project=unique-caldron-775 --secret=ci-dockerhub-password) | |
| DOCKERHUB_PASSWORD=$(gcloud secrets versions access latest --project="${GCP_PROJECT}" --secret=ci-dockerhub-password) |
| --instance-termination-action=DELETE \ | ||
| --metadata-from-file startup-script="$my_dir/vm-bootstrap.sh" \ | ||
| --metadata-from-file startup-script="/tmp/vm-bootstrap.sh" \ | ||
| --service-account=ci-test-vm@unique-caldron-775.iam.gserviceaccount.com \ |
There was a problem hiding this comment.
[nitpick] Hardcoded service account and project ID. The value [email protected] contains a hardcoded project ID. Consider parameterizing this using a variable like ${GCP_PROJECT} for consistency with configure-test-vm (line 22).
| --service-account=ci-test-vm@unique-caldron-775.iam.gserviceaccount.com \ | |
| --service-account=ci-test-vm@${GCP_PROJECT}.iam.gserviceaccount.com \ |
- Get docker login secret from Google secret manager. - Avoid needing an extra scp/ssh commmand.
fasaxc
force-pushed
the
streamline-vm-bootstrap
branch
from
2c6b9f3 to
0a9d59f
Compare
fasaxc
added
docs-not-required
2 participants
Description
Related issues/PRs
Todos
Release Note
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one
docs-*label.docs-pr-required: This change requires a change to the documentation that has not been completed yet.docs-completed: This change has all necessary documentation completed.docs-not-required: This change has no user-facing impact and requires no docs.Every PR needs one
release-note-*label.release-note-required: This PR has user-facing changes. Most PRs should have this label.release-note-not-required: This PR has no user-facing changes.Other optional labels:
cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.