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.

Sign up for GitHub

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

Jump to bottom

add ASO pause logic #3788

Merged
merged 1 commit into from
Aug 7, 2023
Merged

add ASO pause logic #3788

merged 1 commit into from
Aug 7, 2023

Conversation

nojnhuh
Copy link
Contributor

@nojnhuh nojnhuh commented Aug 1, 2023

What type of PR is this?
/kind feature

What this PR does / why we need it:

This PR implements logic to pause ASO resources using the plumbing set up by #3735.

Specifically, the serviceoperator.azure.com/reconcile-policy annotation on ASO resources will be set to skip and the previous value will also be stored in a CAPZ-specific sigs.k8s.io/cluster-api-provider-azure-pre-pause-reconcile-policy annotation. Then when a Cluster is unpaused and reconciliation resumes normally, the pre-pause-reconcile-policy annotation will be used to restore the value as it was before the Cluster was paused. The reconcile policy cannot unconditionally be changed back to manage because ASO representations of BYO Azure resources (like ones created in the portal or CLI) will already be annotated with the skip reconcile policy.

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

Special notes for your reviewer:

  • cherry-pick candidate

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. 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 Aug 1, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 1, 2023
mboersma
mboersma reviewed Aug 3, 2023
View reviewed changes
azure/services/aso/aso.go Outdated Show resolved Hide resolved
azure/services/aso/aso.go Show resolved Hide resolved
azure/services/aso/aso.go Outdated Show resolved Hide resolved
@nojnhuh
Copy link
Contributor Author

nojnhuh commented Aug 3, 2023

(squash reminder)
/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 Aug 3, 2023
@codecov
Copy link

codecov bot commented Aug 3, 2023 •
edited
Loading

Codecov Report

Patch coverage: 68.00% and project coverage change: +0.03% 🎉

Comparison is base (4e44d43) 54.73% compared to head (750e7ca) 54.77%.
Report is 2 commits behind head on main.

❗ Current head 750e7ca differs from pull request most recent head 5929b00. Consider uploading reports for the commit 5929b00 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3788      +/-   ##
==========================================
+ Coverage   54.73%   54.77%   +0.03%     
==========================================
  Files         187      187              
  Lines       19051    19098      +47     
==========================================
+ Hits        10427    10460      +33     
- Misses       8059     8070      +11     
- Partials      565      568       +3
Files Changed Coverage Δ
azure/services/asogroups/groups.go 85.18% <0.00%> (-10.65%) ⬇️
azure/services/aso/aso.go 89.78% <77.27%> (-3.00%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mboersma
Copy link
Contributor

mboersma commented Aug 3, 2023

/assign

@nojnhuh
Copy link
Contributor Author

nojnhuh commented Aug 3, 2023

/retest

1 similar comment
@nojnhuh
Copy link
Contributor Author

nojnhuh commented Aug 3, 2023

/retest

mboersma
mboersma reviewed Aug 4, 2023
View reviewed changes
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

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

LGTM label has been added.

Git tree hash: 15ac076de78034c8359b174b18bc6f875e1e916e

@nojnhuh
Copy link
Contributor Author

nojnhuh commented Aug 4, 2023

squashed!
/hold cancel

@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 Aug 4, 2023
@nojnhuh
Copy link
Contributor Author

nojnhuh commented Aug 5, 2023

Pushed one tiny pointer -> ptr change.

to squash:
/hold

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

/retest

sonasingh46
sonasingh46 reviewed Aug 7, 2023
View reviewed changes
azure/services/aso/aso.go
@@ -37,6 +37,9 @@ import (
const (
// ReconcilePolicyAnnotation is the annotation key for ASO's reconcile policy mechanism.
ReconcilePolicyAnnotation = "serviceoperator.azure.com/reconcile-policy"
// PrePauseReconcilePolicyAnnotation is the annotation key for the value of
// ReconcilePolicyAnnotation that was set before pausing.
PrePauseReconcilePolicyAnnotation = "sigs.k8s.io/cluster-api-provider-azure-pre-pause-reconcile-policy"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the naming pattern of the annotation key different for ReconcilePolicyAnnotation and PrePauseReconcilePolicyAnnotation i.e serviceoperator.azure.com vs sigs.k8s.io?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

serviceoperator.azure.com/reconcile-policy is defined and consumed by ASO: https://azure.github.io/azure-service-operator/guide/annotations/#serviceoperatorazurecomreconcile-policy

Whereas sigs.k8s.io/cluster-api-provider-azure-pre-pause-reconcile-policy is only consumed by CAPZ.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Thank you

sonasingh46
sonasingh46 approved these changes Aug 7, 2023
View reviewed changes
Copy link
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
( Just one minor question )

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

LGTM label has been added.

Git tree hash: 989f97ba9b06ec004795da3245109bade7e3334a

mboersma
mboersma approved these changes Aug 7, 2023
View reviewed changes
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
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mboersma

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 Aug 7, 2023
@nojnhuh
Copy link
Contributor Author

nojnhuh commented Aug 7, 2023

squashed!
/hold cancel

@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 Aug 7, 2023
@k8s-ci-robot k8s-ci-robot merged commit 8893daa into kubernetes-sigs:main Aug 7, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.11 milestone Aug 7, 2023
@nojnhuh nojnhuh deleted the aso-pause branch August 7, 2023 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sonasingh46 sonasingh46 sonasingh46 approved these changes

@mboersma mboersma mboersma approved these changes

@jackfrancis jackfrancis Awaiting requested review from jackfrancis

@jsturtevant jsturtevant Awaiting requested review from jsturtevant

Assignees

@mboersma mboersma

@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. 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
CAPZ Planning
Archived in project
Milestone
v1.11
Development

Successfully merging this pull request may close these issues.
4 participants
@nojnhuh @mboersma @k8s-ci-robot @sonasingh46