Disabled ScheduleDaemonSetPods if kubelet version less than 1.11. #69566

k82cn · 2018-10-09T11:23:55Z

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 #69346

Release note:

Disabled ScheduleDaemonSetPods if kubelet version less than 1.11; and ScheduleDaemonSetPods is not supported on a 1.13 control plane / 1.10 kubelet split.

k82cn · 2018-10-09T11:24:41Z

/kind bug

bsalamat

Thanks, Klaus. It looks correct to me, but please add tests. Also, add a comment describing what the new piece of code does and mention the version of K8s at which we should remove this piece of code.

liggitt · 2018-10-09T18:30:06Z

if this isn't included in 1.13, we'll need a prominent release note that daemonsets will not be supported on a 1.13 control plane / 1.10 kubelet split

k82cn · 2018-10-10T07:09:58Z

Also, add a comment describing what the new piece of code does and mention the version of K8s at which we should remove this piece of code.

I created this PR against 1.12; so we do not need to remove it in 1.13 :)

will not be supported on a 1.13 control plane / 1.10 kubelet split

Is that our version policy, only +2 version support?

~~The unit test (TestSimpleDaemonSetScheduleDaemonSetPodsLaunchesPods) failed because dsc.nodeList.Get() does not work in unit test :(. Not sure how to fix right now; keep working on that.~~

That's because we set node's namespace to default :(, also open a PR in master to fix that.

bsalamat · 2018-10-10T17:53:19Z

I created this PR against 1.12; so we do not need to remove it in 1.13 :)

I see.

Is that our version policy, only +2 version support?

Yes, but I think we should still put it in the release notes since we know that kubelet 1.10 is not going to work with 1.13 control plane. Sometimes an unsupported feature may work fine, but in this case we know that it won't work.

test/integration/daemonset/daemonset_test.go

k82cn · 2018-10-11T00:59:35Z

Sometimes an unsupported feature may work fine, but in this case we know that it won't work.

Sure, that's OK to me :)

pkg/controller/daemon/daemon_controller.go

liggitt · 2018-10-12T13:45:53Z

pkg/controller/daemon/daemon_controller_test.go

+			NodeInfo: v1.NodeSystemInfo{
+				// minimum version required to use matchFields
+				KubeletVersion: "v1.11.0",
+			},
 		},
 	}
 }

 func addNodes(nodeStore cache.Store, startIndex, numNodes int, label map[string]string) {


rather than making these a mixture of old and new nodes (which requires modifying existing tests), I'd recommend:

allow control of the node version being added

make addNodes/newNode default to adding a 1.11+ node, which should allow all existing tests to stay as-is

add a new test that adds some new and some old nodes, and specifically check that pods for the new nodes use MatchFields and pods for the old nodes use nodeName

pkg/controller/daemon/daemon_controller_test.go

liggitt · 2018-10-15T04:39:59Z

lgtm, will defer to @bsalamat for taggging

bsalamat

Looks good. Just a couple of minor comments.

bsalamat · 2018-10-15T19:03:08Z

pkg/controller/daemon/daemon_controller_test.go


+// When ScheduleDaemonSetPods is enabled, DaemonSets without node selectors should


Is this a copy/paste of another test's comment?

bsalamat · 2018-10-15T19:03:49Z

pkg/controller/daemon/daemon_controller_test.go

+	}()
+	setFeatureGate(t, features.ScheduleDaemonSetPods, true)
+
+	nodeNum := 5


s/nodeNum/numNodes/

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

feiskyer · 2018-10-22T12:21:26Z

@bsalamat @liggitt Could you get the PR approved (with both lgtm and approved label) by end of Tuesday, Oct 23th? Or else, it will miss v1.12.2.

liggitt · 2018-10-22T13:15:20Z

/lgtm

liggitt · 2018-10-22T13:15:39Z

/approve

k8s-ci-robot · 2018-10-22T18:50:12Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

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:

pkg/controller/daemon/OWNERS
~~test/integration/daemonset/OWNERS~~ [liggitt]

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

feiskyer · 2018-10-23T04:18:03Z

/retest

k8s-ci-robot requested review from janetkuo and lukaszo October 9, 2018 11:24

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 9, 2018

k82cn added this to the v1.12 milestone Oct 9, 2018

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 9, 2018

bsalamat reviewed Oct 9, 2018

View reviewed changes

k82cn force-pushed the k8s_69346 branch from 4cbab45 to 62c75d8 Compare October 10, 2018 07:29

k82cn changed the title ~~WIP: Disabled ScheduleDaemonSetPods if kubelet version less than 1.11.~~ Disabled ScheduleDaemonSetPods if kubelet version less than 1.11. Oct 10, 2018

