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

Add status tracking with loading spinners, clean up logging #69

Merged
merged 4 commits into from
Oct 23, 2018

Conversation

BenTheElder
Copy link
Member

  • Adds a --loglevel flag for setting the logrus log level
  • moves the default log level to warn
  • adds a nice loading spinner system that tracks phases of creating the cluster for friendlier output while waiting for the cluster to boot

TODO (followup):

  • log kubeadm output at trace level (?) after upgrading logrus, or log it to a debug log file
  • give helpful output when kubeadm init fails
  • add preflight checks for docker, previous clusters, etc. add environment diagnostics #39
  • possibly just replace node.Run with RunQ

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 23, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder

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-ci-robot k8s-ci-robot requested a review from munnerz October 23, 2018 03:13
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 23, 2018
@BenTheElder
Copy link
Member Author

cc @Katharine @munnerz

This gives shiny output like:

$ kind create cluster --name=asdf
Creating cluster 'kind-asdf' ...
 ✓ Ensuring node image (kindest/node:v1.11.3@sha256:855562266d5f647b5770a91c76a0579ed6487eb5bf1dfe908705dad285525483) 🖼
 ✓ [kind-asdf-control-plane] Creating node container 📦
 ✓ [kind-asdf-control-plane] Fixing mounts 🗻
 ✓ [kind-asdf-control-plane] Starting systemd 🖥
 ✓ [kind-asdf-control-plane] Waiting for docker to be ready 🐋
 ✓ [kind-asdf-control-plane] Starting Kubernetes (this may take a minute) ☸
Cluster creation complete. You can now use the cluster with:

export KUBECONFIG="/Users/bentheelder/.kube/kind-config-asdf"
kubectl cluster-info

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 23, 2018
@BenTheElder
Copy link
Member Author

trying to record this with asciinema is amusing https://asciinema.org/a/kNsHMXcXpRAj4z0lUqM8l4CRV

@BenTheElder
Copy link
Member Author

2x speed gif: ezgif-1-a66c6f2cdbf8

@BenTheElder
Copy link
Member Author

@neolit123
Copy link
Member

nice 👍

Copy link
Member

@munnerz munnerz left a comment

Choose a reason for hiding this comment

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

This is awesome 😄

My main comment on this, is making sure it's still possible/easy to debug when failures do happen (or if I just like to see lots of log output 😄)

@@ -209,7 +238,7 @@ func tryUntil(until time.Time, try func() bool) bool {
// LoadImages loads image tarballs stored on the node into docker on the node
func (nh *nodeHandle) LoadImages() {
// load images cached on the node into docker
if err := nh.Run(
if err := nh.RunQ(
Copy link
Member

Choose a reason for hiding this comment

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

It'd be awesome if we can make --loglevel=debug also print all of these messages, for debug purposes.

From what I can tell, with this patch there will no longer be any way to debug failures.

Also, if a particular step fails can we make it print out the stdout/stderr of that failed command?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking maybe --loglevel=trace should do this after we upgrade logrus, but debug actually sounds good.

Definitely we should print out failed command output. Especially for kubeadm.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think what we really want is:

  • when critical things fail, inform the user and tell them what they can do about it
  • if the log level is cranked high enough, log even "silent" command output on exit

@BenTheElder
Copy link
Member Author

BenTheElder commented Oct 23, 2018

Lots of log output will still mostly work with this, you can set --loglevel across all commands now.
FWIW, I've done quite a bit of kind debugging now, and the output from the commands we call has not been helpful. I'll be PRing a kind command for obtaining useful debugging info in a follow up. #69 (comment)

@munnerz
Copy link
Member

munnerz commented Oct 23, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2018
@k8s-ci-robot k8s-ci-robot merged commit d3b6960 into kubernetes-sigs:master Oct 23, 2018
@BenTheElder BenTheElder deleted the ux-improvements branch October 23, 2018 20:27
stg-0 pushed a commit to stg-0/kind that referenced this pull request Mar 24, 2023
…elm_chart_offline

Autoscaler helm chart offline installation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants