Dual Storage Architecture #6
Conversation
* 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>
WalkthroughThe changes introduce extensive enhancements to the dual writer "Mode3" implementation, including structured logging, timing metrics, and asynchronous legacy operations. Comprehensive unit tests for Mode3 are added, covering all CRUD operations and error scenarios. Integration tests are expanded to include Mode3 across multiple storage backends. Minor test code cleanup is also performed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DualWriterMode3
participant Storage
participant LegacyStorage
participant Logger
participant Metrics
Client->>DualWriterMode3: Create/Get/List/Delete/Update/DeleteCollection
DualWriterMode3->>Logger: Log method, kind, context
DualWriterMode3->>Metrics: Start timer for Storage
DualWriterMode3->>Storage: Perform operation
Storage-->>DualWriterMode3: Result or error
DualWriterMode3->>Metrics: Record Storage duration/status
alt Success or Error
DualWriterMode3->>LegacyStorage: (async, with timeout) Perform operation
LegacyStorage-->>DualWriterMode3: Result or error (ignored by client)
DualWriterMode3->>Metrics: Record Legacy duration/status
end
DualWriterMode3-->>Client: Return Storage result/error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
pkg/apiserver/rest/dualwriter_mode3_test.go (2)
119-120: Use the shared Prometheus registry for consistency.The test creates a new Prometheus registry instead of using the shared
pvariable that's used in other tests.- p := prometheus.NewRegistry() - dw := NewDualWriter(Mode3, ls, us, p) + dw := NewDualWriter(Mode3, ls, us, p)
17-356: Consider adding verification for asynchronous legacy operations.The tests verify that the unified storage operations work correctly but don't verify that legacy operations are actually called asynchronously. Since Mode3 runs legacy operations in goroutines with timeouts, consider adding assertions to verify:
- Legacy methods are eventually called
- The main operation doesn't wait for legacy completion
You could use mock's
AssertExpectationswith a timeout or channels to verify async behavior.pkg/apiserver/rest/dualwriter_mode3.go (2)
55-55: Use consistent error variable naming.The error variable is named
errObjectStin some places, which is inconsistent with the simpleerrused elsewhere.- _, errObjectSt := d.Legacy.Create(ctx, obj, createValidation, options) - d.recordLegacyDuration(errObjectSt != nil, mode3Str, options.Kind, method, startLegacy) + _, err := d.Legacy.Create(ctx, obj, createValidation, options) + d.recordLegacyDuration(err != nil, mode3Str, options.Kind, method, startLegacy)- _, _, errObjectSt := d.Legacy.Update(ctx, name, objInfo, createValidation, updateValidation, forceAllowCreate, options) - d.recordLegacyDuration(errObjectSt != nil, mode3Str, options.Kind, method, startLegacy) + _, _, err := d.Legacy.Update(ctx, name, objInfo, createValidation, updateValidation, forceAllowCreate, options) + d.recordLegacyDuration(err != nil, mode3Str, options.Kind, method, startLegacy)Also applies to: 139-139
51-57: Consider adding error logging for legacy operations.While storage operations have error logging, the asynchronous legacy operations don't log their errors. This could make debugging issues with legacy storage difficult.
Example for Create method:
startLegacy := time.Now() - _, errObjectSt := d.Legacy.Create(ctx, obj, createValidation, options) - d.recordLegacyDuration(errObjectSt != nil, mode3Str, options.Kind, method, startLegacy) + _, err := d.Legacy.Create(ctx, obj, createValidation, options) + if err != nil { + log.Error(err, "failed to create object in legacy storage") + } + d.recordLegacyDuration(err != nil, mode3Str, options.Kind, method, startLegacy)Also applies to: 108-114, 134-141, 161-167
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
pkg/apiserver/rest/dualwriter_mode1_test.go(0 hunks)pkg/apiserver/rest/dualwriter_mode3.go(2 hunks)pkg/apiserver/rest/dualwriter_mode3_test.go(1 hunks)pkg/tests/apis/playlist/playlist_test.go(3 hunks)
💤 Files with no reviewable changes (1)
- pkg/apiserver/rest/dualwriter_mode1_test.go
🔇 Additional comments (1)
pkg/tests/apis/playlist/playlist_test.go (1)
132-145: LGTM! Test coverage for Mode3 is comprehensive.The integration tests for Mode3 are well-structured and consistent with the existing test patterns for other dual writer modes. They properly cover all three storage backends (file, unified storage, and etcd).
Also applies to: 191-205, 287-312
| created, err := d.Storage.Create(ctx, obj, createValidation, options) | ||
| if err != nil { | ||
| log.Error(err, "unable to create object in storage") | ||
| d.recordLegacyDuration(true, mode3Str, options.Kind, method, startStorage) |
There was a problem hiding this comment.
Fix incorrect metric recording on storage error.
When storage operations fail, the code incorrectly records legacy duration instead of storage duration.
- d.recordLegacyDuration(true, mode3Str, options.Kind, method, startStorage)
+ d.recordStorageDuration(true, mode3Str, options.Kind, method, startStorage)Also applies to: 129-129
🤖 Prompt for AI Agents
In pkg/apiserver/rest/dualwriter_mode3.go at lines 45 and 129, the code
incorrectly calls recordLegacyDuration when a storage operation fails, but it
should call recordStorageDuration instead. Update these lines to call
d.recordStorageDuration with the appropriate parameters to correctly record the
storage duration metric on storage errors.
| 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) |
There was a problem hiding this comment.
Use the local logger variable consistently.
The Delete method creates a local log variable but then uses d.Log when creating the context.
- ctx = klog.NewContext(ctx, d.Log)
+ ctx = klog.NewContext(ctx, log)📝 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.
| ctx = klog.NewContext(ctx, d.Log) | |
| - ctx = klog.NewContext(ctx, d.Log) | |
| + ctx = klog.NewContext(ctx, log) |
🤖 Prompt for AI Agents
In pkg/apiserver/rest/dualwriter_mode3.go at line 97, the code uses d.Log
instead of the local log variable when creating the new context. Replace d.Log
with the local log variable to ensure consistent use of the local logger
throughout the Delete method.
| 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.
Use consistent metric parameters.
The Delete method uses name instead of options.Kind for metric recording, which is inconsistent with other methods.
- d.recordStorageDuration(false, mode3Str, name, method, startStorage)
+ d.recordStorageDuration(false, mode3Str, options.Kind, method, startStorage)📝 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.
| 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 at line 106, the Delete method calls
recordStorageDuration with the parameter 'name' for metrics, which is
inconsistent with other methods that use 'options.Kind'. Update the call to use
'options.Kind' instead of 'name' to maintain consistency in metric parameters.
| 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.
Fix metric recording for legacy operations.
The DeleteCollection method incorrectly calls recordStorageDuration for legacy operations instead of recordLegacyDuration.
- 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.
| d.recordStorageDuration(err != nil, mode3Str, options.Kind, method, startLegacy) | |
| d.recordLegacyDuration(err != nil, mode3Str, options.Kind, method, startLegacy) |
🤖 Prompt for AI Agents
In pkg/apiserver/rest/dualwriter_mode3.go at line 166, the DeleteCollection
method incorrectly calls recordStorageDuration for legacy operations. Replace
the call to recordStorageDuration with recordLegacyDuration, passing the
appropriate parameters to correctly record the duration for legacy operations.
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had any further activity in the last 2 weeks. Thank you for your contributions! |
2 participants
Test 6
Summary by CodeRabbit
New Features
Improvements
Tests