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

make ASO adopt previously managed resources #3662

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

nojnhuh
Copy link
Contributor

@nojnhuh nojnhuh commented Jun 22, 2023

What type of PR is this?
/kind feature

What this PR does / why we need it:

The goal of this PR is to smooth the transition from non-ASO-backed CAPZ to ASO-backed CAPZ by automatically adopting pre-existing Azure resources that are identified to have been managed by a previous version of CAPZ.

Depending on the resource type, the indicator that it was managed by a previous version of CAPZ will either be the sigs.k8s.io_cluster-api-provider-azure_cluster_* tag on the resource in Azure or a static true for resources not eligible for BYO currently, like managedclusters:

// IsManaged returns always returns true as CAPZ does not support BYO managed cluster.
func (s *Service) IsManaged(ctx context.Context) (bool, error) {
return true, nil
}

More context in the proposal: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/8e1c9aead5a0c7aa7da343331c82c00364a9b9fd/docs/proposals/20230123-azure-service-operator.md#upgrade-strategy

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

Special notes for your reviewer:

This branch includes this change and the necessary plumbing to make this functional if you'd like to test this yourself: main...nojnhuh:cluster-api-provider-azure:aso-wired

  • cherry-pick candidate

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 22, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 22, 2023
@nojnhuh
Copy link
Contributor Author

nojnhuh commented Jun 22, 2023

This is ready for review, but I'd like to do some manual smoke testing since these changes aren't exercised by CI yet.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 22, 2023
@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (8e1c9ae) 53.88% compared to head (562f646) 53.90%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3662      +/-   ##
==========================================
+ Coverage   53.88%   53.90%   +0.01%     
==========================================
  Files         185      185              
  Lines       18618    18626       +8     
==========================================
+ Hits        10032    10040       +8     
  Misses       8046     8046              
  Partials      540      540              
Impacted Files Coverage Δ
azure/services/aso/aso.go 93.95% <100.00%> (+0.06%) ⬆️
azure/services/asogroups/spec.go 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nojnhuh
Copy link
Contributor Author

nojnhuh commented Jun 23, 2023

This is ready for review, but I'd like to do some manual smoke testing since these changes aren't exercised by CI yet.

/hold

Looking good!

/hold cancel
/retest

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 23, 2023
Copy link
Contributor

@adriananeci adriananeci left a comment

Choose a reason for hiding this comment

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

/lgtm
Maybe not directly related to changes from this PR, but why naming the service asogroups instead of asoresourcegroups? Just to keep it in sync with the groups service or because the name would have been too long? IMO asoresourcegroups sounds less confusing compared to asogroups

@k8s-ci-robot
Copy link
Contributor

@adriananeci: changing LGTM is restricted to collaborators

In response to this:

/lgtm
Maybe not directly related to changes from this PR, but why naming the service asogroups instead of asoresourcegroups? Just to keep it in sync with the groups service or because the name would have been too long? IMO asoresourcegroups sounds less confusing compared to asogroups

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.

@nojnhuh
Copy link
Contributor Author

nojnhuh commented Jul 3, 2023

/lgtm Maybe not directly related to changes from this PR, but why naming the service asogroups instead of asoresourcegroups? Just to keep it in sync with the groups service or because the name would have been too long? IMO asoresourcegroups sounds less confusing compared to asogroups

There is a little more context here around the package name: #3574 (comment)

Overall, the plan for the other packages under /azure/services is to re-implement them in-place to use ASO so we won't necessarily need to change the package name to distinguish it as "the ASO one." The resource groups package is a little special because the ASO flavor isn't complete yet, so the existing groups package is the only one in use while we iterate on the ASO framework and ASO groups implementations in parallel. Part of #3527 will be to remove the SDK-backed resource groups implementation.

And in general, I'm not 100% sure all the other /azure/services/* packages will deal with exactly one ASO resource like this one does, so I wonder if naming packages aso<aso resource> might not always work out nicely.

@nojnhuh
Copy link
Contributor Author

nojnhuh commented Jul 5, 2023

/assign @Jont828

Copy link
Contributor

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

/lgtm

Nice work!

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

LGTM label has been added.

Git tree hash: 03fa33634824db8f5b96519ef41565b60d53f183

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
/approve

@k8s-ci-robot
Copy link
Contributor

[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:

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 Jul 7, 2023
@k8s-ci-robot k8s-ci-robot merged commit 736d646 into kubernetes-sigs:main Jul 7, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.10 milestone Jul 7, 2023
@nojnhuh nojnhuh deleted the aso-adopt branch July 7, 2023 20:28
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. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Implement reconciliation of ASO resources
6 participants