From 62f1d3680947e1d78bacf2a7277fb4b2007ebacb Mon Sep 17 00:00:00 2001 From: Lorenz Brun Date: Tue, 14 Nov 2023 16:18:24 +0100 Subject: [PATCH] treewide: stop using LZ4 for initrd compression There are two issues at play here: One is a bug in pierrec/lz4 when using the legacy framing format [1]. This bit us when we hit a broken size region with CL:2130, taking hours to debug. The other is the fact that the Linux LZ4 frame format has significant design issues [2], especially with concatenanted initrds. The first issue could be fixed by switching to a different LZ4 implementation (we do even have the reference impl in the monorepo) but there is no API to generate the legacy frame format and things like [3], a patch carried by Ubuntu to fix more edge cases just do not inspire confidence in such a solution. Thus, this CL switches over to using zstd for compressing initrds. Zstd is slower than LZ4 for decompressing, but it still decompresses at multiple GB/s per core while having a much better compression ratio. It also doesn't have any Linux-specific bits and Linux uses the reference implementation for decoding, which should make it much more robust. So overall I think this is a good tradeoff. [1] https://github.com/pierrec/lz4/issues/156 [2] https://github.com/lz4/lz4/issues/956#issuecomment-736705712 [3] https://launchpadlibrarian.net/507407918/0001-unlz4-Handle-0-size-chunks-discard-trailing-padding-.patch Change-Id: I69cf69f2f361de325f4b39f2d3644ee729643716 Reviewed-on: https://review.monogon.dev/c/monogon/+/2313 Tested-by: Jenkins CI Reviewed-by: Serge Bazanski --- cloud/agent/e2e/BUILD.bazel | 2 +- cloud/agent/e2e/main_test.go | 14 ++++++++------ cloud/takeover/BUILD.bazel | 2 +- cloud/takeover/takeover.go | 14 ++++++++------ go.mod | 3 +-- go.sum | 4 ++-- metropolis/node/build/def.bzl | 2 +- metropolis/node/build/mkcpio/BUILD.bazel | 2 +- metropolis/node/build/mkcpio/main.go | 12 +++++++----- metropolis/test/launch/cluster/cluster.go | 2 +- third_party/go/repositories.bzl | 4 ++-- 11 files changed, 33 insertions(+), 28 deletions(-) diff --git a/cloud/agent/e2e/BUILD.bazel b/cloud/agent/e2e/BUILD.bazel index 2ed2ad56..fa03d66b 100644 --- a/cloud/agent/e2e/BUILD.bazel +++ b/cloud/agent/e2e/BUILD.bazel @@ -16,7 +16,7 @@ go_test( "//metropolis/pkg/pki", "//metropolis/proto/api", "@com_github_cavaliergopher_cpio//:cpio", - "@com_github_pierrec_lz4_v4//:lz4", + "@com_github_klauspost_compress//zstd", "@org_golang_google_grpc//:go_default_library", "@org_golang_google_grpc//credentials", "@org_golang_google_protobuf//proto", diff --git a/cloud/agent/e2e/main_test.go b/cloud/agent/e2e/main_test.go index 27bcd039..277cc553 100644 --- a/cloud/agent/e2e/main_test.go +++ b/cloud/agent/e2e/main_test.go @@ -21,7 +21,7 @@ import ( "time" "github.com/cavaliergopher/cpio" - "github.com/pierrec/lz4/v4" + "github.com/klauspost/compress/zstd" "golang.org/x/sys/unix" "google.golang.org/grpc" "google.golang.org/grpc/credentials" @@ -183,7 +183,7 @@ func TestMetropolisInstallE2E(t *testing.T) { if err != nil { t.Fatal(err) } - initramfsOrigPath, err := datafile.ResolveRunfile("cloud/agent/initramfs.cpio.lz4") + initramfsOrigPath, err := datafile.ResolveRunfile("cloud/agent/initramfs.cpio.zst") if err != nil { t.Fatal(err) } @@ -207,9 +207,11 @@ func TestMetropolisInstallE2E(t *testing.T) { if err != nil { t.Fatal(err) } - compressedOut := lz4.NewWriter(initramfsFile) - compressedOut.Apply(lz4.LegacyOption(true)) - cpioW := cpio.NewWriter(compressedOut) + compressedW, err := zstd.NewWriter(initramfsFile, zstd.WithEncoderLevel(1)) + if err != nil { + t.Fatal(err) + } + cpioW := cpio.NewWriter(compressedW) cpioW.WriteHeader(&cpio.Header{ Name: "/init.pb", Size: int64(len(agentInitRaw)), @@ -217,7 +219,7 @@ func TestMetropolisInstallE2E(t *testing.T) { }) cpioW.Write(agentInitRaw) cpioW.Close() - compressedOut.Close() + compressedW.Close() grpcGuestFwd := fmt.Sprintf("guestfwd=tcp:%s-tcp:127.0.0.1:%d", grpcAddr.String(), grpcListenAddr.Port) blobGuestFwd := fmt.Sprintf("guestfwd=tcp:%s-tcp:127.0.0.1:%d", blobAddr.String(), blobListenAddr.Port) diff --git a/cloud/takeover/BUILD.bazel b/cloud/takeover/BUILD.bazel index 21b891a8..103ea2db 100644 --- a/cloud/takeover/BUILD.bazel +++ b/cloud/takeover/BUILD.bazel @@ -19,7 +19,7 @@ go_library( "//net/dump", "//net/proto", "@com_github_cavaliergopher_cpio//:cpio", - "@com_github_pierrec_lz4_v4//:lz4", + "@com_github_klauspost_compress//zstd", "@org_golang_google_protobuf//proto", "@org_golang_x_sys//unix", ], diff --git a/cloud/takeover/takeover.go b/cloud/takeover/takeover.go index 9de39f52..e265b302 100644 --- a/cloud/takeover/takeover.go +++ b/cloud/takeover/takeover.go @@ -27,7 +27,7 @@ import ( "time" "github.com/cavaliergopher/cpio" - "github.com/pierrec/lz4/v4" + "github.com/klauspost/compress/zstd" "golang.org/x/sys/unix" "google.golang.org/protobuf/proto" @@ -44,7 +44,7 @@ var kernel []byte //go:embed ucode.cpio var ucode []byte -//go:embed cloud/agent/initramfs.cpio.lz4 +//go:embed cloud/agent/initramfs.cpio.zst var initramfs []byte // newMemfile creates a new file which is not located on a specific filesystem, @@ -124,9 +124,11 @@ func setupTakeover() (*api.TakeoverSuccess, error) { } // Append AgentInit spec to initramfs - compressedOut := lz4.NewWriter(initramfsFile) - compressedOut.Apply(lz4.LegacyOption(true)) - cpioW := cpio.NewWriter(compressedOut) + compressedW, err := zstd.NewWriter(initramfsFile, zstd.WithEncoderLevel(1)) + if err != nil { + return nil, fmt.Errorf("while creating zstd writer: %w", err) + } + cpioW := cpio.NewWriter(compressedW) cpioW.WriteHeader(&cpio.Header{ Name: "/init.pb", Size: int64(len(agentInitRaw)), @@ -134,7 +136,7 @@ func setupTakeover() (*api.TakeoverSuccess, error) { }) cpioW.Write(agentInitRaw) cpioW.Close() - compressedOut.Close() + compressedW.Close() agentParams := bootparam.Params{ bootparam.Param{Param: "quiet"}, diff --git a/go.mod b/go.mod index cc88cd0b..9de22c85 100644 --- a/go.mod +++ b/go.mod @@ -108,7 +108,6 @@ require ( github.com/mitchellh/go-wordwrap v1.0.0 github.com/opencontainers/runc v1.1.3 github.com/packethost/packngo v0.29.0 - github.com/pierrec/lz4/v4 v4.1.14 github.com/pkg/errors v0.9.1 github.com/pkg/sftp v1.10.1 github.com/prometheus/node_exporter v1.3.1 @@ -310,7 +309,7 @@ require ( github.com/josharian/native v1.0.0 // indirect github.com/json-iterator/go v1.1.12 // indirect github.com/karrick/godirwalk v1.16.1 // indirect - github.com/klauspost/compress v1.14.2 // indirect + github.com/klauspost/compress v1.17.2 github.com/kr/fs v0.1.0 // indirect github.com/kr/pty v1.1.8 // indirect github.com/libopenstorage/openstorage v1.0.0 // indirect diff --git a/go.sum b/go.sum index 929e7c90..264395c6 100644 --- a/go.sum +++ b/go.sum @@ -1432,8 +1432,9 @@ github.com/klauspost/compress v1.12.2/go.mod h1:8dP1Hq4DHOhN9w426knH3Rhby4rFm6D8 github.com/klauspost/compress v1.13.1/go.mod h1:8dP1Hq4DHOhN9w426knH3Rhby4rFm6D8eO+e+Dq5Gzg= github.com/klauspost/compress v1.13.4/go.mod h1:8dP1Hq4DHOhN9w426knH3Rhby4rFm6D8eO+e+Dq5Gzg= github.com/klauspost/compress v1.13.6/go.mod h1:/3/Vjq9QcHkK5uEr5lBEmyoZ1iFhe47etQ6QUkpK6sk= -github.com/klauspost/compress v1.14.2 h1:S0OHlFk/Gbon/yauFJ4FfJJF5V0fc5HbBTJazi28pRw= github.com/klauspost/compress v1.14.2/go.mod h1:/3/Vjq9QcHkK5uEr5lBEmyoZ1iFhe47etQ6QUkpK6sk= +github.com/klauspost/compress v1.17.2 h1:RlWWUY/Dr4fL8qk9YG7DTZ7PDgME2V4csBXA8L/ixi4= +github.com/klauspost/compress v1.17.2/go.mod h1:ntbaceVETuRiXiv4DpjP66DpAtAGkEQskQzEyD//IeE= github.com/klauspost/cpuid v1.2.0/go.mod h1:Pj4uuM528wm8OyEC2QMXAi2YiTZ96dNQPGgoMS4s3ek= github.com/koneu/natend v0.0.0-20150829182554-ec0926ea948d/go.mod h1:QHb4k4cr1fQikUahfcRVPcEXiUgFsdIstGqlurL0XL4= github.com/konsorten/go-windows-terminal-sequences v1.0.1/go.mod h1:T0+1ngSBFLxvqU3pZ+m/2kptfBszLMUkC4ZK/EgS/cQ= @@ -1824,7 +1825,6 @@ github.com/pierrec/lz4 v2.5.2+incompatible/go.mod h1:pdkljMzZIN41W+lC3N2tnIh5sFi github.com/pierrec/lz4 v2.6.1+incompatible h1:9UY3+iC23yxF0UfGaYrGplQ+79Rg+h/q9FV9ix19jjM= github.com/pierrec/lz4 v2.6.1+incompatible/go.mod h1:pdkljMzZIN41W+lC3N2tnIh5sFi+IEE17M5jbnwPHcY= github.com/pierrec/lz4/v4 v4.1.8/go.mod h1:gZWDp/Ze/IJXGXf23ltt2EXimqmTUXEy0GFuRQyBid4= -github.com/pierrec/lz4/v4 v4.1.14 h1:+fL8AQEZtz/ijeNnpduH0bROTu0O3NZAlPjQxGn8LwE= github.com/pierrec/lz4/v4 v4.1.14/go.mod h1:gZWDp/Ze/IJXGXf23ltt2EXimqmTUXEy0GFuRQyBid4= github.com/pingcap/errors v0.11.0/go.mod h1:Oi8TUi2kEtXXLMJk9l1cGmz20kV3TaQ0usTwv5KuLY8= github.com/pingcap/errors v0.11.5-0.20210425183316-da1aaba5fb63 h1:+FZIDR/D97YOPik4N4lPDaUcLDF/EQPogxtlHB2ZZRM= diff --git a/metropolis/node/build/def.bzl b/metropolis/node/build/def.bzl index d71279c9..4f0e30d8 100644 --- a/metropolis/node/build/def.bzl +++ b/metropolis/node/build/def.bzl @@ -133,7 +133,7 @@ def _fsspec_core_impl(ctx, tool, output_file): return def _node_initramfs_impl(ctx): - initramfs_name = ctx.label.name + ".cpio.lz4" + initramfs_name = ctx.label.name + ".cpio.zst" initramfs = ctx.actions.declare_file(initramfs_name) _fsspec_core_impl(ctx, ctx.executable._mkcpio, initramfs) diff --git a/metropolis/node/build/mkcpio/BUILD.bazel b/metropolis/node/build/mkcpio/BUILD.bazel index f0bb7790..3ea98ae8 100644 --- a/metropolis/node/build/mkcpio/BUILD.bazel +++ b/metropolis/node/build/mkcpio/BUILD.bazel @@ -8,7 +8,7 @@ go_library( deps = [ "//metropolis/node/build/fsspec", "@com_github_cavaliergopher_cpio//:cpio", - "@com_github_pierrec_lz4_v4//:lz4", + "@com_github_klauspost_compress//zstd", "@org_golang_x_sys//unix", ], ) diff --git a/metropolis/node/build/mkcpio/main.go b/metropolis/node/build/mkcpio/main.go index 1c9b39f4..b8f99b9e 100644 --- a/metropolis/node/build/mkcpio/main.go +++ b/metropolis/node/build/mkcpio/main.go @@ -10,7 +10,7 @@ import ( "strings" "github.com/cavaliergopher/cpio" - "github.com/pierrec/lz4/v4" + "github.com/klauspost/compress/zstd" "golang.org/x/sys/unix" "source.monogon.dev/metropolis/node/build/fsspec" @@ -47,7 +47,7 @@ type place struct { Inode interface{} } -// Usage: -out fsspec-path... +// Usage: -out fsspec-path... func main() { flag.Parse() outFile, err := os.Create(*outPath) @@ -55,8 +55,10 @@ func main() { log.Fatalf("Failed to open CPIO output file: %v", err) } defer outFile.Close() - compressedOut := lz4.NewWriter(outFile) - compressedOut.Apply(lz4.LegacyOption(true)) + compressedOut, err := zstd.NewWriter(outFile) + if err != nil { + log.Fatalf("While initializing zstd writer: %v", err) + } defer compressedOut.Close() cpioWriter := cpio.NewWriter(compressedOut) defer cpioWriter.Close() @@ -168,7 +170,7 @@ func main() { }); err != nil { log.Fatalf("Failed to write cpio header for file %q: %v", i.Path, err) } - if _, err := io.Copy(cpioWriter, inF); err != nil { + if n, err := io.Copy(cpioWriter, inF); err != nil || n != inFStat.Size() { log.Fatalf("Failed to copy file %q into cpio: %v", i.SourcePath, err) } inF.Close() diff --git a/metropolis/test/launch/cluster/cluster.go b/metropolis/test/launch/cluster/cluster.go index a312e868..1066d4df 100644 --- a/metropolis/test/launch/cluster/cluster.go +++ b/metropolis/test/launch/cluster/cluster.go @@ -790,7 +790,7 @@ func LaunchCluster(ctx context.Context, opts ClusterOptions) (*Cluster, error) { if err := launch.RunMicroVM(ctxT, &launch.MicroVMOptions{ Name: "nanoswitch", KernelPath: "metropolis/test/ktest/vmlinux", - InitramfsPath: "metropolis/test/nanoswitch/initramfs.cpio.lz4", + InitramfsPath: "metropolis/test/nanoswitch/initramfs.cpio.zst", ExtraNetworkInterfaces: switchPorts, PortMap: portMap, GuestServiceMap: guestSvcMap, diff --git a/third_party/go/repositories.bzl b/third_party/go/repositories.bzl index abfdfe4e..2811cfbf 100644 --- a/third_party/go/repositories.bzl +++ b/third_party/go/repositories.bzl @@ -3180,8 +3180,8 @@ def go_repositories(): go_repository( name = "com_github_klauspost_compress", importpath = "github.com/klauspost/compress", - sum = "h1:S0OHlFk/Gbon/yauFJ4FfJJF5V0fc5HbBTJazi28pRw=", - version = "v1.14.2", + sum = "h1:RlWWUY/Dr4fL8qk9YG7DTZ7PDgME2V4csBXA8L/ixi4=", + version = "v1.17.2", ) go_repository( name = "com_github_klauspost_cpuid",