Skip to content

Fix regression in Modify()#141

Merged
joamaki merged 2 commits intomainfrom
pr/joamaki/fix-modify-merge
Feb 4, 2026
Merged

Fix regression in Modify()#141
joamaki merged 2 commits intomainfrom
pr/joamaki/fix-modify-merge

Conversation

@joamaki
Copy link
Copy Markdown
Contributor

@joamaki joamaki commented Feb 4, 2026

The optimization to Modify() to not call merge/mod when object is not found in 55fa50d#diff-9735ccc98169ae36c8123697c6112cd96c0a8188ef495766455b752b8f1546b0 was broken (see below). The merge function was only called for the object inserted into the primary index and the secondary indexes used the object passed to the Modify() method.

Add regression tests and fix the issue by using the new object constructed by tableIndexTxn.modify for the secondary indexes.

The diff that broke this:

+       if merge == nil {
+               oldObj, oldExists, watch = idIndexTxn.insert(idKey, obj)
+       } else {
+               mod := func(old object) object {
                        if old.revision == 0 {
-                               // Zero revision: the object did not exist so no need to call merge.
-                               obj.data = newData
-                       } else {
-                               obj.data = merge(old.data)
+                               return obj
+                       }
+                       return object{
+                               data:     merge(old.data),
+                               revision: revision,
                        }
-                       return obj
-               })
+               }
+               oldObj, oldExists, watch = idIndexTxn.modify(idKey, obj, mod)
+       }

Earlier we constructed a function closure that called the passed in merge function if the old object was found. This updated obj.data in-place and thus ensured that the secondary indexes got the right data. When refactoring the indexing to support LPM indexes I had simplified this and completely missed this. The tests were using pointers to objects (e.g. *testObject) which modified the object in-place in merge and thus didn't catch this.

The changes to Modify() to not call merge() when the object did not exist
are broken. It only called merge() for the primary index and the other
indexes got the new object without merging it with the old one.

Add a regression test to catch this issue and extend the quick tests
to also check for this.

Signed-off-by: Jussi Maki <jussi.maki@isovalent.com>
The change to not call merge() if the object did not exist was broken
as merge() wasn't called for the object inserted into secondary indexes.

Fix the issue by returning the merged new object from tableIndexTxn.modify
and inserting that into the secondary indexes.

Signed-off-by: Jussi Maki <jussi.maki@isovalent.com>
@joamaki joamaki requested a review from a team as a code owner February 4, 2026 13:15
@joamaki joamaki requested review from pippolo84 and removed request for a team February 4, 2026 13:15
@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 4, 2026

$ make
go build ./...
go: downloading go.yaml.in/yaml/v3 v3.0.3
go: downloading github.com/cilium/hive v0.0.0-20250731144630-28e7a35ed227
go: downloading golang.org/x/time v0.5.0
go: downloading github.com/spf13/cobra v1.8.0
go: downloading github.com/spf13/pflag v1.0.5
go: downloading github.com/cilium/stream v0.0.0-20240209152734-a0792b51812d
go: downloading github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de
go: downloading github.com/spf13/viper v1.18.2
go: downloading go.uber.org/dig v1.17.1
go: downloading golang.org/x/term v0.16.0
go: downloading github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc
go: downloading github.com/mitchellh/mapstructure v1.5.0
go: downloading golang.org/x/sys v0.17.0
go: downloading golang.org/x/tools v0.17.0
go: downloading github.com/fsnotify/fsnotify v1.7.0
go: downloading github.com/sagikazarmark/slog-shim v0.1.0
go: downloading github.com/spf13/afero v1.11.0
go: downloading github.com/spf13/cast v1.6.0
go: downloading github.com/subosito/gotenv v1.6.0
go: downloading github.com/hashicorp/hcl v1.0.0
go: downloading gopkg.in/ini.v1 v1.67.0
go: downloading github.com/magiconair/properties v1.8.7
go: downloading github.com/pelletier/go-toml/v2 v2.1.0
go: downloading gopkg.in/yaml.v3 v3.0.1
go: downloading golang.org/x/text v0.14.0
STATEDB_VALIDATE=1 go test ./... -cover -vet=all -test.count 1
go: downloading github.com/stretchr/testify v1.8.4
go: downloading go.uber.org/goleak v1.3.0
go: downloading golang.org/x/exp v0.0.0-20240119083558-1b970713d09a
go: downloading github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2
ok  	github.com/cilium/statedb	421.468s	coverage: 78.5% of statements
ok  	github.com/cilium/statedb/index	0.005s	coverage: 28.7% of statements
ok  	github.com/cilium/statedb/internal	0.013s	coverage: 42.9% of statements
ok  	github.com/cilium/statedb/lpm	4.748s	coverage: 77.3% of statements
ok  	github.com/cilium/statedb/part	66.602s	coverage: 87.2% of statements
ok  	github.com/cilium/statedb/reconciler	0.257s	coverage: 92.9% of statements
	github.com/cilium/statedb/reconciler/benchmark		coverage: 0.0% of statements
	github.com/cilium/statedb/reconciler/example		coverage: 0.0% of statements
go test -race ./... -test.count 1
ok  	github.com/cilium/statedb	39.598s
ok  	github.com/cilium/statedb/index	1.013s
ok  	github.com/cilium/statedb/internal	1.028s
ok  	github.com/cilium/statedb/lpm	3.045s
ok  	github.com/cilium/statedb/part	35.367s
ok  	github.com/cilium/statedb/reconciler	1.339s
?   	github.com/cilium/statedb/reconciler/benchmark	[no test files]
?   	github.com/cilium/statedb/reconciler/example	[no test files]
go test ./... -bench . -benchmem -test.run xxx
goos: linux
goarch: amd64
pkg: github.com/cilium/statedb
cpu: AMD EPYC 7763 64-Core Processor                
BenchmarkDB_WriteTxn_1-4                      	  707736	      1680 ns/op	    595192 objects/sec	    1000 B/op	      16 allocs/op
BenchmarkDB_WriteTxn_10-4                     	 1718546	       698.8 ns/op	   1430942 objects/sec	     520 B/op	       8 allocs/op
BenchmarkDB_WriteTxn_100-4                    	 1993484	       568.2 ns/op	   1759930 objects/sec	     490 B/op	       7 allocs/op
BenchmarkDB_WriteTxn_1000-4                   	 1959999	       615.4 ns/op	   1625030 objects/sec	     447 B/op	       7 allocs/op
BenchmarkDB_WriteTxn_100_SecondaryIndex-4     	  803155	      1319 ns/op	    757941 objects/sec	    1007 B/op	      20 allocs/op
BenchmarkDB_WriteTxn_CommitOnly_100Tables-4   	  964545	      1219 ns/op	    1112 B/op	       5 allocs/op
BenchmarkDB_WriteTxn_CommitOnly_1Table-4      	 1598378	       749.3 ns/op	     224 B/op	       5 allocs/op
BenchmarkDB_NewWriteTxn-4                     	 1763450	       679.6 ns/op	     200 B/op	       4 allocs/op
BenchmarkDB_WriteTxnCommit100-4               	 1000000	      1224 ns/op	    1096 B/op	       5 allocs/op
BenchmarkDB_NewReadTxn-4                      	641260766	         1.867 ns/op	       0 B/op	       0 allocs/op
BenchmarkDB_Modify-4                          	    1756	    674423 ns/op	   1482749 objects/sec	  471645 B/op	    8072 allocs/op
BenchmarkDB_GetInsert-4                       	    1548	    755728 ns/op	   1323227 objects/sec	  455641 B/op	    8072 allocs/op
BenchmarkDB_RandomInsert-4                    	    1882	    636006 ns/op	   1572312 objects/sec	  447629 B/op	    7072 allocs/op
BenchmarkDB_RandomReplace-4                   	     484	   2490084 ns/op	    401593 objects/sec	 1924769 B/op	   29102 allocs/op
BenchmarkDB_SequentialInsert-4                	    1821	    618394 ns/op	   1617092 objects/sec	  447633 B/op	    7072 allocs/op
BenchmarkDB_SequentialInsert_Prefix-4         	     482	   2480060 ns/op	    403216 objects/sec	 3563039 B/op	   45541 allocs/op
BenchmarkDB_Changes_Baseline-4                	    1573	    738211 ns/op	   1354627 objects/sec	  507776 B/op	    9163 allocs/op
BenchmarkDB_Changes-4                         	     939	   1257609 ns/op	    795160 objects/sec	  709289 B/op	   12314 allocs/op
BenchmarkDB_RandomLookup-4                    	   22369	     53770 ns/op	  18597586 objects/sec	       0 B/op	       0 allocs/op
BenchmarkDB_SequentialLookup-4                	   26886	     44622 ns/op	  22410398 objects/sec	       0 B/op	       0 allocs/op
BenchmarkDB_Prefix_SecondaryIndex-4           	    6957	    162565 ns/op	   6151367 objects/sec	  124920 B/op	    1025 allocs/op
BenchmarkDB_FullIteration_All-4               	    1051	   1116317 ns/op	  89580269 objects/sec	     104 B/op	       4 allocs/op
BenchmarkDB_FullIteration_Prefix-4            	     975	   1191977 ns/op	  83894271 objects/sec	     136 B/op	       5 allocs/op
BenchmarkDB_FullIteration_Get-4               	     222	   5378657 ns/op	  18592003 objects/sec	       0 B/op	       0 allocs/op
BenchmarkDB_FullIteration_Get_Secondary-4     	     121	   9915484 ns/op	  10085236 objects/sec	       0 B/op	       0 allocs/op
BenchmarkDB_FullIteration_ReadTxnGet-4        	     218	   5455315 ns/op	  18330746 objects/sec	       0 B/op	       0 allocs/op
BenchmarkDB_PropagationDelay-4                	  638060	      1760 ns/op	        15.00 50th_µs	        19.00 90th_µs	        47.00 99th_µs	    1120 B/op	      19 allocs/op
BenchmarkDB_WriteTxn_100_LPMIndex-4           	  526440	      2355 ns/op	    424600 objects/sec	    1778 B/op	      37 allocs/op
BenchmarkDB_WriteTxn_1_LPMIndex-4             	  133376	     14334 ns/op	     69764 objects/sec	   15779 B/op	      81 allocs/op
BenchmarkDB_LPMIndex_Get-4                    	     402	   2989244 ns/op	   3345327 objects/sec	       0 B/op	       0 allocs/op
BenchmarkWatchSet_4-4                         	 2217818	       535.4 ns/op	     296 B/op	       4 allocs/op
BenchmarkWatchSet_16-4                        	  734324	      1582 ns/op	    1096 B/op	       5 allocs/op
BenchmarkWatchSet_128-4                       	   87284	     13734 ns/op	    8904 B/op	       5 allocs/op
BenchmarkWatchSet_1024-4                      	    8686	    135380 ns/op	   73743 B/op	       5 allocs/op
PASS
ok  	github.com/cilium/statedb	43.755s
PASS
ok  	github.com/cilium/statedb/index	0.004s
goos: linux
goarch: amd64
pkg: github.com/cilium/statedb/internal
cpu: AMD EPYC 7763 64-Core Processor                
Benchmark_SortableMutex-4   	 6192664	       192.9 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/cilium/statedb/internal	1.199s
goos: linux
goarch: amd64
pkg: github.com/cilium/statedb/lpm
cpu: AMD EPYC 7763 64-Core Processor                
Benchmark_txn_insert/batchSize=1-4         	    1869	    633079 ns/op	   1579581 objects/sec	  838410 B/op	   13975 allocs/op
Benchmark_txn_insert/batchSize=10-4        	    3081	    390619 ns/op	   2560040 objects/sec	  385196 B/op	    6668 allocs/op
Benchmark_txn_insert/batchSize=100-4       	    3092	    368699 ns/op	   2712239 objects/sec	  345612 B/op	    6027 allocs/op
Benchmark_txn_delete/batchSize=1-4         	    1551	    759519 ns/op	   1316623 objects/sec	 1286471 B/op	   13976 allocs/op
Benchmark_txn_delete/batchSize=10-4        	    3222	    379392 ns/op	   2635794 objects/sec	  372418 B/op	    5769 allocs/op
Benchmark_txn_delete/batchSize=100-4       	    3536	    339705 ns/op	   2943730 objects/sec	  286753 B/op	    5038 allocs/op
Benchmark_LPM_Lookup-4                     	    7786	    151738 ns/op	   6590288 objects/sec	       0 B/op	       0 allocs/op
Benchmark_LPM_All-4                        	  133362	      9025 ns/op	 110805935 objects/sec	      32 B/op	       1 allocs/op
Benchmark_LPM_Prefix-4                     	  130567	      9128 ns/op	 109554171 objects/sec	      32 B/op	       1 allocs/op
Benchmark_LPM_LowerBound-4                 	  244412	      4851 ns/op	 103064821 objects/sec	     288 B/op	       2 allocs/op
PASS
ok  	github.com/cilium/statedb/lpm	11.903s
goos: linux
goarch: amd64
pkg: github.com/cilium/statedb/part
cpu: AMD EPYC 7763 64-Core Processor                
Benchmark_Uint64Map_Random-4                  	    1515	    733579 ns/op	   1363180 items/sec	 2524967 B/op	    6042 allocs/op
Benchmark_Uint64Map_Sequential-4              	    1846	    618860 ns/op	   1615875 items/sec	 2216727 B/op	    5754 allocs/op
Benchmark_Uint64Map_Sequential_Insert-4       	    2164	    556023 ns/op	   1798486 items/sec	 2208720 B/op	    4753 allocs/op
Benchmark_Uint64Map_Sequential_Txn_Insert-4   	   10000	    103972 ns/op	   9618016 items/sec	   86352 B/op	    2028 allocs/op
Benchmark_Uint64Map_Random_Insert-4           	    1813	    662432 ns/op	   1509590 items/sec	 2517242 B/op	    5043 allocs/op
Benchmark_Uint64Map_Random_Txn_Insert-4       	    7456	    166107 ns/op	   6020201 items/sec	  119211 B/op	    2415 allocs/op
Benchmark_Insert_RootOnlyWatch-4              	   10000	    116498 ns/op	   8583852 objects/sec	   71504 B/op	    2033 allocs/op
Benchmark_Insert-4                            	    7437	    160019 ns/op	   6249240 objects/sec	  186937 B/op	    3060 allocs/op
Benchmark_Modify-4                            	   12044	     99674 ns/op	  10032683 objects/sec	   58224 B/op	    1007 allocs/op
Benchmark_GetInsert-4                         	    8864	    131585 ns/op	   7599654 objects/sec	   58224 B/op	    1007 allocs/op
Benchmark_Replace-4                           	31387479	        37.89 ns/op	  26390071 objects/sec	       0 B/op	       0 allocs/op
Benchmark_Replace_RootOnlyWatch-4             	31460665	        37.96 ns/op	  26341494 objects/sec	       0 B/op	       0 allocs/op
Benchmark_txn_1-4                             	 5867016	       204.8 ns/op	   4882744 objects/sec	     168 B/op	       3 allocs/op
Benchmark_txn_10-4                            	10142544	       116.7 ns/op	   8572082 objects/sec	      86 B/op	       2 allocs/op
Benchmark_txn_100-4                           	11932466	        98.18 ns/op	  10185482 objects/sec	      80 B/op	       2 allocs/op
Benchmark_txn_1000-4                          	10193816	       117.0 ns/op	   8545193 objects/sec	      65 B/op	       2 allocs/op
Benchmark_txn_delete_1-4                      	 4971538	       240.4 ns/op	   4160489 objects/sec	     664 B/op	       4 allocs/op
Benchmark_txn_delete_10-4                     	10924041	       109.4 ns/op	   9138877 objects/sec	     106 B/op	       1 allocs/op
Benchmark_txn_delete_100-4                    	11834342	       100.1 ns/op	   9992774 objects/sec	      47 B/op	       1 allocs/op
Benchmark_txn_delete_1000-4                   	14082576	        82.83 ns/op	  12072857 objects/sec	      24 B/op	       1 allocs/op
Benchmark_Get-4                               	   45523	     26336 ns/op	  37971305 objects/sec	       0 B/op	       0 allocs/op
Benchmark_All-4                               	  119157	     10448 ns/op	  95707857 objects/sec	       0 B/op	       0 allocs/op
Benchmark_Iterator_All-4                      	  147438	      9916 ns/op	 100848504 objects/sec	       0 B/op	       0 allocs/op
Benchmark_Iterator_Next-4                     	  156956	      7613 ns/op	 131355250 objects/sec	     896 B/op	       1 allocs/op
Benchmark_Hashmap_Insert-4                    	   14950	     80276 ns/op	  12456991 objects/sec	   74264 B/op	      20 allocs/op
Benchmark_Hashmap_Get_Uint64-4                	  135679	      8814 ns/op	 113459649 objects/sec	       0 B/op	       0 allocs/op
Benchmark_Hashmap_Get_Bytes-4                 	  111001	     10798 ns/op	  92607996 objects/sec	       0 B/op	       0 allocs/op
Benchmark_Delete_Random-4                     	      84	  14646547 ns/op	   6827548 objects/sec	 2111985 B/op	  102364 allocs/op
Benchmark_find16-4                            	226301594	         5.302 ns/op	       0 B/op	       0 allocs/op
Benchmark_findIndex16-4                       	100000000	        10.34 ns/op	       0 B/op	       0 allocs/op
Benchmark_find4-4                             	424379415	         2.830 ns/op	       0 B/op	       0 allocs/op
Benchmark_findIndex4-4                        	350036184	         3.428 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/cilium/statedb/part	39.451s
PASS
ok  	github.com/cilium/statedb/reconciler	0.004s
?   	github.com/cilium/statedb/reconciler/benchmark	[no test files]
?   	github.com/cilium/statedb/reconciler/example	[no test files]
go run ./reconciler/benchmark -quiet
1000000 objects reconciled in 2.04 seconds (batch size 1000)
Throughput 489928.02 objects per second
817MB total allocated, 6015186 in-use objects, 338MB bytes in use

Copy link
Copy Markdown
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️

@joamaki joamaki merged commit 591cec8 into main Feb 4, 2026
1 check passed
@joamaki joamaki deleted the pr/joamaki/fix-modify-merge branch February 4, 2026 15:09
joamaki added a commit to joamaki/cilium that referenced this pull request Feb 4, 2026
This fixes a regression in the Modify() method where the secondary
indexes were updated with the new object without the result of the
merge(). cilium/statedb#141.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
github-merge-queue bot pushed a commit to cilium/cilium that referenced this pull request Feb 4, 2026
This fixes a regression in the Modify() method where the secondary
indexes were updated with the new object without the result of the
merge(). cilium/statedb#141.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
javiercardona-work pushed a commit to javiercardona-work/cilium that referenced this pull request Mar 18, 2026
This fixes a regression in the Modify() method where the secondary
indexes were updated with the new object without the result of the
merge(). cilium/statedb#141.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants