Documentation: Basic internals docs #542

phyber · 2018-10-15T13:26:29Z

What this PR does / why we need it: Adds an internals section to the docs/ directory and adds basic internals documentation for actuators and controllers. Also adds a few .gitignores for vim related files.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Related to #505

Special notes for your reviewer: This is by no means complete, but hopefully provides a small base for people to start from.

Release note:

NONE

k8s-ci-robot · 2018-10-15T13:26:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: phyber
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: jessicaochen

If they are not already assigned, you can assign the PR to them by writing /assign @jessicaochen in a comment when ready.

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

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

randomvariable · 2018-10-15T16:40:11Z

This has helped us internally get started with Cluster API, hence PRing here.

/lgtm

randomvariable · 2018-10-15T16:40:17Z

/ok-to-test

davidewatson

This is a good start to a hard problem. We've discussed the need for documentation of this sort for a while now, and probably should again, especially now that there are more mature providers, in particular, cluster-api-provider-aws.

Some ideas for moving this forward are to 0) start with the original design, and/or 1) submit an outline so that others can fill in the details. It's unclear if the latter idea will work since writing by committee doesn't lend itself to great (or even tolerable) prose. I'd like to see if I can help here. I've been out of pocket for the last few weeks but should be mostly free for the rest of the month.

davidewatson · 2018-10-15T23:27:38Z

docs/internals/controllers.md

+is that the machine controller has to take into account that a resource in
+a request may not exist, as such, it must account for things like having to
+create them to begin with.
+The cluster controller has the privilege of knowing that the cluster already


This may not be true. There are providers for which the Cluster and Machine objects exist in a cluster which is different than the cluster they represent (cf. Gardner, cluster-api-provider-ssh, etc.)

Some differences between the cluster and machine controllers are that they 0) manage different cluster-api resources, 1) Cluster resources may contain configuration and status which is shared between Machine resources, etc.

I was wondering that while writing this. I've been working in the AWS provider, so the above made sense there. I'll try to reword this based on what you've said here.

docs/internals/machine_actuator.md

davidewatson · 2018-10-16T03:18:58Z

docs/internals/machine_actuator.md

+resource request and calls the appropriate machine actuator methods in order to
+realise a machine state.
+
+## Basic Actuator Flow


This is great!

davidewatson · 2018-10-16T03:32:15Z

Note that, modulo my comments above, I am not necessarily opposed to merging this.

phyber · 2018-10-16T10:56:46Z

I've realised that the flow I've put under Basic Actuator Flow is actually the Basic Controller Flow. I'll be taking care of this in some upcoming pushes.

phyber · 2018-10-16T13:25:32Z

OK, I've changed the language here and (hopefully) improved things a little. It is now noted that the definition of a cluster and machine is up to the provider implementation, and that in the simple case they may be things like networks, linux instances, etc, but not necessarily.

Basic flow for the cluster controller was added and retryable errors were noted in both cluster and machine controllers. The main controllers doc was expanded a little and links to the cluster and machine controller docs.

dlipovetsky · 2018-10-17T20:31:20Z

docs/internals/controllers.md

+The main controller implementations ([cluster] and [machine]) are located
+within  the `cluster-api` library, and each perform almost identical basic
+steps within their `Reconcile` methods. The controllers have the responsibility
+of receiving incoming requests and dispatching them to the appropriate actuator


It helps me to think of this in different terms than "a request is received from Kubernetes." Namel,y the controller watches for changes to the resources. For example, the machine controller watches for changes to the machine resource. If a Machine object is created, updated, or deleted, the controller receives that object.

Fair. I've fixed some language in this area to mention the watches, and fixed up the basic flow to mention that after a watch is observed, a reconcile request is received before the controller fetches the cluster/machine object from Kubernetes.

Thanks a lot!

phyber · 2018-10-19T08:56:20Z

This is ready for another review and merge if there are no more issues.

randomvariable · 2018-10-22T11:10:57Z

/lgtm

docs/internals/cluster_controller.md

roberthbailey · 2018-10-23T03:55:36Z

docs/internals/cluster_controller.md

+machine controller has been waiting for the cluster to be ready before it
+starts working on creating the machine resources.
+
+## Basic Controller Flow


I assume that this is documenting the current flow, which is subject to change.

roberthbailey · 2018-10-23T03:56:32Z

docs/internals/controllers.md

@@ -0,0 +1,34 @@
+# Controllers
+
+The main controller implementations ([cluster] and [machine]) are located


These are the main controllers that providers care about; for users, there are also the machine deployment and machine set controllers.

roberthbailey · 2018-10-23T04:01:18Z

docs/internals/controllers.md

+  - Controller watches for changes on a resource type
+  - A change on a watched resource type is observed and the controller
+    receives a reconcile request
+  - An attempt is made to fetch the appropriate object from Kubernetes; and if


Is this still true after switching to CRDs? I thought the framework took care of this part now.

This does still appear to be the case in the machine/controller.go and cluster/controller.go files.

k8s-ci-robot · 2018-10-24T11:14:56Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2018-10-24T11:18:06Z

@phyber: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-cluster-api-test	`cbf87de`	link	`/test pull-cluster-api-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

randomvariable · 2018-10-24T11:27:12Z

I assume that this is documenting the current flow, which is subject to change.

We should have docs for the current flow, because that's what implementers have to deal with right now.

Either let's get this PR merged in an acceptable state or get @davidewatson 's gitbook in, but please don't let perfect be the enemy of having some documentation that's helpful to people who are not the original developers of Cluster API.

I want to onboard more engineers from our side onto provider implementations, but we're fumbling around, trying to figure out original intent, looking at the GCP implementation, instead of having anything resembling a clear guide.

roberthbailey · 2018-10-25T08:55:42Z

Either let's get this PR merged in an acceptable state or get @davidewatson 's gitbook in, but please don't let perfect be the enemy of having some documentation that's helpful to people who are not the original developers of Cluster API.

I totally agree. We discussed during the call today that we want the githbook, but since that will be at least a week out I'm ok merging this in the mean time if you'd prefer.

randomvariable · 2018-10-25T16:41:22Z

From Slack, by @phyber :

@dwat I hear that the controller documentation from my PR is making its way into the Gitbook in an upcoming PR of yours. That's fine, and I'm happy to close my PR. No point merging it to have it be removed again when the book shows up shortly 🙂 (Will leave the PR open until an ACK later)

roberthbailey · 2018-10-30T02:34:46Z

Closing in favor of #566.

/close

k8s-ci-robot · 2018-10-30T02:34:47Z

@roberthbailey: Closing this PR.

In response to this:

Closing in favor of #566.

/close

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.

…er-config Add missing cloud provider config

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 15, 2018

k8s-ci-robot requested review from krisnova and mkjelland October 15, 2018 13:26

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 15, 2018

k8s-ci-robot assigned randomvariable Oct 15, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 15, 2018

davidewatson reviewed Oct 16, 2018

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2018

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 16, 2018

dlipovetsky reviewed Oct 17, 2018

View reviewed changes

phyber force-pushed the docs-internals branch from 1e43943 to 5df5a8e Compare October 22, 2018 09:53

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 22, 2018

roberthbailey reviewed Oct 23, 2018

View reviewed changes

docs: Add basic internals documentation around controllers

cbf87de

phyber force-pushed the docs-internals branch from 5df5a8e to cbf87de Compare October 24, 2018 11:14

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2018

phyber mentioned this pull request Oct 29, 2018

REQUEST: New membership for phyber kubernetes/org#200

Closed

6 tasks

k8s-ci-robot closed this Oct 30, 2018

jayunit100 pushed a commit to jayunit100/cluster-api that referenced this pull request Jan 31, 2020

Merge pull request kubernetes-sigs#542 from akutz/bugfix/cloud-provid…

8441c9b

…er-config Add missing cloud provider config

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Documentation: Basic internals docs #542

Documentation: Basic internals docs #542

phyber commented Oct 15, 2018

k8s-ci-robot commented Oct 15, 2018

randomvariable commented Oct 15, 2018

randomvariable commented Oct 15, 2018

davidewatson left a comment

davidewatson Oct 15, 2018 •

edited

Loading

phyber Oct 16, 2018

davidewatson Oct 16, 2018

davidewatson commented Oct 16, 2018

phyber commented Oct 16, 2018

phyber commented Oct 16, 2018

dlipovetsky Oct 17, 2018

phyber Oct 18, 2018

dlipovetsky Oct 19, 2018

phyber commented Oct 19, 2018

randomvariable commented Oct 22, 2018

roberthbailey Oct 23, 2018

roberthbailey Oct 23, 2018

roberthbailey Oct 23, 2018

phyber Oct 24, 2018

k8s-ci-robot commented Oct 24, 2018

k8s-ci-robot commented Oct 24, 2018

randomvariable commented Oct 24, 2018

roberthbailey commented Oct 25, 2018

randomvariable commented Oct 25, 2018

roberthbailey commented Oct 30, 2018

k8s-ci-robot commented Oct 30, 2018

		@@ -0,0 +1,34 @@
		# Controllers

		The main controller implementations ([cluster] and [machine]) are located

Documentation: Basic internals docs #542

Documentation: Basic internals docs #542

Conversation

phyber commented Oct 15, 2018

k8s-ci-robot commented Oct 15, 2018

randomvariable commented Oct 15, 2018

randomvariable commented Oct 15, 2018

davidewatson left a comment

Choose a reason for hiding this comment

davidewatson Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidewatson commented Oct 16, 2018

phyber commented Oct 16, 2018

phyber commented Oct 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phyber commented Oct 19, 2018

randomvariable commented Oct 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 24, 2018

k8s-ci-robot commented Oct 24, 2018

randomvariable commented Oct 24, 2018

roberthbailey commented Oct 25, 2018

randomvariable commented Oct 25, 2018

roberthbailey commented Oct 30, 2018

k8s-ci-robot commented Oct 30, 2018

davidewatson Oct 15, 2018 •

edited

Loading