Implement verbosity feature for kubeadm #57661

vbmade2000 · 2017-12-27T17:18:31Z

[WIP] Adds verbosity feature to init command hierarchy of kubeadm utility.

What this PR does / why we need it:
Implements verbosity feature to kubeadm

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 kubernetes/kubeadm#340

Special notes for your reviewer:
I will be splitting this work into a smaller PR to keep it separate and clean.

Release note:

Implements verbosity logging feature for kubeadm commands

xiangpengzhao · 2017-12-28T03:49:00Z

/ok-to-test

luxas

cc @kubernetes/sig-cluster-lifecycle-pr-reviews @ncdc @thockin @sttts @kubernetes/sig-cli-pr-reviews

What kind of logging solution would you prefer for kubeadm to implement?

sttts · 2018-01-02T06:48:16Z

What kind of logging solution would you prefer for kubeadm to implement?

glog is still our logging library. Structured logging is mentioned once in a while, also at KubeCon again. But I haven't seen further investment into that.

timothysc · 2018-01-05T22:29:50Z

I like logrus and we use it on our tools.

vbmade2000 · 2018-01-09T16:55:21Z

@timothysc Should We change it to use logrus then ? I am waiting for some conclusion so that I can work further on this issue.

dixudx · 2018-01-11T03:08:21Z

Should We change it to use logrus then ?

@vbmade2000 Not now. Currently we still use glog in Kubernetes.

vbmade2000 · 2018-01-11T04:29:02Z

@dixudx Ok. So can I proceed further with glog ?

dixudx · 2018-01-11T07:58:49Z

Ok. So can I proceed further with glog ?

@sttts Any objection on still using glog?

jamiehannaford · 2018-01-12T00:55:37Z

Since logrus is already listed as a dep, and the eventual aim is for kubeadm to live in its own repo, is there any reason why not to use it if most of us deem it a better tool? Just wondering if adhering to precedent is worth it here.

vbmade2000 · 2018-01-18T14:08:15Z

@luxas @timothysc Any conclusion ? This issue is pending since long time.

timothysc

I think it's fine to stay with glog. I'm not a fan of the explicit leveling for most of the default statements, they should all be Infof for now and we can decide later what their verbosity is.

vbmade2000 · 2018-01-19T17:23:00Z

@timothysc Sure.

I'm not a fan of the explicit leveling for most of the default statements, they should all be Infof for now.
Didn't get it.

jamiehannaford

Thanks for submitting this! Generally LGTM, but I think we should document flag usage, especially the different verbosity levels

jamiehannaford · 2018-01-19T20:33:37Z

cmd/kubeadm/app/cmd/init.go

