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 pause handling for AzureManagedControlPlane #3783

Conversation

nojnhuh
Copy link
Contributor

@nojnhuh nojnhuh commented Jul 31, 2023

What type of PR is this?
/kind feature

What this PR does / why we need it:

This PR adds the capability for the AzureManagedControlPlane controller to react when resources are paused using the same pattern as #3735. See that PR for more context.

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. labels Jul 31, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 31, 2023
@nojnhuh
Copy link
Contributor Author

nojnhuh commented Jul 31, 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 Jul 31, 2023
@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Patch coverage: 63.41% and project coverage change: +0.30% 🎉

Comparison is base (3d85151) 54.43% compared to head (2a939cc) 54.74%.
Report is 2 commits behind head on main.

❗ Current head 2a939cc differs from pull request most recent head 150f44f. Consider uploading reports for the commit 150f44f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3783      +/-   ##
==========================================
+ Coverage   54.43%   54.74%   +0.30%     
==========================================
  Files         187      187              
  Lines       19029    19051      +22     
==========================================
+ Hits        10359    10429      +70     
+ Misses       8113     8057      -56     
- Partials      557      565       +8     
Files Changed Coverage Δ
controllers/azuremanagedcontrolplane_controller.go 40.66% <48.27%> (+20.06%) ⬆️
controllers/azuremanagedcontrolplane_reconciler.go 38.23% <100.00%> (+38.23%) ⬆️

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

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

(My multiline string comment is just a style thing.) Looks good. I compared it to #3735.

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

LGTM label has been added.

Git tree hash: 5320f19a7b31e47664d07f40ba54f284e06d30ee

if err = c.Watch(
&source.Kind{Type: &clusterv1.Cluster{}},
handler.EnqueueRequestsFromMapFunc(amcpr.ClusterToAzureManagedControlPlane),
predicates.ClusterUnpausedAndInfrastructureReady(log),
predicates.ResourceNotPausedAndHasFilterLabel(log, amcpr.WatchFilterValue),
predicates.Any(log, predicates.ClusterCreateInfraReady(log), predicates.ClusterUpdateInfraReady(log), ClusterUpdatePauseChange(log)),
Copy link
Contributor

Choose a reason for hiding this comment

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

For predicates.Any(), doesn't it return true if any of the predicates inside return true? If so, in this case shouldn't we be only returning true if all three are true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was adapted from ClusterUnpausedAndInfrastructureReady to essentially remove the ClusterCreateNotPaused condition and replace ClusterUpdateUnpaused with ClusterUpdatePauseChange. https://github.com/kubernetes-sigs/cluster-api/blob/54dfd423d80e42f79c33c0149f9719dba3863f11/util/predicates/cluster_predicates.go#L221

These update predicates only look at one particular change of a resource and not just its updated state, so ClusterUpdateInfraReady only passes if the old object's infra is not ready and the updated object's is, and that likely won't correspond to the same update when a Cluster is paused/unpaused, so we need Any to check if any of those things happened in a particular update and ignore all other ones.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2023
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

Pending squash of commits.

@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: 1826820a17dd7df50db4e60021f79eb3f2e9dd91

@nojnhuh nojnhuh force-pushed the aso-pause-azuremanagedcontrolplane branch from 2a939cc to 150f44f Compare August 4, 2023 15:24
@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
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.

/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 4, 2023
@k8s-ci-robot k8s-ci-robot merged commit 3b81e27 into kubernetes-sigs:main Aug 4, 2023
21 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.11 milestone Aug 4, 2023
@nojnhuh nojnhuh deleted the aso-pause-azuremanagedcontrolplane branch August 7, 2023 19:27
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.

4 participants