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

Add support for custom vm extensions #2631

Merged
merged 1 commit into from
Sep 29, 2022

Conversation

willie-yao
Copy link
Contributor

@willie-yao willie-yao commented Sep 6, 2022

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR adds support for deployment of custom VM extensions from the Azure Extension gallery as part of a VM and VMSS deployment.

Which issue(s) this PR fixes:
Fixes #2221

Special notes for your reviewer:
In order to test the extension provisions successfully, I used Tilt to deploy the default flavor. I added the VMExtensions field to the default cluster template's AzureMachineTemplate spec like so:

vmExtensions:
      - name: CustomScript
        publisher: Microsoft.Azure.Extensions
        version: '2.1'
        settings:
          skipDos2Unix: false
        protectedSettings:
          commandToExecute: |
            #!/bin/sh
            echo "Updating packages ..."
            apt update
            apt upgrade -y

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Add support for custom vm extensions

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 6, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @willie-yao. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 6, 2022
@willie-yao willie-yao changed the title WIP: Add support for custom vm extensions Add support for custom vm extensions Sep 8, 2022
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 8, 2022
@mboersma
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 14, 2022
@willie-yao willie-yao changed the title Add support for custom vm extensions WIP: Add support for custom vm extensions Sep 14, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 14, 2022
@willie-yao willie-yao force-pushed the custom-vm-extensions branch 3 times, most recently from 421e910 to 0d97956 Compare September 15, 2022 17:37
@willie-yao
Copy link
Contributor Author

/retest

1 similar comment
@willie-yao
Copy link
Contributor Author

/retest

@willie-yao willie-yao changed the title WIP: Add support for custom vm extensions Add support for custom vm extensions Sep 22, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 22, 2022
@willie-yao
Copy link
Contributor Author

/retest

@CecileRobertMichon
Copy link
Contributor

/lgtm
/assign @Jont828 @jackfrancis

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2022
@@ -0,0 +1,68 @@
# Custom VM Extensions

## Overview
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this doc, looks good 💯

Can you please add it to https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/7d8dc8583b1c8db99a84768e5ed7f2f140983a05/docs/book/src/SUMMARY.md?plain=1 under Topics so it shows up in the book Table Of Content?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! I'll add it under the other "custom" topics. I'm not sure if there is a specific ordering of topics, but I felt like that would be the best place to add it!

Copy link
Contributor

Choose a reason for hiding this comment

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

Topics are ordered alphabetically I think (except the first two)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so too, but it seems like a few are out of order. "Custom Images" is under "Custom Private DNS Zone name", and there are a few others like "SSH Access to nodes". Should we standardize how we order the topics?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing "Custom Private DNS Zone" is ordered by file name "custom-dns" (d comes before i) and "SSH Access to nodes" was likely just added at the end by someone who didn't realize the list was in order. Feel free to reorganize/rename however you see fit, the ordered list is becoming less practical now that we have so many topics, maybe we need sub-sections now. This can be a separate PR so it doesn't block this one.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2022
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2022
@willie-yao
Copy link
Contributor Author

@CecileRobertMichon I saw that there is a failed test with broken links. I'm assuming this is intended because the link for the new docs page hasn't been published yet. Is that correct?

@jackfrancis
Copy link
Contributor

@CecileRobertMichon I saw that there is a failed test with broken links. I'm assuming this is intended because the link for the new docs page hasn't been published yet. Is that correct?

It's just bad luck, I got the same error on my PR, here's the fix:

https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/2680/files#diff-4d554fade8fefc047dcf883418cc72311cb5b62f3e0d20f7cbbfb849faa7335aR525

tl;dr k8s maintains 4 versions of that link, and then garbage collects the oldest one when a new release is needed

Not sure if we wanna include it in this PR or rebase this on top of a PR that fixes it :(

@willie-yao
Copy link
Contributor Author

@jackfrancis Oh that makes sense! If this blocks merging, I'm not sure what's the better way to do it. I'm leaning towards including this in a separate PR since it's not related to custom vm extensions.

@jackfrancis
Copy link
Contributor

@willie-yao you can rebase on top of latest main as this PR delivered the needful: #2680

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2022
@jackfrancis jackfrancis added this to the v1.6 milestone Sep 29, 2022
@willie-yao
Copy link
Contributor Author

@jackfrancis @CecileRobertMichon Did the rebase and the test passes now. Does everything look good for approval?

Copy link
Contributor

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 29, 2022
@k8s-ci-robot k8s-ci-robot merged commit 68f0b75 into kubernetes-sigs:main Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Custom VM Extensions for AzureMachines
6 participants