-
Notifications
You must be signed in to change notification settings - Fork 1.4k
📖 docs: machinepool contract spec #12971
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
base: main
Are you sure you want to change the base?
📖 docs: machinepool contract spec #12971
Conversation
d1d60f8 to
271c8df
Compare
271c8df to
5cc7517
Compare
This adds documentation that details the contract for providers when implementing an infrastructure machine pool. This has been created retrospectively from looking at a number of providers and the MachinePool controller. Signed-off-by: Richard Case <[email protected]>
5cc7517 to
53b1241
Compare
fabriziopandini
left a comment
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.
Thanks for this PR!
This really helps folks like me to step up knowledge about MachinePools!
docs/book/src/developer/providers/contracts/infra-machinepool.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/providers/contracts/infra-machinepool.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/providers/contracts/infra-machinepool.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/providers/contracts/infra-machinepool.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/providers/contracts/infra-machinepool.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/providers/contracts/infra-machinepool.md
Outdated
Show resolved
Hide resolved
|
|
||
| The value from this field is surfaced via the MachinePool's `status.replicas` field. | ||
|
|
||
| ### InfraMachinePool: terminal failures |
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 think we should align ASAP to other CAPI resources WRT to terminal failures (deprecated, not anymore relevant for the controller)
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 agree and slightly changed the wording.
docs/book/src/developer/providers/contracts/infra-machinepool.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/providers/contracts/infra-machinepool.md
Outdated
Show resolved
Hide resolved
|
|
||
| By opting into MachinePool Machines its the responsibility of the provider to create an instance of a InfraMachine for every replica and manage their lifecycle. | ||
|
|
||
| ### InfraMachinePool: instances |
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 add a definition to what an instance is in the context of MP
Also, if this field is not used by CAPI let's make it clear.
e.g.
Please note that this field is not used by CAPI. Nevertheless, it is documented in this contract to foster design choice that will ensure a consistent user experience across all the MachinePool implementations,
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.
Updated this section.
|
/assign Would like to review after Fabrizio lgtm and before merge |
|
Thanks for your feedback @fabriziopandini . I will make updates based on this. |
|
[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 |
Changes after the first review by Fabrizio. Signed-off-by: Richard Case <[email protected]>
cbf9bc7 to
22f729e
Compare
|
@fabriziopandini - i have updated the doc based on your feedback. |
docs/book/src/developer/providers/contracts/infra-machinepool.md
Outdated
Show resolved
Hide resolved
|
|
||
| The goal of an InfraMachinePool is to manage the lifecycle of a provider-specific pool of machines using a provider specific service (like auto-scale groups in AWS & virtual machine scalesets in Azure). | ||
|
|
||
| The machines in the pool may be physical or virtual instances (although most likely virtual), and they represent the infrastructure for Kubernetes nodes. |
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.
| The machines in the pool may be physical or virtual instances (although most likely virtual), and they represent the infrastructure for Kubernetes nodes. | |
| The machines in the pool may be physical or virtual instances, and they represent the infrastructure for Kubernetes nodes. |
(comment is not relevant for the contract)
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.
Theres a few lines in this document that are not directly relevant to the contract, but which instead provide background or hints. So i'm inclined to keep this as is.
|
|
||
| The [MachinePool's controller](../../core/controllers/machine-pool.md) is responsible to coordinate operations of the InfraMachinePool, and the interaction between the MachinePool's controller and the InfraMachinePool is based on the contract rules defined in this page. | ||
|
|
||
| Once contract rules are satisfied by an InfraMachinePool implementation, other implementation details |
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.
What does this sentence mean? Can it be removed? Should we explicitly mention optional features such as single machine deletion here?
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.
This sentence needs to stay as it's stating that the MachinePool controller coordinates with the InfraMachinePool and that this interaction is governed by the contract.
Its also consistent with the contract docs for InfraMachine: https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/book/src/developer/providers/contracts/infra-machine.md?plain=1#L10
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.
Should we explicitly mention optional features such as single machine deletion here?
Is there something that is exposed via the contract that is relevant to this feature? There are general features of MachinePools that are not covered here (and should be covered in the general MachinePool docs IMHO).
docs/book/src/developer/providers/contracts/infra-machinepool.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/providers/contracts/infra-machinepool.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/providers/contracts/infra-machinepool.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/providers/contracts/infra-machinepool.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/providers/contracts/infra-machinepool.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/providers/contracts/infra-machinepool.md
Outdated
Show resolved
Hide resolved
|
@richardcase are we not going to document anything on the upgrades? My understanding is that the status would change based on the update strategy. For example: Atomic updates:
Rolling updates:
Should providers declare their update strategy in the contract so CAPI can adapt behavior accordingly? |
Changes after the first review by Fabrizio. Signed-off-by: Richard Case <[email protected]>
Some updates after an additional review by Andreas. Signed-off-by: Richard Case <[email protected]>
@bnallapeta - i think we should follow up on this as we are trying to document the current state of the contract. Providers declaring an "update strategy" would be a change to the current contract. We are going to need to update the MachinePool controller document based on the introduction of this contract doc. Perhaps we should include behaviour type stuff when we do that? |
|
@richardcase quoting from #10496,
I think we should talk about this in the contract. A few questions on this: Should providers watch for bootstrap config changes and trigger updates? If yes, what's the signal? Hash of the secret data? ConfigRef version? |
|
Note for the maintainers, i will squash the commits when we are all happy with the doc. |
I agree i do think we need to document this in the MachinPools documentation. However, i don't think this sits in the contract document. Lets add this elsewhere, perhaps where i suggested earlier: https://cluster-api.sigs.k8s.io/developer/core/controllers/machine-pool when we make changes to that. |
no need to /label tide/merge-method-squash |
What this PR does / why we need it:
This adds documentation that details the contract for providers when implementing an infrastructure machine pool.
This has been created retrospectively from looking at a number of providers and the MachinePool controller.
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 #12799
/area machinepool