-
Notifications
You must be signed in to change notification settings - Fork 244
api: support hot reload of registry authentication #1864
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: master
Are you sure you want to change the base?
Conversation
009e6db to
1c76116
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1864 +/- ##
==========================================
+ Coverage 56.53% 56.72% +0.18%
==========================================
Files 199 200 +1
Lines 52094 52438 +344
Branches 44897 45241 +344
==========================================
+ Hits 29453 29745 +292
- Misses 21156 21208 +52
Partials 1485 1485
🚀 New features to boost your workflow:
|
466d21f to
319eba9
Compare
Add HTTP API endpoints to update and query daemon configuration at runtime without restarting nydusd. This is particularly useful for refreshing registry credentials when tokens expire. Key changes: - Add GET/PUT /api/v1/config endpoints for configuration management - Add config module in utils for centralized configuration updates - Add smoke test cases for hot reload configuration API - Update documentation with hot reload usage examples Signed-off-by: imeoer <[email protected]>
319eba9 to
25bb400
Compare
Fricounet
left a comment
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.
LGTM in terms of doing what we need for the mentioned issue. I'm not a big rust expert though so take my review with a pinch of salt.
Nice documentation though!
Zephyrcf
left a comment
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.
LGTM!
| impl Registry { | ||
| #[allow(clippy::useless_let_if_seq)] | ||
| pub fn new(config: &RegistryConfig, id: Option<&str>) -> Result<Registry> { | ||
| let id = id.ok_or_else(|| einval!("Registry backend requires blob_id"))?; |
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.
It looks like the id parameter now represents a mountpoint ID rather than a blob ID.
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.
Thanks, let me check this, the blob ID seems to be useless, it should be used as a mountpoint.
| let registry = Registry { | ||
| connection, | ||
| state, | ||
| metrics: BackendMetrics::new(id, "registry"), |
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.
Because this id is no longer blobdid, the corresponding previous read and write statistics for blobid level no longer exist, this behavior change is very large, I think we need to consider adding a mountpoint id as a parameter, instead of directly changing the original blobid to mountpointid.
Overview
Add HTTP API endpoints to update and query daemon configuration at
runtime without restarting nydusd. This is particularly useful for
refreshing registry credentials when tokens expire.
Key changes:
Related Issues
containerd/nydus-snapshotter#690
Change Details
Please describe your changes in detail:
Test Results
If you have any relevant screenshots or videos that can help illustrate your changes, please add them here.
Change Type
Please select the type of change your pull request relates to:
Self-Checklist
Before submitting a pull request, please ensure you have completed the following: