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

make: strip symbol tables from all binaries by default #10167

Merged
merged 1 commit into from Mar 17, 2020

Conversation

tklauser
Copy link
Member

@tklauser tklauser commented Feb 12, 2020

Marked RFC because I'm not sure this is something we want or should do by default. While it doesn't break panic backtraces it makes debugging of production systems harder or even impossible.

Feedback appreciated @tgraf @aanm @joestringer @raybejjani


Strip the symbol tables and debug information from all binaries by
default. This will not remove annotations needed for stack traces, so
panic backtraces will still be readable.

If debug information and symbol tables are needed during debugging, use
make NOSTRIP=1 to compile.

This reduces the binary sizes as follows:

before:

-rwxr-xr-x 1 tklauser tklauser 45M Feb 27 12:35 cilium/cilium
-rwxr-xr-x 1 tklauser tklauser 20M Feb 27 12:35 cilium-health/cilium-health
-rwxr-xr-x 1 tklauser tklauser 70M Feb 27 12:35 daemon/cilium-agent
-rwxr-xr-x 1 tklauser tklauser 74M Feb 27 12:35 operator/cilium-operator
-rwxr-xr-x 1 tklauser tklauser 20M Feb 27 12:35 plugins/cilium-cni/cilium-cni

after:

-rwxr-xr-x 1 tklauser tklauser 45M Feb 27 12:37 cilium/cilium
-rwxr-xr-x 1 tklauser tklauser 15M Feb 27 12:37 cilium-health/cilium-health
-rwxr-xr-x 1 tklauser tklauser 51M Feb 27 12:37 daemon/cilium-agent
-rwxr-xr-x 1 tklauser tklauser 54M Feb 27 12:37 operator/cilium-operator
-rwxr-xr-x 1 tklauser tklauser 15M Feb 27 12:37 plugins/cilium-cni/cilium-cni

summary:

cilium           0MB (already stripped previously, see 8596bd9c)
cilium-agent    ~19MB
cilium-health   ~5MB
cilium-operator ~20MB
cilium-cni      ~5MB

Also see https://blog.filippo.io/shrink-your-go-binaries-with-this-one-weird-trick/

Updates #6099
Updates #10056

Signed-off-by: Tobias Klauser tklauser@distanz.ch


This change is Reviewable

@tklauser tklauser requested a review from a team as a code owner February 12, 2020 15:33
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Feb 12, 2020
@tklauser tklauser added the dont-merge/preview-only Only for preview or testing, don't merge it. label Feb 12, 2020
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@coveralls
Copy link

coveralls commented Feb 12, 2020

Coverage Status

Coverage increased (+0.02%) to 45.549% when pulling 7f89309 on pr/tklauser/strip-symbols-tables-by-default into b676c4c on master.

@aanm aanm added this to the 1.8 milestone Feb 18, 2020
@tklauser tklauser force-pushed the pr/tklauser/strip-symbols-tables-by-default branch from c55e911 to f7db7ae Compare February 21, 2020 13:11
@tklauser
Copy link
Member Author

test-me-please

@tgraf
Copy link
Member

tgraf commented Feb 21, 2020

Does this have an impact on the memory consumption of cilium-agent or only on binary size?

@tklauser
Copy link
Member Author

tklauser commented Feb 21, 2020

Does this have an impact on the memory consumption of cilium-agent or only on binary size?

AFAICS it only affects binary size (i.e. for #6099). Memory consumption is more or less the same.

@joestringer
Copy link
Member

it makes debugging of production systems harder or even impossible

panic backtraces will still be readable.

Do you have example production debug workflows that you're concerned we will be unable to follow if we merge this PR? I think that these would help us to make a decision.

Most debugging workflows I've used involve the cilium-sysdump and the information you gather there; panic backtraces would be an exception, but as you note these should continue to work.

For instance, does this break gops?

Strip the symbol tables and debug information from all binaries by
default. This will not remove annotations needed for stack traces, so
panic backtraces will still be readable.

If debug information and symbol tables are needed during debugging, use
`make NOSTRIP=1` to compile.

This reduces the binary sizes (and thus the base memory consumption) as
follows:

before:

    -rwxr-xr-x 1 tklauser tklauser 46M Feb 21 14:05 cilium/cilium
    -rwxr-xr-x 1 tklauser tklauser 21M Feb 21 14:05 cilium-health/cilium-health
    -rwxr-xr-x 1 tklauser tklauser 71M Feb 21 14:05 daemon/cilium-agent
    -rwxr-xr-x 1 tklauser tklauser 74M Feb 21 14:05 operator/cilium-operator
    -rwxr-xr-x 1 tklauser tklauser 46M Feb 21 14:04 plugins/cilium-cni/cilium-cni

after:

    -rwxr-xr-x 1 tklauser tklauser 46M Feb 21 14:08 cilium/cilium
    -rwxr-xr-x 1 tklauser tklauser 15M Feb 21 14:08 cilium-health/cilium-health
    -rwxr-xr-x 1 tklauser tklauser 52M Feb 21 14:08 daemon/cilium-agent
    -rwxr-xr-x 1 tklauser tklauser 55M Feb 21 14:08 operator/cilium-operator
    -rwxr-xr-x 1 tklauser tklauser 34M Feb 21 14:08 plugins/cilium-cni/cilium-cni

summary:

    cilium           0MB (already stripped previously, see 8596bd9)
    cilium-agent    ~19MB
    cilium-health   ~6MB
    cilium-operator ~19MB
    cilium-cni      ~12MB

Also see https://blog.filippo.io/shrink-your-go-binaries-with-this-one-weird-trick/

Updates #6099
Updates #10056

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser
Copy link
Member Author

Do you have example production debug workflows that you're concerned we will be unable to follow if we merge this PR? I think that these would help us to make a decision.

I don't have a lot of experience of debugging Cilium in production yet, so I cannot really name an example where debug symbols would be useful. AFAICT panic backtraces, gops and cilium-sysdump would probably be enough for me to get a first idea.

I just wanted to raise this in order to make others aware that e.g. debugging using gdb or delve would no longer work on production binaries.

For instance, does this break gops?

gops will still work AFAICS, e.g. the stackcommand, the top10 and even the list <symbol> command in pprof-heap still resolve the function names.

@aanm
Copy link
Member

aanm commented Feb 27, 2020

Do you have example production debug workflows that you're concerned we will be unable to follow if we merge this PR? I think that these would help us to make a decision.

I don't have a lot of experience of debugging Cilium in production yet, so I cannot really name an example where debug symbols would be useful. AFAICT panic backtraces, gops and cilium-sysdump would probably be enough for me to get a first idea.

I just wanted to raise this in order to make others aware that e.g. debugging using gdb or delve would no longer work on production binaries.

Why? Wouldn't it work even if ship the unstripped binary and have it available to be used whenever we need to use delve and / or gdb? AFAIK, delve allows to pass a binary for a running process.

For instance, does this break gops?

gops will still work AFAICS, e.g. the stackcommand, the top10 and even the list <symbol> command in pprof-heap still resolve the function names.

@tklauser tklauser force-pushed the pr/tklauser/strip-symbols-tables-by-default branch from f7db7ae to 7f89309 Compare February 27, 2020 11:41
@tklauser
Copy link
Member Author

test-me-please

@tklauser
Copy link
Member Author

Why? Wouldn't it work even if ship the unstripped binary and have it available to be used whenever we need to use delve and / or gdb? AFAIK, delve allows to pass a binary for a running process.

True. And I think the same is possible in gdb with the symbol-file command.

@tklauser tklauser changed the title [RFC] make: strip symbol tables from all binaries by default make: strip symbol tables from all binaries by default Feb 27, 2020
@tklauser tklauser added release-note/misc This PR makes changes that have no direct user impact. and removed dont-merge/preview-only Only for preview or testing, don't merge it. wip labels Feb 27, 2020
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

One more note, shouldn't we have 2 docker images (one for the non-stripped and the other with the stripped) created from the same binary? Similar to what it is being proposed here cilium/proxy#16 (comment)

@joestringer
Copy link
Member

@tklauser are you waiting on us to merge this or are you following up on some feedback?

@tklauser
Copy link
Member Author

@joestringer sorry, this PR somehow fell between the cracks on my side. As per @aanm's feedback I tried to build 2 docker images for stripped and unstripped binaries but this will need a bit more work (also to provide both images on quay.io). Thus, I'd propose to merge this as is (like the linked cilium/proxy#16 PR) and provide an image with unstripped binaries once the need arises.

Would that be OK with you both?

@tgraf
Copy link
Member

tgraf commented Mar 17, 2020

One more note, shouldn't we have 2 docker images (one for the non-stripped and the other with the stripped) created from the same binary? Similar to what it is being proposed here cilium/proxy#16 (comment)

Why do we need unstripped container images? Are you expecting users to run them? If so, why?

@tgraf
Copy link
Member

tgraf commented Mar 17, 2020

Merging this. We can resolve the unstripped binary images separately.

@tgraf tgraf merged commit 508fee9 into master Mar 17, 2020
1.8.0 automation moved this from In progress to Merged Mar 17, 2020
@tgraf tgraf deleted the pr/tklauser/strip-symbols-tables-by-default branch March 17, 2020 10:52
@aanm
Copy link
Member

aanm commented Mar 17, 2020

One more note, shouldn't we have 2 docker images (one for the non-stripped and the other with the stripped) created from the same binary? Similar to what it is being proposed here cilium/proxy#16 (comment)

Why do we need unstripped container images? Are you expecting users to run them? If so, why?

@tgraf we need an unstripped binary in case we want to debug a live cilium process. If that live process is running the stripped binary we should have the unstripped binary also available.

@tgraf
Copy link
Member

tgraf commented Mar 17, 2020

@tgraf we need an unstripped binary in case we want to debug a live cilium process. If that live process is running the stripped binary we should have the unstripped binary also available.

What exact operation is to debug? Are you talking about running a debugger? Are you talking about gops debugging? How commonly do we need this information available right now?

I assume users will typically run the unstripped version and we would require them to run a different image and reproduce? Is a Makefile target to build unstripped images on demand sufficient?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

5 participants