Hi @pgimalac. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/ok-to-test

@pgimalac i am curious, how did you find the issue and the fix? thanks!

/are code-organization

👋 @dims I think what made me aware of the issue in general is this talk https://www.youtube.com/watch?v=EkG177eRcco on the topic and this associated pull request in cobra spf13/cobra#1956.

Over the past few months I've been actively working on enabling the optimization in the binaries from https://github.com/DataDog/datadog-agent, by fixing upstream dependencies when possible, or just removing them / forking them otherwise.
This is the very last change, needed for the last binary.

In order to find for a given binary what is disabling the optimization, you can add -ldflags=-dumpdep to the go build args, so that it generates the symbol dependency graph, and pass it to https://github.com/aarzilli/whydeadcode to get a readable output.

For this specific change, here is the output of the tool:

text/template.(*state).evalField reachable from:
	 text/template.(*state).evalFieldChain
	 text/template.(*state).evalCommand
	 text/template.(*state).evalPipeline
	 text/template.(*state).walk
	 text/template.(*Template).execute
	 html/template.(*Template).Execute
	 k8s.io/apiserver/pkg/server/routes.DebugFlags.Index
	 k8s.io/apiserver/pkg/server/routes.DebugFlags.Index-fm
	 k8s.io/apiserver/pkg/server/routes.(*DebugSocket).InstallDebugFlag
	 k8s.io/apiserver/pkg/server.installAPI
	 k8s.io/apiserver/pkg/server.completedConfig.New
	 sigs.k8s.io/custom-metrics-apiserver/pkg/apiserver.CompletedConfig.New
	 github.com/DataDog/datadog-agent/cmd/cluster-agent/custommetrics.RunServer
	 github.com/DataDog/datadog-agent/cmd/cluster-agent/subcommands/start.start.func5
	 github.com/DataDog/datadog-agent/cmd/cluster-agent/subcommands/start.start
	 github.com/DataDog/datadog-agent/cmd/cluster-agent/subcommands/start.start·f
	 github.com/DataDog/datadog-agent/cmd/cluster-agent/subcommands/start.Commands.func1
	 github.com/DataDog/datadog-agent/cmd/cluster-agent/subcommands/start.Commands
	 github.com/DataDog/datadog-agent/cmd/cluster-agent/subcommands/start.Commands·f
	 main.main
	 runtime.main_main·f
	 runtime.main
	 runtime.mainPC
	 runtime.rt0_go
	 main
	 _

We can see that the problematic code is the use of template removed by this PR.
After patching, the output is empty, and I can confirm that the binary is significantly smaller.

@pgimalac thank you for the very detailed explanation :)

/priority important-soon
/assign @liggitt @deads2k

Relevant discussion golang/go#72895

Are we not using all those reflection method in other places?

@pgimalac can you add to the description what is the exact effective improvement on the apiserver binary?

From current kind job in this pr

registry.k8s.io/kube-apiserver-amd64 v1.34.0-alpha.0.960_310105bfd4e357 f837585c16f15 104MB

From another kind job in another pr

registry.k8s.io/kube-apiserver-amd64 v1.34.0-alpha.0.962_3fb0cea0496ec4 57d0fcdf48fb4 104MB

I can't see the difference

To clarify, the problem seems to be the reflection method, is just that library uses them , so removing html template is necessary condition but not enough, as other libraries or code maybe using them

