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

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

Merged
merged 1 commit into from May 8, 2016

Conversation

uluyol
Copy link
Contributor

@uluyol uluyol commented Mar 15, 2016

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

set -e
set -x
REPO = docker.io/uluyol/cross
TAG = musl-1.1.14
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@uluyol
Copy link
Contributor Author

uluyol commented Mar 15, 2016

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

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 15, 2016
@ikehz ikehz assigned thockin and unassigned ikehz Mar 15, 2016
@ikehz
Copy link
Contributor

ikehz commented Mar 15, 2016

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

@uluyol
Copy link
Contributor Author

uluyol commented Mar 15, 2016

Should have linked this earlier: #15140

@uluyol
Copy link
Contributor Author

uluyol commented Mar 16, 2016

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

@luxas
Copy link
Member

luxas commented Mar 16, 2016

@uluyol Thanks, I'll check this out.

@luxas
Copy link
Member

luxas commented Mar 16, 2016

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
Copy link
Contributor Author

uluyol commented Mar 16, 2016

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
Copy link
Member

luxas commented Mar 16, 2016

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.

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@luxas
Copy link
Member

luxas commented Mar 16, 2016

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


TAG = 2.0
#TRIPLE = arm-linux-musleabi
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@uluyol
Copy link
Contributor Author

uluyol commented Mar 20, 2016

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

@uluyol
Copy link
Contributor Author

uluyol commented Mar 20, 2016

Container sizes:

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

#include <unistd.h>

static void sigdown(int signo) {
exit(0);
Copy link
Member

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
Copy link
Member

thockin commented Mar 21, 2016

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.

if (signal(SIGINT, sigdown) == SIG_ERR)
return 1;
if (signal(SIGTERM, sigdown) == SIG_ERR)
return 1;
Copy link
Member

Choose a reason for hiding this comment

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

return 2 here

@thockin
Copy link
Member

thockin commented May 4, 2016

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

@@ -0,0 +1 @@
.build-*
Copy link
Member

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"

@thockin
Copy link
Member

thockin commented May 4, 2016

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

@thockin
Copy link
Member

thockin commented May 4, 2016

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
Copy link
Contributor Author

uluyol commented May 4, 2016

Applied the changes, this is definitely cleaner. PTAL

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

Choose a reason for hiding this comment

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

should be push-* ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@thockin
Copy link
Member

thockin commented May 4, 2016

nit on gitignore.

please squash

@thockin
Copy link
Member

thockin commented May 4, 2016

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

@thockin
Copy link
Member

thockin commented May 4, 2016

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
Copy link
Member

thockin commented May 4, 2016

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

@uluyol uluyol force-pushed the c-pause branch 2 times, most recently from 6d67d46 to e186ab1 Compare May 5, 2016 01:42
Builds statically against glibc. References to the old pause
image have been updated.

push-legacy-too: push
push-legacy: .push-legacy-$(ARCH)
Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

okay, fine

@uluyol
Copy link
Contributor Author

uluyol commented May 6, 2016

@thockin e2e has passed. PTAL

@thockin
Copy link
Member

thockin commented May 6, 2016

LGTM thanks!

@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 6, 2016
@thockin thockin changed the title build/pause: reimplement in C. Reimplement 'pause' in C - smaller footprint all around May 6, 2016
@k8s-bot
Copy link

k8s-bot commented May 8, 2016

GCE e2e build/test passed for commit f3690e2.

@k8s-github-robot
Copy link

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

@uluyol
Copy link
Contributor Author

uluyol commented May 8, 2016

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

@k8s-bot
Copy link

k8s-bot commented May 8, 2016

GCE e2e build/test passed for commit f3690e2.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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.

None yet

10 participants