Address comments

kubernetes-sigs · Sep 26, 2024 · 50b0de4 · 50b0de4
1 parent 3a7e93b
commit 50b0de4
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 53 deletions.
diff --git a/docs/book/src/developer/core/controllers/cluster.md b/docs/book/src/developer/core/controllers/cluster.md
@@ -5,7 +5,7 @@ The Cluster controller is responsible for reconciling the Cluster resource.
 In order to allow Cluster provisioning on different type of infrastructure, The Cluster resource references
 an InfraCluster object, e.g. AWSCluster, GCPCluster etc.
 
-The [InfraCluster resource contract](../../providers/contracts/infra-cluster.md) defines a set of rules a provider is expected to comply in order to allow
+The [InfraCluster resource contract](../../providers/contracts/infra-cluster.md) defines a set of rules a provider is expected to comply with in order to allow
 the expected interactions with the Cluster controller. 
 
 Among those rules:
@@ -18,7 +18,7 @@ Among those rules:
 Similarly, in order to support different solutions for control plane management, The Cluster resource references
 an ControlPlane object, e.g. KubeadmControlPlane, EKSControlPlane etc.
 
-The [ControlPlane resource contract](../../providers/contracts/control-plane.md) defines a set of rules a provider is expected to comply in order to allow
+The [ControlPlane resource contract](../../providers/contracts/control-plane.md) defines a set of rules a provider is expected to comply with in order to allow
 the expected interactions with the Cluster controller.
 
 Considering all the info above, the Cluster controller's main responsibilities are:

diff --git a/docs/book/src/developer/core/controllers/machine.md b/docs/book/src/developer/core/controllers/machine.md
@@ -3,23 +3,23 @@
 The Machine controller is responsible for reconciling the Machine resource.
 
 In order to allow Machine provisioning on different type of infrastructure, The Machine resource references
-an Machine object, e.g. AWSMachine, GCMachine etc.
+an InfraMachine object, e.g. AWSMachine, GCMachine etc.
 
-The [InfraMachine resource contract](../../providers/contracts/infra-machine.md) defines a set of rules a provider is expected to comply in order to allow
+The [InfraMachine resource contract](../../providers/contracts/infra-machine.md) defines a set of rules a provider is expected to comply with in order to allow
 the expected interactions with the Machine controller.
 
 Among those rules:
 - InfraMachine MUST report a [provider ID](../../providers/contracts/infra-machine.md#inframachine-provider-id) for the Machine
-- InfraMachine SHOULD define a [failure domain](../../providers/contracts/infra-machine.md#inframachine-failure-domain) where machines should be placed in
+- InfraMachine SHOULD take into account the [failure domain](../../providers/contracts/infra-machine.md#inframachine-failure-domain) where machines should be placed in
 - InfraMachine SHOULD surface machine's [addresses](../../providers/contracts/infra-machine.md#inframachine-addresses) to help operators when troubleshooting issues
-- InfraMachine MUST report when Cluster's infrastructure is [fully provisioned](../../providers/contracts/infra-machine.md#inframachine-initialization-completed)
+- InfraMachine MUST report when Machine's's infrastructure is [fully provisioned](../../providers/contracts/infra-machine.md#inframachine-initialization-completed)
 - InfraMachine SHOULD report [conditions](../../providers/contracts/infra-machine.md#inframachine-conditions)
 - InfraMachine SHOULD report [terminal failures](../../providers/contracts/infra-machine.md#inframachine-terminal-failures)
 
 Similarly, in order to support different machine bootstrappers, The Machine resource references
-an BootstrapConfig object, e.g. KubeadmBoostrapConfig etc.
+a BootstrapConfig object, e.g. KubeadmBoostrapConfig etc.
 
-The [BootstrapConfig resource contract](../../providers/contracts/bootstrap-config.md) defines a set of rules a provider is expected to comply in order to allow
+The [BootstrapConfig resource contract](../../providers/contracts/bootstrap-config.md) defines a set of rules a provider is expected to comply with in order to allow
 the expected interactions with the Machine controller.
 
 Considering all the info above, the Machine controller's main responsibilities are:
@@ -31,7 +31,7 @@ Considering all the info above, the Machine controller's main responsibilities a
   * Setting NodeRefs to be able to associate machines and Kubernetes nodes.
   * Monitor Kubernetes nodes and propagate labels to them.
 * Cleanup of all owned objects so that nothing is dangling after deletion.
-  * Drain nodes and wait for volume being detached by the CSI provider.
+  * Drain nodes and wait for volumes being detached by CSI plugins.
 
 ![](../../../images/cluster-admission-machine-controller.png)
 

diff --git a/docs/book/src/developer/providers/contracts/infra-machine.md b/docs/book/src/developer/providers/contracts/infra-machine.md
@@ -2,7 +2,7 @@
 
 Infrastructure providers SHOULD implement an InfraMachine resource.
 
-The goal of an InfraMachine resource is to manage the lifecycle of provider-specific machine instances.
+The goal of an InfraMachine resource is to manage the lifecycle of a provider-specific machine instances.
 These may be physical or virtual instances, and they represent the infrastructure for Kubernetes nodes.
 
 The InfraMachine resource will be referenced by one of the Cluster API core resources, Machine.
@@ -43,22 +43,22 @@ repo or add an item to the agenda in the [Cluster API community meeting](https:/
 
 ## Rules (contract version v1beta1)
 
-| Rule                                                                | Mandatory | Note                                  |
-|---------------------------------------------------------------------|-----------|---------------------------------------|
-| [All resources: scope]                                              | Yes       |                                       |
-| [All resources: `TypeMeta` and `ObjectMeta`field]                   | Yes       |                                       |
-| [All resources: `APIVersion` field value]                           | Yes       |                                       |
-| [InfraMachine, InfraMachineList resource definition]                | Yes       |                                       |
-| [InfraMachine: provider ID]                                         | Yes       |                                       |
-| [InfraMachine: failure domain]                                      | No        |                                       |
-| [InfraMachine: addresses]                                           | No        |                                       |
-| [InfraMachine: initialization completed]                            | Yes       |                                       |
-| [InfraMachine: conditions]                                          | No        |                                       |
-| [InfraMachine: terminal failures]                                   | No        |                                       |
-| [InfraMachineTemplate, InfraMachineTemplateList resource definition] | No        | Mandatory for ClusterClasses support  |
-| [InfraMachineTemplate: support for SSA dry run]                     | No        | Mandatory for ClusterClasses support  |
-| [Multi tenancy]                                                     | No        | Mandatory for clusterctl CLI support  |
-| [Clusterctl support]                                                | No        | Mandatory for clusterctl CLI support  |
+| Rule                                                                 | Mandatory | Note                                 |
+|----------------------------------------------------------------------|-----------|--------------------------------------|
+| [All resources: scope]                                               | Yes       |                                      |
+| [All resources: `TypeMeta` and `ObjectMeta`field]                    | Yes       |                                      |
+| [All resources: `APIVersion` field value]                            | Yes       |                                      |
+| [InfraMachine, InfraMachineList resource definition]                 | Yes       |                                      |
+| [InfraMachine: provider ID]                                          | Yes       |                                      |
+| [InfraMachine: failure domain]                                       | No        |                                      |
+| [InfraMachine: addresses]                                            | No        |                                      |
+| [InfraMachine: initialization completed]                             | Yes       |                                      |
+| [InfraMachine: conditions]                                           | No        |                                      |
+| [InfraMachine: terminal failures]                                    | No        |                                      |
+| [InfraMachineTemplate, InfraMachineTemplateList resource definition] | Yes       |                                      |
+| [InfraMachineTemplate: support for SSA dry run]                      | No        | Mandatory for ClusterClasses support |
+| [Multi tenancy]                                                      | No        | Mandatory for clusterctl CLI support |
+| [Clusterctl support]                                                 | No        | Mandatory for clusterctl CLI support |
 
 Note:
 - `All resources` refers to all the provider's resources "core" Cluster API interacts with;
@@ -100,6 +100,7 @@ rules:
     - infrastructure.foo.com
   resources:
     - foomachines
+    - foomachinetemplates
   verbs:
     - create
     - delete
@@ -108,21 +109,10 @@ rules:
     - patch
     - update
     - watch
-- apiGroups:
-    - infrastructure.foo.com
-  resources:
-    - foomachinetemplates
-  verbs:
-    - get
-    - list
-    - patch
-    - update
-    - watch
 ```
 
-Note: The write permissions allow the Machine controller to set owner references and labels on the InfraMachine resources;
-write permissions are not used for general mutations of InfraMachine resources, unless specifically required (e.g. when
-using ClusterClass and managed topologies).
+Note: The write permissions are required because Cluster API manages InfraMachines generated from InfraMachineTemplates;
+when using ClusterClass and managed topologies, also InfraMachineTemplates are managed directly by Cluster API.
 
 #### All resources: version
 
@@ -163,8 +153,8 @@ The InfraMachine resource name must have the format produced by `sigs.k8s.io/clu
 Note: Cluster API is using such a naming convention to avoid an expensive CRD lookup operation when looking for labels from
 the CRD definition of the InfraMachine resource.
 
-It is a generally applied convention to use names in the format `${env}Cluster`, where ${env} is a, possibly short, name
-for the environment in question. For example `GCPCluster` is an implementation for the Google Cloud Platform, and `AWSCluster`
+It is a generally applied convention to use names in the format `${env}Machine`, where ${env} is a, possibly short, name
+for the environment in question. For example `GCPMachine` is an implementation for the Google Cloud Platform, and `AWSMachine`
 is one for Amazon Web Services.
 
 ```go
@@ -227,26 +217,43 @@ type FooMachineSpec struct {
 Once `spec.providerID` is set on the InfraMachine resource and the [InfraMachine initialization completed],
 the Cluster controller will bubble up this info in Machine's `spec.providerID`.
 
-If instead you are developing an infrastructure provider which is NOT responsible to provide a control plane endpoint,
-the implementer should exit reconciliation until it sees Cluster's `spec.controlPlaneEndpoint` populated.
-
 ### InfraMachine: failure domain
 
 In case you are developing an infrastructure provider which has a notion of failure domains where machines should be
-placed in, the InfraMachine resource MUST have a `spec.failureDomain` field to allow users to express the placement decision
-for the corresponding infrastructure.
+placed in, the InfraMachine resource MUST comply to the value that exists in the `spec.failureDomain` field of the Machine
+(in other words, the InfraMachine MUST be placed in the failure domain specified at Machine level).
+
+Please note, that for allowing transition from the failure domain model in use before CAPI 1.3.0, 
+Cluster API still supports a _deprecated_ reverse process, where the InfraMachine is authoritative WRT to 
+to the failure domain placement (instead of the the Machine).
+
+In the _deprecated_ reverse process, the the failure domain where the machine is actually placed is defined in the InfraMachine's 
+`spec.failureDomain` field; the value of this field is then surfaced on the corresponding field at Machine level.
+
+<aside class="note warning">
+
+<h1>Heads up! this will change with the v1beta2 contract</h1>
+
+In order to avoid the confusion of the two-way contract that exists for historical reasons, Machine's controller
+will stop reading InfraMachine's `spec.failureDomain` from machines.
+
+However, InfraMachine will be allowed to surface the failure domain where the machine is actually placed in by 
+implementing a new, optional `status.failureDomain`; this info will then surface at Machine level in a 
+new corresponding field (also in status).
 
 ```go
-type FooMachineSpec struct {
-    // FailureDomain is the failure domain unique identifier this Machine should be placed in.
-    // For this Foo infrastructure provider, the name is equivalent to the name of one of the available the regions.
+type FooMachineStatus struct {
+    // FailureDomain is the unique identifier of the failure domain where this Machine has been be placed in.
+    // For this Foo infrastructure provider, the name is equivalent to the name of one of the available regions.
     FailureDomain *string `json:"failureDomain,omitempty"`
 
-    // See other rules for more details about mandatory/optional fields in InfraMachineSpec.
+    // See other rules for more details about mandatory/optional fields in InfraMachineStatus.
     // Other fields SHOULD be added based on the needs of your provider.
 }
 ```
 
+</aside>
+
 ### InfraMachine: addresses
 
 Infrastructure provider have the opportunity to surface machines addresses on the InfraMachine resource; this information
@@ -256,7 +263,7 @@ In case you want to surface machine's addresses, you MUST surface them in `statu
 
 ```go
 type FooMachineStatus struct {
-    // Addresses contains the associated addresses for the docker machine.
+    // Addresses contains the associated addresses for the machine.
     // +optional
     Addresses []clusterv1.MachineAddress `json:"addresses,omitempty"`
 
@@ -388,7 +395,9 @@ See [Improving status in CAPI resources].
 
 ### InfraMachineTemplate, InfraMachineTemplateList resource definition
 
-For a given InfraMachine resource, you should also add a corresponding InfraMachineTemplate resources in order to use it in ClusterClasses.
+For a given InfraMachine resource, you MUST also add a corresponding InfraMachineTemplate resources in order to use it
+when defining set of machines, e.g. MachineDeployments.
+
 The template resource MUST be named as `<InfraMachine>Template`.
 
 ```go
@@ -418,7 +427,7 @@ type FooMachineTemplateResource struct {
 ```
 
 NOTE: in this example InfraMachineTemplate's `spec.template.spec` embeds `FooMachineSpec` from InfraMachine. This might not always be
-the best choice depending of if/how InfraMachine's spec fields applies to many clusters vs only one.
+the best choice depending of if/how InfraMachine's spec fields applies to many machines vs only one.
 
 For each InfraMachineTemplate resource, you MUST also add the corresponding list resource.
 The list resource MUST be named as `<InfraMachineTemplate>List`.