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

Update v1.Taint parser to accept the form `key:effect` and `key=:effect-` #74159

Merged

Conversation

@dlipovetsky
Copy link
Contributor

commented Feb 16, 2019

/kind bug

What this PR does / why we need it:
Updates v1.Taint parser to accept the form key:effect, which is the string representation of a v1.Taint that has a nil Value (the empty string).

Also updates the parser to accept the form key=:effect-, since key=:effect is a valid string representation of a v1.Taint with a nil Value.

Which issue(s) this PR fixes:
Fixes #73249

Does this PR introduce a user-facing change?:

Support parsing more v1.Taint forms. `key:effect`, `key=:effect-` are now accepted.

/cc @liggitt

@dlipovetsky

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2019

/sig scheduling

@dlipovetsky

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2019

@neolit123
Copy link
Member

left a comment

thanks @dlipovetsky
/priority important-longterm

could you please add a user facing release note instead of NONE? e.g.

Support parsing v1.Taint forms such as xx, yy

@dlipovetsky dlipovetsky force-pushed the dlipovetsky:issue-73249-revise-parsetaint branch from 2899f57 to ce41194 Feb 16, 2019

@dlipovetsky

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2019

Thanks @neolit123! Added release note.

@dlipovetsky dlipovetsky force-pushed the dlipovetsky:issue-73249-revise-parsetaint branch from 72fc753 to ef0c768 Feb 16, 2019

@dlipovetsky

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2019

I added two new invalid spec tests and fixed a bug in the updated parser. (I also verified that the original parser passed these new tests)

@dlipovetsky

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

Once the changes to pkg/util/taints/ are finalized, I'll make corresponding changes to pkg/kubectl/cmd/taint/.

@Huang-Wei
Copy link
Member

left a comment

LGTM for the pkg/util/taints/ part.

@kevin-wangzefeng

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

LGTM for pkg/util/taints/, I think you can go ahead for the rest parts.

@dlipovetsky

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Thanks @kevin-wangzefeng and @Huang-Wei.

After I updated parseTaints and parseTaint in pkg/kubectl/cmd/taint/util.go, an existing test in pkg/kubectl/cmd/taint/taint_test.go failed:

--- FAIL: TestTaint (0.03s)
    --- FAIL: TestTaint/node_has_two_taints_with_the_same_key_but_different_effect,_remove_all_of_them_with_wildcard (0.00s)
        taint_test.go:313: Recovered: error: invalid taint spec: dedicated
            See 'taint -h' for help and examples
        taint_test.go:332: node has two taints with the same key but different effect, remove all of them with wildcard: node not tainted

I did not realize that kubectl taint accepts a key with no value or effect to remove a taint; the example in the kubectl usage is:

  # Remove from node 'foo' all the taints with key 'dedicated'
  kubectl taint nodes foo dedicated-

This use case was introduced in #30590. To be backward compatible, this PR should not break this use case.

There was no test in pkg/util/taints for removing a taint using the key- form. The parser prior to this PR accepts this form, but the parser in this PR rejects this form. My plan is to:

  1. Add a test to pkg/util/taints for removing a taint using the key- form
  2. Update the parser in this PR to accept the key- form
@dlipovetsky

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

The last commit passes the tests in pkg/util/taints and pkg/kubectl/cmd/taint/.

I have changed parseTaint to accept the key form, and ensured in ParseTaints that a taint with no effect cannot be added. The alternative is for parseTaint to continue rejecting this form, and handling the form as a special case in ParseTaints. I prefer the former, because the code already passes around taints without values or effects, which implies that the key form is valid, although it must not be used to create a taint.

@kevin-wangzefeng

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

I'm fine with it. Empty/null value and effect should not block parsing, though taints with only keys indicated should not be used for creating.

@bsalamat

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

I reviewed and it looks fine. I have only a few minor comments. If @dlipovetsky addresses them sooner, we can get it merged.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 11, 2019

@dlipovetsky

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

@bsalamat Thanks a lot for your review. My apologies for taking a while to address your comments. I address them using separate commits for the code in pkg/util/taints and pkg/kubectl/cmd/taint, so that I can easily fixup the original commits for those packages before merging.

@dlipovetsky

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

I also added a test to verify that the empty string is an invalid Taint spec.

@dlipovetsky

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

Test failed due to known flake, see #74931

/retest

@bsalamat

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

Thanks, @dlipovetsky! Looks good. Could you please squash commits?

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 11, 2019

@bsalamat

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

I updated the release note. Please check it.

dlipovetsky added some commits Feb 16, 2019

Update v1.Taint parser to accept `key:effect`, `key=:effect-`, `key`,…
… and `key-` forms

Also add missing tests for `key=:value` form.
Update kubectl taint usage
- Explain that the value is optional.
- Add example of adding a taint with no value to kubectl taint usage.

@dlipovetsky dlipovetsky force-pushed the dlipovetsky:issue-73249-revise-parsetaint branch from 3402228 to ab6d901 Mar 11, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 11, 2019

@dlipovetsky

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

@bsalamat Squashed.

As for the release note, I'm not sure it needs to mention the key- form, since that has always been accepted--this PR just changes where its parsed internally (please see #74159 (comment)).

@bsalamat

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

@dlipovetsky Thanks again. I fixed the release notes.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 11, 2019

@bsalamat

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

@liggitt Back to you for approval.

@dlipovetsky

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

/assign @liggitt

(So that this PR shows up in your dashboard 😄)

@liggitt

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, dlipovetsky, liggitt

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:

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

@dlipovetsky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

@ravisantoshgudimetla Please cancel the hold. Thanks!

@ravisantoshgudimetla

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

/hold cancel

Done @dlipovetsky. Thanks for working on this PR.

@fejta-bot

This comment has been minimized.

Copy link

commented Apr 1, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

2 similar comments
@fejta-bot

This comment has been minimized.

Copy link

commented Apr 2, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

commented Apr 2, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 7d15d41 into kubernetes:master Apr 2, 2019

17 checks passed

cla/linuxfoundation dlipovetsky authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.