SPIKE: Maestro integration to adapter framework #82
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip You can disable sequence diagrams in the walkthrough.Disable the WalkthroughThis pull request reorganizes the HyperFleet Adapter Framework configuration structure by replacing two legacy template files (adapter-config-template-MVP.yaml and adapter-environment-config-template.yaml) with two new templates under a configs/ subdirectory: adapter-business-logic-template-MVP.yaml and adapter-deployment-config-template.yaml. The new deployment template introduces a Maestro transport configuration for remote Kubernetes resource management. Additionally, two documentation files are added: a spike document detailing the Maestro integration architecture and a document describing Maestro event flow patterns. A new AdapterConfig CRD is introduced with fields for Maestro transport, authentication, API configuration, and Kubernetes settings. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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
🤖 Fix all issues with AI agents
In
`@hyperfleet/components/adapter/framework/configs/adapter-deployment-config-template.yaml`:
- Around line 38-50: The maestro configuration block is incorrectly nested under
spec.adapter (the symbol "maestro" appears inside "adapter"); move the "maestro"
section so it is a direct child of "spec" (i.e., use spec.maestro instead of
spec.adapter.maestro), ensuring any comments/indentation reflect the new
top-level placement and that any references to "maestro" in templates or docs
match spec.maestro.
🧹 Nitpick comments (7)
hyperfleet/components/adapter/maestro-integration/maestro-architecture.md (1)
114-162: Documentation content is comprehensive and well-structured.The event flow documentation provides valuable insights into Maestro's architecture. One minor improvement: the code evidence section (lines 158-161) references specific line numbers without specifying the source file path.
Consider adding the file path for clarity:
**Code Evidence:** -- Line 93: `PageList(ctx, m.logger, m.apiClient, ...)` - REST API for initial state -- Line 43: `cloudEventsClient.Subscribe(ctx, watcherStore.HandleReceivedResource)` - CloudEvents subscription -- Line 117: `HandleReceivedResource` processes incoming work updates +- `watcher.go` Line 93: `PageList(ctx, m.logger, m.apiClient, ...)` - REST API for initial state +- `watcher.go` Line 43: `cloudEventsClient.Subscribe(ctx, watcherStore.HandleReceivedResource)` - CloudEvents subscription +- `watcher.go` Line 117: `HandleReceivedResource` processes incoming work updateshyperfleet/components/adapter/framework/configs/adapter-business-logic-template-MVP.yaml (2)
86-91: Consider documenting the expected API response structure.The deeply nested field path
status.conditions.placement.data.clusterNameassumes a specific API response structure. If this path doesn't exist, the capture will silently fail or return null.Consider either:
- Adding a comment with the expected API response schema
- Using a default value mechanism similar to CEL's
.orValue()- name: "placementClusterName" field: "status.conditions.placement.data.clusterName" default: "" # Optional: provide default if path doesn't exist
233-246: Consider adding error handling guidance for postActions.The postActions section defines retry behavior but doesn't specify what happens on final failure after all retries are exhausted.
Consider documenting the expected behavior:
postActions: - name: "reportClusterStatus" apiCall: # ... existing config ... retryAttempts: 3 retryBackoff: "exponential" # Optional: Document or add failure handling # onFailure: "log" # Options: log, skip, failhyperfleet/components/adapter/framework/configs/adapter-deployment-config-template.yaml (2)
62-91: Clarify that TLS and token configs are mutually exclusive.Both
tlsConfigandtokenConfigare defined, but only one should be active based onauth.type. Consider adding comments or restructuring to make this clearer.♻️ Suggested clarification
# Authentication configuration auth: # Authentication type: "tls" (certificate-based) or "token" (bearer token) + # NOTE: Configure ONLY the section that matches your auth type type: "tls" - # TLS certificate configuration + # TLS certificate configuration (used when type: "tls") tlsConfig: # ... - # Alternative: Bearer token configuration (for token-based authentication) + # Bearer token configuration (used when type: "token") + # NOTE: Comment out or remove this section if using TLS authentication tokenConfig:
106-112: Consider increasinghyperfleetApi.timeoutvalue.A 2-second timeout for HTTP API calls may be too aggressive for production environments, especially during network congestion or when the API is under load. The business logic template uses
10sfor precondition API calls and30sfor postActions.♻️ Suggested change
hyperfleetApi: # HTTP client timeout for API calls - timeout: 2s + timeout: 10s # Number of retry attempts for failed API calls retryAttempts: 3hyperfleet/components/adapter/maestro-integration/SPIKE-maestro-adapter-integration.md (2)
34-46: Add language specifier to fenced code block.The ASCII diagram code block lacks a language specifier, which triggers markdown lint warning MD040.
♻️ Suggested fix
-``` +```text ┌─────────────────┐ ┌─────────────────┐ ┌──────────────────-┐ │ Sentinel │────▶│ HyperFleet │────▶│ Deployment Cluster│
488-493: Add language specifier to fenced code block.The event flow diagram code block lacks a language specifier.
♻️ Suggested fix
-``` +```text Maestro Events → Adapter (Event Processor) → HyperFleet Broker → Sentinel → HyperFleet API</details> </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| spec: | ||
| # Adapter Information | ||
| adapter: | ||
| # Adapter version | ||
| version: "0.2.0" | ||
|
|
||
| # ============================================================================ | ||
| # NEW: Transport Configuration | ||
| # ============================================================================ | ||
| # Defines how resources are managed (direct K8s API vs Maestro transport) | ||
|
|
||
| # Maestro-specific configuration this is optional and only used if the adapter uses Maestro transport | ||
| maestro: |
There was a problem hiding this comment.
Inconsistent structure placement for maestro configuration.
The maestro section is nested under spec.adapter (line 49-50), but the SPIKE document (SPIKE-maestro-adapter-integration.md lines 81-117) shows it at spec.maestro level.
♻️ Suggested structure to match SPIKE document
spec:
# Adapter Information
adapter:
# Adapter version
version: "0.2.0"
- # Maestro-specific configuration this is optional and only used if the adapter uses Maestro transport
- maestro:
+ # Maestro-specific configuration - optional, only used if adapter uses Maestro transport
+ maestro:
# Maestro gRPC server connection
serverAddress: "maestro-grpc.aro-hcp.example.com:8090"📝 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.
| spec: | |
| # Adapter Information | |
| adapter: | |
| # Adapter version | |
| version: "0.2.0" | |
| # ============================================================================ | |
| # NEW: Transport Configuration | |
| # ============================================================================ | |
| # Defines how resources are managed (direct K8s API vs Maestro transport) | |
| # Maestro-specific configuration this is optional and only used if the adapter uses Maestro transport | |
| maestro: | |
| spec: | |
| # Adapter Information | |
| adapter: | |
| # Adapter version | |
| version: "0.2.0" | |
| # ============================================================================ | |
| # NEW: Transport Configuration | |
| # ============================================================================ | |
| # Defines how resources are managed (direct K8s API vs Maestro transport) | |
| # Maestro-specific configuration - optional, only used if adapter uses Maestro transport | |
| maestro: |
🤖 Prompt for AI Agents
In
`@hyperfleet/components/adapter/framework/configs/adapter-deployment-config-template.yaml`
around lines 38 - 50, The maestro configuration block is incorrectly nested
under spec.adapter (the symbol "maestro" appears inside "adapter"); move the
"maestro" section so it is a direct child of "spec" (i.e., use spec.maestro
instead of spec.adapter.maestro), ensuring any comments/indentation reflect the
new top-level placement and that any references to "maestro" in templates or
docs match spec.maestro.
| # ========================================================================== | ||
| - name: "clusterNamespace" | ||
| transport: | ||
| type: "direct" |
There was a problem hiding this comment.
nitpick: is "direct" meaningful enough? should it be called kubernetes, or kubectl?
There was a problem hiding this comment.
I am a bit of struggle here, kubernetes may caused confusion about which kubernetes or kubernetes to which cluster? Let me ask AI to generate a better name
There was a problem hiding this comment.
AI gave me
My Recommendation
Use Direct.
It provides the cleanest contrast. When someone looks at your Job CR or Epic, seeing Direct vs Maestro immediately explains the networking and security posture of that specific task without needing further documentation.
There was a problem hiding this comment.
It's the second recommendation by AI. If no objections I think we can use it.
hyperfleet/components/adapter/framework/configs/adapter-deployment-config-template.yaml
Show resolved
Hide resolved
| # Maestro-specific configuration this is optional and only used if the adapter uses Maestro transport | ||
| maestro: | ||
| # Maestro gRPC server connection | ||
| serverAddress: "maestro-grpc.aro-hcp.example.com:8090" |
There was a problem hiding this comment.
Do we need also the HTTP address to query maestro?
In the other document we say:
Key Finding: Watch uses hybrid approach - HTTP REST API for initial state + CloudEvents for live updates!
There was a problem hiding this comment.
Good catch. We need!
hyperfleet/components/adapter/framework/configs/adapter-deployment-config-template.yaml
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/framework/configs/adapter-deployment-config-template.yaml
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/maestro-integration/SPIKE-maestro-adapter-integration.md
Show resolved
Hide resolved
|
|
||
| // ListOptions for both K8s and Maestro list operations | ||
| type ListOptions struct { | ||
| Namespace string // For Maestro: this is the consumer name (target cluster) |
There was a problem hiding this comment.
Is this what the DSL names as targetCluster ? should the property have that name?
There was a problem hiding this comment.
It is. It's named Namespace in maestro. I don't think it's a good idea to put Namespace in our config.
hyperfleet/components/adapter/maestro-integration/SPIKE-maestro-adapter-integration.md
Show resolved
Hide resolved
| - ✅ **Documentation**: Complete development and operational documentation | ||
|
|
||
|
|
||
| ## Alternative Approach: Ultra-High-Volume Watch Processing |
There was a problem hiding this comment.
Markdown makes it VERY difficult to know what this alternative is related to...
I had to go to code view and find what other titles are using level 2 ##
Could we mark the alternatives more clearly?
e.g. prefixing it with a) b) c)
or having them listed before going into each one
A table of contents may be also useful at the top to understand the structure of the document
hyperfleet/components/adapter/maestro-integration/SPIKE-maestro-adapter-integration.md
Outdated
Show resolved
Hide resolved
| - name: "clusterName" | ||
| field: "name" | ||
| - name: "clusterPhase" | ||
| field: "status.phase" |
There was a problem hiding this comment.
Would it be better to use the latest structure based on Angel’s recent changes to cluster status to avoid user's confusion? And also the below conditions.
| # ========================================================================== | ||
| - name: "clusterNamespace" | ||
| transport: | ||
| type: "direct" |
| hyperfleet.io/managed-by: "{{ .metadata.name }}" | ||
| hyperfleet.io/resource-type: "namespace" | ||
| annotations: | ||
| hyperfleet.io/created-by: "hyperfleet-adapter" |
There was a problem hiding this comment.
Would it be better to have this added uniformly by the adapter framework? If it’s not needed, can it be removed?
| name: "{{ .clusterId | lower }}" | ||
| labels: | ||
| hyperfleet.io/cluster-id: "{{ .clusterId }}" | ||
| hyperfleet.io/managed-by: "{{ .metadata.name }}" |
There was a problem hiding this comment.
Does {{ .metadata.name }} represent the Namespace name, or the adapter’s name (e.g., validation-adapter)?
| annotations: | ||
| hyperfleet.io/created-by: "hyperfleet-adapter" | ||
| hyperfleet.io/generation: "{{ .generationId }}" | ||
| discovery: |
There was a problem hiding this comment.
Maestro supports submitting and configuring multiple Kubernetes resources in a single request. If we continue using it, does that mean we need to split the Kubernetes resources and submit them to Maestro in multiple requests?
About the resource discovery, any ideas or patterns we can take from Maestro?
| # | ||
| # TRANSPORT MODES: | ||
| # ================ | ||
| # - direct: Traditional direct Kubernetes API access (existing behavior) |
There was a problem hiding this comment.
Is it better to remove (existing behavior)?
| # Client private key file path | ||
| keyFile: "/etc/maestro/certs/hyperfleet-client.key" # Can be overridden by environment variable MAESTRO_KEY_FILE | ||
| # Server name for TLS verification | ||
| serverName: "maestro-grpc.aro-hcp.example.com" # Can be overridden by environment variable MAESTRO_SERVER_NAME |
There was a problem hiding this comment.
What's the difference or relationship between serverName and above serverAddress?
| - **Capacity**: 10,000+ events per second | ||
| - **Architecture**: Direct broker subscription with event transformation | ||
|
|
||
| ### Connection Management |
There was a problem hiding this comment.
Is this section a little duplicated with Key Finding? They all outlines the details for gRPC Mode and MQTT/Pub-Sub Mode.
xueli181114
force-pushed
the
HYPERFLEET-530
branch
from
8585dd9 to
f1f6d3a
Compare
3 participants
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.