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

feat: Replace CGO_ENABLED=0 with //go:build ignore #950

Merged
merged 2 commits into from
Nov 15, 2024

Conversation

timraymond
Copy link
Member

It will soon become necessary to enable CGO in builds in order to use the MS Go distribution. Disabling CGO was always somewhat of a hack since we didn't need it anyway for eBPF. Now that we do, another solution is necessary. This uses the //go:build ignore directive to exclude all C source files from the Go toolchain. This is necessary even within C source files even though these C source files exist within an underscore-prefixed directory. Go's behavior here is likely erroneous, and an issue has been filed for its repair:
golang/go#69639

@timraymond timraymond requested a review from a team as a code owner November 5, 2024 21:42
rbtr
rbtr previously approved these changes Nov 6, 2024
Copy link
Collaborator

@rbtr rbtr left a comment

Choose a reason for hiding this comment

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

does this change anything wrt the produced binary now being dynamically vs statically linked and needing a libc in our final image?

@timraymond
Copy link
Member Author

@rbtr I had wondered the same, but my testing in KIND of the images produced was successful. I would like to see the windows images functioning, but I currently lack a handy environment to test them.

@timraymond
Copy link
Member Author

So @rbtr, through rebuilding retina images via other means, I think we end up with a libc one way or another by virtue of the fact that we bundle clang and friends in the final images. I'm not 100% on that theory, so I'm still going to let e2es in the merge queue be the final judge of this.

@timraymond timraymond added this pull request to the merge queue Nov 11, 2024
snguyen64
snguyen64 previously approved these changes Nov 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 11, 2024
@timraymond timraymond added this pull request to the merge queue Nov 11, 2024
@timraymond
Copy link
Member Author

Looks like E2Es failed on operator due to timeout. It's possible that this is an actual issue, but I'll try again to confirm it.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 11, 2024
@rbtr
Copy link
Collaborator

rbtr commented Nov 11, 2024

So @rbtr, through rebuilding retina images via other means, I think we end up with a libc one way or another by virtue of the fact that we bundle clang and friends in the final images. I'm not 100% on that theory, so I'm still going to let e2es in the merge queue be the final judge of this.

We definitely have them, I'm specifically interested in whether this now means that the Go bins are dynamic and need them

@timraymond
Copy link
Member Author

@rbtr I experimented and found that the answer is yes: we take a dependency on libc with this change.

With CGO_ENABLED=1 (the paths are odd because I built it in nix):

$  ldd result/bin/retina-agent
        linux-vdso.so.1 (0x00007ffff6d27000)
        libresolv.so.2 => /nix/store/c10zhkbp6jmyh0xc5kd123ga8yy2p4hk-glibc-2.39-52/lib/libresolv.so.2 (0x00007f698f617000)
        libpthread.so.0 => /nix/store/c10zhkbp6jmyh0xc5kd123ga8yy2p4hk-glibc-2.39-52/lib/libpthread.so.0 (0x00007f698f612000)
        libdl.so.2 => /nix/store/c10zhkbp6jmyh0xc5kd123ga8yy2p4hk-glibc-2.39-52/lib/libdl.so.2 (0x00007f698f60d000)
        libc.so.6 => /nix/store/c10zhkbp6jmyh0xc5kd123ga8yy2p4hk-glibc-2.39-52/lib/libc.so.6 (0x00007f698f420000)
        /nix/store/c10zhkbp6jmyh0xc5kd123ga8yy2p4hk-glibc-2.39-52/lib/ld-linux-x86-64.so.2 => /nix/store/dbcw19dshdwnxdv5q2g6wldj6syyvq7l-glibc-2.39-52/lib64/ld-linux-x86-64.so.2 (0x00007f698f62a000)

Currently (CGO_ENABLED=0):

$  ldd result/bin/retina-agent
        not a dynamic executable

It will soon become necessary to enable CGO in builds in order to use
the MS Go distribution. Disabling CGO was always somewhat of a hack
since we didn't need it anyway for eBPF. Now that we do, another
solution is necessary. This uses the `//go:build ignore` directive to
exclude all C source files from the Go toolchain. This is necessary even
within C source files even though these C source files exist within an
underscore-prefixed directory. Go's behavior here is likely erroneous,
and an issue has been filed for its repair:
golang/go#69639
As a consequence of removing CGO_ENABLED=0, we now require glibc in the
final runtime environment of both retina-agent and retina-operator.
`retina-agent` had this already by consequence of the inclusion of clang
and other machinery necessary to compile ebpf. `retina-operator`,
however, did not. This adds the contents of /lib and /usr/lib to the
final image in order to include glibc into `retina-operator`'s runtime
environment.
@timraymond
Copy link
Member Author

Operator failed to start in successive runs of E2Es. Upon further investigation locally, this was a result of exec reporting that /retina-operator did not exist. Confusingly, that executable did exist when unpacking the image tarball. After more research into execve, it also throws ENOENT when any of the libraries the executable that is being run are also not found. In this case, and as mentioned previously, we now depend on glibc, which was not present in the retina-operator final image. The solution is to copy in glibc like we do with retina-agent. See details in 2a94fbb .

@timraymond timraymond added this pull request to the merge queue Nov 15, 2024
Merged via the queue into main with commit 3a3ff41 Nov 15, 2024
26 checks passed
@timraymond timraymond deleted the traymond/go-build-ignore branch November 15, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants