Improve API consistency

[fabriziopandini]

Within the context of the v1beta2 effort (see #10852), we always had a generic goal to improve API consistency across objects in a few areas:

• Audit Machine-level fields across CC, Cluster.spec.topology, MD, MS, KCP and check if we can make it more consistent

• Audit all strategy fields, including rolloutStrategy / rolloutBefore / rolloutAfter Configurable machine replacement #10946 Improve API around machine deletion strategy #7021 (also future-proof for in-place & for Cluster.nodeDeletionStrategy) / deletionStrategy (?, e.g. timeout fields)

We now came up with a proposal for this effort, that can be looked up by comparing current and desired state

current state

Cluster:

spec:
  topology:
    classRef:
      name: ""
    controlPlane:
      machineHealthCheck:
        enable: false
        maxUnhealthy:
        nodeStartupTimeoutSeconds: 0
        remediationTemplate: {}
        unhealthyNodeConditions: []
        unhealthyRange: ""
      metadata: {}
      readinessGates: []
      nodeDeletionTimeoutSeconds: 0
      nodeDrainTimeoutSeconds: 0
      nodeVolumeDetachTimeoutSeconds: 0
    workers:
      machineDeployments:
      - name: ""
        class: ""
        failureDomain: ""
        metadata: {}
        machineHealthCheck:
          enable: false
          maxUnhealthy:
          nodeStartupTimeoutSeconds: 0
          remediationTemplate: {}
          unhealthyNodeConditions: []
          unhealthyRange: ""
        strategy:
          remediation:
            maxInFlight:
          rollingUpdate:
            deletePolicy: ""
            maxSurge:
            maxUnavailable:
          type: ""
        minReadySeconds: 0
        readinessGates: []
        nodeDeletionTimeoutSeconds: 0
        nodeDrainTimeoutSeconds: 0
        nodeVolumeDetachTimeoutSeconds: 0

ClusterClass:

spec:
  controlPlane:
    metadata: {}
    templateRef: {}
    namingStrategy:
      template: ""
    machineInfrastructure:
      templateRef: {}
    machineHealthCheck:
      maxUnhealthy:
      nodeStartupTimeoutSeconds: 0
      remediationTemplate: {}
      unhealthyNodeConditions: []
      unhealthyRange: ""
    readinessGates: []
    nodeDeletionTimeoutSeconds: 0
    nodeDrainTimeoutSeconds: 0
    nodeVolumeDetachTimeoutSeconds: 0
  infrastructure:
    namingStrategy:
      template: ""
    templateRef: {}
  workers:
    machineDeployments:
      class: ""
      failureDomain: ""
      metadata: {}
      namingStrategy:
        template: ""
      bootstrap:
        templateRef: {}
      infrastructure:
        templateRef: {}
      machineHealthCheck:
        maxUnhealthy:
        nodeStartupTimeoutSeconds: 0
        remediationTemplate: {}
        unhealthyNodeConditions: []
        unhealthyRange: ""
      strategy:
        remediation:
          maxInFlight:
        rollingUpdate:
          deletePolicy: ""
          maxSurge:
          maxUnavailable:
        type: ""
      minReadySeconds: 0
      readinessGates: []
      nodeDeletionTimeoutSeconds: 0
      nodeDrainTimeoutSeconds: 0
      nodeVolumeDetachTimeoutSeconds: 0

KubeadmControlPlane

spec:
  machineNamingStrategy:
    template: ""
  remediationStrategy:
    maxRetry: 0
    minHealthyPeriodSeconds: 0
    retryPeriodSeconds: 0
  rolloutAfter: ""
  rolloutBefore:
    certificatesExpiryDays: 0
  rolloutStrategy:
    type: ""
    rollingUpdate:
      maxSurge:
  machineTemplate:
    metadata: {}
    readinessGates: []
    nodeDeletionTimeoutSeconds: 0
    nodeDrainTimeoutSeconds: 0
    nodeVolumeDetachTimeoutSeconds: 0
    infrastructureRef: {}

MachineDeployment:

spec:
  machineNamingStrategy:
    template: ""
  rolloutAfter: ""
  strategy:
    remediation:
      maxInFlight:
    rollingUpdate:
      deletePolicy: ""
      maxSurge:
      maxUnavailable:
    type: ""
  template:
    metadata: {}
    spec:
      failureDomain: ""
      minReadySeconds: 0
      readinessGates: []
      nodeDeletionTimeoutSeconds: 0
      nodeDrainTimeoutSeconds: 0
      nodeVolumeDetachTimeoutSeconds: 0

MachineSet:

spec:
  machineNamingStrategy:
    template: ""
  deletePolicy: ""
  template:
    metadata: {}
    spec:
      failureDomain: ""
      minReadySeconds: 0
      readinessGates: []
      nodeDeletionTimeoutSeconds: 0
      nodeDrainTimeoutSeconds: 0
      nodeVolumeDetachTimeoutSeconds: 0

Machine:

spec:
  failureDomain: ""
  minReadySeconds: 0
  readinessGates: []

  nodeDeletionTimeoutSeconds: 0
  nodeDrainTimeoutSeconds: 0
  nodeVolumeDetachTimeoutSeconds: 0

MachineHealthCheck

spec:
  nodeStartupTimeoutSeconds: 0
  unhealthyNodeConditions: []
  maxUnhealthy:
  unhealthyRange: ""
  remediationTemplate: {}

desired state

Cluster:

spec:
  topology:
    classRef:
      name: ""
    controlPlane:
      healthCheck:
        enabled: false
        checks:
          nodeStartupTimeoutSeconds: 0
          unhealthyNodeConditions: []
        remediation:
          triggerIf:
            unhealthyLessThanOrEqualTo: 100%
            unhealthyInRange: "[1-4]"
          templateRef: {}
      metadata: {}
      readinessGates: []
      deletion:
        nodeDeletionTimeoutSeconds: 0
        nodeDrainTimeoutSeconds: 0
        nodeVolumeDetachTimeoutSeconds: 0
    workers:
      machineDeployments:
        - name: ""
          class: ""
          failureDomain: ""
          metadata: {}
          healthCheck:
            enabled: false
            checks:
              nodeStartupTimeoutSeconds: 0
              unhealthyNodeConditions: []
            remediation:
              triggerIf:
                unhealthyLessThanOrEqualTo: 100%
                unhealthyInRange: "[1-4]"
              templateRef: {}
              maxInFlight:
          rollout:
            strategy:
              type: ""
              rollingUpdate:
                maxSurge:
                maxUnavailable:
          minReadySeconds: 0
          readinessGates: []
          deletion:
            order: "" # could al so be policy (current: deletePolicy) Maybe order is not ideal because the correlation to Machine is not obvious
            nodeDeletionTimeoutSeconds: 0
            nodeDrainTimeoutSeconds: 0
            nodeVolumeDetachTimeoutSeconds: 0

Note:

• for now we decided to keep readinessGates and minReadySeconds top level (and do not create a group)

ClusterClass:

spec:
  controlPlane:
    metadata: {}
    templateRef: {}
    naming:
      template: "" # TODO: think about if there is an alternative to template + if there will be a nice way to add machineName template from KCP (same in other places in Cluster and CC)
    machineInfrastructure::
      templateRef: {}
    healthCheck:
      checks:
        nodeStartupTimeoutSeconds: 0
        unhealthyNodeConditions: []
      remediation:
        triggerIf:
          unhealthyLessThanOrEqualTo: 100%
          unhealthyInRange: "[1-4]"
        templateRef: {}    
    readinessGates: []
    deletion:
      nodeDeletionTimeoutSeconds: 0
      nodeDrainTimeoutSeconds: 0
      nodeVolumeDetachTimeoutSeconds: 0
  infrastructure:
    naming:
      template: ""
    templateRef: {}
  workers:
    machineDeployments:
      class: ""
      failureDomain: ""
      metadata: {}
      naming:
        template: ""
      bootstrap:
        templateRef: {}
      infrastructure:
        templateRef: {}
      healthCheck:
        checks:
          nodeStartupTimeoutSeconds: 0
          unhealthyNodeConditions: []
        remediation:
          triggerIf:
            unhealthyLessThanOrEqualTo: 100%
            unhealthyInRange: "[1-4]"
          templateRef: {}
          maxInFlight:
      rollout:
        strategy:
          rollingUpdate:
            maxSurge:
            maxUnavailable:
          type: ""
      minReadySeconds: 0
      readinessGates: []
      deletion:
        order: "" # could also be policy (current: deletePolicy) Maybe order is not ideal because the correlation to Machine is not obvious
        nodeDeletionTimeoutSeconds: 0
        nodeDrainTimeoutSeconds: 0
        nodeVolumeDetachTimeoutSeconds: 0

Note:
for now we decided to keep clusterclass.controlPlane.machineInfrastructure (and not align this to infrastructure used e.g. in machineDeployment) because clusterclass.controlPlanehas two templates, one for the control plane itself and one for the machine itself, so the prefix helps to distinguish between the two.
Also, it is somehow consistent with the fact that we decided to keep machineTemplate in the control plane contract (see KCP below).

KubeadmControlPlane

spec:
  machineNaming: # TODO: think about if there is an alternative to template + if there is a way to aling to naming: ... pattern used in Cluster/CC (same in MD)
    template: ""
  remediation:
    maxRetry: 0
    minHealthyPeriodSeconds: 0
    retryPeriodSeconds: 0
  rollout:
    after: ""
    before:
      certificatesExpiryDays: 0
    strategy:
      type: ""
      rollingUpdate:
        maxSurge:
  machineTemplate:
    metadata: {}
    spec:
      readinessGates: []
      deletion:
        nodeDeletionTimeoutSeconds: 0
        nodeDrainTimeoutSeconds: 0
        nodeVolumeDetachTimeoutSeconds: 0
      infrastructureRef: {}

Note:

• we are keeping machineTemplate instead of template because a control plane implementation might have templates for stuff that is not machines (so let's be explicit in this case, which is also part of the controlPlane contract)
• In future we might consider to move also spec.kubeadmConfigSpec to spec.machineTemplate.spec.bootstrap for consistency with MD (too late for this change in v1beta2)

MachineDeployment:

spec:
  machineNaming: 
    template: ""
  rollout:
    after: ""
    strategy:
      type: ""
      rollingUpdate:
        maxSurge:
        maxUnavailable:
  remediation:
    maxInFlight:
  deletion:
    order: "" # could also be policy (current: deletePolicy) Maybe order is not ideal because the correlation to Machine is not obvious
  template:
    metadata: {}
    spec:
      failureDomain: ""
      minReadySeconds: 0
      readinessGates: []
      deletion:
        nodeDeletionTimeoutSeconds: 0
        nodeDrainTimeoutSeconds: 0
        nodeVolumeDetachTimeoutSeconds: 0

MachineSet:

spec:
  machineNaming: 
    template: ""
  deletion:
    order: "" # could also be policy (current: deletePolicy) Maybe order is not ideal because the correlation to Machine is not obvious
  template:
    metadata: {}
    spec:
      failureDomain: ""
      minReadySeconds: 0
      readinessGates: []
      deletion:
        nodeDeletionTimeoutSeconds: 0
        nodeDrainTimeoutSeconds: 0
        nodeVolumeDetachTimeoutSeconds: 0

Machine:

spec:
  failureDomain: ""
  minReadySeconds: 0
  readinessGates: []
  deletion:
    nodeDeletionTimeoutSeconds: 0
    nodeDrainTimeoutSeconds: 0
    nodeVolumeDetachTimeoutSeconds: 0

MachineHealthCheck

spec:
  checks:
    nodeStartupTimeoutSeconds: 0
    unhealthyNodeConditions: []
  remediation:
    triggerIf:
      unhealthyLessThanOrEqualTo: 100%
      unhealthyInRange: "[1-4]"
    templateRef: {}

This issue is for collecting feedback about the proposed desired state

[fabriziopandini]

cc @chrischdi @JoelSpeed @sbueringer @enxebre @vincepri

[sbueringer]

As discussed, fine from my side!

[JoelSpeed]

+1 from me, I think the new structures look great, a couple of thoughts as I read through

• remediation.triggerIf, not sure if other names were considered, but could also be triggerWhen, or triggerIfNodes/triggerWhenNodes - I got thinking about whether nodes should be in the context, since it's not in the child fields?
• rollout.strategy should be implemented now as a discriminated union, will need to check if that's possible given prior validations

[sbueringer]

@JoelSpeed Thx. I think I'm lacking some knowledge. How would this look like as a discriminated union?

The current proposal is this

        strategy:
          type: "RollingUpdate|OnDelete"
          rollingUpdate: # only applies if type == RollingUpdate
            maxSurge:
            maxUnavailable:

(we'll achieve that purely by moving remediation and rollingUpdate.deletePolicy out of strategy)

Isn't this already a discriminated union? (+/- I think we don't have the validation that rollingUpdate is only set if type == RollingUpdate and I'm not sure if we can add that validation now)