@@ -120,6 +121,7 @@ func NewCmdInit(out io.Writer) *cobra.Command {
 		Short: "Run this command in order to set up the Kubernetes master.",
 		Run: func(cmd *cobra.Command, args []string) {
 			var err error
+


nit: can you remove these extra lines? doesn't seem related to changeset. prefer to keep diffs small :)

Sure. This is not a final push. This is just to show approach and get confirmation from members but thanks, I'll keep that in mind :)

jamiehannaford · 2018-01-19T20:34:18Z

cmd/kubeadm/app/cmd/init.go

 }

 // NewInit validates given arguments and instantiates Init struct with provided information.
 func NewInit(cfgPath string, cfg *kubeadmapi.MasterConfiguration, ignorePreflightErrors sets.String, skipTokenPrint, dryRun bool, criSocket string) (*Init, error) {

 	if cfgPath != "" {
+		glog.V(1).Infof("[init] Reading config file from : " + cfgPath)


So we only have two verbosity levels? 0 (default) and 1, right? might be good to document this somewhere

Also, should be: from:

Good observation for from:. I'll take care of that from now on.

jamiehannaford · 2018-01-19T20:36:18Z

cmd/kubeadm/app/cmd/init.go

 	}

 	// Temporarily set cfg.CertificatesDir to the "real value" when writing controlplane manifests
 	// This is needed for writing the right kind of manifests
 	i.cfg.CertificatesDir = realCertsDir

 	// PHASE 3: Bootstrap the control plane
+	glog.V(1).Infof("[init] Bootstraping the control plane")


bootstrapping

jamiehannaford · 2018-01-19T20:36:58Z

cmd/kubeadm/app/cmd/init.go

 	err := configutil.SetInitDynamicDefaults(cfg)
 	if err != nil {
 		return nil, err
 	}

+	glog.V(1).Infof("[init] Validating kubernetes version")


Kubernetes (capitalised)

jamiehannaford · 2018-01-19T20:37:39Z

cmd/kubeadm/app/cmd/init.go


 	// Warn about the limitations with the current cloudprovider solution.
 	if cfg.CloudProvider != "" {
-		fmt.Println("[init] WARNING: For cloudprovider integrations to work --cloud-provider must be set for all kubelets in the cluster.")
-		fmt.Println("\t(/etc/systemd/system/kubelet.service.d/10-kubeadm.conf should be edited for this purpose)")
+		glog.Warningln("[init] For cloudprovider integrations to work --cloud-provider must be set for all kubelets in the cluster.")


Here you're terminating with a period ., but in other places you remove them. Can we pick one style and make that consistent?

jamiehannaford · 2018-01-19T20:38:54Z

cmd/kubeadm/app/cmd/init.go

 	if err := nodebootstraptokenphase.AutoApproveNodeCertificateRotation(client); err != nil {
 		return err
 	}

 	// Create the cluster-info ConfigMap with the associated RBAC rules
+	glog.V(1).Infof("[init] Creating bootstrap config map")


nit: configmap

jamiehannaford · 2018-01-19T20:39:09Z

cmd/kubeadm/app/cmd/init.go

 	if err := clusterinfophase.CreateBootstrapConfigMapIfNotExists(client, adminKubeConfigPath); err != nil {
 		return fmt.Errorf("error creating bootstrap configmap: %v", err)
 	}
+	glog.V(1).Infof("[init] Creating ClsuterInfo RBAC rules")


ClusterInfo

timothysc

/approve

thockin · 2018-03-05T15:44:19Z

This change is

xiangpengzhao · 2018-03-06T02:52:22Z

/retest

Fixes #340 Adds functionality to see logs with various level of verbosity. Currently there are two verbosity levels: 0 and 1

timothysc

Thank you for the patch and your patience.

/lgtm
/approve

k8s-ci-robot · 2018-04-10T14:02:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: timothysc, vbmade2000

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:

~~cmd/kubeadm/OWNERS~~ [timothysc]

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

k8s-github-robot · 2018-04-10T14:03:28Z

/test all

Tests are more than 96 hours old. Re-running tests.

k8s-github-robot · 2018-04-11T05:53:17Z

Automatic merge from submit-queue (batch tested with PRs 59027, 62333, 57661, 62086, 61584). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-github-robot assigned jamiehannaford and krousey Dec 27, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 28, 2017

luxas reviewed Dec 30, 2017

View reviewed changes

k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Dec 30, 2017

timothysc assigned timothysc and unassigned jamiehannaford Jan 5, 2018

timothysc suggested changes Jan 19, 2018

View reviewed changes

jamiehannaford suggested changes Jan 19, 2018

View reviewed changes

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

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 3, 2018

timothysc approved these changes Mar 5, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 5, 2018

Implement verbosity feature for kubeadm init

9c4706f

Fixes #340 Adds functionality to see logs with various level of verbosity. Currently there are two verbosity levels: 0 and 1

timothysc approved these changes Apr 10, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 10, 2018

k8s-github-robot merged commit 0023c41 into kubernetes:master Apr 11, 2018

Implement verbosity feature for kubeadm #57661

Implement verbosity feature for kubeadm #57661

Conversation

vbmade2000 commented Dec 27, 2017 • edited Loading

xiangpengzhao commented Dec 28, 2017

luxas left a comment

Choose a reason for hiding this comment

sttts commented Jan 2, 2018

timothysc commented Jan 5, 2018

vbmade2000 commented Jan 9, 2018

dixudx commented Jan 11, 2018

vbmade2000 commented Jan 11, 2018

dixudx commented Jan 11, 2018

jamiehannaford commented Jan 12, 2018

vbmade2000 commented Jan 18, 2018

timothysc left a comment

Choose a reason for hiding this comment

vbmade2000 commented Jan 19, 2018

jamiehannaford left a 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

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

Choose a reason for hiding this comment

timothysc left a comment

Choose a reason for hiding this comment

thockin commented Mar 5, 2018

xiangpengzhao commented Mar 6, 2018

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 10, 2018

k8s-github-robot commented Apr 10, 2018

k8s-github-robot commented Apr 11, 2018

vbmade2000 commented Dec 27, 2017 •

edited

Loading