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

Implement verbosity feature for kubeadm #57661

Merged
merged 1 commit into from Apr 11, 2018

Conversation

@vbmade2000
Contributor

vbmade2000 commented Dec 27, 2017

[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

This comment has been minimized.

Member

xiangpengzhao commented Dec 28, 2017

/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

This comment has been minimized.

Contributor

sttts commented Jan 2, 2018

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

This comment has been minimized.

Member

timothysc commented Jan 5, 2018

I like logrus and we use it on our tools.

@timothysc timothysc assigned timothysc and unassigned jamiehannaford Jan 5, 2018

@vbmade2000

This comment has been minimized.

Contributor

vbmade2000 commented Jan 9, 2018

@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

This comment has been minimized.

Member

dixudx commented Jan 11, 2018

Should We change it to use logrus then ?

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

@vbmade2000

This comment has been minimized.

Contributor

vbmade2000 commented Jan 11, 2018

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

@dixudx

This comment has been minimized.

Member

dixudx commented Jan 11, 2018

Ok. So can I proceed further with glog ?

@sttts Any objection on still using glog?

@jamiehannaford

This comment has been minimized.

Member

jamiehannaford commented Jan 12, 2018

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

This comment has been minimized.

Contributor

vbmade2000 commented Jan 18, 2018

@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

This comment has been minimized.

Contributor

vbmade2000 commented Jan 19, 2018

@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

@@ -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

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 19, 2018

Member

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

This comment has been minimized.

@vbmade2000

vbmade2000 Jan 19, 2018

Contributor

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 :)

}
// 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)

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 19, 2018

Member

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

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 19, 2018

Member

Also, should be: from:

This comment has been minimized.

@vbmade2000

vbmade2000 Jan 19, 2018

Contributor

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

}
// 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")

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 19, 2018

Member

bootstrapping

This comment has been minimized.

@vbmade2000

vbmade2000 Jan 19, 2018

Contributor

Got it.

err := configutil.SetInitDynamicDefaults(cfg)
if err != nil {
return nil, err
}
glog.V(1).Infof("[init] Validating kubernetes version")

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 19, 2018

Member

Kubernetes (capitalised)

This comment has been minimized.

@vbmade2000

vbmade2000 Jan 19, 2018

Contributor

Got it.

// 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.")

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 19, 2018

Member

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

This comment has been minimized.

@vbmade2000

vbmade2000 Jan 19, 2018

Contributor

Sure.

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")

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 19, 2018

Member

nit: configmap

This comment has been minimized.

@vbmade2000

vbmade2000 Jan 19, 2018

Contributor

Got it.

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")

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 19, 2018

Member

ClusterInfo

This comment has been minimized.

@vbmade2000

vbmade2000 Jan 19, 2018

Contributor

Got it.

@timothysc

/approve

@thockin

This comment has been minimized.

Member

thockin commented Mar 5, 2018

This change is Reviewable

@xiangpengzhao

This comment has been minimized.

Member

xiangpengzhao commented Mar 6, 2018

/retest

Implement verbosity feature for kubeadm init
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 k8s-ci-robot added the lgtm label Apr 10, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 10, 2018

[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:

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 10, 2018

/test all

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 11, 2018

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-merge-robot k8s-merge-robot merged commit 0023c41 into kubernetes:master Apr 11, 2018

15 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation vbmade2000 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment