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

make nodecontroller delete terminating pods on 1.0 nodes #15930

Merged
merged 1 commit into from
Oct 22, 2015

Conversation

mikedanese
Copy link
Member

TODO: make sure it actually works and then write tests

@k8s-github-robot
Copy link

Labelling this PR as size/L

@smarterclayton
Copy link
Contributor

Other than having one more cache of pods, seems accurate.

@mikedanese
Copy link
Member Author

This works but exposed an issue with mirror pods. Filed #15960

@@ -238,6 +289,48 @@ func (nc *NodeController) getCondition(status *api.NodeStatus, conditionType api
return nil
}

func (nc *NodeController) checkPod(obj interface{}) {
Copy link
Member

Choose a reason for hiding this comment

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

this function needs a name that (1) implies it sometimes deletes pods, (2) if possible tries to describe the conditions under which it deletes pods. If (2) is not possible to express concisely you could just call the function maybeDeletePod()

also, please add a function-level comment explaining the reason for each of the deletion circumstances, or put inline comments for each of the blocks where you call nc.forcefullyDeletePod() explaining the reason

@bgrant0607
Copy link
Member

cc @smarterclayton

@k8s-bot
Copy link

k8s-bot commented Oct 21, 2015

GCE e2e test build/test passed for commit 63060919f055a9e44fe4204943be60814c5631ab.

@mikedanese mikedanese changed the title WIP make nodecontroller delete terminating pods on 1.0 nodes make nodecontroller delete terminating pods on 1.0 nodes Oct 21, 2015
@k8s-bot
Copy link

k8s-bot commented Oct 21, 2015

GCE e2e test build/test passed for commit fd17cd347243f3f2e36fc1b6c174c6a2e1d504d0.

// versions less than 1.1.0
node := nodeObj.(*api.Node)
if strings.HasPrefix(node.Status.NodeInfo.KubeletVersion, "v1.0") {
// delete terminating pods that have been scheudled on
Copy link
Member

Choose a reason for hiding this comment

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

typo: scheduled

@k8s-bot
Copy link

k8s-bot commented Oct 21, 2015

GCE e2e build/test failed for commit 836b683.

@mikedanese
Copy link
Member Author

@k8s-bot ok to test

@k8s-bot
Copy link

k8s-bot commented Oct 21, 2015

GCE e2e test build/test passed for commit 5c5ee8b23fda9ab9a1604abe4b49900b2cdbebc3.

@k8s-bot
Copy link

k8s-bot commented Oct 21, 2015

GCE e2e test build/test passed for commit 836b683.

@davidopp
Copy link
Member

LGTM

@davidopp davidopp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2015
saad-ali added a commit that referenced this pull request Oct 22, 2015
make nodecontroller delete terminating pods on 1.0 nodes
@saad-ali saad-ali merged commit f960b05 into kubernetes:master Oct 22, 2015
@mikedanese mikedanese deleted the nc branch October 22, 2015 19:45
@ikehz
Copy link
Contributor

ikehz commented Oct 24, 2015

@mikedanese Can you please file an issue under kind/workaround-removal for removing this when we drop support for v1.0? Thanks!

@mikedanese
Copy link
Member Author

We want this forever as well, except for the last block

k8s-github-robot pushed a commit that referenced this pull request Oct 24, 2015
…5153-#15900-#15930-upstream-release-1.1

Auto commit by PR queue bot
@ikehz
Copy link
Contributor

ikehz commented Oct 24, 2015

Ack. Then just an issue for the last block?

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
…pick-of-#15153-kubernetes#15900-kubernetes#15930-upstream-release-1.1

Auto commit by PR queue bot
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
…pick-of-#15153-kubernetes#15900-kubernetes#15930-upstream-release-1.1

Auto commit by PR queue bot
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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet