Backport PR #2832 to release/v1.7 for Add metadata to target host for benchmark job #2870
Conversation
* ✨ update chart Signed-off-by: vankichi <kyukawa315@gmail.com> * ♻️ Bind metadata Signed-off-by: vankichi <kyukawa315@gmail.com> * ♻️ update charts Signed-off-by: vankichi <kyukawa315@gmail.com> --------- Signed-off-by: vankichi <kyukawa315@gmail.com> Co-authored-by: Yusuke Kato <kpango@vdaas.org>
Deploying vald with
|
| Latest commit: |
570875e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0dee9737.vald.pages.dev |
| Branch Preview URL: | https://backport-release-v1-7-impl-b.vald.pages.dev |
📝 WalkthroughWalkthroughThe changes add a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JobService
participant GRPC
Client->>JobService: Submit job configuration (includes target.meta)
JobService->>JobService: Initialize job with metadata option (WithMetadata)
JobService->>GRPC: Create metadata using NewMetadata(map)
JobService->>JobService: Wrap original context with metadata (NewOutgoingContext)
JobService->>Client: Execute job using updated context
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[config_reader] The configuration option ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
charts/vald-benchmark-operator/crds/valdbenchmarkscenario.yaml (1)
108-110: Metadata Schema Addition in CRD:
The addition of themetafield under thetargetsection—withtype: objectandx-kubernetes-preserve-unknown-fields: true—is implemented correctly. This provides the flexibility to include additional metadata without enforcing a strict structure.Consider adding a brief descriptive comment that explains the intended usage of the
metafield to aid future maintainers.charts/vald-benchmark-operator/schemas/scenario-values.yaml (1)
26-30: Metadata Schema Definition in JSON Schema:
The update adds ametaobject with fixed subfieldskeyandvalue, which clearly documents the metadata structure for the target host.Consider whether a more flexible map structure (allowing arbitrary key-value pairs) might be beneficial in the long term, should additional metadata fields be needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml(1 hunks)charts/vald-benchmark-operator/crds/valdbenchmarkscenario.yaml(1 hunks)charts/vald-benchmark-operator/schemas/job-values.schema.json(1 hunks)charts/vald-benchmark-operator/schemas/job-values.yaml(1 hunks)charts/vald-benchmark-operator/schemas/scenario-values.schema.json(1 hunks)charts/vald-benchmark-operator/schemas/scenario-values.yaml(1 hunks)charts/vald-benchmark-operator/values/benchmark-job.yaml(1 hunks)charts/vald-benchmark-operator/values/benchmark-scenario.yaml(1 hunks)example/helm/benchmark/job-values.yaml(1 hunks)example/helm/benchmark/scenario-values.yaml(1 hunks)internal/config/benchmark.go(1 hunks)internal/net/grpc/metadata.go(1 hunks)k8s/tools/benchmark/operator/crds/valdbenchmarkjob.yaml(1 hunks)k8s/tools/benchmark/operator/crds/valdbenchmarkscenario.yaml(1 hunks)pkg/tools/benchmark/job/service/job.go(3 hunks)pkg/tools/benchmark/job/service/option.go(2 hunks)pkg/tools/benchmark/job/usecase/benchmarkd.go(1 hunks)
🔇 Additional comments (15)
internal/net/grpc/metadata.go (1)
28-30: LGTM!The
NewMetadatafunction is a clean and focused wrapper aroundmetadata.New. It follows the package's naming conventions and is well-placed in the grpc package.pkg/tools/benchmark/job/service/option.go (1)
260-268: LGTM!The
WithMetadatafunction is well-documented and follows the established option pattern. It includes proper validation by checking for non-empty map before setting metadata.pkg/tools/benchmark/job/usecase/benchmarkd.go (1)
128-128: LGTM!The addition of metadata configuration to job service initialization is clean and aligns with the PR objectives.
pkg/tools/benchmark/job/service/job.go (2)
84-88: LGTM! Clean addition of metadata field.The
metafield is appropriately typed asgrpc.MDfor gRPC metadata handling.
290-295: LGTM! Proper metadata propagation implementation.The implementation correctly:
- Only creates new context when metadata exists
- Uses debug logging for observability
- Properly propagates metadata via gRPC context
charts/vald-benchmark-operator/values/benchmark-job.yaml (1)
46-47: LGTM! Clear example of metadata configuration.The metadata configuration provides a clear example with a token, which is helpful for users implementing authentication.
charts/vald-benchmark-operator/schemas/scenario-values.schema.json (1)
59-59: LGTM! Well-defined schema for metadata.The metadata schema is:
- Correctly typed as an object
- Well-documented with a clear description
- Optional, maintaining backward compatibility
example/helm/benchmark/job-values.yaml (1)
57-58: LGTM! Consistent example configuration.The metadata example aligns with the schema and other configuration files, providing a clear template for users.
k8s/tools/benchmark/operator/crds/valdbenchmarkscenario.yaml (1)
108-110: Consistent Metadata Schema in Internal CRD:
The newmetaproperty is added in a consistent manner within the internal CRD definition. The use ofx-kubernetes-preserve-unknown-fields: trueensures that any extra key-value metadata is preserved correctly.charts/vald-benchmark-operator/values/benchmark-scenario.yaml (1)
179-180: Metadata Token in Scenario Values:
The newmetafield with thetokenkey is correctly integrated into thetargetsection. This sample token ("sample-token") aligns with the intended use of metadata for target configuration in benchmark scenarios.Please confirm that the sample value meets the requirements for your environment, and update it if necessary.
example/helm/benchmark/scenario-values.yaml (1)
199-200: Metadata Token Addition in Helm Values:
The addition of themetafield containing thetokenkey is correctly applied, ensuring consistency with similar changes in other configuration files.charts/vald-benchmark-operator/crds/valdbenchmarkjob.yaml (1)
793-795: Newmetaproperty added in CRD.
The newmetafield under thetargetproperties is correctly defined as an object and includes the annotationx-kubernetes-preserve-unknown-fields: true. This allows users to pass arbitrary metadata without schema validation issues. Ensure downstream tools and documentation are updated to reflect this new capability.k8s/tools/benchmark/operator/crds/valdbenchmarkjob.yaml (1)
793-795: Consistent addition of themetafield in the operator CRD.
The newmetafield, together with its preservation annotation, is properly integrated in thetargetsection. This change mirrors similar modifications in other CRDs, ensuring consistency across configurations.charts/vald-benchmark-operator/schemas/job-values.schema.json (1)
1199-1199: Addedmetaproperty to JSON schema.
The new"meta"property is defined as an object with a clear description ("metadata for target host"). This extension increases schema flexibility without affecting existing properties.charts/vald-benchmark-operator/schemas/job-values.yaml (1)
26-30: Newmetaobject added in target configuration.
The diff introduces ametasection withkeyandvaluefields under thetargetobject. This structured approach to including metadata for the target host is clear and consistent. Consider documenting the accepted values and usage scenarios for better end-user guidance.
|
[CHATOPS:HELP] ChatOps commands.
|
|
[WARNING:INTCFG] Changes in |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/v1.7 #2870 +/- ##
===============================================
Coverage ? 16.92%
===============================================
Files ? 136
Lines ? 15057
Branches ? 0
===============================================
Hits ? 2548
Misses ? 12210
Partials ? 299 ☔ View full report in Codecov by Sentry. |
vankichi
deleted the
backport/release/v1.7/impl/benchmark/add-metadata-to-target-host
branch
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit