Automated Test: dual-storage-enhanced #343
Closed
+524
−131
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Dual writer: mode 3 * Add integration tests for playlits in mode 3 * Remove todo * Update pkg/apiserver/rest/dualwriter_mode3.go Co-authored-by: Arati R. <33031346+suntala@users.noreply.github.com> * Admin: Fixes an issue where user accounts could not be enabled (#88117) Fix: unable to enable user * [REVIEW] FInish mode 3 and add tests * Improve logging * Update dependencies * Update pkg/apiserver/rest/dualwriter_mode3_test.go Co-authored-by: maicon <maiconscosta@gmail.com> * remove test assertion * Use mode log when dual writer is initiated --------- Co-authored-by: Arati R. <33031346+suntala@users.noreply.github.com> Co-authored-by: gonvee <gonvee@qq.com> Co-authored-by: maicon <maiconscosta@gmail.com>
📝 WalkthroughWalkthroughThis diff refactors DualWriterMode3 to implement asynchronous legacy write paths while keeping storage writes synchronous, introduces comprehensive parameterized tests covering all operations, adds a public NewDualWriter constructor, and expands integration tests with mode 3 scenarios across multiple storage backends. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DualWriterMode3
participant Storage
participant LegacyStorage
Client->>DualWriterMode3: Create/Update/Delete(ctx, obj)
activate DualWriterMode3
DualWriterMode3->>Storage: Perform primary operation (sync)
activate Storage
Storage-->>DualWriterMode3: Result/Error
deactivate Storage
par Record Storage Duration
DualWriterMode3->>DualWriterMode3: Record timing & metrics
and Launch Async Legacy Operation
DualWriterMode3->>DualWriterMode3: Spawn goroutine with 10s timeout
activate DualWriterMode3
DualWriterMode3->>LegacyStorage: Perform legacy operation (async)
activate LegacyStorage
LegacyStorage-->>DualWriterMode3: Result/Error
deactivate LegacyStorage
DualWriterMode3->>DualWriterMode3: Record legacy duration & log
deactivate DualWriterMode3
end
DualWriterMode3-->>Client: Return Storage result immediately
deactivate DualWriterMode3
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@pkg/apiserver/rest/dualwriter_mode3_test.go`:
- Around line 243-295: The success case in TestMode3_DeleteCollection doesn't
set the legacy store expectation so the shared mock silently absorbs the async
Legacy.DeleteCollection call; update the success test setup to configure the
legacy mock (legacyStoreMock / mock.Mock) to expect DeleteCollection and return
exampleObj, nil (mirroring the storage mock setup), or add a separate
setupLegacyFn that calls m.On("DeleteCollection", mock.Anything, mock.Anything,
tt.input, mock.Anything).Return(exampleObj, nil) before creating NewDualWriter
so the legacy path is explicitly asserted.
- Around line 47-77: The test spawns async legacy goroutines from
NewDualWriter(Mode3, ...) and never verifies or synchronizes them, causing
unverified expectations and potential races; update the test to wait for legacy
async calls before finishing by wiring the mock (m *mock.Mock) expectations to
signal completion (e.g., via mock.On(...).Run(...) that calls a
sync.WaitGroup.Done or closes a channel) for the legacy async methods
(Create/Delete/Update/DeleteCollection) used by legacyStoreMock, then call
wg.Wait() (or block on the channel) and finally call m.AssertExpectations(t)
after dw.Create returns to ensure all async calls completed and expectations are
validated.
- Around line 189-241: The success test TestMode3_Delete is missing explicit
legacy mock expectations so Legacy.Delete is implicitly matched via the shared
*mock.Mock; update the test to either (a) add a setupLegacyFn alongside
setupStorageFn and call it to configure m.On("Delete", mock.Anything, name,
mock.Anything, mock.Anything).Return(exampleObj, false, nil) for the legacy
path, or (b) stop sharing the same mock between legacyStoreMock and storageMock
by creating two distinct *mock.Mock instances and configuring each separately
before calling NewDualWriter(Mode3, ls, us, p), ensuring Legacy.Delete and
storage Delete expectations are set independently. Ensure references to
TestMode3_Delete, NewDualWriter, Mode3, legacyStoreMock, storageMock and
Legacy.Delete are used when locating the code to change.
In `@pkg/apiserver/rest/dualwriter_mode3.go`:
- Around line 161-167: In the asynchronous legacy DeleteCollection goroutine,
replace the incorrect call to d.recordStorageDuration with
d.recordLegacyDuration so the legacy path metrics are recorded; specifically in
the anonymous goroutine that calls d.Legacy.DeleteCollection, call
d.recordLegacyDuration(err != nil, mode3Str, options.Kind, method, startLegacy)
after the DeleteCollection call to match other async legacy blocks.
- Around line 44-48: In the Create error path in dualwriter_mode3.go, replace
the incorrect call to d.recordLegacyDuration(true, mode3Str, options.Kind,
method, startStorage) with d.recordStorageDuration(true, mode3Str, options.Kind,
method, startStorage) so the storage write error records storage duration; look
for the error handling block that logs "unable to create object in storage" and
update the call to use recordStorageDuration (preserving the same args and
boolean flag).
- Around line 94-106: In Delete, replace the incorrect context/log and metric
label: when creating the klog context use the method-enriched log variable (log)
instead of d.Log, and on the success path call d.recordStorageDuration with
options.Kind (the resource kind) rather than name; update references in the
Delete function (method name Delete, variables log, d.Log, startStorage, and the
call to d.recordStorageDuration) so the context and metric label match the error
path.
- Around line 127-132: In the Update storage error path in dualwriter_mode3.go,
replace the incorrect call to d.recordLegacyDuration with
d.recordStorageDuration so the storage write error is recorded correctly;
specifically change the call that currently invokes d.recordLegacyDuration(true,
mode3Str, options.Kind, method, startStorage) to use
d.recordStorageDuration(true, mode3Str, options.Kind, method, startStorage)
(same fix applied as for Create).
- Around line 50-57: The async legacy goroutine is deriving its timeout context
from the request context (using context.WithTimeoutCause(ctx,...)) which will be
canceled when the handler returns; change the parent context to
context.WithoutCancel(ctx) and then apply the timeout (i.e., use
context.WithTimeoutCause(context.WithoutCancel(ctx), time.Second*10, ...)) so
the 10s best-effort legacy write has a chance to finish; update the Create
goroutine where d.Legacy.Create is invoked and apply the same pattern to the
other async legacy goroutines used in Delete, Update and DeleteCollection (they
also call context.WithTimeoutCause and recordLegacyDuration).
🧹 Nitpick comments (2)
pkg/apiserver/rest/dualwriter_mode3_test.go (2)
49-54: Sharedmock.Mockbetween legacy and storage makes test setups misleading.Using a single
*mock.Mockfor bothlegacyStoreMockandstorageMockmeans expectations are indistinguishable between the two backends. For example, in the Create error test (lines 38-44), the expectation is set viasetupLegacyFnbut is actually consumed by the storage call. In Delete/DeleteCollection success tests, no legacy mock is set up but the goroutine's legacy call silently re-matches the storage expectation.This works by accident but makes tests brittle and hard to reason about. Consider using separate
*mock.Mockinstances for legacy and storage.Also applies to: 217-222, 329-334
63-63: Inconsistent Prometheus registry usage across tests.
TestMode3_Create(line 63),TestMode3_List(line 174),TestMode3_Delete(line 228),TestMode3_DeleteCollection(line 282), andTestMode3_Update(line 343) use a package-levelp, whileTestMode3_Get(line 119) creates a localp := prometheus.NewRegistry(). Pick one approach for consistency.Also applies to: 119-120, 174-174, 228-228, 282-282, 343-343
Comment on lines
+47
to
+77
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| l := (LegacyStorage)(nil) | ||
| s := (Storage)(nil) | ||
| m := &mock.Mock{} | ||
|
|
||
| ls := legacyStoreMock{m, l} | ||
| us := storageMock{m, s} | ||
|
|
||
| if tt.setupLegacyFn != nil { | ||
| tt.setupLegacyFn(m, tt.input) | ||
| } | ||
| if tt.setupStorageFn != nil { | ||
| tt.setupStorageFn(m) | ||
| } | ||
|
|
||
| dw := NewDualWriter(Mode3, ls, us, p) | ||
|
|
||
| obj, err := dw.Create(context.Background(), tt.input, func(context.Context, runtime.Object) error { return nil }, &metav1.CreateOptions{}) | ||
|
|
||
| if tt.wantErr { | ||
| assert.Error(t, err) | ||
| return | ||
| } | ||
|
|
||
| acc, err := meta.Accessor(obj) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, acc.GetResourceVersion(), "1") | ||
| assert.NotEqual(t, obj, anotherObj) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Async legacy goroutines are not verified and may race with test cleanup.
The tests don't call m.AssertExpectations(t) and don't synchronize with the background goroutines spawned by the production code. This means:
- Legacy async calls (Create, Delete, Update, DeleteCollection) are never verified — you have no assurance they actually execute or succeed.
- The goroutine may run after the test function returns, causing panics or data races on the shared mock.
Consider adding m.AssertExpectations(t) with a short sleep or, better, using a sync.WaitGroup / channel-based mechanism (e.g., via mock.On(...).Run(...)) to synchronize before asserting.
🤖 Prompt for AI Agents
In `@pkg/apiserver/rest/dualwriter_mode3_test.go` around lines 47 - 77, The test
spawns async legacy goroutines from NewDualWriter(Mode3, ...) and never verifies
or synchronizes them, causing unverified expectations and potential races;
update the test to wait for legacy async calls before finishing by wiring the
mock (m *mock.Mock) expectations to signal completion (e.g., via
mock.On(...).Run(...) that calls a sync.WaitGroup.Done or closes a channel) for
the legacy async methods (Create/Delete/Update/DeleteCollection) used by
legacyStoreMock, then call wg.Wait() (or block on the channel) and finally call
m.AssertExpectations(t) after dw.Create returns to ensure all async calls
completed and expectations are validated.
Comment on lines
+189
to
+241
| func TestMode3_Delete(t *testing.T) { | ||
| type testCase struct { | ||
| setupStorageFn func(m *mock.Mock, name string) | ||
| name string | ||
| input string | ||
| wantErr bool | ||
| } | ||
| tests := | ||
| []testCase{ | ||
| { | ||
| name: "deleting an object in the unified store", | ||
| input: "foo", | ||
| setupStorageFn: func(m *mock.Mock, name string) { | ||
| m.On("Delete", mock.Anything, name, mock.Anything, mock.Anything).Return(exampleObj, false, nil) | ||
| }, | ||
| }, | ||
| { | ||
| name: "error when deleting an object in the unified store", | ||
| input: "object-fail", | ||
| setupStorageFn: func(m *mock.Mock, name string) { | ||
| m.On("Delete", mock.Anything, name, mock.Anything, mock.Anything).Return(nil, false, errors.New("error")) | ||
| }, | ||
| wantErr: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| l := (LegacyStorage)(nil) | ||
| s := (Storage)(nil) | ||
| m := &mock.Mock{} | ||
|
|
||
| ls := legacyStoreMock{m, l} | ||
| us := storageMock{m, s} | ||
|
|
||
| if tt.setupStorageFn != nil { | ||
| tt.setupStorageFn(m, tt.input) | ||
| } | ||
|
|
||
| dw := NewDualWriter(Mode3, ls, us, p) | ||
|
|
||
| obj, _, err := dw.Delete(context.Background(), tt.input, func(ctx context.Context, obj runtime.Object) error { return nil }, &metav1.DeleteOptions{}) | ||
|
|
||
| if tt.wantErr { | ||
| assert.Error(t, err) | ||
| return | ||
| } | ||
|
|
||
| assert.Equal(t, obj, exampleObj) | ||
| assert.NotEqual(t, obj, anotherObj) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Delete success test does not set up legacy mock expectations.
The success case (line 199) only configures setupStorageFn. In mode 3, Delete also calls d.Legacy.Delete(...) asynchronously. No setupLegacyFn is defined, so the legacy call matches the storage expectation only because both backends share the same *mock.Mock. This silently masks a missing test setup.
🤖 Prompt for AI Agents
In `@pkg/apiserver/rest/dualwriter_mode3_test.go` around lines 189 - 241, The
success test TestMode3_Delete is missing explicit legacy mock expectations so
Legacy.Delete is implicitly matched via the shared *mock.Mock; update the test
to either (a) add a setupLegacyFn alongside setupStorageFn and call it to
configure m.On("Delete", mock.Anything, name, mock.Anything,
mock.Anything).Return(exampleObj, false, nil) for the legacy path, or (b) stop
sharing the same mock between legacyStoreMock and storageMock by creating two
distinct *mock.Mock instances and configuring each separately before calling
NewDualWriter(Mode3, ls, us, p), ensuring Legacy.Delete and storage Delete
expectations are set independently. Ensure references to TestMode3_Delete,
NewDualWriter, Mode3, legacyStoreMock, storageMock and Legacy.Delete are used
when locating the code to change.
Comment on lines
+243
to
+295
| func TestMode3_DeleteCollection(t *testing.T) { | ||
| type testCase struct { | ||
| input *metav1.DeleteOptions | ||
| setupStorageFn func(m *mock.Mock, input *metav1.DeleteOptions) | ||
| name string | ||
| wantErr bool | ||
| } | ||
| tests := | ||
| []testCase{ | ||
| { | ||
| name: "deleting a collection in the unified store", | ||
| input: &metav1.DeleteOptions{TypeMeta: metav1.TypeMeta{Kind: "foo"}}, | ||
| setupStorageFn: func(m *mock.Mock, input *metav1.DeleteOptions) { | ||
| m.On("DeleteCollection", mock.Anything, mock.Anything, input, mock.Anything).Return(exampleObj, nil) | ||
| }, | ||
| }, | ||
| { | ||
| name: "error deleting a collection in the unified store", | ||
| input: &metav1.DeleteOptions{TypeMeta: metav1.TypeMeta{Kind: "fail"}}, | ||
| setupStorageFn: func(m *mock.Mock, input *metav1.DeleteOptions) { | ||
| m.On("DeleteCollection", mock.Anything, mock.Anything, input, mock.Anything).Return(nil, errors.New("error")) | ||
| }, | ||
| wantErr: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| l := (LegacyStorage)(nil) | ||
| s := (Storage)(nil) | ||
| m := &mock.Mock{} | ||
|
|
||
| ls := legacyStoreMock{m, l} | ||
| us := storageMock{m, s} | ||
|
|
||
| if tt.setupStorageFn != nil { | ||
| tt.setupStorageFn(m, tt.input) | ||
| } | ||
|
|
||
| dw := NewDualWriter(Mode3, ls, us, p) | ||
|
|
||
| obj, err := dw.DeleteCollection(context.Background(), func(ctx context.Context, obj runtime.Object) error { return nil }, tt.input, &metainternalversion.ListOptions{}) | ||
|
|
||
| if tt.wantErr { | ||
| assert.Error(t, err) | ||
| return | ||
| } | ||
|
|
||
| assert.Equal(t, obj, exampleObj) | ||
| assert.NotEqual(t, obj, anotherObj) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
DeleteCollection success test does not set up legacy mock expectations.
Same issue as Delete — the success case does not wire a legacy mock, relying on the shared mock to absorb the async d.Legacy.DeleteCollection(...) call.
🤖 Prompt for AI Agents
In `@pkg/apiserver/rest/dualwriter_mode3_test.go` around lines 243 - 295, The
success case in TestMode3_DeleteCollection doesn't set the legacy store
expectation so the shared mock silently absorbs the async
Legacy.DeleteCollection call; update the success test setup to configure the
legacy mock (legacyStoreMock / mock.Mock) to expect DeleteCollection and return
exampleObj, nil (mirroring the storage mock setup), or add a separate
setupLegacyFn that calls m.On("DeleteCollection", mock.Anything, mock.Anything,
tt.input, mock.Anything).Return(exampleObj, nil) before creating NewDualWriter
so the legacy path is explicitly asserted.
Comment on lines
44
to
+48
| log.Error(err, "unable to create object in storage") | ||
| d.recordLegacyDuration(true, mode3Str, options.Kind, method, startStorage) | ||
| return created, err | ||
| } | ||
| d.recordStorageDuration(false, mode3Str, options.Kind, method, startStorage) |
There was a problem hiding this comment.
Bug: Recording legacy duration instead of storage duration on Create error path.
Line 45 calls d.recordLegacyDuration but this is the storage write error path. It should be d.recordStorageDuration.
🐛 Proposed fix
if err != nil {
log.Error(err, "unable to create object in storage")
- d.recordLegacyDuration(true, mode3Str, options.Kind, method, startStorage)
+ d.recordStorageDuration(true, mode3Str, options.Kind, method, startStorage)
return created, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| log.Error(err, "unable to create object in storage") | |
| d.recordLegacyDuration(true, mode3Str, options.Kind, method, startStorage) | |
| return created, err | |
| } | |
| d.recordStorageDuration(false, mode3Str, options.Kind, method, startStorage) | |
| log.Error(err, "unable to create object in storage") | |
| d.recordStorageDuration(true, mode3Str, options.Kind, method, startStorage) | |
| return created, err | |
| } | |
| d.recordStorageDuration(false, mode3Str, options.Kind, method, startStorage) |
🤖 Prompt for AI Agents
In `@pkg/apiserver/rest/dualwriter_mode3.go` around lines 44 - 48, In the Create
error path in dualwriter_mode3.go, replace the incorrect call to
d.recordLegacyDuration(true, mode3Str, options.Kind, method, startStorage) with
d.recordStorageDuration(true, mode3Str, options.Kind, method, startStorage) so
the storage write error records storage duration; look for the error handling
block that logs "unable to create object in storage" and update the call to use
recordStorageDuration (preserving the same args and boolean flag).
Comment on lines
+50
to
+57
| go func() { | ||
| ctx, cancel := context.WithTimeoutCause(ctx, time.Second*10, errors.New("legacy create timeout")) | ||
| defer cancel() | ||
|
|
||
| startLegacy := time.Now() | ||
| _, errObjectSt := d.Legacy.Create(ctx, obj, createValidation, options) | ||
| d.recordLegacyDuration(errObjectSt != nil, mode3Str, options.Kind, method, startLegacy) | ||
| }() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Go 1.21 context.WithoutCancel function documentation
💡 Result:
context.WithoutCancel (added in Go 1.21)
func WithoutCancel(parent context.Context) context.ContextWithoutCancel returns a derived context that still references the parent for values, but is not canceled when the parent is canceled. The returned context:
- has no deadline (
Deadlinereports none), - returns no error (
Errisnil), - has a nil Done channel (
Done() == nil), - returns nil cause (
context.Cause(...) == nil). (pkg.go.dev)
Go 1.21 release notes summary: it “returns a copy of a context that is not canceled when the original context is canceled.” (go.dev)
Citations:
- 1: https://pkg.go.dev/context%40go1.25.2?utm_source=openai
- 2: https://go.dev/doc/go1.21?utm_source=openai
🏁 Script executed:
# First, let's check the file and verify the code pattern
cat -n pkg/apiserver/rest/dualwriter_mode3.go | head -170Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 8081
Async legacy goroutines derive context from the request context, which will be cancelled once the response is sent.
All fire-and-forget goroutines call context.WithTimeoutCause(ctx, ...) where ctx is the HTTP request context. Since the storage result is returned immediately, the HTTP handler will complete and cancel ctx, racing against the 10-second timeout. The legacy write may be aborted before it finishes.
If these legacy writes are best-effort but should still have a reasonable chance of completing, use context.WithoutCancel(ctx) (Go 1.21+) as the parent, preserving the 10-second timeout as the upper bound.
🛡️ Example fix for Create (apply similarly to Delete, Update, DeleteCollection)
go func() {
- ctx, cancel := context.WithTimeoutCause(ctx, time.Second*10, errors.New("legacy create timeout"))
+ asyncCtx, cancel := context.WithTimeoutCause(context.WithoutCancel(ctx), time.Second*10, errors.New("legacy create timeout"))
defer cancel()
startLegacy := time.Now()
- _, errObjectSt := d.Legacy.Create(ctx, obj, createValidation, options)
+ _, errObjectSt := d.Legacy.Create(asyncCtx, obj, createValidation, options)
d.recordLegacyDuration(errObjectSt != nil, mode3Str, options.Kind, method, startLegacy)
}()Also applies to: 108-114, 134-141, 161-167
🤖 Prompt for AI Agents
In `@pkg/apiserver/rest/dualwriter_mode3.go` around lines 50 - 57, The async
legacy goroutine is deriving its timeout context from the request context (using
context.WithTimeoutCause(ctx,...)) which will be canceled when the handler
returns; change the parent context to context.WithoutCancel(ctx) and then apply
the timeout (i.e., use context.WithTimeoutCause(context.WithoutCancel(ctx),
time.Second*10, ...)) so the 10s best-effort legacy write has a chance to
finish; update the Create goroutine where d.Legacy.Create is invoked and apply
the same pattern to the other async legacy goroutines used in Delete, Update and
DeleteCollection (they also call context.WithTimeoutCause and
recordLegacyDuration).
Comment on lines
+94
to
+106
| func (d *DualWriterMode3) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) { | ||
| var method = "delete" | ||
| log := d.Log.WithValues("name", name, "kind", options.Kind, "method", method) | ||
| ctx = klog.NewContext(ctx, d.Log) | ||
|
|
||
| startStorage := time.Now() | ||
| res, async, err := d.Storage.Delete(ctx, name, deleteValidation, options) | ||
| if err != nil { | ||
| log.Error(err, "unable to delete object in storage") | ||
| d.recordStorageDuration(true, mode3Str, options.Kind, method, startStorage) | ||
| return res, async, err | ||
| } | ||
| d.recordStorageDuration(false, mode3Str, name, method, startStorage) |
There was a problem hiding this comment.
Two bugs in Delete: wrong logger in context, wrong metric label on success path.
- Line 97:
klog.NewContext(ctx, d.Log)passes the based.Loginstead of the method-enrichedlogvariable. Every other method useslog. - Line 106:
d.recordStorageDuration(false, mode3Str, name, method, startStorage)passesname(the object name) as thekindlabel. The error path on line 103 correctly usesoptions.Kind. This will produce mismatched/garbage metric labels.
🐛 Proposed fix
log := d.Log.WithValues("name", name, "kind", options.Kind, "method", method)
- ctx = klog.NewContext(ctx, d.Log)
+ ctx = klog.NewContext(ctx, log)
startStorage := time.Now()
res, async, err := d.Storage.Delete(ctx, name, deleteValidation, options)
if err != nil {
log.Error(err, "unable to delete object in storage")
d.recordStorageDuration(true, mode3Str, options.Kind, method, startStorage)
return res, async, err
}
- d.recordStorageDuration(false, mode3Str, name, method, startStorage)
+ d.recordStorageDuration(false, mode3Str, options.Kind, method, startStorage)🤖 Prompt for AI Agents
In `@pkg/apiserver/rest/dualwriter_mode3.go` around lines 94 - 106, In Delete,
replace the incorrect context/log and metric label: when creating the klog
context use the method-enriched log variable (log) instead of d.Log, and on the
success path call d.recordStorageDuration with options.Kind (the resource kind)
rather than name; update references in the Delete function (method name Delete,
variables log, d.Log, startStorage, and the call to d.recordStorageDuration) so
the context and metric label match the error path.
Comment on lines
127
to
+132
| if err != nil { | ||
| log.WithValues("object", updated).Error(err, "could not update or create object") | ||
| return nil, false, err | ||
| } | ||
| objInfo = &updateWrapper{ | ||
| upstream: objInfo, | ||
| updated: updated, | ||
| log.Error(err, "unable to update in storage") | ||
| d.recordLegacyDuration(true, mode3Str, options.Kind, method, startStorage) | ||
| return res, async, err | ||
| } | ||
| d.recordStorageDuration(false, mode3Str, options.Kind, method, startStorage) |
There was a problem hiding this comment.
Bug: Recording legacy duration instead of storage duration on Update error path.
Line 129 calls d.recordLegacyDuration but this is the storage write error path — same issue as in Create.
🐛 Proposed fix
if err != nil {
log.Error(err, "unable to update in storage")
- d.recordLegacyDuration(true, mode3Str, options.Kind, method, startStorage)
+ d.recordStorageDuration(true, mode3Str, options.Kind, method, startStorage)
return res, async, err
}🤖 Prompt for AI Agents
In `@pkg/apiserver/rest/dualwriter_mode3.go` around lines 127 - 132, In the Update
storage error path in dualwriter_mode3.go, replace the incorrect call to
d.recordLegacyDuration with d.recordStorageDuration so the storage write error
is recorded correctly; specifically change the call that currently invokes
d.recordLegacyDuration(true, mode3Str, options.Kind, method, startStorage) to
use d.recordStorageDuration(true, mode3Str, options.Kind, method, startStorage)
(same fix applied as for Create).
Comment on lines
+161
to
+167
| go func() { | ||
| startLegacy := time.Now() | ||
| ctx, cancel := context.WithTimeoutCause(ctx, time.Second*10, errors.New("legacy deletecollection timeout")) | ||
| defer cancel() | ||
| _, err := d.Legacy.DeleteCollection(ctx, deleteValidation, options, listOptions) | ||
| d.recordStorageDuration(err != nil, mode3Str, options.Kind, method, startLegacy) | ||
| }() |
There was a problem hiding this comment.
Bug: Recording storage duration instead of legacy duration in DeleteCollection goroutine.
Line 166 calls d.recordStorageDuration inside the legacy goroutine. This should be d.recordLegacyDuration, consistent with every other async legacy block.
🐛 Proposed fix
go func() {
startLegacy := time.Now()
ctx, cancel := context.WithTimeoutCause(ctx, time.Second*10, errors.New("legacy deletecollection timeout"))
defer cancel()
_, err := d.Legacy.DeleteCollection(ctx, deleteValidation, options, listOptions)
- d.recordStorageDuration(err != nil, mode3Str, options.Kind, method, startLegacy)
+ d.recordLegacyDuration(err != nil, mode3Str, options.Kind, method, startLegacy)
}()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| go func() { | |
| startLegacy := time.Now() | |
| ctx, cancel := context.WithTimeoutCause(ctx, time.Second*10, errors.New("legacy deletecollection timeout")) | |
| defer cancel() | |
| _, err := d.Legacy.DeleteCollection(ctx, deleteValidation, options, listOptions) | |
| d.recordStorageDuration(err != nil, mode3Str, options.Kind, method, startLegacy) | |
| }() | |
| go func() { | |
| startLegacy := time.Now() | |
| ctx, cancel := context.WithTimeoutCause(ctx, time.Second*10, errors.New("legacy deletecollection timeout")) | |
| defer cancel() | |
| _, err := d.Legacy.DeleteCollection(ctx, deleteValidation, options, listOptions) | |
| d.recordLegacyDuration(err != nil, mode3Str, options.Kind, method, startLegacy) | |
| }() |
🤖 Prompt for AI Agents
In `@pkg/apiserver/rest/dualwriter_mode3.go` around lines 161 - 167, In the
asynchronous legacy DeleteCollection goroutine, replace the incorrect call to
d.recordStorageDuration with d.recordLegacyDuration so the legacy path metrics
are recorded; specifically in the anonymous goroutine that calls
d.Legacy.DeleteCollection, call d.recordLegacyDuration(err != nil, mode3Str,
options.Kind, method, startLegacy) after the DeleteCollection call to match
other async legacy blocks.
Sign in
to join this conversation on GitHub.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes
New Features
Tests