I'm not familiar with the binaries generated in this repo so I can't say without looking into it further, but very likely this is not enough for your binaries, eg. I know go-cmp is used in this repo and it disables DCE (related issues: #104821, #130201, google/go-cmp#373).

I can at least say that one of the binaries I work on imports code from k8s.io/apiserver, and this use of template is the only thing disabling the optimization...

I built the binary in cmd/kube-apiserver (just did a go build, I don't know if you have a more complex build mechanism), and I can confirm you at least need to update cobra to 1.9.1. I'll try to check if more changes are needed and list them here.

Well it's not that bad, the changes needed to enable the optimization in kube-apiserver are:

• Update github.com/spf13/cobra to benefit from Allow linker to perform deadcode elimination for program using Cobra spf13/cobra#1956
• Update github.com/modern-go/reflect2 to benefit from fix deadcode disabling in Go modern-go/reflect2#30
• Remove one use of template https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/component-base/zpages/statusz/statusz.go#L44
• Add build tag grpcnotrace to remove the import of golang.org/x/net/trace in google.golang.org/grpc (see x/net/trace: dependency on html/template inhibits dead code elimination golang/go#62024 and https://github.com/grpc/grpc-go/blob/996aabeb3f417890627e08d51b895000839db489/trace_notrace.go#L23-L25)
• Avoid using go-cmp or use a patched version (see Use go-cmp/cmp only in tests #104821 and Allow linker to perform dead code elimination for programs using go-cmp google/go-cmp#373)

go-cmp is probably the most annoying one, I tried fixing upstream but they weren't interested, so on my side I ended up replacing the dependency with a patched fork... (https://github.com/DataDog/go-cmp/tree/dce-patch/v0.7.0)

Building locally on Darwin arm64, the stripped binary kube-apiserver goes from 119MiB to 100MiB (-16%).

I'd be happy to open PRs for each change, but unless you're open to using a fork of go-cmp, it's not that easy to get rid of (so you wouldn't be able to enable the optimization).

make WHAT=cmd/kube-apiserver will generate the binary in the _output/ folder

I think is better to merge all changes together and some sort of regression test

@aojea as I mentioned in my previous message, unless you're willing to use a fork of https://github.com/google/go-cmp (or to drop it entirely), the optimization can't be enabled for the kube-apiserver binary...
So to be clear, do you still want me to do a PR with all the other changes ?

Also in general I would argue in favor of doing those changes in separate PRs to keep things clean, eg. updating dependency has value in itself, even without considering the optimization.

sorry, I was replying from mobile and didn't see the context,

@aojea as I mentioned in my previous message, unless you're willing to use a fork of https://github.com/google/go-cmp (or to drop it entirely), the optimization can't be enabled for the kube-apiserver binary... So to be clear, do you still want me to do a PR with all the other changes ?
Also in general I would argue in favor of doing those changes in separate PRs to keep things clean, eg. updating dependency has value in itself, even without considering the optimization.

forking go-cmp sounds problematic, updating deps that improve things sounds a good thing, although this final decision will be in the area-organization people, @liggitt and @dims best persons to judge

My personal take on this optimization, is that without having a detection mechanism that prevents you to get bitten again by the same problem, using modified code to overcome it seems that the risk you take can not be guaranteed

On my side in the datadog-agent repo we have CI to ensure that the various binaries we generate don't become bigger, so once the optimization is enabled it can't be accidentally disabled again (since that would add back dozens of MiB)

I'd love to be able to enable the optimization for your binaries, but it likely will have to wait until #104821 is fixed somehow, or until they accept a fix upstream (feel free to comment on the issue by the way !)

In any case I'd really like to be able to get this one change merged so that I can benefit from the optimization if you're fine with it

@liggitt i was planning on doing a verify script with the whydeadcode utility which is more useful than looking for the html/template so did not offer up that suggestion. If you think we should do both, i'll suggest something tomorrow.

I didn't know about that one... I just assumed the importer checker would be the easiest path. I'd probably wait to merge until we have a verify script ready (either using existing import tools or something else, and whether it's in this PR or a separate PR, but ready)

@liggitt @pgimalac let's do the following here. I will rework later in a follow up PR

diff --git a/cmd/kubeadm/app/cmd/util/join.go b/cmd/kubeadm/app/cmd/util/join.go
index cbf58238ab0c..feded1d06d75 100644
--- a/cmd/kubeadm/app/cmd/util/join.go
+++ b/cmd/kubeadm/app/cmd/util/join.go
@@ -19,7 +19,7 @@ package util
 import (
        "bytes"
        "crypto/x509"
-       "html/template"
+       "html/template" //nolint:depguard
        "strings"

        "k8s.io/client-go/tools/clientcmd"
diff --git a/hack/golangci.yaml b/hack/golangci.yaml
index 2306878a6bfe..94dcd95e711d 100644
--- a/hack/golangci.yaml
+++ b/hack/golangci.yaml
@@ -268,6 +268,8 @@ linters:
           deny:
             - pkg: "github.com/google/go-cmp/cmp"
               desc: "cmp is allowed only in test files"
+            - pkg: "html/template"
+              desc: "template is allowed only in test files as it disables dead code elimination"
     forbidigo:
       analyze-types: true
       forbid:

@dims I pushed a commit with your suggestions 👍

/lgtm
/approve

LGTM label has been added.

@pgimalac oops! forgot to mention there are 3 files in there where we have to add the deny snippet

❯ ls -1 hack/golang*
hack/golangci-hints.yaml
hack/golangci.yaml
hack/golangci.yaml.in

/lgtm
/approve

🤞🏾

LGTM label has been added.

@dims One nolint was missing in apiserver/pkg/server/routes/flags.go

@pgimalac if you want to squash the commits, that would be great as well.

/approve
/lgtm

LGTM label has been added.

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, liggitt, pgimalac

The full list of commands accepted by this bot can be found here.

The pull request process is described here