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

fix: build cvm on cvm hardware #5643

Draft
wants to merge 51 commits into
base: master
Choose a base branch
from
Draft

Conversation

zachary-bailey
Copy link
Collaborator

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR changes the build process to use CVM hardware for CVM image builds. This will allow the use of apt-get dist-upgrade in the install scripts, resulting in the ability to install newer kernel versions.

Which issue(s) this PR fixes:

This will fix the CVEs that are currently unmitigated for CVM nodepools.

Requirements:

@@ -1174,7 +1174,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

Choose a reason for hiding this comment

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

let's make sure we have quota for this size

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cameronmeissner I was actually going to bring this up in standup today. I attempted to check in TME yesterday, but for some reason every time I tried to switch the directory it was unsuccessful.

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 block

@@ -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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought I had gotten all of them, thanks!

@@ -71,6 +71,10 @@ if [ "$MODE" == "linuxVhdMode" ] && [ -z "${PACKER_BUILD_LOCATION}" ]; then
exit 1
fi

if [ "$MODE" == "linuxVhdMode" ] && [ -z "${CVM_PACKER_BUILD_LOCATION}" ]; then
CVM_PACKER_BUILD_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.

should we echo something out here to note that CVM_PACKER_BUILD_LOCATION wasn't specified and we're defaulting to PACKER_BUILD_LOCATION?

Copy link
Collaborator

@cameronmeissner cameronmeissner Jan 23, 2025

Choose a reason for hiding this comment

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

actually, could we just add something like

if [ -z "${CVM_PACKER_BUILD_LOCATION}" ]; then
    echo "CVM: will use ${CVM_PACKER_BUILD_LOCATION} as PACKER_BUILD_LOCATION"
    PACKER_BUILD_LOCATION=$CVM_PACKER_BUILD_LOCATION
fi

then we wouldn't need to change how we set VNET_RG_NAME/VNET_NAME

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perfect. Will change.

@@ -167,6 +180,8 @@ if [[ "${MODE}" == "linuxVhdMode" ]]; then
elif [[ "${IMG_OFFER,,}" == "azure-linux-3" ]]; then
# for Azure Linux 3.0, only use AzureLinux prefix
SIG_IMAGE_NAME="AzureLinux${SIG_IMAGE_NAME}"
elif [[ ${IMG_SKU} == "20_04-lts-cvm" ]]; then
SIG_IMAGE_NAME+="Specialized"
Copy link
Collaborator

Choose a reason for hiding this comment

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

indenting seems weird here

{
"variables": {
"subscription_id": "{{env `AZURE_SUBSCRIPTION_ID`}}",
"cvm_packer_build_location": "{{env `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.

nit: this can just be packer_build_location and come from PACKER_BUILD_LOCATION like the other templates

@@ -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 --tags "source=AgentBaker" "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.

duplicate --tags directives

Copy link
Collaborator

Choose a reason for hiding this comment

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

instead we could probably also do something like this again:

if [ -z "${CVM_PACKER_BUILD_LOCATION}" ]; then
    echo "CVM: will use ${CVM_PACKER_BUILD_LOCATION} as PACKER_BUILD_LOCATION"
    PACKER_BUILD_LOCATION=$CVM_PACKER_BUILD_LOCATION
fi

and leave the az group create ... unchanged

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do the same as above.

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