feat: Automatically fetch headers for Google's Container OS linux header install via initContainer

[dalehamel]

This should be reasonably ready for review, and should fix #47

As of now, the current status is:

• This works! I can run a test script on container OS now.
• It appears to be able to successfully download and generate headers to a shared location, and this is reasonably fast, subsequent loads don't have any noticeable overhead from the initContainer as I had hoped
• When headers are already installed, the initContainer exists almost immediately and the tracer can start
• Headers will be read unconditionally from a cached path at /var/cache/linux-headers, which should dependably be writeable (see https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch05s05.html). Since we are literally just caching the headers, this should be fine
• For hosts where headers are already installed, this will symlink to those.
• For hosts missing headers, they will be downloaded here and the symlink will point to that instead.

This may actually make #7 not needed / "fix" #7, as the approach can just be to ensure that headers are always pointed to by this symlink.

Work still to do:

• Modify tracer container to use custom header path if specified I went with a different approach
• Verify that this works end-to-end
• Cleanups in preparation for merge.

I would like if someone else could test this against another environment where headers are already present, to ensure that it uses the headers at /lib/modules.host, as I haven't tested this codepath. Just running a trace should be sufficient to prove that this works, but examining the symlinks in /var/cache/linux-headers/modules_dir would show this conclusively. My main concern is that this fixes my use-case, and breaks for other users.

If there are hosts where /var/cache is not writeable (which would frankly baffle me), then this will break kubectl-trace for those users.

@fntlnz can you give this a review, and help me out by QA'ing against any environments you think might make sense? I would hate to have this be merged and break kubectl trace for other users in a way I hadn't anticipated :)

[dalehamel]

Instead of just warning, a future version could check if it is apparent that headers exist.

If there are no headers, then a generic handling of checking kernel version via uname, and fetching from kernel.org or some other kernel mirror would be a good fallback.

For distros like ubuntu, arch, fedora, etc. if the administrator hasn't installed the kernel headers package, we could also try to do something clever by fetching the headers directly. for them here, as they should be available as binary archives (and even though alpine won't have access to the host's package manager, it should be able to determine how to fetch these archives directly from a package mirror easily enough, as these packages are basically just fancy tarballs anyways).

Until that becomes a practical issue for someone though, I don't see a reason to implement it and add the complexity.

[dalehamel]

@fntlnz should be ready for a serious review if you have a chance

[fntlnz]

This looks shiny, I need two more eyes from @leodido and I'm testing this on GKE before merge.

[fntlnz]

I'm very thankful you did this @dalehamel - one of the main goals of this project is to address every Linux machine in a kubernetes cluster and this work here sets the foundations to let us support more and more different cases.

[dalehamel]

one of the main goals of this project is to address every Linux machine in a kubernetes cluster

Yeah there are approaches that download headers directly from kernel.org, that could be used for other cases where headers are missing.

[ghost]

Found some fixes!

P.S. share your ideas, feedbacks or issues with us at https://github.com/fixmie/feedback (this message will be removed after the beta stage).

[dalehamel]

updated my branch to add resource requests to the initcontainer, since #50 was merged.

I also noticed some whitespace errors I introduced with that one, fixed those here - sorry about that!

[leodido]

I've just tried this and it works for me. PTAL @fntlnz

[leodido]

LGTM 🎉

p.s.: thanks for the whitespaces thing I did not noticed in the other PR review :)

[dalehamel]

@leodido thanks for the review - i'll fix the comment issue shortly.

I also saw a comment briefly about the image hosting being under my quay namespace, I agree it shouldn't be. I think both images should be moved to iovisor. I think we can probably handle that after merging this though, I'm not a member of iovisor org though, so it'll have to be someone who is.

The initContainer also doesn't have an automated build as part of CI, so whoever does that could just make sure both images are built and published to the right place automatically - maybe we should file a separate issue for that?

[fntlnz]

@dalehamel I was thinking about making this step optional, and having a short parameter that will enable the initContainer. Wdyt?

[fntlnz]

@dalehamel yes we need to address ci for those new images, we can do that in a separate PR, that's just ok to merge this first

[dalehamel]

I was thinking about making this step optional, and having a short parameter that will enable the initContainer. Wdyt?

Would be good for compatibility purposes / for cases where users can dependably mount the headers from their host system, but we'd need to adjust this PR in some way, as the symlink that it creates over /lib/modules is created by the initContainer, so it's currently coupled.

[fntlnz]

What do you think about adding a flag to make the init container optional @leodido ?

@dalehamel I'm not yet sure I want to do that, I'm trying to think about different situations and asking as many users I can reach what they think about this. Also I want to make you aware of the fact that I'm at a company retreat this week and I'm having problems in keeping up with all the awesome things you are doing because I don't have many time available. So expect some delays until Monday but feel free to keep doing what you have in mind, I will find time to give feedback but I can't personally do proper code reviews now.

[dalehamel]

I'm not yet sure I want to do that

Yeah, this was my main hesitation with the PR, I think gating it as a default-disabled flag makes sense. However, if it's a flag to enable/disable, we can make the same flag toggle back to the default behavior of detecting from /lib/modules.

Related to #53, it would be nice to be able to toggle this with both an environment variable and command line flag.

So expect some delays until Monday but feel free to keep doing what you have in mind

No worries on the delay, but I was a little worried about overwhelming you with stuff to review so thanks for the confirmation.

I also did a bit of a deep dive on the bpftrace issue for pid namespaces and hit a bit of a brick wall. Would be good to sync up with you on that to figure out what the options are until BTF is available, as being able to trace (just) a specific pod (#33) is a bit of a "holy grail" for my use case.

[fntlnz]

@dalehamel yes we need to be able to change between the two modes, the flag would be the best way to do that while using kubectl trace to change the behavior on the fly while the environment variable would be the way to explicitly set a behavior as a default for this machine , like hey I always want to use the init container and this specific image with it.

I'm glad to hear that the pid namespace feature is important for you too, that's really what we wanted to achieve since the begging and it's something I need so bad too so feel free to continue exploring and working on it while I'm away this week and after we can find a way to do that together as a group.

[dalehamel]

I like @leodido's suggestion for --fetch-headers from #53, and probably a ENV['KUBECTL_TRACE_FETCH_HEADERS'] or similar to toggle this.

That also makes it clearer what this PR should do that it doesn't now:

• If the user doesn't specify --fetch-headers, then the behavior should be to only look at /lib/modules (and perhaps in a future version, a path specified by an env var as in Support setting the kernel headers path via BPFTRACE_KERNEL_SOURCE #7 )
• The script should add a handling for the generic case, where it just fetches kernel sources from a reputable mirror like kernel.org, so that --fetch-headers will work with more than container OS.

[leodido]

Yup, I'm working on it (PR for flags etc.) 👍

…

On Fri, Feb 1, 2019, 6:37 AM Dale Hamel ***@***.*** wrote: I like @leodido <https://github.com/leodido>'s suggestion for --fetch-headers, and probably a ENV['KUBECTL_TRACE_FETCH_HEADERS'] or similar to toggle this. That also makes it clearer what this PR should do that it doesn't now: - If the user doesn't specify --fetch-headers, then the behavior should be to only look at /lib/modules (and perhaps in a future version, a path specified by an env var as in #7 <#7> ) - The script should add a handling for the generic case, where it just fetches kernel sources from a reputable mirror like kernel.org, so that --fetch-headers will work with more than container OS. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#48 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHU88WjueTPWvNm5i3mrmfB1SsarAIgks5vJFEPgaJpZM4aUkAb> .

[fntlnz]

Ok good, I think that the solution we ended up with is looking good.

@dalehamel I just merged the PR that introduces flags, #55 @leodido also added flags for the init container and fetch headers that are not used right now but are solely made for your pr here.

In the master now I changed the init container image to be under fntlnz, I'm talking with @drzaeus77 to have an iovisor owned registry to stop using my quay account.

So I would say, test your changes using the flags but when you rebase make sure you leave my image as default so that the CI can push it on a merge on master and in branches from members.

Also @dalehamel , good job here, thank you!

[dalehamel]

@fntlnz @leodido sorry for the long turnaround time 😅 should be ready for a review now though!

I've rebased against master and made use of the --fetch-headers flag.

The default behavior should be unchanged, it would be great if someone could verify this for me in their own environment.

Currently you need both --fetch-headers and --init-imagename for this to work, as the default init image doesn't exist. In a follow-up PR, we should probably update to point to images in the iovisor namespace, and ensure the init image is built and pushed there.

In my test, this worked as expected:

kubectl trace run NODE_NAME -f tcpconnect.bt --fetch-headers --init-imagename=quay.io/dalehamel/kubectl-trace-init:latest

[dalehamel]

if we ever have more than one container, this will lead to some "fun" behavior, but I don't think that is likely to occur.

[leodido]

Yup, you're right.

BTW, not something to approach in this PR imho. :)

[dalehamel]

I'll follow up with another patch to try a generic approach to downloading headers once this is merged, rather than just warning and bailing

[leodido]

Thanks a lot man! I'll try out this awesome work tomorrow morning.

…

On Wed, Mar 6, 2019, 5:30 PM Dale Hamel ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In init/fetch-linux-headers.sh <#48 (comment)>: > + touch "${SOURCES_DIR}/.installed" + fi + fi +} + +install_headers() +{ + distro=$(grep ^NAME ${OS_RELEASE_FILE} | cut -d = -f 2) + + case $distro in + *"Container-Optimized OS"*) + install_cos_linux_headers + HEADERS_TARGET=${SOURCES_DIR} + ;; + *) + echo "WARNING: ${distro} is not a supported distro, cannot install headers, ensure they are installed to /lib/modules" I'll follow up with another patch to try a generic approach to downloading headers once this is merged, rather than just warning and bailing — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#48 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHU8_pm1Qo2CsXKeZ6t18rAEh4Ti9WFks5vT-0ogaJpZM4aUkAb> .

[leodido]

I just tried it. Looks good to me 🎉

[leodido]

P.S.: I'm going to push images to iovisor quay org soon, and then opening a little PR for that. Ideally we should rebase this on top of that.

[dalehamel]

I’m happy to rebase on that if you like, ping me on the review when it’s up. The one kink in that is the build for the init image is in this PR, so it might be better to just do a fixup afterwards.

…

On Thu, Mar 7, 2019 at 18:52 Leo Di Donato ***@***.***> wrote: P.S.: I'm going to push images to iovisor quay org soon, ant then opening a little PR for that. Ideally we should rebase this on top of that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#48 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlwd8_9Y0TYSFGaSojAQHmK4lJkoTydks5vUaZBgaJpZM4aUkAb> .

[leodido]

I just made a PR (#65) to use iovisor as the image's org.

I also added some suggestion here so to build the image here with the right name.

Please rebase this when #65 is merged in.

[dalehamel]

thanks @leodido I tested and committed your changes. I'll happily rebase once #65 is done, hopefully we can merge this next week :)

[fntlnz]

This looks shiny @dalehamel ! #65 had been merged when you rebase I'll do another review then I think we will be good to go! 🦝

[dalehamel]

@fntlnz rebased and tested locally, I had to override the image name since nothing has been pushed to quay.io/iovisor/kubectl-trace-init:latest yet, but otherwise it looks good.

[fntlnz]

LGTM 🥇

[fntlnz]

Good job @dalehamel thanks