Add APIs for machine phases

[hardikdr]

What this PR does / why we need it: This PR adds the necessary machines-phases and states in the machine-api stack. Machine phases and states are a way of representing the lifecycle of the machines. PR is based on the following proposal doc: proposal

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 #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Added APIs for machine phases

[ggaurav10]

Replace MachineRunning with MachineTerminating

[hardikdr]

done, thanks for pointing out.

[scruplelesswizard]

/ok-to-test

[scruplelesswizard]

Just a nit from me

[scruplelesswizard]

nit: MachineOperationState would be a better name as it represents the state of the operation, not of the machine

[hardikdr]

Yes, that makes sense, thanks.
Updating the occurrences wherever needed.

[hardikdr]

done.

[alvaroaleman]

Background for the failing tests: The timeout for the test controlplane is too low, so it often fails to start: 2018/10/11 09:27:09 timeout waiting for process kube-apiserver to start

This already happened multiple times on different PRs. For the time being the workaround is to hit /retest until they pass.

I've created a PR in kubebuilder to get this configurable so we can increase the timeout: kubernetes-sigs/controller-runtime#169 It will however take some time until we can use that, because even when that PR is merged we have to wait for the next release.

[hardikdr]

/retest

[hardikdr]

thanks, @alvaroaleman for the information, I will then retry after some time.

[scruplelesswizard]

/retest

[alvaroaleman]

@hardikdr Actually the failures here a real, check out the logs

[hardikdr]

@alvaroaleman Yes, I just corrected it and test-check seems to be passing. Thanks for the help.

[davidewatson]

/lgtm

[alvaroaleman]

@hardikdr Can you quickly explain what this allows us to do what we can currently not do?

Because:

• The machine-controller itself should only take the .Spec into account, not the .Status
• Other controllers like the machineSet controller basically care only about one condition: Is the associated Node existing and/or ready? They can check that via .Status.NodeRef already: https://github.com/kubernetes-sigs/cluster-api/blob/master/pkg/controller/machineset/status.go#L55
• For humans, there are events

Is there anything I missed in the above list? Is there anything I got wrong in the above list?

[hardikdr]

Yes sure.
The idea is to basically define the life-cycle of the machines in a descriptive manner.
I see use cases mainly from 2 broad categories: Expressiveness and Consumption.

1. Expressiveness:

• Seems it will be nice to have a framework to express different lifecycle-events of machines as accurately as possible.

• Taking a subtle example:
  Machine gets into the failed-phase. There could be multiple reasons behind it, for instance:

  • Kubelet did not contact APISever from last 10 minutes.
  • Or Disk-pressure is high from last ~30minutes due to which apps are suffering.
  • Or simply machine-creation itself has failed due to error either from cloud-provider or say mis-configuration of software-stack on machine.

It seems it will be great to express all three scenarios in a structured fashion.

• For first 2 cases, we could set MStatus.LastOperation to Health-check and MStatus.LastOparation.State/Description to clearly mention it failed due to kubelet didn't respond in first case and diskPressure was high in second case.

• The third case is different in a way that creation itself failed, and LastOperation could be then set to Create and LO.State/Description could be set :

  • to saying cloud-provide could not create the machine,
  • or cloud-provider created the machine but kubelet could not register itself.

• Essentially proposed framework will offer equipped means of expressing the current status-quo of the machine. On top LastOperation field will also provide the latest opreration history of machine-status.

2. Consumption:

• Machine could go through many different phases during life-span and machineSetController or User might want to take different actions based on the exact phase.
  For instance,
• MachineSet controller might not want to delete the machine with Unknown phase, but rather give some more time to machine to connect back within timeout period.
• MachineSet controller might want to delete the machine immediately to replace it by newer one if its in the Failed phase - assuming machine would had been given enough time already before to join back.
• MachineSet controller might want to ignore machines in Standby phase in case of baremetals, and more.
• Essentially, set of phases should be well-defined for machineSet/other-higher controller to take the right-actions.

To answer the question: Can you quickly explain what this allows us to do what we can currently not do?

• I would say, in current model we do not have means of expressing the phases in detailed and structured way - as described in use case 1 above.
• And also we do not have well-defined set of inputs for machineSet/other-controllers to take the right actions based on the current status of the machine. - use case 2.

Though machine-shared-controller and machine-external-controller would still require a contract to update the machineStatus, but that's a separate thread. Turned out to be a long one, but I hope it makes sense :) Feedback is most welcome.

[alvaroaleman]

Thanks for the detailed answer, @hardikdr !

It seems it will be great to express all three scenarios in a structured fashion.

We can derive all this information from .Status.NodeRef: failed machine creation from the fact that the .Status.NodeRef does not exist or does not point to a valid node, the two other examples you mentioned are available as conditions on the Node object.

MachineSet controller might not want to delete the machine with Unknown phase, but rather give some more time to machine to connect back within timeout period.

I don't think it should be the concern of the MachineSet controller to re-create cloud provider instances when the node gets lost or never connects, that is something the Machine controller does/should do. And again, all this info here is available from the Machine object or the referenced Node object.

I would say, in current model we do not have means of expressing the phases in detailed and structured way - as described in use case 1 above

Can you explain why this is needed? For users we can use events, for other controllers do you have an example use-case where the information available via the .Status.NodeRef is not sufficient? Because to me it feels like we are duplicating the Node object and its features here (conditions, heartbeats)

And also we do not have well-defined set of inputs for machineSet/other-controllers to take the right actions based on the current status of the machine

Basically the same question as above, do you have a sample of a controller who needs this? The machineSet controller only needs to know if a node exists and if its healthy, this works perfectly fine with the existing .Status.NodeRef

[hardikdr]

thanks for the comments @alvaroaleman .

We can derive all this information from .Status.NodeRef: failed machine creation from the fact that the .Status.NodeRef does not exist or does not point to a valid node,

Yes, NodeRef is essential and will provide much info, but I am not sure if there is a way to understand if creation/deletion or other operation has failed or succeeded on a particular machine- via NodeRef's existence. Deriving certain possibilities only based on the availability of NodeRef or pointer to wrong machines- seems less desirable.

I don't think it should be the concern of the MachineSet controller to re-create cloud provider instances when the node gets lost or never connects, that is something the Machine controller does/should do.

I would rather expect machine-controller should mostly be resposible for creation/deletion of the machines and reporting right health-status via MachineStatus field. MachineSet controller should then look for Failed machines and try to replace them. This is more from the perspective that MachienSet controller should only make sure to have # of healthy machines and take necessary steps - and not participate in race-condition with MachineController while recreation of machines.

Can you explain why this is needed? For users we can use events, for other controllers do you have an example use-case where the information available via the .Status.NodeRef is not sufficient?

Fundamentally, NodeRef is bound by the possible values available in the NodeObject. We would defininitely want to expand our usecases to include new phases such as Draining/Standby[for barementals] and more.

[davidewatson]

MachineOperationType and MachineOperationState can be useful for GUIs built on top of the ClusterAPI. In order to determine if an upgrade is compelte, we currently compare the Version.Kubelet and Version.ControlPlane with the version reported by the Node. This is somewhat limited since kubelet versions are not the only reason for an upgrade.

Others have suggested using Events instead. Can events be lost however, if a controller reboots for example?

From a user perspective I am not sure there is a more reliable way to answer these questions. Is the machine still upgrading, is it being deleted, etc? This is valuable for user feedback within a GUI.

[roberthbailey]

As discussed during the meeting today, we will merge this in ~24 hours unless there are objections before then.

/approve
/hold

[scruplelesswizard]

/retest

[hardikdr]

@chaosaffe thanks for the suggestions. All of them looks good to me. I also made the necessary changes.

[scruplelesswizard]

/lgtm

[alvaroaleman]

Please make this a pointer as its an optional struct

[hardikdr]

yes sure, done.

[alvaroaleman]

/lgtm

[hardikdr]

/retest

[roberthbailey]

It looks like we've pretty much reached consensus on this PR. Let's make sure there are no further comments or objections at the next meeting and then merge it if all looks good.

[sidharthsurana]

minor naming suggestion, otherwise lgtm

[sidharthsurana]

Nit: Can we change the name lastUpdateTime to lastUpdated just to be consistent with the similar fields in other places and objects.

[hardikdr]

done, thanks.

[sidharthsurana]

/LGTM

[k8s-ci-robot]

@sidharthsurana: changing LGTM is restricted to assignees, and only kubernetes-sigs/cluster-api repo collaborators may be assigned issues.

In response to this:

/LGTM

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.

[scruplelesswizard]

/lgtm

…

On Wed, Nov 28, 2018, 19:51 k8s-ci-robot, ***@***.***> wrote: @sidharthsurana <https://github.com/sidharthsurana>: changing LGTM is restricted to assignees, and only kubernetes-sigs/cluster-api repo collaborators may be assigned issues. In response to this <#531 (comment)> : /LGTM Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/guide/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#531 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APFttC5E71J0j4kN-21iFMb5B1jsAj8wks5uztswgaJpZM4XVdFZ> .

[roberthbailey]

/approve

[roberthbailey]

/hold cancel

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hardikdr, roberthbailey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [roberthbailey]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[derekwaynecarr]

I apologize for the late question, but is there a write-up that summarizes a response from Daniel's feedback here:
https://docs.google.com/document/d/12TsBPn1lfMk50_yydzbXNZ9PT-8-88o4tSNi7eqPCVg/edit?disco=AAAACWAG9QI

similar to him, I worry about the use of a single phase based on experience with pods.

[roberthbailey]

@hardikdr -- can you answer @derekwaynecarr's question? I know that you had a chance to connect with @lavalamp at KubeCon and chat about phases / conditions / etc.