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

fix node.labels leak in scheduler predicates e2e #26886

Conversation

kevin-wangzefeng
Copy link
Member

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jun 6, 2016
@@ -529,6 +528,14 @@ var _ = framework.KubeDescribe("SchedulerPredicates [Serial]", func() {
labelPod, err := c.Pods(ns).Get(labelPodName)
framework.ExpectNoError(err)
Expect(labelPod.Spec.NodeName).To(Equal(nodeName))

By("removing the label " + k + " off the node " + nodeName)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you wrap this into a function?

Copy link
Member

Choose a reason for hiding this comment

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

probably also worth doing this in a defer, as otherwise it will only run on success.

Copy link
Member

Choose a reason for hiding this comment

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

(or maybe in afterEach?)

@ixdy
Copy link
Member

ixdy commented Jun 6, 2016

I agree with both of @gmarek's comments.

@kevin-wangzefeng
Copy link
Member Author

@k8s-bot e2e test this please, issue #23545

@@ -125,6 +125,24 @@ func cleanupPods(c *client.Client, ns string) {
}
}

func removeLabelOffNode(c *client.Client, nodeName string, labelKey string) {
By("removing the label " + labelKey + " off the node " + nodeName)
framework.RunKubectlOrDie("label", "nodes", nodeName, labelKey+"-")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please do ordinary update? Let's not depend on kubectl in this test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, this line should be removed.
The lines following are in the way ordinary update.

@gmarek gmarek added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 7, 2016
@yujuhong
Copy link
Contributor

yujuhong commented Jun 7, 2016

@k8s-bot node e2e test this issue: #26431

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2016
@kevin-wangzefeng
Copy link
Member Author

@gmarek squashed, PTAL

@davidopp
Copy link
Member

davidopp commented Jun 8, 2016

LGTM

Thanks!

I'll let @gmarek apply the LGTM label if he's satisfied with the changes

@gmarek gmarek added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2016
@davidopp davidopp added this to the v1.3 milestone Jun 12, 2016
@davidopp
Copy link
Member

@k8s-bot unit test this issue: #27205

@davidopp davidopp removed this from the v1.3 milestone Jun 12, 2016
@k8s-bot
Copy link

k8s-bot commented Jun 16, 2016

GCE e2e build/test passed for commit 6c335b9.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jun 18, 2016

GCE e2e build/test passed for commit 6c335b9.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 14e0506 into kubernetes:master Jun 18, 2016
@kevin-wangzefeng kevin-wangzefeng deleted the fix_scheduling_e2e_test_leak branch June 21, 2016 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. 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.

PodAffinity SchedulerPredicates are not well isolated
8 participants