Reimplement 'pause' in C - smaller footprint all around #23009

uluyol · 2016-03-15T20:55:33Z

Statically links against musl. Size of amd64 binary is 3560 bytes.

I couldn't test the arm binary since I have no hardware to test it on, though I assume we want it to work on a raspberry pi.

This PR also adds the gcc5/musl cross compiling image used to build the binaries.

@thockin

uluyol · 2016-03-15T20:56:09Z

build/musl-cross/Makefile

-set -e
-set -x
+REPO = docker.io/uluyol/cross
+TAG = musl-1.1.14


REPO needs to be changed before submission along with docker push command.

uluyol · 2016-03-15T20:58:09Z

I wasn't sure how (if at all) to integrate building these with any other part of the build system.

ikehz · 2016-03-15T21:10:53Z

Reassigning to @thockin for triage. I'll ask, though: why?

uluyol · 2016-03-15T21:13:18Z

Should have linked this earlier: #15140

uluyol · 2016-03-16T04:52:29Z

@k8s-merge-robot you're a little too eager

luxas · 2016-03-16T05:30:57Z

@uluyol Thanks, I'll check this out.

luxas · 2016-03-16T17:09:01Z

First off, why do this in musl? It should be statically linked anyway.
Also it would be good to not use a new musl-cross image but kube-cross.

How much cpu/ram does this consume? 1 RSS? The thing with C should be to reduce the usage to a minimum.
I would also be great to get the binary slighty smaller.
Quoting @thockin

a C program (I just did it) has an RSS of 1 page. The cost is, as you point out, you need a C compiler to rebuild it. If you want to make the binary tiny you also need dietlibc.

Maybe dietlibc would be something to look into?

uluyol · 2016-03-16T17:20:05Z

First off, why do this in musl? It should be statically linked anyway.
Also it would be good to not use a new musl-cross image but kube-cross.

glibc doesn't support static linking and isn't exactly lightweight. Does kube-cross have a C cross-compiler? I was under the impression that it's just for Go.

How much cpu/ram does this consume? 1 RSS? The thing with C should be to reduce the usage to a minimum.
I would also be great to get the binary slighty smaller.

The binary is already smaller than 4KB, I haven't measured the memory usage but the entire binary could fit in a single page.

Maybe dietlibc would be something to look into?

I don't see any advantage to using dietlibc over musl. musl works with kubernetes dns for things that care now and is MIT licensed (dietlibc is GPL). Even if we use dietlibc, we still need a dedicated cross-compiler setup for it so it wouldn't simplify the build process at all.

luxas · 2016-03-16T17:29:44Z

Thanks.
Sorry I read the binary size wrongly (thought it was 3608 kB). Of course 4KB is very good.
The kube-cross image is here. I think it would be a better place to put it in, but it may just be me.

luxas · 2016-03-16T17:32:58Z

build/musl-cross/arm/config.sh

+GCC_CONFFLAGS="--with-arch=armv5 --with-float=softfp"
+
+# Enable this to build the bootstrap gcc (thrown away) without optimization, to reduce build time
+GCC_STAGE1_NOOPT=1


I think we always want to optimize the binary, although it takes a little more time.
The compilation will only be done one or twice, but the image will be used many thousands of times

Building gcc requires building it twice (first build the compiler, then use it to build itself). This disables optimizations when building the throwaway compiler.

Thanks. Noticed that when I tried out the code later on.

luxas · 2016-03-16T18:01:47Z

@uluyol FYI, there should be an image for ppc64le and arm64 too

luxas · 2016-03-16T18:18:16Z

build/pause/Makefile


-TAG = 2.0
+#TRIPLE = arm-linux-musleabi


Please do:

ARCH?=amd64 ifeq($(ARCH),amd64) TRIPLE?=x86_64-linux-musl endif ifeq($(ARCH),arm) TRIPLE?=arm-linux-musleabi endif ifeq($(ARCH),arm64) TRIPLE?=aarch64-linux-musl endif ifeq($(ARCH),ppc64le) TRIPLE?=powerpc-linux-musl # Not sure about the naming here endif

One shouldn't have to edit the file to cross-build, just specify args

uluyol · 2016-03-20T21:44:02Z

musl (and dietlibc) don't have support for ppc64 or arm64 so those continue to use Go.

uluyol · 2016-03-20T21:51:12Z

Container sizes:

pause-arm           4.856 kB
pause-amd64         3.56 kB
pause-ppc64le       1.338 MB
pause-arm64         1.269 MB

thockin · 2016-03-21T04:53:07Z

build/pause/pause.c

