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.

Sign up for GitHub

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

Jump to bottom

Update nodeenv plugin and DS controller to not process openshift.io/node-selector if scheduler.alpha.kubernetes.io/node-selector is set on pods namespace #21033

Merged
merged 2 commits into from
Sep 19, 2018
Merged

Update nodeenv plugin and DS controller to not process openshift.io/node-selector if scheduler.alpha.kubernetes.io/node-selector is set on pods namespace #21033

merged 2 commits into from
Sep 19, 2018

Conversation

aveshagarwal
Copy link
Contributor

@aveshagarwal aveshagarwal commented Sep 19, 2018 •
edited
Loading

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 19, 2018
@aveshagarwal aveshagarwal changed the title Do not process pods that have their namespaces configured with the annotation key scheduler.alpha.kubernetes.io/node-selector in nodeenv admission plugin [WIP] Do not process pods that have their namespaces configured with the annotation key scheduler.alpha.kubernetes.io/node-selector in nodeenv admission plugin Sep 19, 2018
@openshift-ci-robot openshift-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 Sep 19, 2018
smarterclayton
smarterclayton reviewed Sep 19, 2018
View reviewed changes
pkg/project/apiserver/admission/nodeenv/admission.go

// If scheduler.alpha.kubernetes.io/node-selector is set on the pod,
// do not process the pod further.
if len(namespace.ObjectMeta.Annotations) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way this could break an existing cluster?

Copy link
Contributor Author

@aveshagarwal aveshagarwal Sep 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes i think, in case both nodeenv and podnodeselector are being used and that means both openshift.io/node-selector and scheduler.alpha.kubernetes.io/node-selector are configured. For example, lets say pod has k1=v1, and openshift.io/node-selector is k2=v2 and scheduler.alpha.kubernetes.io/node-selector is k3=v3 so result would be all three (k1=v1,k2=v2,k3=v3), but with this PR result would be (k1=v1, k3=v3). Though I am not sure how frequently both nodeenv and podnodeselector are being used together.

As far as I remember, the initial design thoughts were to have possibility of processing both upstream (scheduler.alpha.kubernetes.io/node-selector) and downstream (openshift.io/node-selector) at the same time. So this PR breaks that. But dont see any choice so far with conformance tests and default openshift configuration.

Also I noticed that I need to modify vendor/k8s.io/kubernetes/pkg/controller/daemon/patch_nodeselector.go too to make DS controller in sync with this change, because patched DS controller also takes the same approach i.e. processing both node selectors annotations at the same time.

So I am still testing my changes to vendor/k8s.io/kubernetes/pkg/controller/daemon/patch_nodeselector.go.

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 am curious that have we been not running kube conformance tests regularly? as how we caught the breakage recently only?

@openshift-ci-robot openshift-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 Sep 19, 2018
@aveshagarwal aveshagarwal changed the title [WIP] Do not process pods that have their namespaces configured with the annotation key scheduler.alpha.kubernetes.io/node-selector in nodeenv admission plugin Do not process pods that have their namespaces configured with the annotation key scheduler.alpha.kubernetes.io/node-selector in nodeenv admission plugin Sep 19, 2018
@openshift-ci-robot openshift-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 Sep 19, 2018
@aveshagarwal
Copy link
Contributor Author

I have updated this PR, and so far I have tested it with upstream DS conformance tests with the upstream pr (kubernetes/kubernetes#68793), and DS tests are passing. In case anyone wants to double check, that would be helpful too.

@smarterclayton @derekwaynecarr @sjenning

derekwaynecarr
derekwaynecarr reviewed Sep 19, 2018
View reviewed changes
vendor/k8s.io/kubernetes/pkg/controller/daemon/patch_nodeselector.go
selector, err := labels.Parse(originNodeSelector)
if err == nil {
if !selector.Matches(labels.Set(node.Labels)) {
kubeNodeSelector, ok := ns.Annotations["scheduler.alpha.kubernetes.io/node-selector"]
Copy link
Member

Choose a reason for hiding this comment

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

this should be in a carry commit.

@aveshagarwal
Do not process pods that have their namespaces configured with the an…
9eafa33 
…notation

key scheduler.alpha.kubernetes.io/node-selector in nodeenv admission plugin.
@aveshagarwal
Copy link
Contributor Author

@derekwaynecarr updated. PTAL.

@aveshagarwal aveshagarwal changed the title Do not process pods that have their namespaces configured with the annotation key scheduler.alpha.kubernetes.io/node-selector in nodeenv admission plugin Update nodeenv plugin and DS controller to not process openshift.io/node-selector if scheduler.alpha.kubernetes.io/node-selector is set on pods namespace Sep 19, 2018
@aveshagarwal
Copy link
Contributor Author

Looking into test failures.

@aveshagarwal
UPSTREAM: <carry>: Node selector aware DS controller should not process
f2d0786 
openshift-io/node-selector if scheduler.alpha.kubernetes.io/node-selector is set.
@aveshagarwal
Copy link
Contributor Author

Fixed unit tests, and updated PR.

@aveshagarwal
Copy link
Contributor Author

Current failures dont look like related to this PR, so restarting those tests.

@aveshagarwal
Copy link
Contributor Author

/test end_to_end
/test integration
/test gcp

@aveshagarwal
Copy link
Contributor Author

/test integration
/test gcp

@derekwaynecarr
Copy link
Member

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aveshagarwal, derekwaynecarr

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 19, 2018
@derekwaynecarr
Copy link
Member

/cherrypick release-3.11

@openshift-cherrypick-robot
Copy link

@derekwaynecarr: once the present PR merges, I will cherry-pick it on top of release-3.11 in a new PR and assign it to you.

In response to this:

/cherrypick release-3.11

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.

@sjenning
Copy link
Contributor

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit e9dc2ee into openshift:master Sep 19, 2018
@openshift-cherrypick-robot
Copy link

@derekwaynecarr: #21033 failed to apply on top of branch "release-3.11":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pkg/project/apiserver/admission/nodeenv/admission.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/project/apiserver/admission/nodeenv/admission.go
CONFLICT (content): Merge conflict in pkg/project/apiserver/admission/nodeenv/admission.go
Patch failed at 0001 Do not process pods that have their namespaces configured with the annotation key scheduler.alpha.kubernetes.io/node-selector in nodeenv admission plugin.

In response to this:

/cherrypick release-3.11

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.

@aveshagarwal
Copy link
Contributor Author

Do i need to create the patch manually for 3.11 now?

@derekwaynecarr
Copy link
Member

@aveshagarwal yes

@aveshagarwal aveshagarwal mentioned this pull request Sep 20, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekwaynecarr derekwaynecarr derekwaynecarr left review comments

@smarterclayton smarterclayton smarterclayton left review comments

@deads2k deads2k Awaiting requested review from deads2k

Assignees

@derekwaynecarr derekwaynecarr

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@aveshagarwal @derekwaynecarr @openshift-ci-robot @openshift-cherrypick-robot @sjenning @openshift-bot @smarterclayton @openshift-merge-robot