kubeadm: change service activity check logic in ServiceCheck #60304

mythi · 2018-02-23T13:46:38Z

What this PR does / why we need it:
The PR brings kubeadm improvements.

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 #

Special notes for your reviewer:
The PR changes the service activity check logic in ServiceCheck. It is necessary because kubelet may be running (in the standalone mode) and the user doesn't necessarily now why the other checks (especially Port10250) are failing.

Release note:

kubeadm: change service activity check logic in ServiceCheck

k8s-ci-robot · 2018-02-23T13:46:40Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

mythi · 2018-02-23T15:50:53Z

cnfc-cla should be OK now.

timothysc · 2018-02-23T22:11:48Z

cmd/kubeadm/app/preflight/checks.go

@@ -148,10 +148,16 @@ func (sc ServiceCheck) Check() (warnings, errors []error) {
 				sc.Service, sc.Service))
 	}

-	if sc.CheckIfActive && !initSystem.ServiceIsActive(sc.Service) {
+	if sc.WantActive != initSystem.ServiceIsActive(sc.Service) {
+		status  := [2]string{"active", "not active"}


I'm guessing some tests will need to be updated for this?

make check WHAT="./cmd/kubeadm/app/preflight" GOFLAGS="-v" passes with these changes. I'll need to see why the automated PR checks are failing.

xiangpengzhao · 2018-02-24T03:00:08Z

/ok-to-test

@timothysc FYI: removing needs-ok-to-test manually doesn't trigger tests :)

xiangpengzhao · 2018-02-24T03:04:07Z

@mythi seems like CLA status is not detected...

k8s-ci-robot · 2018-02-26T19:14:51Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

xiangpengzhao · 2018-02-27T07:03:12Z

/retest
Seems like they are test flakes.

xiangpengzhao · 2018-02-27T07:07:25Z

k8s-ci-robot added the cncf-cla: no label 12 hours ago

@mythi your CLA status seems still not correct :)

xiangpengzhao · 2018-02-27T07:09:37Z

      diff -u ./cmd/kubeadm/app/preflight/checks.go.orig ./cmd/kubeadm/app/preflight/checks.go
--- ./cmd/kubeadm/app/preflight/checks.go.orig	2018-02-24 03:22:20.945495368 +0000
+++ ./cmd/kubeadm/app/preflight/checks.go	2018-02-24 03:22:20.945495368 +0000
@@ -115,9 +115,9 @@
 // detect a supported init system however, all checks are skipped and a warning is
 // returned.
 type ServiceCheck struct {
-	Service       string
+	Service    string
 	WantActive bool
-	Label         string
+	Label      string
 }
 
 // Name returns label for ServiceCheck. If not provided, will return based on the service parameter
@@ -149,10 +149,10 @@
 	}
 
 	if sc.WantActive != initSystem.ServiceIsActive(sc.Service) {
-		status  := [2]string{"active", "not active"}
+		status := [2]string{"active", "not active"}
 		command := [2]string{"stop", "start"}
-		index   := 0
-		if (sc.WantActive) {
+		index := 0
+		if sc.WantActive {
 			index = 1
 		}
 		errors = append(errors,

Run ./hack/update-gofmt.sh

mythi · 2018-02-27T08:17:38Z

On Tue, Feb 27, 2018 at 9:08 AM, Peter (XiangPeng) Zhao wrote: k8s-ci-robot added the cncf-cla: no label 12 hours ago @mythi <https://github.com/mythi> your CLA status seems still not correct :)

I noticed this and I'm currently checking why the robot thinks it's not OK. I have it signed.

mythi · 2018-02-27T08:22:04Z

On Tue, Feb 27, 2018 at 9:10 AM, Peter (XiangPeng) Zhao wrote: Run ./hack/update-gofmt.sh

Thanks!

mythi · 2018-02-28T08:50:22Z

/test cla/linuxfoundation

mythi · 2018-03-01T13:29:37Z

/retest

BenTheElder · 2018-03-08T06:44:06Z

Hmm is that the entire output? The last person I talked to with kazel failing just had kubernetes checked out to the wrong location (should be $GOPATH/src/k8s.io/kubernetes)

mythi · 2018-03-08T07:01:55Z

@BenTheElder That was it!

mythi · 2018-03-08T07:07:59Z

/cc @kad

mythi · 2018-03-13T07:36:21Z

retriggering cla check

xiangpengzhao · 2018-03-14T03:12:59Z

retriggering cla check

It works at last! 💯

timothysc

Also please squash commits.

timothysc · 2018-04-05T14:11:30Z

cmd/kubeadm/app/preflight/checks.go

-	if sc.CheckIfActive && !initSystem.ServiceIsActive(sc.Service) {
+	if sc.WantActive != initSystem.ServiceIsActive(sc.Service) {
+		status := [2]string{"active", "not active"}
+		command := [2]string{"stop", "start"}


This parsing of intent from the return is odd to me. I'd think that having a utility function, or augmenting the return arg would be cleaner.

I've changed this.

dims · 2018-04-30T12:33:52Z

/uncc @dims

Currently, the service activity checks logic is either a) I care about the active status and it needs to be active or b) I don't care about the active status This limits the usage and won't allow to check c) I care about the status and it needs to be NOT active The additional check is necessary to be control kubelet is started right time in the bootstrap process. The change in the preflight checks now errors if kubelet is running when kubeadm is called. Additionally, we change to start kubelet later in the bootstrap process. Currently, kubelet is started right after the preflight checks but before the configs are created. At that point, kubeadm has the assumption that kubelet goes in a crash-loop and starts OK after the configs are created. It may be that the crash-loop does not happen but kubelet runs in a standalone mode (e.g., in case if kubelet is started without --bootstrap-kubeconfig). This makes kubeadm to fail due to a timeout in markmaster when running 'kubeadm init'.

mythi · 2018-05-15T13:08:35Z

rebased + addressed the review feedback.

k8s-ci-robot · 2018-05-15T14:00:28Z

@mythi: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-verify	`0ffaf2a`	link	`/test pull-kubernetes-verify`
pull-kubernetes-e2e-gce	`0ffaf2a`	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.

timothysc · 2018-05-16T01:55:12Z

/assign @detiber @stealthybox

detiber · 2018-05-16T02:16:09Z

cmd/kubeadm/app/preflight/checks.go

@@ -987,7 +990,7 @@ func addCommonChecks(execer utilsexec.Interface, cfg kubeadmapi.CommonConfigurat
 		IsPrivilegedUserCheck{},
 		HostnameCheck{nodeName: cfg.GetNodeName()},
 		KubeletVersionCheck{KubernetesVersion: cfg.GetKubernetesVersion(), exec: execer},
-		ServiceCheck{Service: "kubelet", CheckIfActive: false},
+		ServiceCheck{Service: "kubelet", WantActive: false},


This seems to change the behavior of the kubelet check significantly here... It looks like previously CheckIfActive would skip the active check completely, but now with WantActive this is testing specifically for the kublet not being active.

Yes, that's what I'm suggesting. The reasons are explained in the commit message.

Right, but since we are checking for active or activating here wouldn't this preflight check fail by default on any system that has the kubelet enabled/started before running kubeadm (which is the default case on Ubuntu/Debian systems)?

Here's my observation: if kubelet is active, kubeadm init would fail in markmaster timeout (I was hit by this). If kubelet is activating, it's actually crash-looping and can only start/recover once the configurations are created by kubeadm.

So I thought to make it more bullet proof and make sure we start with kubelet not running at all. That's the state where kubeadm reset leaves it too.

@mythi that behavior makes perfect sense to me and is the default behavior when installing from the rpms. However the expected behavior with debs is that the service is enabled and started after installation, which is also the behavior with the current debs that we ship.

While I personally don't agree with the default behavior of enabling/starting daemons on package install, it is the expected behavior on Debian/Ubuntu systems.

That said, I believe the only reason that we are currently checking for active or activating is for the check in kubeadm.

@detiber I'm not sure I got your point wrt. to my change proposal. I don't mind to keep kubelet running as it is today (and I don't mind if this PR is not OK) as long as the behaviour is consistent. Right now you have to have it crash-looping in order to get kubeadm init to succeed. For newcomers like me it'll take some time to realize why active is not sufficient.

@mythi maybe I'm not fully grokking these changes, but it appears to me that the preflight check would currently fail on an Ubuntu host because we are testing that initSystem.ServiceIsActive() returns false for the kubelet, since ServiceIsActive() is checking for 'active' or 'activating' and the kubelet service on Ubuntu will be in 'activating' by default.

@detiber Currently the preflight check for kubelet ignores the activity status (CheckIfActive is set to false).

k8s-ci-robot · 2018-05-19T18:00:20Z

@mythi: PR needs rebase.

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.

mythi · 2018-05-22T04:48:06Z

I don't have the urgent need to get this merged since I know the workaround needed to address the failures I'm seeing. I'm closing this and look into alternatives to improve documentation to ensure people don't hit the same problem.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 23, 2018

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 23, 2018

k8s-ci-robot requested review from dims and xiangpengzhao February 23, 2018 13:46

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 23, 2018

mythi force-pushed the kubeadm branch from 78e7d60 to 464f802 Compare February 23, 2018 15:49

timothysc removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 23, 2018

timothysc self-assigned this Feb 23, 2018

timothysc suggested changes Feb 23, 2018

View reviewed changes

mythi force-pushed the kubeadm branch from 464f802 to eca3ec3 Compare February 26, 2018 19:14

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 26, 2018

mythi force-pushed the kubeadm branch from eca3ec3 to 206d610 Compare February 27, 2018 08:21

k8s-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 Feb 27, 2018

mythi force-pushed the kubeadm branch from 206d610 to f39c1f5 Compare February 28, 2018 07:57

mythi force-pushed the kubeadm branch from f39c1f5 to c6bf7a9 Compare March 1, 2018 13:26

mythi force-pushed the kubeadm branch from 1f02a49 to b034341 Compare March 8, 2018 07:03

k8s-ci-robot requested a review from kad March 8, 2018 07:08

timothysc approved these changes Mar 11, 2018

View reviewed changes

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 13, 2018

mythi force-pushed the kubeadm branch from b034341 to d9552d4 Compare April 5, 2018 10:51

timothysc suggested changes Apr 5, 2018

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2018

k8s-ci-robot removed the request for review from dims April 30, 2018 12:33

mythi force-pushed the kubeadm branch from d9552d4 to 0ffaf2a Compare May 15, 2018 13:06

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 15, 2018

k8s-ci-robot assigned detiber and stealthybox May 16, 2018

detiber reviewed May 16, 2018

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2018

mythi closed this May 22, 2018

kubeadm: change service activity check logic in ServiceCheck #60304

kubeadm: change service activity check logic in ServiceCheck #60304

Conversation

mythi commented Feb 23, 2018 • edited

k8s-ci-robot commented Feb 23, 2018

mythi commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangpengzhao commented Feb 24, 2018

xiangpengzhao commented Feb 24, 2018

k8s-ci-robot commented Feb 26, 2018

xiangpengzhao commented Feb 27, 2018

xiangpengzhao commented Feb 27, 2018

xiangpengzhao commented Feb 27, 2018

mythi commented Feb 27, 2018 via email

mythi commented Feb 27, 2018 via email

mythi commented Feb 28, 2018

mythi commented Mar 1, 2018

BenTheElder commented Mar 8, 2018

mythi commented Mar 8, 2018

mythi commented Mar 8, 2018

mythi commented Mar 13, 2018

xiangpengzhao commented Mar 14, 2018

timothysc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dims commented Apr 30, 2018

mythi commented May 15, 2018

k8s-ci-robot commented May 15, 2018 • edited

timothysc commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 19, 2018

mythi commented May 22, 2018

mythi commented Feb 23, 2018 •

edited

k8s-ci-robot commented May 15, 2018 •

edited