Hopefully, this will be my least interesting comment: it is easier to review markdown when an ~ 80 character line limit is observed. This makes it easier for the reviewer to point to the specific text they are commenting on. Cf. https://sembr.org/, https://rhodesmill.org/brandon/2012/one-sentence-per-line/

If Bootstrap were a CRD and not just a type then this field could copy the ProviderSpec pattern:

Isn't the Bootstrap.ConfigRef going to point to a provider-specific CR like you describe?

Maybe? When I saw ConfigRef I assumed it pointed to a ConfigMap. Maybe it could use a better name...

I agree. ProviderDataRef?

I do like the idea of finding a better name, but I would like to avoid ProviderDataRef, since I think it could cause confusion based on having only a singular "Provider" in v1alpha1.

I also found this separate structure confusing at first. I don't see the rationale and as @dhellmann says, it is not consistent with other references. I second to simplify the model in this sense, unless the bootstrap structure has further motivation.

The thought process was around yaml/json UX. Option 1:

spec:
  bootstrap:
    data: "xyz"
    configRef:
      apiVersion: foo/v1
      kind: Foo
      name: foo123
  ...

Option 2

spec:
  bootstrapData: "xyz"
  bootstrapConfigRef:
      apiVersion: foo/v1
      kind: Foo
      name: foo123
  ...

With Option 1, we're grouping multiple things related to bootstrapping together. With Option 2, you have to write "bootstrap" multiple times.

If everyone is largely in agreement with the concept of a reference to a custom resource for bootstrap configuration, and a field to hold the bootstrap data, could we do the following?

1. Indicate that you agree w/the concept
2. Continue to debate the name and structure, BUT
3. After all other comments/issues/concerns are addressed, let the proposal merge in whatever state these bootstrap fields happen to be in, and we can continue to have the naming/structure discussion, if needed, after the proposal merges

Per k8s API Naming Conventions:

The name of a field referring to another resource of kind Foo by name should be called fooName. The name of a field referring to another resource of kind Foo by ObjectReference (or subset thereof) should be called fooRef.

Therefore I think it should be bootstrapConfigRef and option 2 (to avoid stuttering).

I prefer option 1, better grouping of related fields/data. @davidewatson This kinda falls outside of that naming convention (although I understand why you make the point) because there is no BootstrapConfig kind here, but rather provider-specific CRDs, called arbitrary names.

That's fair.

Isn't the Bootstrap.ConfigRef going to point to a provider-specific CR like you describe?

At one point we discussed removing the single "phase" field in favor of having a separate status field for each aspect we expect to be managed by a separate controller (see BootstrapPhase and InfrastructurePhase below). If we still want to do that, I can make those edits in a follow-up PR.

You are right, top-level fields or conditions were discussed. Personally, I would like to consider the "outlook" model. History has proven this could be a long conversation though. Bias for action, at least while we are still in alpha... :)

I second @dhellmann proposal. Having conditions is more flexible and considering we are moving to implement v1alpha2 as a mean of learning, leaving room for extensions without force breaking changes is an interesting strategy.

If we have individual "statuses" that we can represent as separate boolean values, +1 to that. I am opposed to an arbitrary array of strings for conditions, however.

I'd definitely like to see a better approach to a string-based-Phase as a follow-up CAEP 👍

@davidewatson I also like the outlook model here, would be interested in chatting about this for potential follow up.

Implementation question: Will the machine controller do that or do we expect each provider implementation to do it?

Ideally we should be doing this in a centralized manner, especially since the underlying "infrastructure" provider here would not really have knowledge of Kubernetes itself as it does today.

This needs to be the responsibility of a singular cluster-api component, presumably the machine controller.

OK. There's also a NodeMaintenance operator that the KubeVirt team might be willing to bring further upstream. https://github.com/kubevirt/node-maintenance-operator

And as it happens, that is already in progress: kubernetes/enhancements#1080

As an end user, I want symmetry: If CAPI joins the node, I expect CAPI to gracefully remove that node, too.

At the KubeCon EU Contributors' Summit, I asked @lavalamp about the idea of a "server-side drain," and, from what I recall, he thought applying taints would have the desired effect.

I think we decided that the CAPI control plane has access to the workload cluster API. The Machine controller would have to (1) identify the node (it should be able to do that from the Machine object) and (2) apply Taint(s) to that node.

s/Some generic controllers like MachineDeployment and Machineset live in the cluster-api repo, while some provider specific controllers like Cluster and Machine , live in the provider specific repo.

Deployment of controllers depends on choice. For example, in OpenShift, all controllers are deployed in single pod. Vendoring of cluster-api is cumbersome though.

I'm not sure if this actually has to change. The motivation is meant to define the current state of the world for kubernetes-sigs repositories and code organization. While we're aware that other implementation exist outside, the main focus is to provide a holistic view limited the subproject's repositories.

I don't see direct implication of the second motivation point wrt. Machine States & Preboot Bootstrapping title. Though I understand it's important to provide layout and manifests that reflect structure of the new to-be-done cluster-api plane and relation between individual components. And also that's up to each consumer to decide if all the components of the cluster-api plane will live inside the same pod (or, will be distributed among multiple pods/deployments). Given the fact, implication of the second motivation item transcends beyond cluster-api, making what the right view might look like on cluster-api repository not easily predictable and questionable. Still, it's important to provide some default view.

As I read the second item, it does not sufficiently express its intention in the scope of the proposal. Does the first part reflect the following rephrasing?

Both users and developers are confused by the code organization split where generic controllers (MachineDeployment, MachineSet) live fully inside cluster-api repository while other (Cluster, Machine) are partially implemented in cluster-api repository but vendored in provider repositories that implement provider specific bits. Thus, giving existence to separate pods which both deploy all controllers instead of a single one.

If so, can we be more specific in what We would like for the code location to correspond to the pod, which hopefully will reduce both user and developer confusion. actually means? Is it to create completely new layout where machine controller will become fully general without injecting actuator implementation? And, actuators will be turned into separate controllers with their own life-cycle where a relation between machine life-cycle and actuator will be provided? Or, machine controllers will still be fully vendored in provider repositories as it is today and the intention is to just change currently provided manifests in the cluster-api repository?

EDIT: Asking the question in the context mentioned above so the motivation is clear enough without reading the rest of the proposal.

@ingvagabund @vikaschoudhary16 I think this point is just poorly phrased and I'll need to rework it a little later or tomorrow. Thanks for the great feedback. I read it again multiple times and I think it's unclear what we initially wanted to communicate.

How does that sound?

I understand the intention of this to be that a clearer separation of concerns between infrastructure and bootstrap providers will be reflected in code organisation and thus easier for developers to grok. It's the pod bit here that confuses me: that is a deployment issue, rather than a code organisation issue.

Sounds good to me. Thank you!!!

To be honest I thought the wording made sense, could be refined. However I want to make certain we are not conflating ideas.

Deployment of controllers depends on choice. For example, in OpenShift, all controllers are deployed in single pod. Vendoring of cluster-api is cumbersome though.

Is orthogonal.

Reworded, ptal

Is this looking for the phases from the above diagrams (Provisioning, Provisioned, Running, etc) or something more detailed? For example, a more complex on-prem environment might have separate steps for requisitioning a machine, bringing up the machine, installing a base, etc that are understandable and relevant to the operator.

For the first iteration, this is expressed by the Phase field as you mentioned. In the future, we might want to allow for more details to be bubbled up in the Status.

What business/usage reasons cause CRUD to be a restriction?

I'll defer to @dhellmann which contributed the above user story

Bare metal hosts can't be created or deleted, so we've had to simulate doing those in those particular methods of our current actuator. "Creating" a bare metal host involves a provisioning operation that may take as long as 20 minutes. Ideally we would have some way to continue to report status to the Machine during that time, which is easier if we have a reconciliation loop than if we have to fit those updates into the CRUD API. "Deleting" the host also takes many minutes while the storage is wiped.

Figures 3, 4, 5, 6, 7, and 8 all have the same title (with different numbers). This is confusing since they have different content. Maybe the titles could be changed to highlight the focus of the individual diagrams?

Can we document more examples of what Bootstrap controllers might look like? I'm concerned this is too theoretical right now and we don't fully understand the needs to know if it'll satisfy our goals.

For example, the KubeadmBootstrap approach is somehow generating an output (user data / cloud-init), however as a community we have not yet established support for a) if OS images are expected to support a single input/user-data format, b) if these bootstrap controllers are supposed to know who/what they're generating the data for (to influence its output format), c) if providers are going to end up needing to transform these bootstrap-specific outputs.

Also, it's not clear to me how bootstrapping (or with KubeadmBootstrap) would be dealing with the concerns around installing a "specific Kubernetes distribution". Is that going to be a part of the kubeadm bootstrap spec? Is that some other undocumented convention which is expected to be shared across bootstrap implementers?

My understanding is that having the logic of the boostrap configuration separated into its own controller(s) avoids the need to fix which user data format is expected. Each infrastructure provider will support its own format(s) and the different controllers (ideally independent of the infrastructure) will provide the boostrap data in an specific format, which could be used in different infrastructures. This is the approach used, for instance in the Gardener project for handling bootstrap.

I'm going to be honest... Almost 0 specs strictly adhere to their original design due to discovery that occurs during implementation. The goal here is to enumerate a limited set of options, pick one, go and iterate.

I fully expect we will make mistakes, and that is ok, we will refine over time. So long as we've done a reasonable level of diligence.

In figure 3, the first three "opt" boxes that overlap the blue column all have the same condition ("[Required only if the object hasn't been seen before]"). I think all three of them could be a single box, since if one of them happens, then all of them happen.

Similarly, the two opt boxes that overlap both columns share a single condition, and could also be combined into a single box.

Having one reference assumes there's only one bootstrap provider involved. However, consider the following actual use case:

• One controller provides the basic OS configuration (e.g. cloud-init)
• Another controller provides patches for the OS configuration depending on, for example, the machine class, to accommodate some specific workload (e.g. kernel settings).

These two configuration could be made by the same controller, but separating these two concerns makes more sense as the generic OS configuration could be used in multiple scenarios and providers.

Therefore, the ConfigrRef could be a list of named references (a map) so that each controller knows which reference(s) to set/access.

The need for multiple references could be extended to other stages, like providing infrastructure configuration before actual provisioning of the machine.

I don't think this proposal prevents that possibility. I had imagined that being a bootstrap provider problem, something like:

kind: Machine
...
  configRef:
    apiVersion: mergable.something/v1
    kind: MergableBootstrapConfig
    name: my-config
---
apiVersion: mergable.something/v1
kind: MergableBootstrapConfig
spec:
  my-base-config:
      apiVersion: kubeadm.whatever/v1
      kind: KubeadmBootstrapConfig
      name: base-config
  my-workload-tuning-config:
      apiVersion: kernel.config/v1
      kind: SomeCustomKernelConfig
      name: workload-tuning

with the implementation of whatever controller reads the MergableBootstrapConfig resources defining the merge semantics and preserving the contract with the consumer of the .Data.

Yes, this is possible if ONE provider manage that, but not if two independent providers must collaborate.

I don't follow what you mean. My example had three bootstrap providers: Kubeadm, SomeCustomKernel, and a third "merging" provider whose responsibility it is to broker the interaction between the other two. Wouldn't Kubeadm and SomeCustomKernel count as independent?

My point is that all these three controllers must agree on working on a common CRD which is not specified as part or CAPI, so there's some dependency between them.

Another controller provides patches for the OS configuration depending on, for example, the machine class, to accommodate some specific workload (e.g. kernel settings).

That's a pretty wild scenario, if you are doing this I'd recommend against it. The recommended approach would be to pre-bake your images and simplify your bootstrap sequence.

Another controller provides patches for the OS configuration depending on, for example, the machine class, to accommodate some specific workload (e.g. kernel settings).

That's a pretty wild scenario, if you are doing this I'd recommend against it. The recommended approach would be to pre-bake your images and simplify your bootstrap sequence.

This is not a wild scenario. This is essentially what we do. Provisioning an instance has nothing to do with creating the artifacts necessary to provide to a cloud (such as user-data), nor does it have much to do with taking an action on that instance afterwards. Assuming the 'preboot' portion is the same controller as the 'postboot' portion is not a good fit.

Software provisioning is it's own fractal tool chain. We should allow extensions to enable, but it's never been a primary design of cluster api since epoch, and should be completely separate from cluster API.

I think I'd like to see the simple case working before we deal with the complexity of merging configurations in the way described here.

Software provisioning is it's own fractal tool chain. We should allow extensions to enable, but it's never been a primary design of cluster api since epoch, and should be completely separate from cluster API.

I understand that the value of providing this kind of extension mechanisms is precisely to allow customization to diverse use cases without having to deal with them in CAPI. The bootstrap controllers live outside CAPI.

What I understand CAPI should ensure, within reasonable boundaries of complexity, is to offer an extension mechanism with is flexible enough to handle diverse use cases. In this case, changing from one single element to a map of elements doesn't seams to be utterly complex.

I agree, however, with @dhellmann that we can iterate over the design.

However, the example we use for boostrap configuration is cloud-init which allows setting up OS configuration and installing required software, therefore making this distinction arbitrary.

True, but cloud-init is not a universal standard (e.g. we're considering building an ignition-driven bootstrap provider). I read that statement as saying it's not the Machine's responsibility to define what software need be installed, but instead that belongs to the interaction between the bootstrap provider and the thing that ultimately provisions the resources.

Maybe there's a misunderstanding here about who uses the bootstrap information. It is provided by the bootstrap controller and used by the provider specific infrastructure provisioner. Both of these elements live outside CAPI. Therefore, I don't see the point about been opinionated with respect of what the bootstrap data contains or is used.

Did you mean a "Custom Resource" (an object) or a "Custom Resource Definition" (a schema)?