k8s-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 Oct 10, 2018

k82cn force-pushed the k8s_69346 branch from 62c75d8 to bfa6622 Compare October 10, 2018 08:07

k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Oct 10, 2018

k82cn force-pushed the k8s_69346 branch from bfa6622 to c8ab3ba Compare October 10, 2018 08:16

rphillips mentioned this pull request Oct 10, 2018

Kubernetes v1.12.x doesn't restore pod-checkpointer kubernetes-retired/bootkube#1001

Closed

2 tasks

bsalamat reviewed Oct 10, 2018

View reviewed changes

test/integration/daemonset/daemonset_test.go Show resolved Hide resolved

ravisantoshgudimetla reviewed Oct 12, 2018

View reviewed changes

pkg/controller/daemon/daemon_controller.go Show resolved Hide resolved

ravisantoshgudimetla reviewed Oct 12, 2018

View reviewed changes

pkg/controller/daemon/daemon_controller.go Show resolved Hide resolved

k82cn force-pushed the k8s_69346 branch from c8ab3ba to 33f773c Compare October 12, 2018 06:56

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 12, 2018

liggitt reviewed Oct 12, 2018

View reviewed changes

k82cn force-pushed the k8s_69346 branch from 33f773c to b480fa0 Compare October 15, 2018 04:11

liggitt reviewed Oct 15, 2018

View reviewed changes

pkg/controller/daemon/daemon_controller_test.go Show resolved Hide resolved

bsalamat reviewed Oct 15, 2018

View reviewed changes

Disabled ScheduleDaemonSetPods if kubelet version less than 1.11.

73d252d

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

k82cn force-pushed the k8s_69346 branch from b480fa0 to 73d252d Compare October 16, 2018 02:31

bsalamat added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 22, 2018

k8s-ci-robot assigned liggitt Oct 22, 2018

feiskyer added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. labels Oct 23, 2018

k8s-ci-robot merged commit b17431a into kubernetes:release-1.12 Oct 23, 2018

k82cn deleted the k8s_69346 branch October 23, 2018 13:42

liggitt mentioned this pull request Nov 29, 2018

1.10 kubelet doesn't start any daemonset containers with 1.12 control plane #69346

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disabled ScheduleDaemonSetPods if kubelet version less than 1.11. #69566

Disabled ScheduleDaemonSetPods if kubelet version less than 1.11. #69566

k82cn commented Oct 9, 2018 •

edited

Loading

k82cn commented Oct 9, 2018

bsalamat left a comment

liggitt commented Oct 9, 2018

k82cn commented Oct 10, 2018 •

edited

Loading

bsalamat commented Oct 10, 2018

k82cn commented Oct 11, 2018

liggitt Oct 12, 2018

k82cn Oct 15, 2018

liggitt commented Oct 15, 2018

bsalamat left a comment

bsalamat Oct 15, 2018

k82cn Oct 18, 2018

bsalamat Oct 15, 2018

k82cn Oct 18, 2018

feiskyer commented Oct 22, 2018

liggitt commented Oct 22, 2018

liggitt commented Oct 22, 2018

k8s-ci-robot commented Oct 22, 2018

feiskyer commented Oct 23, 2018


		// When ScheduleDaemonSetPods is enabled, DaemonSets without node selectors should

Disabled ScheduleDaemonSetPods if kubelet version less than 1.11. #69566

Disabled ScheduleDaemonSetPods if kubelet version less than 1.11. #69566

Conversation

k82cn commented Oct 9, 2018 • edited Loading

k82cn commented Oct 9, 2018

bsalamat left a comment

Choose a reason for hiding this comment

liggitt commented Oct 9, 2018

k82cn commented Oct 10, 2018 • edited Loading

bsalamat commented Oct 10, 2018

k82cn commented Oct 11, 2018

liggitt Oct 12, 2018

Choose a reason for hiding this comment

k82cn Oct 15, 2018

Choose a reason for hiding this comment

liggitt commented Oct 15, 2018

bsalamat left a comment

Choose a reason for hiding this comment

bsalamat Oct 15, 2018

Choose a reason for hiding this comment

k82cn Oct 18, 2018

Choose a reason for hiding this comment

bsalamat Oct 15, 2018

Choose a reason for hiding this comment

k82cn Oct 18, 2018

Choose a reason for hiding this comment

feiskyer commented Oct 22, 2018

liggitt commented Oct 22, 2018

liggitt commented Oct 22, 2018

k8s-ci-robot commented Oct 22, 2018

feiskyer commented Oct 23, 2018

k82cn commented Oct 9, 2018 •

edited

Loading

k82cn commented Oct 10, 2018 •

edited

Loading