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

Removed no-empty validation of nodeSelectorTerm.matchExpressions. #62448

Merged
merged 1 commit into from Apr 17, 2018

Conversation

@k82cn
Copy link
Member

k82cn commented Apr 12, 2018

Signed-off-by: Da K. Ma klaus1982.cn@gmail.com

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

Release note:

Pod affinity `nodeSelectorTerm.matchExpressions` may now be empty, and works as previously documented: nil or empty `matchExpressions` matches no objects in scheduler.
@k82cn

This comment has been minimized.

Copy link
Member Author

k82cn commented Apr 12, 2018

/retest

@k82cn

This comment has been minimized.

Copy link
Member Author

k82cn commented Apr 14, 2018

/cc @liggitt

@k8s-ci-robot k8s-ci-robot requested a review from liggitt Apr 14, 2018

@k82cn k82cn changed the title WIP: Removed no-empty validation of nodeSelectorTerm.matchExpressions. Removed no-empty validation of nodeSelectorTerm.matchExpressions. Apr 14, 2018

// if len(term.MatchExpressions) == 0 {
// return append(allErrs, field.Required(fldPath.Child("matchExpressions"), "must have at least one node selector requirement"))
// }

This comment has been minimized.

@liggitt

liggitt Apr 14, 2018

Member

Remove rather than comment?

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Apr 14, 2018

Nit on removing code rather than commenting it out… I don't anticipate this validation returning.

LGTM otherwise, this makes the code behave as documented, and be forward compatible with field selector additions. A second from @kubernetes/api-reviewers would also be good.

Removed no-empty validation of nodeSelectorTerm.matchExpressions.
Signed-off-by: Da K. Ma <klaus1982.cn@gmail.com>

@k82cn k82cn force-pushed the k82cn:k8s_62002 branch from 77d173b to d8e6dbf Apr 14, 2018

@k82cn

This comment has been minimized.

Copy link
Member Author

k82cn commented Apr 14, 2018

/retest

@liggitt liggitt added this to the v1.10 milestone Apr 14, 2018

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Apr 14, 2018

@k82cn

This comment has been minimized.

Copy link
Member Author

k82cn commented Apr 17, 2018

ping @kubernetes/api-reviewers for comments, some other feature dependent on that PR :)

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Apr 17, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 17, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Apr 17, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k82cn, 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

@k82cn

This comment has been minimized.

Copy link
Member Author

k82cn commented Apr 17, 2018

/retest

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Apr 17, 2018

[MILESTONENOTIFIER] Milestone Pull Request Needs Approval

@k82cn @liggitt @kubernetes/sig-scheduling-misc

Action required: This pull request must have the status/approved-for-milestone label applied by a SIG maintainer. If the label is not applied within 4 days, the pull request will be moved out of the v1.10 milestone.

Pull Request Labels
  • sig/scheduling: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/bug: Fixes a bug discovered during the current release.
Help
@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Apr 17, 2018

/retest

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Apr 17, 2018

/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 comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Apr 17, 2018

/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 comment for consistent failures.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Apr 17, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k82cn

This comment has been minimized.

Copy link
Member Author

k82cn commented Apr 17, 2018

/retest

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Apr 17, 2018

Automatic merge from submit-queue (batch tested with PRs 62448, 59317, 59947, 62418, 62352). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 084715a into kubernetes:master Apr 17, 2018

13 of 15 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
pull-kubernetes-e2e-gce Job triggered.
Details
cla/linuxfoundation k82cn authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce 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

@k82cn k82cn deleted the k82cn:k8s_62002 branch Apr 17, 2018

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Apr 18, 2018

Thanks. Can you pick this to 1.10 as well?

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Apr 18, 2018

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

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce d8e6dbf link /test pull-kubernetes-e2e-gce

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.

k8s-github-robot pushed a commit that referenced this pull request Apr 20, 2018

Kubernetes Submit Queue
Merge pull request #62759 from k82cn/automated-cherry-pick-of-#62448-…
…upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #62448: Removed no-empty validation of nodeSelectorTerm.matchExpressions.

Cherry pick of #62448 on release-1.10.

#62448: Removed no-empty validation of nodeSelectorTerm.matchExpressions.

satyasm pushed a commit to satyasm/kubernetes that referenced this pull request Apr 25, 2018

Kubernetes Submit Queue
Merge pull request kubernetes#63067 from k82cn/k8s_63027
Automatic merge from submit-queue (batch tested with PRs 62982, 63075, 63067, 62877, 63141). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Removed e2e test on empty NodeAffinity.

Signed-off-by: Da K. Ma <klaus1982.cn@gmail.com>

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes kubernetes#63027 

**Special notes for your reviewer**:
In kubernetes#62448, we removed the validation on empty `nodeAffinity` which is already handled in scheduler: select no objects.

**Release note**:
```release-note
None
```
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.