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

Move root filesystem from rootfs to tmpfs #5133

Merged
merged 4 commits into from
Aug 26, 2019

Conversation

afbjorklund
Copy link
Collaborator

This moves the root filesystem of the ISO, from rootfs to tmpfs.

This allows us to stop having to use --no-pivot flag with runc.

The Docker configuration is flexible enough to work with both...

Thanks to @kfox1111 for suggesting a much easier way to do it.

Issue #3512

@afbjorklund afbjorklund requested a review from tstromberg August 19, 2019 17:47
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 19, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afbjorklund

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 19, 2019
@kfox1111
Copy link

Looks good to me. :)

@@ -0,0 +1,11 @@
#!/bin/sh
mkdir /sysroot
mount -t tmpfs -o size=90% tmpfs /sysroot
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment explaining how you arrived at 90% being the correct choice here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That’s easy, I stole it from tinycore (and boot2docker). 😀
I think it needs some space left, so using 100% won’t work. But we could probably get away with 95% since our VM is twice as big ? Can experiment with it

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine. Just mention that you have copied the number from elsewhere then. I was afraid it might have some magical principle to it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll stick with 90%, until I know better. Will leave a reference to the original

Copy link
Contributor

@tstromberg tstromberg left a comment

Choose a reason for hiding this comment

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

This looks great! Minor nits, but please feel free to merge once you feel it is ready.

@@ -0,0 +1,11 @@
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

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

For future archeologists, do you mind adding a quick comment describing why this file exists, and what it's intent is?

Copy link
Member

Choose a reason for hiding this comment

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

future archeologists hahaha ! love it ! hello future archeologists from 2019 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Explaining init(1) ?

Or maybe what is the difference between this init and the normal one would be more appropriate. It doesn’t really help that the final /sbin/init is actually not init but just Lennart (systemd) posing

Copy link
Contributor

Choose a reason for hiding this comment

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

I for one didn't realize this took the place of the system init, though it makes sense given the location.

It'd be nice to have an explanation of why data is being copied from one directory to another before exec'ing out to another init. =)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Settled for adding a reference to switch_root(8)

DESCRIPTION
switch_root moves already mounted /proc, /dev, /sys and /run to newroot and makes newroot the new root filesystem and starts init process.

WARNING: switch_root removes recursively all files and directories on the current root filesystem.

@leseb
Copy link

leseb commented Aug 20, 2019

This should also close #4072. Thanks!

pkg/provision/buildroot.go Outdated Show resolved Hide resolved
@TravisBuddy
Copy link

Travis tests have failed

Hey @afbjorklund,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

View build log

make test
which go-bindata || GO111MODULE=off GOBIN=/home/travis/gopath/bin go get github.com/jteeuwen/go-bindata/...
PATH="/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.12.9.linux.amd64/bin:/home/travis/bin:/home/travis/bin:/home/travis/.local/bin:/usr/local/lib/jvm/openjdk11/bin:/opt/pyenv/shims:/home/travis/.phpenv/shims:/home/travis/perl5/perlbrew/bin:/home/travis/.nvm/versions/node/v8.12.0/bin:/home/travis/.rvm/gems/ruby-2.5.3/bin:/home/travis/.rvm/gems/ruby-2.5.3@global/bin:/home/travis/.rvm/rubies/ruby-2.5.3/bin:/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.11.1.linux.amd64/bin:/usr/local/maven-3.6.0/bin:/usr/local/cmake-3.12.4/bin:/usr/local/clang-7.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin:/home/travis/.rvm/bin:/home/travis/.phpenv/bin:/opt/pyenv/bin:/home/travis/.yarn/bin:/home/travis/gopath/bin" go-bindata -nomemcopy -o pkg/minikube/assets/assets.go -pkg assets deploy/addons/...
gofmt -s -w pkg/minikube/assets/assets.go
which go-bindata || GO111MODULE=off GOBIN=/home/travis/gopath/bin go get github.com/jteeuwen/go-bindata/...
/home/travis/gopath/bin/go-bindata
PATH="/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.12.9.linux.amd64/bin:/home/travis/bin:/home/travis/bin:/home/travis/.local/bin:/usr/local/lib/jvm/openjdk11/bin:/opt/pyenv/shims:/home/travis/.phpenv/shims:/home/travis/perl5/perlbrew/bin:/home/travis/.nvm/versions/node/v8.12.0/bin:/home/travis/.rvm/gems/ruby-2.5.3/bin:/home/travis/.rvm/gems/ruby-2.5.3@global/bin:/home/travis/.rvm/rubies/ruby-2.5.3/bin:/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.11.1.linux.amd64/bin:/usr/local/maven-3.6.0/bin:/usr/local/cmake-3.12.4/bin:/usr/local/clang-7.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin:/home/travis/.rvm/bin:/home/travis/.phpenv/bin:/opt/pyenv/bin:/home/travis/.yarn/bin:/home/travis/gopath/bin" go-bindata -nomemcopy -o pkg/minikube/translate/translations.go -pkg translate translations/...
gofmt -s -w pkg/minikube/translate/translations.go
./test.sh
= go mod ================================================================
ok
= make lint =============================================================
golangci/golangci-lint info checking GitHub for tag 'v1.17.1'
golangci/golangci-lint info found version: 1.17.1 for v1.17.1/linux/amd64
golangci/golangci-lint info installed out/linters/golangci-lint
test/integration/a_download_only_test.go:68:35: GetKubeadmCachedBinaries not declared by package constants (typecheck)
				for _, bin := range constants.GetKubeadmCachedBinaries() {
				                              ^
Makefile:323: recipe for target 'lint-ci' failed
make[1]: *** [lint-ci] Error 1
= boilerplate ===========================================================
ok
= schema_check ==========================================================
ok
= go test ===============================================================
ok  	k8s.io/minikube/cmd/minikube/cmd	0.074s	coverage: 15.7% of statements
ok  	k8s.io/minikube/cmd/minikube/cmd/config	0.051s	coverage: 18.9% of statements
ok  	k8s.io/minikube/pkg/drivers	0.009s	coverage: 19.6% of statements
ok  	k8s.io/minikube/pkg/drivers/kvm	0.066s	coverage: 2.3% of statements
ok  	k8s.io/minikube/pkg/minikube/assets	0.029s	coverage: 61.8% of statements
ok  	k8s.io/minikube/pkg/minikube/bootstrapper	3.087s	coverage: 72.9% of statements
ok  	k8s.io/minikube/pkg/minikube/bootstrapper/kubeadm	0.079s	coverage: 29.5% of statements
ok  	k8s.io/minikube/pkg/minikube/cluster	0.814s	coverage: 54.2% of statements
ok  	k8s.io/minikube/pkg/minikube/config	0.020s	coverage: 76.0% of statements
ok  	k8s.io/minikube/pkg/minikube/cruntime	0.009s	coverage: 62.4% of statements
ok  	k8s.io/minikube/pkg/minikube/extract	0.018s	coverage: 56.7% of statements
ok  	k8s.io/minikube/pkg/minikube/kubeconfig	0.060s	coverage: 75.6% of statements
ok  	k8s.io/minikube/pkg/minikube/logs	0.024s	coverage: 1.5% of statements
ok  	k8s.io/minikube/pkg/minikube/machine	0.031s	coverage: 12.0% of statements
ok  	k8s.io/minikube/pkg/minikube/notify	0.024s	coverage: 77.8% of statements
ok  	k8s.io/minikube/pkg/minikube/out	0.009s	coverage: 70.3% of statements
ok  	k8s.io/minikube/pkg/minikube/problem	0.020s	coverage: 42.9% of statements
ok  	k8s.io/minikube/pkg/minikube/proxy	0.010s	coverage: 67.3% of statements
ok  	k8s.io/minikube/pkg/minikube/registry	0.012s	coverage: 81.8% of statements
ok  	k8s.io/minikube/pkg/minikube/service	0.035s	coverage: 35.9% of statements
ok  	k8s.io/minikube/pkg/minikube/sshutil	0.225s	coverage: 75.0% of statements
ok  	k8s.io/minikube/pkg/minikube/translate	0.010s	coverage: 8.4% of statements
ok  	k8s.io/minikube/pkg/minikube/tunnel	1.842s	coverage: 64.5% of statements
ok  	k8s.io/minikube/pkg/util	0.958s	coverage: 61.6% of statements
ok  	k8s.io/minikube/pkg/util/retry	0.003s	coverage: 0.0% of statements
ok
Makefile:229: recipe for target 'test' failed
make: *** [test] Error 4
TravisBuddy Request Identifier: 0cc8f740-c50b-11e9-8712-75d78f9b457f

@afbjorklund
Copy link
Collaborator Author

@TravisBuddy : Build failed due to #5176

@kfox1111
Copy link

Looks like the travis fix may be merged.
/retest

@medyagh
Copy link
Member

medyagh commented Aug 23, 2019

@TravisBuddy : Build failed due to #5176

@TravisBuddy sorry it was my fault ! should had pulled upstream

The docker configuration is determined at runtime,
so make it work with both old rootfs and new tmpfs.
It is only intended for compatibility with the old
rootfs ISO, and not needed with the new tmpfs ISO.
@tstromberg
Copy link
Contributor

tstromberg commented Aug 24, 2019

PR looks good, but I'm concerned about the apparently reliable Gvisor failure:

FAIL: TestContainerd/GvisorRestart (613.33s)
...
08:44:04 | ! [kubelet-check] Initial timeout of 40s passed.

We'll need to confirm if this issue occurs at master without this PR.

UPDATE: opened #5191

@afbjorklund
Copy link
Collaborator Author

The test results look fairly non-obvious to me.

They seem to fail, no matter if the code is broken (7b5f648) or if it is working (edea8f9) ?
So either I will override and go on pushing broken code, or be afraid to submit working code.

That being said, it works here (don't use gvisor)

@tstromberg tstromberg merged commit b59a8b8 into kubernetes:master Aug 26, 2019
@leseb
Copy link

leseb commented Aug 26, 2019

What's the target release for this? Thanks

@afbjorklund
Copy link
Collaborator Author

v1.4.0

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

8 participants