Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add reference documentation for built-in controllers #15374

Closed

Conversation

sftim
Copy link
Contributor

@sftim sftim commented Jul 11, 2019

Relevant to issue #4135.

Add a reference section that lists the built-in controllers and explains what each of them does.

This change fits in with an idea I have to restructure https://kubernetes.io/docs/concepts/workloads/

People developing things to deploy on Kubernetes need to understand that there is (eg) a Deployment resource; it doesn't matter as much how the Deployment resource gets translated into API actions like creating or removing ReplicaSet objects. Some resources happen to involve more than one controller (eg Job with the Job controller and the TTL-after-finished controller).

If you are interested in looking behind the scenes, I think it helps to have all kube-controller-manager controllers listed in one place, no matter what kind of resource they interact with (Endpoint? ReplicaSet? Namespace?)

/sig apps

You can see this change in broader context by viewing the deployment preview for PR #15373 (or looking at the list of files changed there, etc).

Credit to @lavalamp for the excellent Kubecon talk that helped me a lot with drafting these documents.

@k8s-ci-robot k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 11, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign zparnold
You can assign the PR to them by writing /assign @zparnold in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@sftim
Copy link
Contributor Author

sftim commented Jul 11, 2019

/assign @zparnold

@sftim
Copy link
Contributor Author

sftim commented Jul 11, 2019

@bradtopol I'd welcome your feedback on this.

@netlify
Copy link

netlify bot commented Jul 11, 2019

Deploy preview for kubernetes-io-master-staging ready!

Built with commit c96ed15

https://deploy-preview-15374--kubernetes-io-master-staging.netlify.com

Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phew, this was a big one :)

Mostly awesome work on this @sftim. I have a number of inline suggestions, but the biggest ones are around separating out information that is implementation details about the controllers from information that is more end-user oriented that explains how to use the different objects like Job, Deployment, StatefulSet. I think that kind information -- the more end-user oriented -- best belongs in the concepts/workloads/ documentation.

Anyway, love to hear your thoughts on my comments.

Best,
-jay

@@ -0,0 +1,39 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recommend naming the file volume-attach-detach.md instead of just attach-detach.md. You never know, we could get additional attaching-detaching controllers in the future. :)

Copy link
Contributor Author

@sftim sftim Jul 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I should probably rename all the controller documents based on the subdirectory they're in in the kubernetes/kubernetes source code.

Which I will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a better idea. I will rename them based on the names visible to cluster operators, as per --controllers in https://kubernetes.io/docs/reference/command-line-tools-reference/kube-controller-manager/

(these must also be in the source code, so I'll see if I can spot how that's done)

content/en/docs/reference/controllers/attach-detach.md Outdated Show resolved Hide resolved
to detach it.

Different nodes might have different ways of attaching volumes, and different
Volumes can have different values for VolumeNodeAffinity, so the decision
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider linking VolumeNodeAffinity to https://kubernetes.io/docs/concepts/storage/persistent-volumes/#node-affinity.

This sentence is hard for me to understand, though, in particular the part "so the decision around volume attachment happens after scheduling." What specifically are you conveying here? Are you trying to say that normally node affinity constraints are handled by the scheduler, but in some cases node affinity constraints are handled on the node instead?

content/en/docs/reference/controllers/built-in.md Outdated Show resolved Hide resolved

This controller limits the quantity of objects that can be created in a namespace
by object type, as well as the total amount of compute resources that may be
consumed by resources in that project.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"that project" is used, but there is no mention of "project" earlier in the paragraph. This is likely due to legacy Google-specific terminology? Probably best to rephrase it here.

The controller constructs the pod name label valuebased on a pattern:
`$(statefulset name)-$(ordinal)`.

### Storage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to Deployment and Job, I think much of the remainder of this page should be in a concepts/workloads/statefulset.md document that is written from an end-user or object reference perspective. And leave this doc for implementation specifics about the controller itself.

of expired Jobs can still happen, even if you requested a longer TTL
and you saw a successful API response to your update request.

## Time skew
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this probably belongs in this document since it's implementation details about the controller itself.

its dependent objects together with it. Note that when the resource is deleted,
its lifecycle guarantees, such as finalizers, will be honored.

The TTL seconds can be set at any time. Here are some examples for setting the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From here down in this section kind of feels like it belongs in a concepts/workloads/job.md document that is more oriented to the end user than the person interested in how the controller works under the hood.

@sftim sftim changed the title Add reference documentation for built-in controllers [WIP] Add reference documentation for built-in controllers Jul 16, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 16, 2019
@lavalamp
Copy link
Member

My last kubecon talk was almost exactly about this: https://youtu.be/zCXiXKMqnuE

@sftim sftim mentioned this pull request Jul 16, 2019
2 tasks
@sftim sftim force-pushed the 20190711_add_controller_reference_pages branch from 44a63a9 to 038645a Compare July 16, 2019 20:55
@sftim sftim changed the title [WIP] Add reference documentation for built-in controllers Add reference documentation for built-in controllers Jul 16, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 16, 2019
@sftim
Copy link
Contributor Author

sftim commented Jul 16, 2019

What do you reckon @jaypipes? How's this now?

@sftim
Copy link
Contributor Author

sftim commented Jul 16, 2019

My last kubecon talk was almost exactly about this: https://youtu.be/zCXiXKMqnuE

@lavalamp a good talk, too! Please do let me know if I've got any details wrong.

Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ great work on this, Tim.


### Namespace lifecycle controller {#controller-namespace}

When you (or any Kubernetes API client) remove a {{< glossary_tooltip term_id="namespace" >}},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack, great explanation @lavalamp, appreciated! @sftim might be good to paste Daniel's explanation on the difference between the two controllers into this doc in a followup PR.


### Taints and Tolerations

the controller automatically adds the following
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Capitalize "The controller"

@sftim sftim force-pushed the 20190711_add_controller_reference_pages branch 2 times, most recently from d432671 to 7cf5a57 Compare July 19, 2019 08:29
@kbhawkey
Copy link
Contributor

hello, @sftim. I am in the process of reading through the files 😄 . My initial impression is that this will serve as a nice resource. Since the list of controllers is long and you have categorized some of the controllers on the Built-in controllers page, I would suggest to create several groupings to list on the left nav and then put the individual controllers in a category. If there are a few that do not have a single category, then list them independently. Perhaps you do not need the Built-in Controllers page.
The Controllers main list item in the left nav could also be listed after the API-related folder items.
Not sure about listing vendor links in the reference, for example, third party ingress controllers.

New Controllers root page preview
https://deploy-preview-15374--kubernetes-io-master-staging.netlify.com/docs/reference/controllers


## Resource management controllers {#controllers-resource-management}

* [Resource quota controller](/reference/access-authn-authz/admission-controllers/#resourcequota)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sftim don't you need a /docs too ?


When kubelet is setting up on a new node, kubelet will generate a CSR and submit it
to the Kubernetes API server using its
[bootstrap](https://kubernetes.io/docs/reference/access-authn-authz/bootstrap-tokens/)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sftim pls use the relative url so it does work for localization too ;)

all the pods from the node (using graceful termination) if the node continues
to be unreachable. (The default timeouts are 40s to start reporting
ConditionUnknown and 5m after that to start evicting pods.) The node controller
checks the state of each node every `--node-monitor-period` seconds.
Copy link
Contributor

@DanyC97 DanyC97 Jul 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sftim imo i'd turn into a number list the first, second and third roles, easy to read


In most cases, node controller limits the eviction rate to
`--node-eviction-rate` (default 0.1) per second, meaning it won't evict pods
from more than 1 node per 10 seconds.
Copy link
Contributor

@DanyC97 DanyC97 Jul 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sftim imo i find that we could maybe reword it a bit like

won't evict pods from more than 1 node during 10 seconds window

of the the DaemonSet the Pod belongs to.

If it's managing a Pod on a Node that no longer matches the DaemonSet,
or where the Node no longer exists, the controller will remove that Pod.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is it?

{{% /capture %}}

{{% capture body %}}

Copy link
Contributor

@kbhawkey kbhawkey Jul 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General comment for all pages:
Can you add a ## Controller behavior section so that the content is consistent with the other pages?

{{% capture overview %}}

One of a pair of controllers for the {{< glossary_tooltip text="ServiceAccount" term_id="service-account" >}}
resource.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove resource?
A ServiceAccount Token Controller generates and assigns API access tokens for a ServiceAccount.

@sftim sftim changed the title Add reference documentation for built-in controllers [WIP] Add reference documentation for built-in controllers Jul 22, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2019
@sftim
Copy link
Contributor Author

sftim commented Jul 24, 2019

This should help a little with (closed) issue #10160, about documenting NodeLease.

@sftim sftim force-pushed the 20190711_add_controller_reference_pages branch 2 times, most recently from 2ef10d7 to f884084 Compare July 24, 2019 09:25
runs separately from the kube-controller-manager. This separation helps
with control plane performance.

The controllers that run inside kube-controller-manager are:
Copy link
Contributor

@bradtopol bradtopol Jul 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The transition on the page from "The controllers that run inside kube-controller-manager are:" to then going to a bold heading of "Controllers for running workloads on Kubernetes" looks really weird. Check out https://kubernetes.io/docs/setup/ which shows a nice transition from a list of topics (Learning Environment, Production environment) to their sections below the introductory paragraph. The https://kubernetes.io/docs/concepts/ also as some nice examples of how to do this as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback on this.

@sftim sftim force-pushed the 20190711_add_controller_reference_pages branch from f884084 to bab6332 Compare July 28, 2019 15:20
@sftim
Copy link
Contributor Author

sftim commented Jul 31, 2019

Something that occurred to me a little while ago: possibly, this documentation could live as reference documentation in kubernetes/kubernetes

I'll continue with this approach for now, if people reading this like the above idea though I can look into taking a different tack.

@sftim
Copy link
Contributor Author

sftim commented Aug 26, 2019

Didn't get round to this yet. I still plan to!

@zacharysarah
Copy link
Contributor

@sftim Please feel free to reopen this when you're ready!

/close

@k8s-ci-robot
Copy link
Contributor

@zacharysarah: Closed this PR.

In response to this:

@sftim Please feel free to reopen this when you're ready!

/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.

@sftim
Copy link
Contributor Author

sftim commented Oct 13, 2019

/retitle Add reference documentation for built-in controllers

@k8s-ci-robot k8s-ci-robot changed the title [WIP] Add reference documentation for built-in controllers Add reference documentation for built-in controllers Oct 13, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 13, 2019
@sftim
Copy link
Contributor Author

sftim commented Oct 14, 2019

Rebased & tweaked
/reopen

@k8s-ci-robot
Copy link
Contributor

@sftim: Failed to re-open PR: state cannot be changed. The 20190711_add_controller_reference_pages branch was force-pushed or recreated.

In response to this:

Rebased & tweaked
/reopen

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants