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

docker: Improve overlay module check (behavior & UX) #9163

Merged
merged 8 commits into from
Sep 2, 2020

Conversation

tstromberg
Copy link
Contributor

@tstromberg tstromberg commented Sep 2, 2020

Some improvements to the driver performance UX, based on testing in WSL2:

  • Fix & simplify built-in overlay module detection (we had forgotten to strip the newline from the kernel version)
  • Display suggested performance improvement in a single-line of output again
  • Docker for Desktop is now known as Docker Desktop: https://www.docker.com/products/docker-desktop

Old UX


💨  The 'docker' driver reported a performance issue
💡  Suggestion: enable overlayfs kernel module on your Linux

New UX

💨 For improved Docker performance, enable the overlay Linux kernel module using 'modprobe overlay'

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 2, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tstromberg

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2020
@k8s-ci-robot 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 Sep 2, 2020
@tstromberg tstromberg requested review from priyawadhwa and sharifelgamal and removed request for prasadkatti and afbjorklund September 2, 2020 18:06
@tstromberg tstromberg changed the title UX improvements for driver performance advice, especially overlayfs Fix broken overlay kernel module check, improve UX Sep 2, 2020
@tstromberg tstromberg changed the title Fix broken overlay kernel module check, improve UX docker: Fix broken overlay check when built-in to kernel, improve UX Sep 2, 2020
@tstromberg tstromberg changed the title docker: Fix broken overlay check when built-in to kernel, improve UX docker: Improve overlay module check (behavior & UX) Sep 2, 2020
@tstromberg
Copy link
Contributor Author

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Sep 2, 2020
@minikube-pr-bot
Copy link

kvm2 Driver
error collecting results for kvm2 driver: timing run 0 with Minikube (PR 9163): timing cmd: [/home/performance-monitor/.minikube/minikube-binaries/9163/minikube start --driver=kvm2]: starting cmd: fork/exec /home/performance-monitor/.minikube/minikube-binaries/9163/minikube: exec format error
docker Driver
error collecting results for docker driver: timing run 0 with Minikube (PR 9163): timing cmd: [/home/performance-monitor/.minikube/minikube-binaries/9163/minikube start --driver=docker]: starting cmd: fork/exec /home/performance-monitor/.minikube/minikube-binaries/9163/minikube: exec format error

Copy link
Member

@medyagh medyagh left a comment

Choose a reason for hiding this comment

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

this PR is removing the Second way of verifying if the first way is failing, which is bad
because on some distros (that I tried) the first way fails but the second way works

path := fmt.Sprintf("/lib/modules/%s/modules.builtin", string(out))

@tstromberg
Copy link
Contributor Author

tstromberg commented Sep 2, 2020 via email

@tstromberg tstromberg merged commit dab20a2 into kubernetes:master Sep 2, 2020
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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants