-
Notifications
You must be signed in to change notification settings - Fork 3.6k
perf(sqldb): compress offloaded node status. Fixes #13290 #16733
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
1bdcd44
8fa15d7
352cfdf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| //go:build !windows | ||
|
|
||
| package sqldb | ||
|
|
||
| // Integration tests for #13290: offload now stores node status COMPRESSED in the | ||
| // compressednodes column, cutting the volume written on every update of a large | ||
| // workflow. The MySQL 8.4 container pins max_allowed_packet to 16MB, which also makes | ||
| // the size reduction observable: a Save of ~13MB of raw nodes only fits once compressed. | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "strconv" | ||
| "strings" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| testcontainers "github.com/testcontainers/testcontainers-go" | ||
| testmysql "github.com/testcontainers/testcontainers-go/modules/mysql" | ||
| "github.com/testcontainers/testcontainers-go/wait" | ||
| "github.com/upper/db/v4" | ||
|
|
||
| "github.com/argoproj/argo-workflows/v4/config" | ||
| wfv1 "github.com/argoproj/argo-workflows/v4/pkg/apis/workflow/v1alpha1" | ||
| "github.com/argoproj/argo-workflows/v4/util/logging" | ||
| usqldb "github.com/argoproj/argo-workflows/v4/util/sqldb" | ||
| ) | ||
|
|
||
| // setupOffloadRepo starts MySQL 8.4 with max_allowed_packet pinned to 16MB, migrates the | ||
| // argo_workflows offload table, and returns the offload repo plus the session proxy. | ||
| func setupOffloadRepo(ctx context.Context, t *testing.T) (OffloadNodeStatusRepo, *usqldb.SessionProxy) { | ||
| t.Helper() | ||
|
|
||
| c, err := testmysql.Run(ctx, | ||
| "mysql:8.4", | ||
| testmysql.WithDatabase("argo"), | ||
| testmysql.WithUsername("argo"), | ||
| testmysql.WithPassword("argo"), | ||
| // Pin the ceiling to 16MB so ~13MB raw nodes would fail pre-fix but pass compressed. | ||
| testcontainers.WithCmdArgs("--max-allowed-packet=16777216"), | ||
| testcontainers.WithWaitStrategy( | ||
| wait.ForAll( | ||
| wait.ForLog("port: 3306 MySQL Community Server").WithStartupTimeout(120*time.Second), | ||
| wait.ForListeningPort("3306/tcp"), | ||
| )), | ||
| ) | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { | ||
| if termErr := testcontainers.TerminateContainer(c); termErr != nil { | ||
| t.Logf("failed to terminate container: %s", termErr) | ||
| } | ||
| }) | ||
|
|
||
| host, err := c.Host(ctx) | ||
| require.NoError(t, err) | ||
| p, err := c.MappedPort(ctx, "3306/tcp") | ||
| require.NoError(t, err) | ||
| port, err := strconv.Atoi(p.Port()) | ||
| require.NoError(t, err) | ||
|
|
||
| proxy, err := usqldb.NewSessionProxy(ctx, usqldb.SessionProxyConfig{ | ||
| DBConfig: config.DBConfig{ | ||
| MySQL: &config.MySQLConfig{ | ||
| DatabaseConfig: config.DatabaseConfig{Database: "argo", Host: host, Port: port}, | ||
| }, | ||
| }, | ||
| Username: "argo", | ||
| Password: "argo", | ||
| }) | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { proxy.Close() }) | ||
|
|
||
| require.NoError(t, Migrate(ctx, proxy.Session(), "test", "argo_workflows", proxy.DBType())) | ||
|
|
||
| repo, err := NewOffloadNodeStatusRepo(ctx, logging.RequireLoggerFromContext(ctx), proxy, "test", "argo_workflows") | ||
| require.NoError(t, err) | ||
| return repo, proxy | ||
| } | ||
|
|
||
| // makeNodes builds a wfv1.Nodes whose marshalled JSON is >= target bytes. | ||
| func makeNodes(t *testing.T, target int) wfv1.Nodes { | ||
| t.Helper() | ||
| nodes := wfv1.Nodes{} | ||
| chunk := strings.Repeat("x", 64*1024) // 64KB per node | ||
| i := 0 | ||
| for { | ||
| id := fmt.Sprintf("node-%06d", i) | ||
| nodes[id] = wfv1.NodeStatus{ID: id, Name: id, Message: chunk} | ||
| i++ | ||
| if i%16 == 0 { | ||
| b, err := json.Marshal(nodes) | ||
| require.NoError(t, err) | ||
| if len(b) >= target { | ||
| return nodes | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const mb = 1 << 20 | ||
|
|
||
| // TestOffloadCompression_RoundTrip verifies a ~13MB node status (which pre-fix exceeded the | ||
| // 16MB packet ceiling once expanded) now saves compressed, round-trips via Get, and is stored | ||
| // with the raw nodes column holding only the "null" placeholder. | ||
| func TestOffloadCompression_RoundTrip(t *testing.T) { | ||
| ctx := logging.TestContext(t.Context()) | ||
| repo, proxy := setupOffloadRepo(ctx, t) | ||
|
|
||
| nodes := makeNodes(t, 13*mb) | ||
| uid := "uid-roundtrip" | ||
|
|
||
| version, err := repo.Save(ctx, uid, "default", nodes) | ||
| require.NoError(t, err, "compressed Save of ~13MB nodes should succeed under 16MB max_allowed_packet") | ||
|
Comment on lines
+113
to
+117
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Use a raw payload larger than the packet limit.
Generate more than 16 MiB of raw JSON, for example Proposed fix- nodes := makeNodes(t, 13*mb)
+ nodes := makeNodes(t, 17*mb)🤖 Prompt for AI Agents |
||
|
|
||
| got, err := repo.Get(ctx, uid, version) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, nodes, got, "Get must return the original nodes") | ||
|
|
||
| // Storage format: compressed payload present, raw nodes column is the placeholder. | ||
| r := fetchRow(ctx, t, proxy, uid, version) | ||
| assert.NotEmpty(t, r.CompressedNodes, "compressednodes should hold the compressed payload") | ||
| assert.Equal(t, "null", r.Nodes, "nodes column should be the json null placeholder") | ||
| assert.Less(t, len(r.CompressedNodes), 13*mb, "stored compressed payload should be far smaller than raw") | ||
| } | ||
|
|
||
| // TestOffloadCompression_BackwardCompat verifies that a legacy row (raw JSON in nodes, | ||
| // empty compressednodes) still reads correctly via both Get and List after the change. | ||
| func TestOffloadCompression_BackwardCompat(t *testing.T) { | ||
| ctx := logging.TestContext(t.Context()) | ||
| repo, proxy := setupOffloadRepo(ctx, t) | ||
|
|
||
| legacyNodes := wfv1.Nodes{"n1": wfv1.NodeStatus{ID: "n1", Name: "n1", Phase: wfv1.NodeSucceeded}} | ||
| raw, err := json.Marshal(legacyNodes) | ||
| require.NoError(t, err) | ||
|
|
||
| uid, version := "uid-legacy", "fnv:legacy" | ||
| err = proxy.With(ctx, func(s db.Session) error { | ||
| _, insErr := s.Collection("argo_workflows").Insert(&nodesRecord{ | ||
| ClusterName: "test", | ||
| UUIDVersion: UUIDVersion{UID: uid, Version: version}, | ||
| Namespace: "default", | ||
| Nodes: string(raw), | ||
| CompressedNodes: "", // legacy: no compression | ||
| }) | ||
| return insErr | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| got, err := repo.Get(ctx, uid, version) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, legacyNodes, got, "Get must read legacy uncompressed rows") | ||
|
|
||
| list, err := repo.List(ctx, "default") | ||
| require.NoError(t, err) | ||
| assert.Equal(t, legacyNodes, list[UUIDVersion{UID: uid, Version: version}], "List must read legacy uncompressed rows") | ||
| } | ||
|
|
||
| func fetchRow(ctx context.Context, t *testing.T, proxy *usqldb.SessionProxy, uid, version string) nodesRecord { | ||
| t.Helper() | ||
| var r nodesRecord | ||
| err := proxy.With(ctx, func(s db.Session) error { | ||
| return s.SQL().SelectFrom("argo_workflows"). | ||
| Where(db.Cond{"uid": uid}).And(db.Cond{"version": version}).One(&r) | ||
| }) | ||
| require.NoError(t, err) | ||
| return r | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Split this paragraph into one sentence per line.
Line 5 contains multiple sentences on one Markdown line. Split each sentence onto its own line.
As per coding guidelines:
docs/**/*.md: One sentence per line of markdown.🤖 Prompt for AI Agents
Source: Coding guidelines