+#include <unistd.h>
+
+static void sigdown(int signo) {
+    exit(0);


log the signal received

thockin · 2016-03-21T05:07:54Z

I don't understand the comment about glibc not supporting static compilation, which leads to a right MESS of things. On what planet does compiling a trivial C program with -static not work?

thockin@thockin-glaptop2:~/Downloads$ md5sum pause.c 
65ae4962d8f9baf8ff6c15944f3dacd7  pause.c

thockin@thockin-glaptop2:~/Downloads$ gcc -o pause -Os -static pause.c

thockin@thockin-glaptop2:~/Downloads$ ls -l pause
-rwxr-x--- 1 thockin eng 877289 Mar 20 22:00 pause

thockin@thockin-glaptop2:~/Downloads$ ./pause &
[1] 11877

thockin@thockin-glaptop2:~/Downloads$ ps auxw | head -1
USER       PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND

thockin@thockin-glaptop2:~/Downloads$ ps auxw | grep ./pause
thockin  11877  0.0  0.0   1072     4 pts/22   S    22:00   0:00 ./pause

thockin@thockin-glaptop2:~/Downloads$ kill -TERM %1

thockin@thockin-glaptop2:~/Downloads$ 
[1]-  Done                    ./pause

thockin@thockin-glaptop2:~/Downloads$

The binary is larger than I am happy about but it's pretty inconsequential, and still smaller than the Go binary.

thockin · 2016-03-21T05:09:06Z

build/pause/pause.c

+    if (signal(SIGINT, sigdown) == SIG_ERR)
+        return 1;
+    if (signal(SIGTERM, sigdown) == SIG_ERR)
+        return 1;


return 2 here

thockin · 2016-05-04T00:34:07Z

Looks great! Just a nit and I think you don't mean to edit CHANGELOG

thockin · 2016-05-04T00:39:44Z

build/pause/.gitignore

@@ -0,0 +1 @@
+.build-*


This should be /.build-* - the leading slash means "only in this dir"

thockin · 2016-05-04T00:56:11Z

Actually, I have a Makefile patch for you - something smelled wrong. soon

thockin · 2016-05-04T01:36:33Z

It got sort of big, sorry. Pasting it here. It should apply cleanly to this tree as of right now. Once it's merged into your PR, I can actually build and push the images and we can get this PR in.

pr-23009.diff.txt

uluyol · 2016-05-04T04:51:50Z

Applied the changes, this is definitely cleaner. PTAL

thockin · 2016-05-04T06:11:37Z

build/pause/.gitignore

@@ -0,0 +1,3 @@
+/.container-*
+/.push.*


should be push-* ?

thockin · 2016-05-04T06:17:39Z

nit on gitignore.

please squash

thockin · 2016-05-04T06:25:54Z

I'm going to build and push these to see if e2e needs that to run properly

thockin · 2016-05-04T06:35:20Z

One more bugfix:

diff --git a/build/pause/Makefile b/build/pause/Makefile
index 431e3e6..78f5917 100644
--- a/build/pause/Makefile
+++ b/build/pause/Makefile
@@ -49,7 +49,7 @@ endif
 # If you want to build AND push, see the 'push' rule.
 all: container

-$(BIN)-$(ARCH): bin/$(BIN)-$(ARCH)
+build: bin/$(BIN)-$(ARCH)

 bin/$(BIN)-$(ARCH): $(SRCS)
        mkdir -p bin
@@ -60,7 +60,7 @@ bin/$(BIN)-$(ARCH): $(SRCS)
                        $(TRIPLE)-strip $@"

 container: .container-$(ARCH)
-.container-$(ARCH): $(BIN)-$(ARCH)
+.container-$(ARCH): bin/$(BIN)-$(ARCH)
        docker build -t $(IMAGE):$(TAG) --build-arg ARCH=$(ARCH) .
 ifeq ($(ARCH),amd64)
        docker tag -f $(IMAGE):$(TAG) $(LEGACY_AMD64_IMAGE):$(TAG)

This stops 'push' from doing extra work

thockin · 2016-05-04T06:37:12Z

I have built and pushed these. When you repush with last fixes, we'll see about e2e

Builds statically against glibc. References to the old pause image have been updated.

luxas · 2016-05-05T06:33:16Z

build/pause/Makefile


-push-legacy-too: push
+push-legacy: .push-legacy-$(ARCH)


@thockin I'm not sure if we should push legacy of this image anymore, as we've updated all refs to -amd64:3.0 and the 2:0 still exists.
My point is, since the "old" naming is deprecated, it might be best to not push/refer to the old naming at all, to reduce confusion. However, if you've already pushed it, it's okay
WDYT?

I'd prefer to just leave it for a while.

uluyol · 2016-05-06T01:17:08Z

@thockin e2e has passed. PTAL

thockin · 2016-05-06T03:35:15Z

LGTM thanks!

k8s-bot · 2016-05-08T06:23:42Z

GCE e2e build/test passed for commit f3690e2.

k8s-github-robot · 2016-05-08T15:45:18Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

uluyol · 2016-05-08T16:27:51Z

k8s-merge-robot has asked k8s-bot to test this three times now. Something wrong with the merge process?

k8s-bot · 2016-05-08T16:28:03Z

GCE e2e build/test passed for commit f3690e2.

k8s-github-robot · 2016-05-08T16:29:32Z

Automatic merge from submit-queue

googlebot added the cla: yes label Mar 15, 2016

uluyol reviewed Mar 15, 2016
View reviewed changes

k8s-github-robot assigned ikehz Mar 15, 2016

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 15, 2016

ikehz assigned thockin and unassigned ikehz Mar 15, 2016

uluyol force-pushed the c-pause branch from 309507c to 10abd20 Compare March 16, 2016 04:05

k8s-github-robot added the needs-ok-to-merge label Mar 16, 2016

luxas reviewed Mar 16, 2016
View reviewed changes

uluyol force-pushed the c-pause branch from 10abd20 to f0afb77 Compare March 20, 2016 21:40

uluyol force-pushed the c-pause branch from f0afb77 to 767ea4d Compare March 20, 2016 21:49

thockin reviewed Mar 21, 2016
View reviewed changes

build/pause/pause.c

#include <unistd.h>

static void sigdown(int signo) {

exit(0);

Copy link

Member

thockin Mar 21, 2016

Choose a reason for hiding this comment

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

log the signal received

thockin reviewed Mar 21, 2016
View reviewed changes

thockin reviewed May 4, 2016
View reviewed changes

build/pause/.gitignore

@@ -0,0 +1 @@

.build-*

Copy link

Member

thockin May 4, 2016

Choose a reason for hiding this comment

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

This should be /.build-* - the leading slash means "only in this dir"

uluyol force-pushed the c-pause branch from 4f1329b to a803d00 Compare May 4, 2016 04:47

thockin reviewed May 4, 2016
View reviewed changes

uluyol force-pushed the c-pause branch 2 times, most recently from 6d67d46 to e186ab1 Compare May 5, 2016 01:42

build/pause: write in C

f3690e2

Builds statically against glibc. References to the old pause image have been updated.

uluyol force-pushed the c-pause branch from e186ab1 to f3690e2 Compare May 5, 2016 01:45

luxas reviewed May 5, 2016
View reviewed changes

thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 6, 2016

thockin changed the title ~~build/pause: reimplement in C.~~ Reimplement 'pause' in C - smaller footprint all around May 6, 2016

k8s-github-robot merged commit 8a04506 into kubernetes:master May 8, 2016

luxas mentioned this pull request May 29, 2016

Standardize 'pause' container #15140

Closed

uluyol deleted the c-pause branch September 26, 2016 21:25

wking mentioned this pull request Mar 1, 2019

install: Fix reference to pod image openshift/machine-config-operator#508

Merged


		TAG = 2.0
		#TRIPLE = arm-linux-musleabi

		@@ -0,0 +1,3 @@
		/.container-*
		/.push.*


		push-legacy-too: push
		push-legacy: .push-legacy-$(ARCH)

Reimplement 'pause' in C - smaller footprint all around #23009

Reimplement 'pause' in C - smaller footprint all around #23009

Conversation

uluyol commented Mar 15, 2016

Choose a reason for hiding this comment

uluyol commented Mar 15, 2016

ikehz commented Mar 15, 2016

uluyol commented Mar 15, 2016

uluyol commented Mar 16, 2016

luxas commented Mar 16, 2016

luxas commented Mar 16, 2016

uluyol commented Mar 16, 2016

luxas commented Mar 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas commented Mar 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uluyol commented Mar 20, 2016

uluyol commented Mar 20, 2016

Choose a reason for hiding this comment

thockin commented Mar 21, 2016

Choose a reason for hiding this comment

thockin commented May 4, 2016

Choose a reason for hiding this comment

thockin commented May 4, 2016

thockin commented May 4, 2016

uluyol commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented May 4, 2016

thockin commented May 4, 2016

thockin commented May 4, 2016

thockin commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uluyol commented May 6, 2016

thockin commented May 6, 2016

k8s-bot commented May 8, 2016

k8s-github-robot commented May 8, 2016

uluyol commented May 8, 2016

k8s-bot commented May 8, 2016

k8s-github-robot commented May 8, 2016