-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
📖 Update provider contract to account for the paused condition #10519
base: main
Are you sure you want to change the base?
📖 Update provider contract to account for the paused condition #10519
Conversation
Hi @theobarberbany. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/area documentation |
285b232
to
2ff8fbd
Compare
docs/book/src/developer/architecture/controllers/control-plane.md
Outdated
Show resolved
Hide resolved
/ok-to-test |
docs/book/src/developer/architecture/controllers/control-plane.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/architecture/controllers/control-plane.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/architecture/controllers/control-plane.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/architecture/controllers/control-plane.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/architecture/controllers/control-plane.md
Outdated
Show resolved
Hide resolved
PR needs rebase. 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-sigs/prow repository. |
2ff8fbd
to
24a20d0
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
24a20d0
to
09bfcdb
Compare
7b5e290
to
45d23ac
Compare
return ctrl.Result{}, nil | ||
} | ||
|
||
conditions.MarkFalse(m, clusterv1.PausedCondition, clusterv1.ResourceNotPausedReason, clusterv1.ConditionSeverityInfo, "Resource is operating as expected") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I want a FalseConditionWithNegativePolarity
here? Does a resource not being paused, therefore reconciling now count as a positive polarity event?
Although, then the helper does not accept a reason or severity.
Maybe This would be more straightforward with just conditions.Set()
, I think I'm agreeing more with the paused condition not being polar.
cc @sbueringer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say let's use conditions.Set()
Let's only provide severity + reason + message if the object is paused. Goal would be to precisely write in the message why the object is paused (e.g. because the Cluster is paused, or the object itself is paused, but we can finalize wording of that message on the implementation PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok - I've updated the PR to do this! :)
I'm not sure if it makes sense to have reason + message, where reason can be the cluster is paused, or that the annotation is present. What extra would we gain from adding a message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are multiple reasons why a resource could be paused. One example: A Machine can be paused because the Machine itself is paused or because the Cluster is paused.
I would like to surface all the reasons why something is paused so folks know what to change to unpause (and also after which "unpause" the reconciliation continues)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yep, I understand and agree with your motivation there! I just imagined those having different reasons:
// ClusterPausedReason (Severity=Info) documents a CAPI object that is paused due to the cluster being paused (.Spec.Paused).
ClusterPausedReason = "ClusterPaused"
// AnnotationPausedReason (Severity=Info) documents a CAPI object that is paused due to the paused annotation being present.
AnnotationPausedReason = "PausedAnnotationSet"
// MachineDeploymentPausedReason (Severity=Info) documents a CAPI object that is paused due to the machine deployment being paused.
MachineDeploymentPausedReason = "MachineDeploymentPaused"
In this instance I'm not sure what extra useful information we might add in a message? E.g in the scenario where a cluster is paused, adding the name of the cluster? Name of the annotation?
Or would a single reason make more sense, with more information in the message about the specifics of the reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry that was confusing :D. I would use one Reason constant and then just describe the details in the message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh ok - makes sense! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think the latest changes should have short reason + more descriptive message!
e2cf9ad
to
2d6f01f
Compare
4699f72
to
fc90091
Compare
This change updates the provider contract to account for a new paused condition. It is intended to start as an optional condition, but then become required at a later date.
fc90091
to
1604605
Compare
This change updates verify-capi-book-summary.sh to exclude 'code-examples.md' from the summary.
6cb0da9
to
3e9e664
Compare
/lgtm |
LGTM label has been added. Git tree hash: c3e7800d827ef15b2487f3577d3597fbaddee929
|
Bumping this for a review cc @sbueringer :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL #10897 when I'm defining the transition from CAPI custom conditions to K8s conditions (which includes also paused).
Based on the first round of positive feedback, this transition is going to happen in the CAPI 1.9 timeframe, so we should make a call if to implement this or defer.
@@ -264,6 +264,10 @@ The `status` object **may** define several fields: | |||
* `externalManagedControlPlane` - is a bool that should be set to true if the Node objects do not | |||
exist in the cluster. For example, managed control plane providers for AKS, EKS, GKE, etc, should | |||
set this to `true`. Leaving the field undefined is equivalent to setting the value to `false`. | |||
* `.Conditions[Paused]` - is a condition that reports if the cluster or control plane resource is paused. It should check if 'spec.paused' is set on the cluster, and for the paused annotation on the resource. This is currently optional, but will become required in future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
* `.Conditions[Paused]` - is a condition that reports if the cluster or control plane resource is paused. It should check if 'spec.paused' is set on the cluster, and for the paused annotation on the resource. This is currently optional, but will become required in future. | |
* `.Conditions[Paused]` - is a condition that reports if the cluster or control plane resource is paused. It should check if 'spec.paused' is set on the cluster, and for the paused annotation on the resource. Implementing this condition is currently optional, but we might revisit this in the future. |
(same on other files)
@@ -264,6 +264,10 @@ The `status` object **may** define several fields: | |||
* `externalManagedControlPlane` - is a bool that should be set to true if the Node objects do not | |||
exist in the cluster. For example, managed control plane providers for AKS, EKS, GKE, etc, should | |||
set this to `true`. Leaving the field undefined is equivalent to setting the value to `false`. | |||
* `.Conditions[Paused]` - is a condition that reports if the cluster or control plane resource is paused. It should check if 'spec.paused' is set on the cluster, and for the paused annotation on the resource. This is currently optional, but will become required in future. | |||
```go | |||
{{#include ../../providers/code-examples.md:2:28}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to not add a code example now, given that most probably we will implement a util to standardize implementation both in CAPI and across providers
(we can add the example in a follow up)
What this PR does / why we need it:
This change updates the provider contract to account for a new paused
condition.
It is intended to start as an optional condition, but then become
required at a later date. See issue for more context.
Relates #10130
/area documentation