Skip to content
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

feat: build CVM image on CVM hardware #5412

Draft
wants to merge 29 commits into
base: dev
Choose a base branch
from
Draft

Conversation

zachary-bailey
Copy link
Collaborator

What type of PR is this?

/kind feat

What this PR does / why we need it:

This PR makes use of Node SIG's new multi-region capability to build standard VHDs and CVMs in the same pipeline.

Which issue(s) this PR fixes:

Inability to run test builds on cvm hardware.

Requirements:

@@ -2,7 +2,7 @@ name: $(Date:yyyyMMdd)$(Rev:.r)_$(Build.SourceBranchName)_$(BuildID)
trigger: none

pool:
name: $(POOL_NAME)
name: $(POOL_NAME_OVERRIDE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we keep this as is?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, thats just how I have it configured during testing to make use of the new infra

@@ -31,7 +31,11 @@ if [ "${OS_TYPE,,}" == "linux" ]; then
echo "PACKER_BUILD_LOCATION must be set for linux builds"
exit 1
fi
LOCATION=$PACKER_BUILD_LOCATION
if [ "${ENVIRONMENT,,}" == "test" ] && [ "${IMG_SKU}" == "20_04-lts-cvm" ]; then
LOCATION=$CVM_PACKER_BUILD_LOCATION
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's guard against CVM_PACKER_BUILD_LOCATION being unset

@@ -67,6 +71,21 @@ if [[ ${OS_TYPE} == "Linux" && ${ENABLE_TRUSTED_LAUNCH} == "True" ]]; then
} \
} \
}"
elif [[ ${OS_TYPE} == "Linux" && ${IMG_SKU} == "20_04-lts-cvm" ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
elif [[ ${OS_TYPE} == "Linux" && ${IMG_SKU} == "20_04-lts-cvm" ]]; then
elif [ "${OS_TYPE,,}" == "linux" ] && [ "${IMG_SKU,,}" == "20_04-lts-cvm" ]; then

# TODO(cameissner): build out updated pool resources in prod so we don't have to pivot like this
VNET_RG_NAME="nodesig-${ENVIRONMENT}-${PACKER_BUILD_LOCATION}-agent-pool"
if [ "${ENVIRONMENT,,}" == "test" ] && [ "${IMG_SKU}" == "20_04-lts-cvm" ]; then
VNET_RG_NAME="nodesig-${ENVIRONMENT}-${CVM_PACKER_BUILD_LOCATION}-packer-vnet-rg"
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we just set PACKER_BUILD_LOCATION=$CVM_PACKER_BUILD_LOCATION above and keep VNET_RG_NAME="nodesig-${ENVIRONMENT}-${PACKER_BUILD_LOCATION}-packer-vnet-rg"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. I don't think that would persist across steps? But we could just the check for environment and sku every time before the value is used, and then keep the conditionals the same

fi
apt_get_update || exit $ERR_APT_UPDATE_TIMEOUT
apt_get_dist_upgrade || exit $ERR_APT_DIST_UPGRADE_TIMEOUT
if [[ -n "${VHD_BUILD_TIMESTAMP}" && "${OS_VERSION}" == "22.04" ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if [[ -n "${VHD_BUILD_TIMESTAMP}" && "${OS_VERSION}" == "22.04" ]]; then
if [ -n "${VHD_BUILD_TIMESTAMP}" ] && [ "${OS_VERSION}" == "22.04" ]; then

AZURE_LOCATION=$PACKER_BUILD_LOCATION
if [ "${ENVIRONMENT,,}" == "test" ] && [ "${IMG_SKU}" == "20_04-lts-cvm" ]; then
AZURE_LOCATION=$CVM_PACKER_BUILD_LOCATION
else
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: redundant else

@@ -75,6 +79,10 @@ if [ "${OS_TYPE}" == "Linux" ] && [ "${ENABLE_TRUSTED_LAUNCH}" == "True" ]; then
TARGET_COMMAND_STRING+="--security-type TrustedLaunch --enable-secure-boot true --enable-vtpm true"
fi

if [[ "${OS_TYPE}" == "Linux" && ${IMG_SKU} == "20_04-lts-cvm" ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if [[ "${OS_TYPE}" == "Linux" && ${IMG_SKU} == "20_04-lts-cvm" ]]; then
if [ "${OS_TYPE,,}" == "linux" ] && [ "${IMG_SKU,,}" == "20_04-lts-cvm" ]; then

{
"variables": {
"subscription_id": "{{env `AZURE_SUBSCRIPTION_ID`}}",
"cvm_location": "{{env `CVM_PACKER_BUILD_LOCATION`}}",
Copy link
Collaborator

@cameronmeissner cameronmeissner Dec 18, 2024

Choose a reason for hiding this comment

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

we should be able to remove cvm_location if we just set PACKER_BUILD_LOCATION=$CVM_PACKER_BUILD_LOCATION` in init-variables.sh right?

"shared_image_gallery_replica_count": 2,
"shared_image_gallery_destination": {
"specialized": true,
"confidential_vm_image_encryption_type": "EncryptedVMGuestStateOnlyWithPmk",
Copy link
Collaborator

Choose a reason for hiding this comment

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

im assuming this is why we need the separate template?

@@ -45,7 +45,11 @@ SCAN_VM_ADMIN_PASSWORD="ScanVM@$(date +%s)"
set -x

RESOURCE_GROUP_NAME="$SCAN_RESOURCE_PREFIX-$(date +%s)-$RANDOM"
az group create --name $RESOURCE_GROUP_NAME --location ${PACKER_BUILD_LOCATION} --tags "source=AgentBaker" "now=$(date +%s)" "branch=${GIT_BRANCH}"
if [ "${ENVIRONMENT,,}" == "test" ] && [ "${IMG_SKU}" == "20_04-lts-cvm" ]; then
az group create --name $RESOURCE_GROUP_NAME --location ${CVM_PACKER_BUILD_LOCATION} --tags "source=AgentBaker,now=$(date +%s)" "branch=${GIT_BRANCH}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

again, I'd just set PACKER_BUILD_LOCATION=$CVM_PACKER_BUILD_LOCATION and keep the az group create ... the same

@@ -63,6 +67,10 @@ if [[ "${OS_TYPE}" == "Linux" && "${ENABLE_TRUSTED_LAUNCH}" == "True" ]]; then
VM_OPTIONS+=" --security-type TrustedLaunch --enable-secure-boot true --enable-vtpm true"
fi

if [[ "${OS_TYPE}" == "Linux" && ${IMG_SKU} == "20_04-lts-cvm" ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if [[ "${OS_TYPE}" == "Linux" && ${IMG_SKU} == "20_04-lts-cvm" ]]; then
if [ "${OS_TYPE,,}" == "linux" ] && [ "${IMG_SKU}" == "20_04-lts-cvm" ]; then

if [ "${ENVIRONMENT,,}" == "test" ] && [ "${IMG_SKU}" == "20_04-lts-cvm" ]; then
LOCATION=$CVM_PACKER_BUILD_LOCATION
else
LOCATION=$PACKER_BUILD_LOCATION
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: redundant else

@@ -1173,7 +1173,7 @@ stages:
echo '##vso[task.setvariable variable=IMG_SKU]20_04-lts-cvm'
echo '##vso[task.setvariable variable=IMG_VERSION]latest'
echo '##vso[task.setvariable variable=HYPERV_GENERATION]V2'
echo '##vso[task.setvariable variable=AZURE_VM_SIZE]Standard_D16ds_v5'
echo '##vso[task.setvariable variable=AZURE_VM_SIZE]Standard_DC16ads_v5'
Copy link
Collaborator

@cameronmeissner cameronmeissner Dec 18, 2024

Choose a reason for hiding this comment

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

we might want/need to request additional quota for this family, is this confidential-specific hardware?

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.

2 participants