improve detail of podtolerationrestriction behavior #7135

colemickens · 2018-01-30T02:04:34Z

similar to #7134

This PR:

Clarifies the behavior of the PodTolerationRestriction admission controller.
Promote the documented annotations to stable, as per the v1.10 release.

This targets release-1.10.

This PR is dependent on this Kubernetes PR: kubernetes/kubernetes#58818

This change is

k8sio-netlify-preview-bot · 2018-01-30T02:06:13Z

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

Built with commit 2dcf4b2

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

tengqm · 2018-01-30T02:36:13Z

docs/admin/admission-controllers.md

+1. First, the default tolerations for a given namespace are determined:
+
+   i. If the namespace contains an annotation with the key `scheduler.alpha.kubernetes.io/default-tolerations`
+      and a non-empty, the value of the annotation will be considered as the default tolerations for the namespace.


"and a non-empty"?

tengqm · 2018-01-30T02:37:17Z

docs/admin/admission-controllers.md

+      and a non-empty, the value of the annotation will be considered as the default tolerations for the namespace.
+
+   ii. If the namespace lacks an annotation with the key `scheduler.alpha.kubernetes.io/default-tolerations`, then
+       the cluster defaults will be used as the default tolerations for the namespace.


Reads will ask "what are the cluster defaults? how to set these defaults?"

tengqm · 2018-02-01T02:38:58Z

docs/admin/admission-controllers.md

-`scheduler.alpha.kubernetes.io/tolerationsWhitelist`
-annotation keys.
+This admission controller defaults and limits what tolerations may be used on Pods in a given namespace.
+The plugin will build a list of defaults, and a whitelist of allows tolerations based on annotations on the namespace.


allows -> allowed

tengqm · 2018-02-01T02:39:38Z

docs/admin/admission-controllers.md

+When the namespace does not specify a whitelist, the namespace-specifc whitelist from the plugin configuration are used.
+
+**Note:** If the annotations are specified, but empty, they are considered to override the cluster-level defaults.
+{: .note}


tengqm · 2018-02-01T02:40:17Z

docs/admin/admission-controllers.md

+          value: value1
+        - key: key2
+          value: value2
+```


line 541-551 should be unindented

tengqm · 2018-02-01T02:41:35Z

docs/admin/admission-controllers.md

+
+PodTolerationRestriction uses a configuration file to define:
+
+1. A cluster-wide default node selector (this is used if the `` annotation is not present on the target Namespace)


Looks like this line won't format correctly.
What did you mean by "`` annotation" ?

tengqm · 2018-02-01T02:41:54Z

docs/admin/admission-controllers.md

+
+1. A cluster-wide default node selector (this is used if the `` annotation is not present on the target Namespace)
+
+2. Namespace-specific default whitelists (these are used if the `` annotation is not present on the target Namespace)


tengqm · 2018-02-01T02:42:55Z

docs/admin/admission-controllers.md

+2. Namespace-specific default whitelists (these are used if the `` annotation is not present on the target Namespace)
+
+Note that the configuration file format will move to a versioned file in a future release.
+This file may be json or yaml and has the following format:


use "JSON or YAML" when appropriate

tengqm · 2018-02-01T02:44:26Z

docs/admin/admission-controllers.md

+
+#### Internal Logic
+
+1. Determine the `Pod`'s effective tolerations:


No need to add "`" around Pod here?
Also no need to add it around Namespace blow.

heckj · 2018-02-07T02:46:52Z

/assign @tengqm

heckj · 2018-02-24T23:23:35Z

@colemickens if this targets the 1.10 release, could you retarget this PR against the release-1.10 branch please?

Bradamant3 · 2018-03-08T19:53:18Z

@colemickens see comment ^^ from Joe Heck about retargeting against the release-1.10 branch. We need it by 3/9 (tomorrow). Thanks!

colemickens · 2018-03-08T21:03:44Z

The changes didn't merge. I'll rebase and retarget for 1.11.

Bradamant3 · 2018-03-08T21:42:33Z

/hold

waiting on 1.11 branch for docs, which we don't want to create yet. Too much churn with merging master.

zacharysarah · 2018-04-11T18:37:24Z

@colemickens 👋 1.11 release branch is open; changing base to 1.11.

zacharysarah · 2018-05-11T18:28:32Z

@MistyHacks This PR will be affected by any changes to release-1.11 and obviously by the Hugo migration.

k8s-ci-robot · 2018-05-16T18:43:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

Assign the PR to them by writing /assign @bradamant3 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

k8sio-netlify-preview-bot · 2018-05-16T18:46:51Z

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

Built with commit 6255030

https://deploy-preview-7135--kubernetes-io-vnext-staging.netlify.com

mdlinville · 2018-05-16T18:48:21Z

The underlying structure of the docs repository has changed due to the recent
Hugo migration. Specifically, all the files previously under docs/ are now in
content/en/docs/. I have rebased your PR without changing any of your work, so
that your changes are modifying the affected files in the new location, so this
PR is now reviewable again.

If you were using Github's web UI rather than a local Git client, you can stop
reading and continue working in this PR.

If you use the Git command-line client, you will need to make sure your local
branch is up to date with your fork. If you have local commits that have not
been pushed to your fork, it may be complicated to get them back. Do git log
and note their commit hashes for later.

Assuming your your fork's remote is called origin, hard-reset your local
branch to be the same as your fork's branch:

git fetch origin; git reset --hard origin/podtolerationrestriction-docs

If you didn't have local commits, you're now ready to keep working on this PR
locally. You can stop reading now.

If you did have local commits, cherry-pick them from oldest to newest. For
each commit that needs to be cherry-picked:

git cherry-pick <HASH>

You will need to resolve conflicts in these cherry-picked commits, which
involves moving your modified files into the correct new location. All the
content in docs/ moved to content/en/docs/, so you can move your local files
accordingly:

Do git add for each of the moved files
Do git mergetool and accept the
deleted rather than modified file
Continue the cherry-pick by doing git cherry-pick --continue

When you've cherry-picked all local commits back onto the local branch, push the
branch (with new commits) to your fork. You should not need to force the push.

mdlinville · 2018-05-16T18:49:46Z

/no-hold

@tengqm @xiangpengzhao please re-review.

mdlinville · 2018-05-16T19:46:30Z

/hold cancel

mdlinville

I left some comments. All links need to be updated to the new structure where everything is under /content/en/docs/ instead of /docs/. Admonitions need to be updated to use Hugo shortcodes. I found some typos and formatting problems too. Others should be nominated for technical review of the content.

mdlinville · 2018-05-16T19:51:51Z

content/en/docs/admin/admission-controllers.md

-```shell
-kube-apiserver --disable-admission-plugins=PodNodeSelector,AlwaysDeny ...
-```
+{: .note}


Please modify this to use the Hugo short-code. See https://kubernetes.io/docs/test/#admonitions or its source code at https://raw.githubusercontent.com/kubernetes/website/master/content/en/docs/test.md.

mdlinville · 2018-05-16T19:52:37Z

content/en/docs/admin/admission-controllers.md


-Rejects all requests. AlwaysDeny is DEPRECATED as no real meaning.
+Rejects all requests.  Used for testing.


Please remove all double spacing from the file.

mdlinville · 2018-05-16T19:53:11Z

content/en/docs/admin/admission-controllers.md

@@ -140,9 +134,11 @@ enabling this admission controller.

 ### EventRateLimit (alpha)

-This admission controller mitigates the problem where the API server gets flooded by
+This admission controller is introduced in v1.9 to mitigate the problem where the API server gets flooded by


Is it necessary to specify the version? That version will be n-2 when 1.11 ships.

mdlinville · 2018-05-16T19:53:40Z

content/en/docs/admin/admission-controllers.md

@@ -184,16 +180,19 @@ for more details.

 ### ExtendedResourceToleration

-This plug-in facilitates creation of dedicated nodes with extended resources.
+This plug-in is introduced in v1.9 to facilitate creation of dedicated nodes with extended resources.


Same concern as above wrt mentioning a version.

mdlinville · 2018-05-16T19:54:53Z

content/en/docs/admin/admission-controllers.md

@@ -316,23 +314,24 @@ In any case, the annotations are provided by the user and are not validated by K

 ### Initializers (alpha)

+This admission controller is introduced in v1.7.


Again, is the version relevant? 1.7 will be the oldest supported version when 1.11 ships, if I understand it correctly.

mdlinville · 2018-05-16T19:59:46Z

content/en/docs/admin/admission-controllers.md

-If the pod's namespace does not have any associated default or whitelist of
-tolerations, then the cluster-level default or whitelist of tolerations are used
-instead if specified.
+2. Namespace-specific default whitelists (these are used if the `` annotation is not present on the target Namespace)


mdlinville · 2018-05-16T20:00:01Z

content/en/docs/admin/admission-controllers.md

-`scheduler.alpha.kubernetes.io/defaultTolerations` and
-`scheduler.alpha.kubernetes.io/tolerationsWhitelist`
-annotation keys.
+Note that the configuration file format will move to a versioned file in a future release.


s/Note that//

mdlinville · 2018-05-16T20:00:27Z

content/en/docs/admin/admission-controllers.md

-`scheduler.alpha.kubernetes.io/tolerationsWhitelist`
-annotation keys.
+Note that the configuration file format will move to a versioned file in a future release.
+This file may be json or yaml and has the following format:


If JSON or YAML are both allowed, why not have a JSON example too?

mdlinville · 2018-05-16T20:01:06Z

content/en/docs/admin/admission-controllers.md

+plugins:
+- name: PodTolerationRestriction
+  path: podtolerationrestriction.yaml
+...


Is it possible to include a complete and usable YAML example instead of the ellipses?

mdlinville · 2018-05-16T20:01:52Z

content/en/docs/admin/admission-controllers.md

@@ -582,7 +622,7 @@ fails. This admission controller only runs in the validation phase; the webhooks
 mutate the object, as opposed to the webhooks called by the `MutatingAdmissionWebhook` admission controller.

 If a webhook called by this has side effects (for example, decrementing quota) it
-*must* have a reconciliation system, as it is not guaranteed that subsequent
+*must* have a reconcilation system, as it is not guaranteed that subsequent


Revert introduced typo here.

mdlinville · 2018-05-17T20:55:05Z

I was wrong about the need to change /docs/ to /content/en/docs/ so you can ignore the request to do that. The things about Hugo shortcodes still need to be addressed.

colemickens · 2018-05-17T20:57:33Z

@MistyHacks This doc change is dependent on PRs that are stalled out and are not actively being worked on. Additionally, much of the feedback provided applies to sections of the Admin Controllers document that this PR was not meant to address. I think it would probably be a good idea to open a new tracking issue for the various other Hugo/etc changes that are needed to this document. (If the PRs ever proceed, I will certainly incorporate the feedback you left for the PTR/PNS controllers.)

mdlinville · 2018-05-17T21:14:50Z

content/en/docs/admin/admission-controllers.md

@@ -9,7 +9,8 @@ reviewers:
 title: Using Admission Controllers
 ---

-{{< toc >}}


This needs to be put back.

mdlinville · 2018-05-17T21:14:59Z

content/en/docs/admin/admission-controllers.md

@@ -1,5 +1,5 @@
 ---
-reviewers:


This needs to be reverted.

mdlinville · 2018-05-18T16:53:37Z

@colemickens The Hugo problems in this PR are already fixed in the release-1.11 branch and this PR somehow is undoing them. In those places, the changes need to be removed from this PR. The PR happened before the Hugo migration and I rebased it to try to preserve the real work in the PR from needing to be redone. If it's easier for the author to redo the work in a new PR based on release-1.11 rather than remove the lines this PR changes that should not now be changed (such as line 2 in content/en/docs/admin/admission-controllers.md) then that is fine too.

mdlinville · 2018-05-18T16:55:18Z

Also I see that the upstream PR this PR depends upon has been closed without merging. Does that mean this one should be closed too?

colemickens · 2018-05-20T01:06:19Z

Yes, I am going to close it. The things that looked like me undoing the Hugo updates should've gone away on their own, had I rebased my own changes, but given the current state, I'm not going to bother and will just close.

sftim · 2020-01-25T16:38:46Z

Somewhat related: PR #18849

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 30, 2018

k8s-ci-robot requested review from tengqm and xiangpengzhao January 30, 2018 02:04

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

colemickens mentioned this pull request Jan 30, 2018

promote PodNodeSelector and PodTolerationRestriction annotations to stable kubernetes/kubernetes#58818

Closed

tengqm requested changes Jan 30, 2018

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 31, 2018

tengqm requested changes Feb 1, 2018

View reviewed changes

k8s-ci-robot assigned tengqm Feb 7, 2018

colemickens mentioned this pull request Feb 23, 2018

promote PodTolerationRestriction admission controller annotations to stable kubernetes/kubernetes#60285

Closed

heckj added this to the 1.10 milestone Feb 24, 2018

heckj added the Docs Review: Open Issues label Feb 27, 2018

Bradamant3 removed this from the 1.10 milestone Mar 8, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2018

Bradamant3 added this to the 1.11 milestone Mar 8, 2018

zacharysarah changed the base branch from master to release-1.11 April 11, 2018 18:37

zacharysarah force-pushed the release-1.11 branch 2 times, most recently from 2231db1 to 7a66f56 Compare April 17, 2018 19:28

zacharysarah force-pushed the release-1.11 branch from 7a66f56 to 2fae1ff Compare April 23, 2018 20:54

zacharysarah force-pushed the release-1.11 branch from 2fae1ff to a9e0792 Compare April 25, 2018 20:07

mdlinville force-pushed the release-1.11 branch from 8cc303d to 6cba902 Compare May 16, 2018 03:08

respond to feedback, bring in alignment with PodNodeSelector docs

6255030

mdlinville force-pushed the podtolerationrestriction-docs branch from 2dcf4b2 to 6255030 Compare May 16, 2018 18:42

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 May 16, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 16, 2018

mdlinville suggested changes May 16, 2018

View reviewed changes

mdlinville reviewed May 17, 2018

View reviewed changes

colemickens closed this May 20, 2018


		PodTolerationRestriction uses a configuration file to define:

		1. A cluster-wide default node selector (this is used if the `` annotation is not present on the target Namespace)


		1. A cluster-wide default node selector (this is used if the `` annotation is not present on the target Namespace)

		2. Namespace-specific default whitelists (these are used if the `` annotation is not present on the target Namespace)


		#### Internal Logic

		1. Determine the `Pod`'s effective tolerations:


		Rejects all requests. AlwaysDeny is DEPRECATED as no real meaning.
		Rejects all requests. Used for testing.

		@@ -316,23 +314,24 @@ In any case, the annotations are provided by the user and are not validated by K

		### Initializers (alpha)

		This admission controller is introduced in v1.7.

improve detail of podtolerationrestriction behavior #7135

improve detail of podtolerationrestriction behavior #7135

Conversation

colemickens commented Jan 30, 2018 • edited by k8s-reviewable Loading

k8sio-netlify-preview-bot commented Jan 30, 2018 • edited Loading

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

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

Choose a reason for hiding this comment

heckj commented Feb 7, 2018

heckj commented Feb 24, 2018

Bradamant3 commented Mar 8, 2018

colemickens commented Mar 8, 2018

Bradamant3 commented Mar 8, 2018

zacharysarah commented Apr 11, 2018

zacharysarah commented May 11, 2018

k8s-ci-robot commented May 16, 2018

k8sio-netlify-preview-bot commented May 16, 2018

mdlinville commented May 16, 2018

mdlinville commented May 16, 2018

mdlinville commented May 16, 2018

mdlinville left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdlinville commented May 17, 2018

colemickens commented May 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdlinville commented May 18, 2018 • edited Loading

mdlinville commented May 18, 2018

colemickens commented May 20, 2018

sftim commented Jan 25, 2020

colemickens commented Jan 30, 2018 •

edited by k8s-reviewable

Loading

k8sio-netlify-preview-bot commented Jan 30, 2018 •

edited

Loading

mdlinville commented May 18, 2018 •

edited

Loading