Skip to content

Conversation

@bobfuru
Copy link
Contributor

@bobfuru bobfuru commented Dec 10, 2019

BZ 1781233
Borrowing some content from https://docs.openshift.com/container-platform/4.2/logging/config/cluster-logging-kibana.html#cluster-logging-kibana-tolerations_cluster-logging-kibana to explain taint terminology and usage.

Doc update to be merged to master and CP to enterprise-4.3.

@bobfuru bobfuru added this to the Next Release milestone Dec 10, 2019
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 10, 2019
@bobfuru bobfuru requested a review from huffmanca December 10, 2019 19:14
@bobfuru
Copy link
Contributor Author

bobfuru commented Dec 10, 2019

@huffmanca and @liangxia PTAL

@openshift-docs-preview-bot

The preview will be available shortly at:

@bobfuru bobfuru force-pushed the BZ1781233 branch 2 times, most recently from 1e73c4b to 1585fae Compare December 10, 2019 22:13
Copy link
Contributor Author

@bobfuru bobfuru left a comment

Choose a reason for hiding this comment

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

@huffmanca I've made a few comments and applied your suggestions. Thanks for your feedback!

Copy link
Member

Choose a reason for hiding this comment

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

From oc adm taint --help,
A taint consists of a key, value, and effect. As an argument here, it is expressed as key=value:effect.

Copy link
Contributor Author

@bobfuru bobfuru Dec 11, 2019

Choose a reason for hiding this comment

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

From https://docs.openshift.com/container-platform/4.2/nodes/scheduling/nodes-scheduler-taints-tolerations.html:
An operator ('Equal' in the following example of this module) allows you to leave one of these parameters empty.

I added this clarification as a note just before the prereqs. @liangxia Could I get an ack if this looks good, please?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

User will met https://bugzilla.redhat.com/show_bug.cgi?id=1781520

@liangxia I see that this ^^ bug was closed ("will be fixed in master"). So, does this documentation as written look good from QE perspective?

Copy link
Member

Choose a reason for hiding this comment

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

Fix in master(currently for 4.4) means it will not fix in 4.3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liangxia I updated key:value in tolerations spec to match, as explained in <2> annotation. So, will it work in QE if it uses the Equal operator (as described in the note, "An operator allows you to leave one of these parameters empty")?

Based on BZ1781520, I don't understand if all three (key-value-effect) are required, even with Equal/Exists operator. If so, what do you suggest for doc changes?

Copy link
Member

Choose a reason for hiding this comment

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

  • A taint consists of a key, value, and effect. As an argument here, it is expressed as key=value:effect.
  • The key must begin with a letter or number, and may contain letters, numbers, hyphens, dots, and underscores, up to
    253 characters.
  • Optionally, the key can begin with a DNS subdomain prefix and a single '/', like example.com/my-app
  • The value is optional. If given, it must begin with a letter or number, and may contain letters, numbers, hyphens,
    dots, and underscores, up to 63 characters.
  • The effect must be NoSchedule, PreferNoSchedule or NoExecute.
  • Currently taint can only apply to node.

I would like we state here that the taint is "key=value:effect", so customer does not forgot adding the effect. Or else, customer will met the confusing error message describe in above bug.

Copy link
Member

Choose a reason for hiding this comment

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

And QE confirmed that Equal operator does work as stated below in <2> annotation.

Copy link
Contributor Author

@bobfuru bobfuru Dec 16, 2019

Choose a reason for hiding this comment

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

Thanks, Liang. This makes more sense now. I made a couple of small tweaks to the wording so that it states key=value:effect. PTAL and confirm that it looks good:

[IMPORTANT]
====
Taints and tolerations consist of a key, value, and effect. As an argument, it is expressed as key=value:effect. An operator allows you to leave one of these parameters empty.
====

@bobfuru bobfuru force-pushed the BZ1781233 branch 3 times, most recently from e937905 to c37df9d Compare December 11, 2019 19:54
@bobfuru bobfuru added the peer-review-needed Signifies that the peer review team needs to review this PR label Dec 11, 2019
@bmcelvee
Copy link
Contributor

LGTM!

@bmcelvee bmcelvee added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Dec 11, 2019
@bobfuru bobfuru force-pushed the BZ1781233 branch 3 times, most recently from b47e6fe to 219618d Compare December 16, 2019 14:37
@liangxia
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2019
@bobfuru bobfuru merged commit 6feb3bf into openshift:master Dec 17, 2019
@bobfuru
Copy link
Contributor Author

bobfuru commented Dec 17, 2019

/cherrypick enterprise-4.3

@openshift-cherrypick-robot

@bobfuru: new pull request created: #18693

Details

In response to this:

/cherrypick enterprise-4.3

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.

@bobfuru bobfuru modified the milestones: Next Release, Future Release Dec 17, 2019
@bobfuru bobfuru deleted the BZ1781233 branch February 25, 2020 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.3 lgtm Indicates that a PR is ready to be merged. peer-review-done Signifies that the peer review team has reviewed this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants