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

wip: add experimental kicbase image ubuntu20:04 #7884

Closed
wants to merge 22 commits into from

Conversation

alonyb
Copy link

@alonyb alonyb commented Apr 24, 2020

This PR is to #7788

After this PR:

  • image size is the same than older
  • combine layers to 9 in 4 steps, older had 20 layers in 16 steps, you can see the detail here
  • use latest ubuntu 20.04 (minimal ubuntu focal image)
  • more secure by closing 22 on the default expose

@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 24, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @alonyb. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 24, 2020
@medyagh
Copy link
Member

medyagh commented Apr 24, 2020

please move all your changes to a new folder called /hack/images/kic-base-experimental

then we will experiment with this new image, and meanwhile keep our current image, and over time we will switch to it, after we have done enough testing.

@TravisBuddy
Copy link

Travis tests have failed

Hey @alonyb,
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.

TravisBuddy Request Identifier: 9214c6c0-868c-11ea-afe8-f571977cac28

@codecov-io
Copy link

codecov-io commented Apr 25, 2020

Codecov Report

Merging #7884 into master will decrease coverage by 0.36%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7884      +/-   ##
==========================================
- Coverage   35.87%   35.50%   -0.37%     
==========================================
  Files         148      148              
  Lines        9201     9330     +129     
==========================================
+ Hits         3301     3313      +12     
- Misses       5504     5620     +116     
- Partials      396      397       +1     
Impacted Files Coverage Δ
pkg/minikube/cruntime/containerd.go 28.65% <0.00%> (-14.62%) ⬇️
pkg/minikube/cruntime/cruntime.go 58.06% <0.00%> (-6.23%) ⬇️
cmd/minikube/cmd/node_stop.go 14.28% <0.00%> (-5.72%) ⬇️
cmd/minikube/cmd/node_start.go 10.00% <0.00%> (-2.91%) ⬇️
cmd/minikube/cmd/config/prompt.go 13.41% <0.00%> (-2.08%) ⬇️
cmd/minikube/cmd/delete.go 22.13% <0.00%> (-0.56%) ⬇️
cmd/minikube/cmd/config/configure.go 1.44% <0.00%> (-0.17%) ⬇️
cmd/minikube/cmd/start.go 15.42% <0.00%> (ø)
cmd/minikube/cmd/completion.go 0.00% <0.00%> (ø)
pkg/minikube/download/preload.go 0.00% <0.00%> (ø)
... and 5 more

@medyagh medyagh changed the title Move off kindbase image && change Dockerfile wip: Move off kindbase image && change Dockerfile Apr 25, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 25, 2020
@medyagh medyagh changed the title wip: Move off kindbase image && change Dockerfile wip: add experimental kicbase image ubuntu20:04 Apr 25, 2020
@medyagh
Copy link
Member

medyagh commented Apr 25, 2020

I like to see integration test results on this PR, preferably on your machine or your fork. without touching minikube test code.

@afbjorklund
Copy link
Collaborator

afbjorklund commented Apr 25, 2020

We should split this in several steps, instead of doing all-at-once (even if that is how we test it...)

use latest ubuntu 20.04 (minimal ubuntu focal image)

We can time this with the upgrade to Buildroot 2020.02

more secure by closing 22 on the default expose

This one would be good to include in the next release

I can't see how minikube ssh would work without a port, but we should drop the root changes...

EDIT: it drops the default port, and only uses the tunneled random port that is exposed at runtime

@afbjorklund
Copy link
Collaborator

Aside from the ubuntu upgrade, it will be interesting to see how the layers affect the size...

Currently it is those "middle" layers that are building the size (ubuntu itself is only 73 MB)

docker images gcr.io/k8s-minikube/kicbase:v0.0.10

REPOSITORY                    TAG                 IMAGE ID            CREATED             SIZE
gcr.io/k8s-minikube/kicbase   v0.0.10             e6bc41c39dc4        2 days ago          974MB
  • 974MB (unpacked)

docker history gcr.io/k8s-minikube/kicbase:v0.0.10

IMAGE               CREATED             CREATED BY                                      SIZE                COMMENT
<missing>           2 days ago          /bin/sh -c echo '%sudo ALL=(ALL) NOPASSWD:AL…   784B                
<missing>           2 days ago          /bin/sh -c adduser docker sudo                  2.22kB              
<missing>           2 days ago          /bin/sh -c adduser --ingroup docker --disabl…   10kB                
<missing>           2 days ago          /bin/sh -c sed -ri 's/UsePAM yes/#UsePAM yes…   3.24kB              
<missing>           2 days ago          /bin/sh -c sed -ri 's/^#?PermitRootLogin\s+.…   3.23kB              
<missing>           2 days ago          /bin/sh -c echo 'root:root' |chpasswd           744B                
<missing>           2 days ago          /bin/sh -c systemctl enable docker              34B                 
<missing>           2 days ago          /bin/sh -c apt-get install -y --no-install-r…   573kB               
<missing>           2 days ago          /bin/sh -c apt-get install -y --no-install-r…   158MB               
<missing>           2 days ago          /bin/sh -c sh -c "echo 'deb http://download.…   106MB               
<missing>           8 days ago          /bin/sh -c apt-get update && apt-get install…   425MB               
<missing>           6 weeks ago         RUN |3 CONTAINERD_VERSION=v1.3.3-14-g449e926…   210MB               buildkit.dockerfile.v0
<missing>           6 weeks ago         COPY files/ / # buildkit                        10.5kB              buildkit.dockerfile.v0
  • 158MB (podman)
  • 106MB (cri-o-1.17)
  • 425MB (gnupg sudo docker.io openssh-server dnsutils)
  • 210MB (KIND, including containerd, cni and crictl)

Cleaning up the apt cache for each step should help a bit...

@tstromberg
Copy link
Contributor

Do you mind helping me understand the tangible difference of forking these files? I worry because a lot of thought was put into these files, and they do see a fair bit of maintenance.

  • I see SSH mentioned, but I don't see the change nor understand the attack vector
  • I know folks on exotic platforms like ppcl64e have mentioned a wish to fork so long as kind isn't willing to accept build requests for their platform, but I'm not yet convinced it means we need to maintain our own files.
  • I've heard mention about maintaining our own containerd/docker versions in the past, but this PR doesn't address this.
  • I see layers mentioned, but I'm not sure it means much to an end user.

It's worth mentioning that kind has since also moved on to Ubuntu 20.04.

Thanks!

@afbjorklund
Copy link
Collaborator

This needs a squashing of commits, and a rebase towards upstream (kindbase)...

Should also sync Ubuntu 20.04 a bit with the upgraded files for Buildroot 2020.02 ?
For now you can see some of these changes under #6942 (comment) (no real PR yet)

I think the SSH comments was about the combination of "expose 22" and "chpasswd"

The buildroot-based image is currently 529MB, compared with 974MB for (old) kicbase
But I haven't looked at the new sizes, after squashing some of the layers as mentioned.

I see layers mentioned, but I'm not sure it means much to an end user.

Docker layers have the same issue as git commits, if you add big files and then remove them it ends up keeping them around in the "history" which means a bigger download for the end user.

If you instead clean up and remove things in the same commit layer, only the delta gets added. This leads to a smaller git repository or a smaller docker image, which means faster pull/load

@afbjorklund
Copy link
Collaborator

Added PR #8251 with the just the ubuntu upgrade (that was not mentioned at all in #7788)

I don't think that pinning the package versions is a good idea, so I just removed them all.

@afbjorklund
Copy link
Collaborator

Do you mind helping me understand the tangible difference of forking these files? I worry because a lot of thought was put into these files, and they do see a fair bit of maintenance.

There doesn't seem to be any actual changes done to the entrypoint, besides whitespace:

--- ../kind/images/base/files/usr/local/bin/entrypoint	2020-05-22 15:57:25.315972920 +0200
+++ hack/images/kic-base-experimental/files/entrypoint	2020-05-22 15:56:02.498304340 +0200
@@ -14,12 +14,14 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+# using entrypoint created by kind https://github.com/kubernetes-sigs/kind/blob/master/images/base/files/usr/local/bin/entrypoint
+
 set -o errexit
 set -o nounset
 set -o pipefail
 
 fix_mount() {
-  echo 'INFO: ensuring we can execute /bin/mount even with userns-remap' 
+  echo 'INFO: ensuring we can execute /bin/mount even with userns-remap'
   # necessary only when userns-remap is enabled on the host, but harmless
   # The binary /bin/mount should be owned by root and have the setuid bit
   chown root:root /bin/mount
@@ -193,4 +195,4 @@
 enable_network_magic
 
 # we want the command (expected to be systemd) to be PID1, so exec to it
-exec "$@"
+exec "$@"
\ No newline at end of file

But since everything is now forked and pinned, we now miss all upstream improvements...

i.e. we don't get the new packages from ubuntu

and we don't get the changes from kind entrypoint

Copy link
Collaborator

@afbjorklund afbjorklund left a comment

Choose a reason for hiding this comment

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

I don't think we should fork the kind base (yet?) and I don't think we should pin the packages

gnupg=${GNUPG_VERSION} libglib2.0-0=${LIBGLIB2_VERSION} libseccomp2=${LIBSECCOMP2_VERSION} ca-certificates=${CA_CERTIFICATES_VERSION} \
curl=${CURL_VERSION} rsync=${RSYNC_VERSION} \
&& rm -rf /var/lib/apt/lists/* \
&& sh -c "echo 'deb http://download.opensuse.org/repositories/devel:/kubic:/libcontainers:/stable/xUbuntu_19.10/ /' > /etc/apt/sources.list.d/devel:kubic:libcontainers:stable.list" && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

This still uses 19.10, rather than 20.04

@tstromberg
Copy link
Contributor

Based on this discussion, for now, I think we should just upgrade the underlying kicbase.

@medyagh medyagh reopened this Jun 12, 2020
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 12, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alonyb
To complete the pull request process, please assign tstromberg
You can assign the PR to them by writing /assign @tstromberg in a comment when ready.

The full list of commands accepted by this bot can be found 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

@medyagh
Copy link
Member

medyagh commented Jun 12, 2020

Based on this discussion, for now, I think we should just upgrade the underlying kicbase.

based on coversations in office hours, minikube maintainers reached out to kind to ask for a base image that does not have nightly build containerd but they were not open for this request on the logic that kind is meant for testing lastest source code and not meant for local kuberentes experience.

and also based on the cloud shell bug we ended up sed-ing the entrypoint by line number in the dockerfile
#8465

this is an un-maintable approach, and we will need to start from scratch and mantain a fork of the entrypoint in the source code. therefore I re-open this PR

@codecov-commenter
Copy link

Codecov Report

Merging #7884 into master will decrease coverage by 1.82%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7884      +/-   ##
==========================================
- Coverage   35.87%   34.05%   -1.83%     
==========================================
  Files         148      153       +5     
  Lines        9201     9840     +639     
==========================================
+ Hits         3301     3351      +50     
- Misses       5504     6086     +582     
- Partials      396      403       +7     
Impacted Files Coverage Δ
pkg/minikube/bootstrapper/certs.go 44.26% <0.00%> (-27.05%) ⬇️
pkg/minikube/proxy/proxy.go 77.10% <0.00%> (-19.87%) ⬇️
pkg/minikube/perf/start.go 13.15% <0.00%> (-16.85%) ⬇️
pkg/minikube/cruntime/containerd.go 28.65% <0.00%> (-14.62%) ⬇️
pkg/minikube/machine/delete.go 33.92% <0.00%> (-11.36%) ⬇️
pkg/minikube/config/config.go 41.97% <0.00%> (-8.03%) ⬇️
cmd/minikube/cmd/docker-env.go 41.59% <0.00%> (-7.89%) ⬇️
cmd/minikube/cmd/node_stop.go 13.33% <0.00%> (-6.67%) ⬇️
pkg/minikube/cluster/cluster.go 7.69% <0.00%> (-5.65%) ⬇️
cmd/minikube/cmd/node_add.go 17.85% <0.00%> (-4.88%) ⬇️
... and 54 more

@afbjorklund
Copy link
Collaborator

and also based on the cloud shell bug we ended up sed-ing the entrypoint by line number in the dockerfile
#8465

this is an un-maintable approach, and we will need to start from scratch and mantain a fork of the entrypoint in the source code. therefore I re-open this PR

Arguably that change should not go into the public image, for debugging a specific issue ?

And we don't have to re-open this PR, if all we want to do is fork the kind base (#7788).
The main problem with it was that it was doing several unrelated things at the same time...

@RA489
Copy link

RA489 commented Jul 22, 2020

/check-cla

@afbjorklund
Copy link
Collaborator

@medyagh @alonyb : I think we can close this old obsolete PR now, in favor of #8895 and #9225

@tstromberg tstromberg closed this Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.