Skip to content

Enable public MEC on CAPZ#2836

Merged
k8s-ci-robot merged 1 commit into
kubernetes-sigs:mainfrom
upxinxin:pr6
Mar 2, 2023
Merged

Enable public MEC on CAPZ#2836
k8s-ci-robot merged 1 commit into
kubernetes-sigs:mainfrom
upxinxin:pr6

Conversation

@upxinxin
Copy link
Copy Markdown
Contributor

@upxinxin upxinxin commented Nov 21, 2022 •
edited by CecileRobertMichon
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

To make CAPZ able to deploy cluster on Azure Public MEC. Changes we did:

  1. Add extendedlocation in AzureCluster CRD

  2. Passing extendedlocation info to azure SDK

  3. Add a new flavor 'edgezone'. (remove nat-gateway, use OOT, add extendedlocation variables)

  4. Disable availabilitySet and availabilityZone for public MEC

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 #2660

Special notes for your reviewer:

Public MEC is not public accessible right now. To be able to create cluster on public MEC, an msft internal subscription with public MEC location access is needed. We've tested the feature e2e, but I think we may need a discussion on how to verify this feature from reviewer side.

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:

Enable CAPZ to deploy cluster on Azure Public MEC

@k8s-ci-robot 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. labels Nov 21, 2022
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Nov 21, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Nov 21, 2022
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Nov 21, 2022

Welcome @upxinxin!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-azure 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-azure has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Nov 21, 2022

Hi @upxinxin. 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.

Details

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 21, 2022
sonasingh46
sonasingh46 reviewed Nov 23, 2022
View reviewed changes
Comment thread azure/converters/extendedlocation.go
sonasingh46
sonasingh46 reviewed Nov 23, 2022
View reviewed changes
Comment thread azure/scope/cluster.go Outdated
sonasingh46
sonasingh46 reviewed Nov 23, 2022
View reviewed changes
Comment thread controllers/azurecluster_reconciler.go
sonasingh46
sonasingh46 reviewed Nov 23, 2022
View reviewed changes
Comment thread controllers/azuremachine_reconciler.go Outdated
sonasingh46
sonasingh46 reviewed Nov 23, 2022
View reviewed changes
Comment thread controllers/helpers.go Outdated
@sonasingh46
Copy link
Copy Markdown
Contributor

sonasingh46 commented Nov 23, 2022

General questions:

  • For extended location, is the regular azure location relevant or simply it is not used?
  • Having an e2e for validation should be ideal but as it is mentioned that it is not publicly accessible, let us see what other folks have to say on this or if we have some way to navigate it.

@sonasingh46
Copy link
Copy Markdown
Contributor

sonasingh46 commented Nov 23, 2022

/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 Nov 23, 2022
CecileRobertMichon
CecileRobertMichon reviewed Nov 23, 2022
View reviewed changes
Comment thread api/v1beta1/types_class.go
CecileRobertMichon
CecileRobertMichon reviewed Nov 23, 2022
View reviewed changes
Comment thread templates/cluster-template-edgezone.yaml
@upxinxin
Copy link
Copy Markdown
Contributor Author

upxinxin commented Nov 24, 2022

General questions:

  • For extended location, is the regular azure location relevant or simply it is not used?
  • Having an e2e for validation should be ideal but as it is mentioned that it is not publicly accessible, let us see what other folks have to say on this or if we have some way to navigate it.
  1. ExtendedLocation is attached to azure location. Azure location matters.
  2. Yes, we are preparing an e2e test to validate the feature on public MEC. But there still needs discussion for running test.

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Dec 9, 2022 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 9, 2022
@upxinxin
Copy link
Copy Markdown
Contributor Author

upxinxin commented Feb 22, 2023

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

@upxinxin
Copy link
Copy Markdown
Contributor Author

upxinxin commented Feb 22, 2023

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

@upxinxin
Copy link
Copy Markdown
Contributor Author

upxinxin commented Feb 24, 2023

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

@upxinxin
Copy link
Copy Markdown
Contributor Author

upxinxin commented Feb 24, 2023

@CecileRobertMichon We have passed e2e-optional test. Please help review it again.

@CecileRobertMichon
Copy link
Copy Markdown
Contributor

CecileRobertMichon commented Feb 27, 2023

/hold cancel
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 27, 2023
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Feb 27, 2023

LGTM label has been added.

DetailsGit tree hash: cd0b9c5fe1b658f68c99addcdd23fcf5e014e139

sonasingh46
sonasingh46 reviewed Mar 2, 2023
View reviewed changes
Comment thread azure/converters/extendedlocation.go Outdated
sonasingh46
sonasingh46 reviewed Mar 2, 2023
View reviewed changes
Comment thread azure/converters/extendedlocation.go Outdated
sonasingh46
sonasingh46 approved these changes Mar 2, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@sonasingh46 sonasingh46 left a comment

Choose a reason for hiding this comment

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

one minor nit.
Otherwise
/lgtm

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2023
@k8s-ci-robot k8s-ci-robot requested a review from sonasingh46 March 2, 2023 06:27
sonasingh46
sonasingh46 approved these changes Mar 2, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@sonasingh46 sonasingh46 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 Mar 2, 2023
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Mar 2, 2023

LGTM label has been added.

DetailsGit tree hash: cae1ad9f2fe845d9c34dfb6207feccc8560ab101

@upxinxin
Copy link
Copy Markdown
Contributor Author

upxinxin commented Mar 2, 2023

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

@upxinxin
Copy link
Copy Markdown
Contributor Author

upxinxin commented Mar 2, 2023

/retest

@upxinxin
Copy link
Copy Markdown
Contributor Author

upxinxin commented Mar 2, 2023 •
edited
Loading

Several sub-tests in e2e-optional test failed with the error "cannot create more than 100 public IP addresses for this subscription in this region". It seems like public IPs in your CI test sub needs to be cleaned. @CecileRobertMichon

@CecileRobertMichon
Copy link
Copy Markdown
Contributor

CecileRobertMichon commented Mar 2, 2023

yes we ran into quota issues in the subscription yesterday https://kubernetes.slack.com/archives/CEX9HENG7/p1677709131827159, should be fixed now

/retest
/approve

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Mar 2, 2023

[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

Details 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 Mar 2, 2023
@k8s-ci-robot k8s-ci-robot merged commit 2006aad into kubernetes-sigs:main Mar 2, 2023
@CecileRobertMichon CecileRobertMichon mentioned this pull request Mar 2, 2023
Merged
3 tasks
@nojnhuh nojnhuh mentioned this pull request May 28, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jont828 Jont828 Awaiting requested review from Jont828

@CecileRobertMichon CecileRobertMichon Awaiting requested review from CecileRobertMichon

@mboersma mboersma Awaiting requested review from mboersma

2 more reviewers

@mogliang mogliang mogliang left review comments

@sonasingh46 sonasingh46 sonasingh46 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mboersma mboersma

@CecileRobertMichon CecileRobertMichon

@sonasingh46 sonasingh46

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

Milestone

v1.8

Development

Successfully merging this pull request may close these issues.

Enable CAPZ to deploy cluster on Azure Public MEC

9 participants

@upxinxin @k8s-ci-robot @sonasingh46 @CecileRobertMichon @nawazkh @bridgetkromhout @mboersma @mogliang @jackfrancis