Add environment variable to better support skaffold debug

[etanshaul]

skaffold debug uses heuristics to determine if an application can be debugged, as described here:
https://skaffold.dev/docs/workflows/debug/

In it's current state, the Go CR hello-world app doesn't work with skaffold debug without one of the standard go-runtime env vars as explained in the docs.

[etanshaul]

@briandealwis fyi for review as well

[briandealwis]

That'll do the trick.

[etanshaul]

@simonz130 @averikitsch @grayside ptal. Thanks.

[grayside]

1. Does this mean the dlv tool can be removed?

2. While we can add the variable to the Dockerfile, it seems worth asking: should we inject this into the container when starting locally so there's one less thing specializing the Dockerfile for development vs. production?

3. Are their performance implications to this variable? Haven't played with this one much. If we want to guide developers to remove this when not debugging I can follow-up with commented instructions.

[etanshaul]

Are their performance implications to this variable? Haven't played with this one much. If we want to guide developers to remove this when not debugging I can follow-up with commented instructions

@briandealwis do you have any more insight into @grayside 's above questions? My understanding is that this is a commonly used Go setting.

While we can add the variable to the Dockerfile, it seems worth asking: should we inject this into the container when starting locally so there's one less thing specializing the Dockerfile for development vs. production?

It is a chicken/egg problem. skaffold debug uses heuristics to determine if an application can be transformed for debugging. If at least one of these settings isn't present, then it wouldn't know to do any transformation in the first place.

Does this mean the dlv tool can be removed

Yes, IntelliJ doesn't need this - it uses skaffold debug which applies all of the transformations / debug package loading etc. automatically. In fact, I'd love it if debug config from ALL of our samples (k8s included) could be removed. But we need to confirm first with VS Code: @sujit-kamireddy , @quoctruong please for comment regarding removal of debug config from the Dockerfile(s).

[briandealwis]

There’s a chicken-and-egg problem: Go programs can’t be identified from examining the container image metadata alone. So we can’t inject the variable. The GOTRACEBACK docs doesn’t describe any performance impacts, but it’s important to note that it can be set to any value, including none. (The default value is single.)

The stack traces are emitted stderr, and so should be picked up by the container logging.

[etanshaul]

@briandealwis so if we just set GOTRACEBACK to the default value of single, that is sufficient for skaffold debug's heuristics right?

@grayside if you are concerned about other implications of adding this env var, then for now we can just set it to the default value (which is basically a noop).

[etanshaul]

(I updated the PR to use single)

[grayside]

I'm not as concerned about performance as I am about making sure we're communicating why we do things that are not standard practice.

Is this setting needed for debug or a proxy to identify that we're running a go binary? If the latter, wouldn't it make sense to define a dedicated CLOUD_CODE_RUNTIME override variable for go or other edge cases?

[etanshaul]

Is this setting needed for debug or a proxy to identify that we're running a go binary?

Yes, this is needed to identify that we are running a go binary and that we can debug it.

If the latter, wouldn't it make sense to define a dedicated CLOUD_CODE_RUNTIME override variable for go or other edge cases?

The hope is that we'd be able to debug most applications in their default state without having to add any extra stuff (like that override that you mention) . I can see how you may think though that we are doing the same thing by adding the GOTRACEBACK env var (even though it is a standard thing to have). @briandealwis is there nothing in the base goland image already configured that Skaffold can add to its heuristics?

[grayside]

It's pretty common for a golang Dockerfile to be multi-stage, copying the binary into an image without the go toolchain. Would skaffold debug work in that case if it can identify the binary is built by go?

[briandealwis]

We can't identify that a binary is a Go binary. We've considered introducing explicit means to configure the language runtime type (GoogleContainerTools/skaffold#2203) but this is somewhat intrusive. And as it's not uncommon to use one of several Go runtime-specific environment variable(s), particularly GOGC, it makes sense to use them.

[etanshaul]

@simonz130 @grayside could we try to come to a resolution on this? It is blocking debugging support on our Cloud Run Go sample

[briandealwis]

I realized I didn't address some of @grayside's questions/comments. Just to be clear:

Does this mean the dlv tool can be removed?

Yes, providing we identify the image as being Go related via one of these environment variables (more below).

While we can add the variable to the Dockerfile, it seems worth asking: should we inject this into the container when starting locally so there's one less thing specializing the Dockerfile for development vs. production?

This is great idea. skaffold debug can look at the source and try to identify the source types. I've opened issue an issue (GoogleContainerTools/skaffold#3977). This will require some time to implement.

Are their performance implications to this variable? Haven't played with this one much. If we want to guide developers to remove this when not debugging I can follow-up with commented instructions.

There are no performance implications: the use of single is the default value used by the Go runtime.

Is this setting needed for debug or a proxy to identify that we're running a go binary? If the latter, wouldn't it make sense to define a dedicated CLOUD_CODE_RUNTIME override variable for go or other edge cases?

Yes, these variables serve as a proxy identifier. It's not uncommon for containers to define these values, particularly GOGC or GOTRACEBACK=all (if you had a crash in a binary, you'd want to see everything that was going on).

Settling on an agreeable prefix is … difficult. This functionality is delivered in Skaffold and is used independently of Cloud Code. It's actually why I settled on using these proxy identifiers instead as they existed and were in use.

It's pretty common for a golang Dockerfile to be multi-stage, copying the binary into an image without the go toolchain. Would skaffold debug work in that case if it can identify the binary is built by go?

It works for multistage builds: debug currently examines the final image that's produced.(Providing that the environment variable is defined in the final stage!)

[grayside]

I proposed a change to add more context to this change. I think we need to:

1. P1: Indicate why this variable is defined so developers don't remove it as a simplification and have a change to learn via reading the sample about skaffold debug.
2. P2: Link to documentation about this variable so anyone less familiar has a direct line on researching how to use the value now that they have it in front of them.

[etanshaul]

thanks @grayside . Change suggestion accepted. ptal.