Add subnetName support to ammp #3132

LochanRn · 2023-01-31T11:31:57Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Support to configure a different subnet for AzureManagedMachinePool.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1599

Special notes for your reviewer:

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 subnetName to AzureManagedMachinePool.

willie-yao

Overall this looks great! Just a few minor nits from me.

azure/scope/managedmachinepool_test.go

api/v1beta1/azuremanagedmachinepool_types.go

nawazkh · 2023-02-01T22:43:54Z

I am wondering if a similar change as that of #2411 is required for AzureManagedMachinePool ?
cc: @CecileRobertMichon

CecileRobertMichon · 2023-02-01T23:32:01Z

I am wondering if a similar change as that of #2411 is required for AzureManagedMachinePool ?

Good question. I don't believe AKS allows us to pass in custom network interfaces (does it?)

azure/scope/managedmachinepool_test.go

LochanRn · 2023-02-02T01:26:23Z

@CecileRobertMichon If the subnet does not exist should capz create it ??

We do that in the case of vnet and subnet configured in amcp.

willie-yao

/lgtm

k8s-ci-robot · 2023-02-02T19:00:44Z

LGTM label has been added.

Git tree hash: ae2130dfdd293f3779dd1ef8cffa4860270640b0

azure/scope/managedmachinepool.go

api/v1beta1/azuremanagedmachinepool_types.go

CecileRobertMichon · 2023-02-02T19:45:43Z

If the subnet does not exist should capz create it ??

We do that in the case of vnet and subnet configured in amcp.

That's a tricky one. In the case of AzureMachinePool (unmanaged) we don't create/reconcile subnets as part of the MachinePool controller.

Here is the original PR that did this #1411

mboersma · 2023-02-09T17:34:56Z

/milestone v1.8

nawazkh

Apologies if I am too late on sharing my thoughts on this. I went over the PR semantically and have shared some thoughts.
Thanks for looking into this!

api/v1beta1/azuremanagedmachinepool_types.go

api/v1beta1/azuremanagedmachinepool_webhook.go

api/v1beta1/azuremanagedmachinepool_webhook_test.go

LochanRn · 2023-03-19T11:37:58Z

/test pull-cluster-api-provider-azure-e2e-aks

LochanRn · 2023-03-19T11:38:12Z

/test pull-cluster-api-provider-azure-test

LochanRn · 2023-03-19T12:30:11Z

/test pull-cluster-api-provider-azure-e2e-aks

LochanRn · 2023-03-20T04:55:12Z

/test pull-cluster-api-provider-azure-e2e-aks

LochanRn · 2023-03-20T14:22:59Z

@CecileRobertMichon @nawazkh @willie-yao can you guys please take a look at this PR again :)

CecileRobertMichon · 2023-03-20T21:03:03Z

/lgtm
/assign @nawazkh

k8s-ci-robot · 2023-03-20T21:03:14Z

LGTM label has been added.

Git tree hash: 3020abe99d087dc2fb535f7897b8ac8b0208489a

nawazkh

I have a couple of thoughts which I have noted down, especially using pointer package over go-autorest/autorest/to since we are trying to move away from it.
Apart from that, everything else looks great to me. Thank you for putting this together! Solid work!

api/v1beta1/azuremanagedmachinepool_webhook.go

azure/scope/managedmachinepool.go

controllers/azuremanagedmachinepool_reconciler.go

k8s-ci-robot · 2023-03-21T19:51:34Z

@nawazkh:

In response to this:

chatgpt
/honk

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.

azure/scope/managedmachinepool.go

azure/services/agentpools/agentpools.go

CecileRobertMichon · 2023-03-27T18:06:11Z

/lgtm
/approve

k8s-ci-robot · 2023-03-27T18:06:18Z

LGTM label has been added.

Git tree hash: b360f0afa4d4046363eb6640c28ea8bc654af140

k8s-ci-robot · 2023-03-27T18:06:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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:

~~OWNERS~~ [CecileRobertMichon]

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 added 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 Jan 31, 2023

k8s-ci-robot requested review from jsturtevant and nojnhuh January 31, 2023 11:32

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 31, 2023

LochanRn force-pushed the subnet-name-ammp branch from 6cd0dc6 to ec6a860 Compare January 31, 2023 11:34

willie-yao reviewed Jan 31, 2023

View reviewed changes

azure/scope/managedmachinepool_test.go Show resolved Hide resolved

api/v1beta1/azuremanagedmachinepool_types.go Outdated Show resolved Hide resolved

LochanRn force-pushed the subnet-name-ammp branch 2 times, most recently from d69d225 to 706dc6d Compare February 1, 2023 04:43

willie-yao reviewed Feb 2, 2023

View reviewed changes

azure/scope/managedmachinepool_test.go Show resolved Hide resolved

LochanRn force-pushed the subnet-name-ammp branch from 706dc6d to 74586f4 Compare February 2, 2023 01:36

willie-yao approved these changes Feb 2, 2023

View reviewed changes

k8s-ci-robot assigned willie-yao Feb 2, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2023

CecileRobertMichon reviewed Feb 2, 2023

View reviewed changes

azure/scope/managedmachinepool.go Outdated Show resolved Hide resolved

CecileRobertMichon reviewed Feb 2, 2023

View reviewed changes

azure/scope/managedmachinepool.go Outdated Show resolved Hide resolved

CecileRobertMichon reviewed Feb 2, 2023

View reviewed changes

api/v1beta1/azuremanagedmachinepool_types.go Outdated Show resolved Hide resolved

LochanRn force-pushed the subnet-name-ammp branch from 74586f4 to 8b84f4d Compare February 5, 2023 11:24

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2023

k8s-ci-robot requested a review from willie-yao February 5, 2023 11:24

k8s-ci-robot added this to the v1.8 milestone Feb 9, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2023

nawazkh reviewed Mar 1, 2023

View reviewed changes

api/v1beta1/azuremanagedmachinepool_types.go Outdated Show resolved Hide resolved

api/v1beta1/azuremanagedmachinepool_webhook.go Outdated Show resolved Hide resolved

api/v1beta1/azuremanagedmachinepool_webhook_test.go Show resolved Hide resolved

LochanRn force-pushed the subnet-name-ammp branch from 5d0169f to 9cc609a Compare March 19, 2023 11:17

LochanRn force-pushed the subnet-name-ammp branch 2 times, most recently from 142981e to 87555d9 Compare March 20, 2023 09:19

k8s-ci-robot assigned nawazkh and CecileRobertMichon Mar 20, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 20, 2023

nawazkh requested changes Mar 21, 2023

View reviewed changes

api/v1beta1/azuremanagedmachinepool_webhook.go Show resolved Hide resolved

azure/scope/managedmachinepool.go Outdated Show resolved Hide resolved

azure/scope/managedmachinepool.go Outdated Show resolved Hide resolved

controllers/azuremanagedmachinepool_reconciler.go Show resolved Hide resolved

LochanRn force-pushed the subnet-name-ammp branch from 87555d9 to 3b8ef4b Compare March 21, 2023 07:40

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 21, 2023

k8s-ci-robot requested review from CecileRobertMichon and nawazkh March 21, 2023 07:40

LochanRn force-pushed the subnet-name-ammp branch from 3b8ef4b to da8fbeb Compare March 21, 2023 08:52

CecileRobertMichon reviewed Mar 21, 2023

View reviewed changes

azure/scope/managedmachinepool.go Show resolved Hide resolved

add subnetName support to ammp

8f5ec54

LochanRn force-pushed the subnet-name-ammp branch from da8fbeb to 8f5ec54 Compare March 22, 2023 07:10

CecileRobertMichon reviewed Mar 23, 2023

View reviewed changes

azure/services/agentpools/agentpools.go Show resolved Hide resolved

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2023

k8s-ci-robot merged commit ecd2964 into kubernetes-sigs:main Mar 27, 2023

Add subnetName support to ammp #3132

Add subnetName support to ammp #3132

Uh oh!

Conversation

LochanRn commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willie-yao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nawazkh commented Feb 1, 2023

Uh oh!

CecileRobertMichon commented Feb 1, 2023

Uh oh!

Uh oh!

LochanRn commented Feb 2, 2023

Uh oh!

willie-yao left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Feb 2, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CecileRobertMichon commented Feb 2, 2023

Uh oh!

mboersma commented Feb 9, 2023

Uh oh!

nawazkh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LochanRn commented Mar 19, 2023

Uh oh!

LochanRn commented Mar 19, 2023

Uh oh!

LochanRn commented Mar 19, 2023

Uh oh!

LochanRn commented Mar 20, 2023

Uh oh!

LochanRn commented Mar 20, 2023

Uh oh!

CecileRobertMichon commented Mar 20, 2023

Uh oh!

k8s-ci-robot commented Mar 20, 2023

Uh oh!

nawazkh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 21, 2023

Uh oh!

Uh oh!

Uh oh!

CecileRobertMichon commented Mar 27, 2023

Uh oh!

k8s-ci-robot commented Mar 27, 2023

Uh oh!

k8s-ci-robot commented Mar 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

LochanRn commented Jan 31, 2023 •

edited

Loading