HYPERFLEET-474 - Spike: Evaluate Maestro REST vs gRPC API for HyperFleet integration #77

xueli181114 · 2026-01-15T14:01:34Z

Summary by CodeRabbit

Documentation
- Added a comprehensive Maestro integration docs suite: architecture deep dive, integration strategy, and job-based CLI implementation guide covering ManifestWork granularity patterns, multi-cluster naming/isolation, hybrid gRPC+HTTP status polling, full CLI workflows (apply/get/list/describe/delete/validate/wait/watch/diff/build), status/report formats and workflow, deployment and security guidance (TLS/mTLS/JWT), race-condition analysis, monitoring/troubleshooting, migration path and next steps.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2026-01-15T14:01:47Z

Walkthrough

Adds three new HyperFleet Maestro documents: an integration strategy prescribing a job-based maestro-cli approach (gRPC broker for resource operations + HTTP polling for status), ManifestWork-per-adapter-per-cluster naming and three granularity patterns (A/B/C), race-condition analyses, security and deployment requirements, and a roadmap; an architecture deep dive describing Maestro Server/Agent, PostgreSQL, brokers, deployment modes, communication protocols, resource/ManifestWork model, observability and troubleshooting; and a maestro-cli implementation guide specifying commands (apply/get/list/describe/delete/validate/wait/watch/diff/build), ManifestWork-centric workflows, status/result formats, build/merge strategies for nodepools, error-handling, scaling considerations, and migration guidance.

Sequence Diagram(s)

sequenceDiagram
    participant CLI as "maestro-cli (Job)"
    participant Maestro as "Maestro Server / Controller"
    participant Broker as "Broker (gRPC)"
    participant Cluster as "Managed Cluster (ManifestWork)"
    participant Status as "Status Store (HTTP RESULTS_PATH)"

    CLI->>Maestro: submit job (apply/delete/update)
    activate Maestro
    Maestro->>Broker: enqueue resource operation (gRPC)
    Broker->>Cluster: deliver ManifestWork
    Cluster-->>Maestro: ManifestWork status updates (k8s status / HTTP)
    Maestro->>Status: write StatusResult to RESULTS_PATH (HTTP)
    deactivate Maestro
    CLI->>Status: poll/read StatusResult (wait/watch)
    Status-->>CLI: return status/result

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

add HyperShift control plane adapter spike doc #43 — overlaps with Maestro-driven execution and job-based status-report workflow proposals.
update the adapter-config-template according to latest implementation #45 — related changes to adapter status reporting contract and HTTP status payload/endpoint.

Suggested reviewers

rh-amarin

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title indicates a spike to evaluate REST vs gRPC APIs, but the actual changeset adds comprehensive integration strategy documentation recommending a job-based maestro-cli approach with gRPC broker and HTTP polling, which represents a concrete design decision beyond mere evaluation.	Clarify whether the PR is documenting spike findings/recommendations or just evaluation alternatives. Consider updating the title to reflect the actual outcome (e.g., 'Spike: Design Maestro integration strategy with gRPC broker and HTTP polling') if a decision has been reached.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

🧹 Recent nitpick comments

hyperfleet/components/adapter/maestro-cli/maestro-architecture.md (1)
39-48: Add language specifiers to fenced code blocks for better rendering.

The deployment mode diagrams are missing language specifiers. While these are text-based diagrams, adding a language specifier (e.g., text or plaintext) improves markdown rendering and satisfies linting rules.
📝 Proposed fix
-```
+```text
 Components:
   - Maestro Server (with integrated gRPC broker)
Apply similar changes to the MQTT mode (lines 58-67), GCP Pub/Sub mode (lines 76-85), and AWS IoT mode (lines 95-104) diagrams.
Also applies to: 58-67, 76-85, 95-104
hyperfleet/components/adapter/maestro-cli/maestro-cli-implementation.md (1)
103-108: Add language specifiers to fenced code blocks.

The ManifestWork mapping diagram (lines 103-108) and status flow diagram (lines 691-693) are missing language specifiers, which improves rendering consistency.
📝 Proposed fix
-```
+```text
 Each ManifestWork = One Resource-Bundle
Apply similar change to the status flow diagram at line 691.
Also applies to: 691-693
hyperfleet/components/adapter/maestro-cli/maestro-adapter-integration-strategy.md (1)

547-547: Optional: Consider style simplification for redundant phrases.

LanguageTool suggests simplifying "completely eliminate" to "eliminate" in lines 547 and 560. However, the emphasis on "completely" effectively communicates that race conditions are entirely avoided (not merely reduced), which is a key advantage of Pattern B. The current phrasing is acceptable given the technical context.

✏️ Alternative phrasing

If you prefer more concise language:

Line 547: "to eliminate race conditions"

Line 560: "Eliminates race conditions"

Also applies to: 560-560

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 061b84b and 6fdd7f0.

📒 Files selected for processing (3)

hyperfleet/components/adapter/maestro-cli/maestro-adapter-integration-strategy.md
hyperfleet/components/adapter/maestro-cli/maestro-architecture.md
hyperfleet/components/adapter/maestro-cli/maestro-cli-implementation.md

🧰 Additional context used

🪛 LanguageTool

hyperfleet/components/adapter/maestro-cli/maestro-adapter-integration-strategy.md

[style] ~547-~547: This phrase is redundant. Consider writing “eliminate”.
Context: ...er Resource Per Cluster(pattern B)** to completely eliminate race conditions: ```bash # Each resour...

(COMPLETELY_ANNIHILATE)

[style] ~560-~560: This phrase is redundant. Consider writing “eliminates”.
Context: ...depool-storage ``` Benefits: - ✅ Completely eliminates race conditions - no shared state any...

(COMPLETELY_ANNIHILATE)

[uncategorized] ~651-~651: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...Operation Frequency** | Low to medium | High frequency operations | | Latency Requirements...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

🪛 markdownlint-cli2 (0.18.1)

hyperfleet/components/adapter/maestro-cli/maestro-cli-implementation.md

103-103: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

691-691: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

hyperfleet/components/adapter/maestro-cli/maestro-architecture.md

39-39: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

58-58: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

76-76: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

95-95: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🔇 Additional comments (7)

hyperfleet/components/adapter/maestro-cli/maestro-architecture.md (1)

1-318: Strong architecture documentation with comprehensive coverage.

This deep dive effectively covers Maestro's architecture, deployment options, security considerations, and operational guidance. The comparison of deployment modes (gRPC, MQTT, GCP Pub/Sub, AWS IoT) is particularly valuable, and the event consumption risk analysis helps readers understand the trade-offs between broker types.

hyperfleet/components/adapter/maestro-cli/maestro-cli-implementation.md (3)

88-385: Well-justified ManifestWork client approach over CloudEvents.

The decision to use Maestro's ManifestWork client instead of low-level CloudEvents is well-reasoned. The comparison table clearly shows the benefits: semantic naming, built-in apply logic, type safety, and reduced complexity. The implementation patterns with detailed code examples provide clear guidance for developers.

442-464: HTTP polling rationale is sound for job-based operations.

The justification for HTTP polling over gRPC subscription for status monitoring is appropriate for the job-based CLI approach. The trade-off analysis correctly identifies that job startup overhead dominates, making the polling latency acceptable. The decision simplifies implementation and avoids broker-specific setup complexity.

735-985: Build command design enables safe nodepool merging.

The build command implementation elegantly solves the fetch-merge-apply workflow for nodepool updates. The merge strategies (merge vs. replace) provide flexibility, and the integration with the apply command streamlines automation. The nodepool configuration format is well-structured for the intended use cases.

hyperfleet/components/adapter/maestro-cli/maestro-adapter-integration-strategy.md (3)

79-111: Excellent race condition analysis justifies pattern choice.

The detailed explanation of why one-ManifestWork-per-cluster was rejected is particularly valuable. The timeline scenario clearly demonstrates how resource loss can occur even when operations "succeed," and the root cause analysis (complete manifests required, no coordination between adapters) is spot-on. This justification strongly supports the chosen approach.

572-638: Clear pattern comparison guides implementation decisions.

The comparison of Patterns A, B, and C provides actionable guidance for choosing the appropriate ManifestWork granularity. Pattern A (per-adapter-per-cluster) balances simplicity and safety for most use cases, while Pattern B (per-resource-per-cluster) correctly identifies scenarios requiring maximum isolation (e.g., parallel nodepool operations). The trade-offs are honestly presented.

641-697: Thoughtful framework integration roadmap.

The future adapter framework integration section provides a realistic migration path without over-committing to premature optimization. The decision criteria (job startup overhead > 50% of operation time, frequency > 10 ops/minute) give concrete thresholds for when framework integration becomes worthwhile. The acknowledgment that ManifestWork patterns may evolve with framework integration shows appropriate flexibility.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@hyperfleet/components/adapter/maestro-cli/maestro-cli-implementation.md`:
- Around line 1208-1219: Change manifestKey to return (string, error) and
surface JSON unmarshal failures instead of returning an empty string: when
unstructured.Unstructured.UnmarshalJSON fails return a wrapped error; on success
return the formatted key via fmt.Sprintf and nil error. Update all callers that
use manifestKey(...) to handle the returned error (e.g., key, err :=
manifestKey(manifest); if err != nil { return fmt.Errorf("failed to generate
manifest key: %w", err) }) so failures are propagated instead of being silently
ignored.
- Around line 1153-1180: mergeManifestWork currently dereferences out-of-scope
flags.Name and flags.Consumer when existingWork is nil; update the function to
either accept explicit name and namespace parameters (e.g., add name, namespace
args to mergeManifestWork) or return an error when existingWork is nil, then
create the new work using those provided name/namespace values instead of flags.
Also validate the strategy when existingWork is nil (ensure only supported
strategies are allowed) and keep calling mergeStrategy or replaceStrategy on a
non-nil mergedWork; remove any references to flags.Name/flags.Consumer and use
the new parameters or explicit error paths.

🧹 Nitpick comments (9)

hyperfleet/components/adapter/maestro-cli/maestro-adapter-integration-strategy.md (4)

9-9: Format bare URL as a proper markdown link.

The bare URL should be formatted as a markdown link for better readability and to comply with markdown best practices.

📝 Proposed fix

-| **Repository** | https://github.com/openshift-online/maestro |
+| **Repository** | [openshift-online/maestro](https://github.com/openshift-online/maestro) |

431-431: Format bare URL as a proper markdown link.

The bare URL should be formatted as a markdown link for better readability.

📝 Proposed fix

-See: https://github.com/openshift-hyperfleet/status-reporter
+See: [HyperFleet status-reporter](https://github.com/openshift-hyperfleet/status-reporter)

481-481: Add language specifiers to fenced code blocks.

Code blocks should specify the language type for proper syntax highlighting and better readability.

📝 Proposed fixes

For line 481:

-```
+```text
 maestro-cli (writes RESULTS_PATH) → status-reporter (reads RESULTS_PATH) → HyperFleet status updates


For line 502:
```diff
-```
+```text
 Each Adapter → K8s Job (maestro-cli apply) → Maestro → Creates ManifestWork with semantic name → Stored in Adapter's status data

</details>


Also applies to: 502-502

---

`619-673`: **Clarify the recommended pattern selection criteria.**

The document presents Pattern A and Pattern B but the recommendation logic could be clearer. Lines 654-658 say "Use Pattern A" when each adapter manages single resources, but lines 659-668 warn that Pattern A "still requires fetching" for multiple resources, which seems contradictory.



Consider restructuring the recommendation as:

```markdown
### Recommended Approach

**Default: Pattern A (One ManifestWork Per Adapter Per Cluster)**
- ✅ Use when each adapter type manages a single logical component
- ✅ Simpler tracking and ownership model
- ⚠️  Requires fetch-merge-apply for adapters managing multiple resources (e.g., multiple nodepools)

**When to use Pattern B (One ManifestWork Per Resource Per Cluster)**
- ✅ Adapter manages multiple independent resources (e.g., 5+ nodepools)
- ✅ High-frequency parallel operations on resources
- ✅ Independent lifecycle for each resource is critical
- ⚠️  Accept higher operational overhead of tracking many ManifestWorks

This makes the decision criteria more actionable.

hyperfleet/components/adapter/maestro-cli/maestro-architecture.md (3)

39-39: Add language specifiers to deployment mode diagrams.

The deployment mode diagrams in code blocks should have a language specifier for better rendering and clarity.
📝 Proposed fix
-```
+```text
 Components:
   - Maestro Server (with integrated gRPC broker)
Apply similar changes to lines 57, 75, and 94.
</details>


Also applies to: 57-57, 75-75, 94-94

---

`165-189`: **Clarify the API endpoint support matrix.**

The API endpoint support matrix at lines 170-174 shows that `/api/maestro/v1/resource-bundles` supports POST: ❌, but the "HTTP API Use Cases" section (lines 184-188) doesn't clarify what operations can be performed via HTTP on resource-bundles.



Consider adding a note in the matrix:

```markdown
| API Endpoint | GET | POST | PATCH | DELETE | Purpose |
|--------------|-----|------|-------|--------|---------|
| `/api/maestro/v1/consumers` | ✅ | ✅ | ❌ | ✅ | Consumer metadata management |
| `/api/maestro/v1/resource-bundles` | ✅ Read-only | ❌ Use gRPC | ❌ Use gRPC | ✅ | ManifestWork status/monitoring |

**Note**: ManifestWork creation and updates require gRPC CloudEvents. HTTP DELETE is supported for cleanup operations only.
This makes the API capabilities more explicit.

263-267: Add guidance for log level recommendations.

The logging strategy section mentions KLOG_V but doesn't provide recommendations for appropriate verbosity levels for different scenarios.

Consider adding:
# Configurable log levels
- name: KLOG_V
  value: "2"  # Production: 0-2, Debug: 3-5, Trace: 6+
And document:

Level 0-1: Errors and warnings only (production)

Level 2: Important info messages (default production)

Level 3-4: Debug information (troubleshooting)

Level 5+: Verbose trace (development only)

hyperfleet/components/adapter/maestro-cli/maestro-cli-implementation.md (2)

103-103: Add language specifiers to code blocks.

Code blocks should have language specifiers for proper rendering.

📝 Proposed fixes

Line 103:

-```
+```text
 Each ManifestWork = One Resource-Bundle


Line 115:
```diff
-```
+```text
 cluster-west-1 (consumer):


Line 917:
```diff
-```
+```text
 maestro-cli (writes RESULTS_PATH) → status-reporter (reads RESULTS_PATH) → HyperFleet status updates

</details>


Also applies to: 115-115, 917-917

---

`1182-1206`: **Consider idempotency guarantees in mergeStrategy.**

The `mergeStrategy` function replaces resources with the same kind/name/namespace but doesn't validate that the merge is idempotent. If called multiple times with the same input, it should produce the same result.



Consider adding:
1. **Deterministic ordering** of manifests after merge
2. **Validation** that merged manifests don't contain duplicates
3. **Documentation** of merge behavior when manifests only differ in spec

```go
func mergeStrategy(work *workv1.ManifestWork, newManifests []workv1.Manifest) (*workv1.ManifestWork, error) {
    existingManifests := work.Spec.Workload.Manifests

    // Build index of existing resources by kind/name/namespace
    existingIndex := make(map[string]int)
    for i, manifest := range existingManifests {
        key := manifestKey(manifest)
        if key == "" {
            return nil, fmt.Errorf("invalid manifest at index %d: cannot extract key", i)
        }
        existingIndex[key] = i
    }

    // Merge new manifests
    for _, newManifest := range newManifests {
        key := manifestKey(newManifest)
        if key == "" {
            return nil, fmt.Errorf("invalid manifest in new manifests: cannot extract key")
        }
        if existingIdx, exists := existingIndex[key]; exists {
            // Replace existing resource
            existingManifests[existingIdx] = newManifest
        } else {
            // Add new resource
            existingManifests = append(existingManifests, newManifest)
        }
    }

    work.Spec.Workload.Manifests = existingManifests
    return work, nil
}

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 52c6a4f and d84b354.

📒 Files selected for processing (3)

hyperfleet/components/adapter/maestro-cli/maestro-adapter-integration-strategy.md
hyperfleet/components/adapter/maestro-cli/maestro-architecture.md
hyperfleet/components/adapter/maestro-cli/maestro-cli-implementation.md

🧰 Additional context used

🪛 LanguageTool

hyperfleet/components/adapter/maestro-cli/maestro-adapter-integration-strategy.md

[style] ~594-~594: This phrase is redundant. Consider writing “eliminate”.
Context: ...ifestWork Per Resource Per Cluster** to completely eliminate race conditions: ```bash # Each resour...

(COMPLETELY_ANNIHILATE)

[style] ~607-~607: This phrase is redundant. Consider writing “eliminates”.
Context: ...age-cluster123 ``` Benefits: - ✅ Completely eliminates race conditions - no shared state any...

(COMPLETELY_ANNIHILATE)

🪛 markdownlint-cli2 (0.18.1)

hyperfleet/components/adapter/maestro-cli/maestro-adapter-integration-strategy.md

9-9: Bare URL used

(MD034, no-bare-urls)

431-431: Bare URL used

(MD034, no-bare-urls)

481-481: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

502-502: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

hyperfleet/components/adapter/maestro-cli/maestro-cli-implementation.md

103-103: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

115-115: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

917-917: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

hyperfleet/components/adapter/maestro-cli/maestro-architecture.md

39-39: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

57-57: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

75-75: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

94-94: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🔇 Additional comments (6)

hyperfleet/components/adapter/maestro-cli/maestro-adapter-integration-strategy.md (3)

10-10: Consider documenting image version maintenance strategy.

The container image reference uses a specific SHA digest that may become outdated. Consider adding a note about version maintenance or using a tagged version reference.

Do you have a process for keeping this SHA updated, or would it be better to reference a stable tag (e.g., latest or version tag) with a note about verifying the digest?

153-170: Verify CronJob schedule syntax and resource requests.

The CronJob example uses "*/2 * * * *" (every 2 minutes) which may be too frequent for production scenarios. Additionally, the image hyperfleet/maestro-sync:latest appears to be a placeholder.

Consider documenting:

Recommended sync intervals for different environments (dev/staging/prod)

Whether the hyperfleet/maestro-sync image exists or is planned

Appropriate resource requests/limits for the sync controller

466-477: Verify the priority logic for RESULTS_PATH resolution.

The getResultsPath function checks flags.ResultsPath first, then RESULTS_PATH env var, then defaults to "/shared/results.json". However, line 394 shows --results-path flag being passed which would override the env var from line 399. Ensure the documented priority order matches the actual implementation.

Confirm that:

The flag priority (flag > env > default) is correctly implemented

The default path /shared/results.json is appropriate for all execution contexts

The emptyDir volume mount at line 417 supports the default path
hyperfleet/components/adapter/maestro-cli/maestro-cli-implementation.md (2)
214-235: Verify error handling for version conflicts in applyManifestWork.

The applyManifestWork function uses Patch to update existing ManifestWork but doesn't handle version conflicts explicitly. Given the race condition concerns discussed in the integration strategy doc, this should handle optimistic locking failures.

Consider adding retry logic for version conflicts:
func applyManifestWork(ctx context.Context, workClient workv1client.WorkV1Interface, consumer string, manifestwork *workv1.ManifestWork) error {
    // Try to get existing work
    existingWork, err := workClient.ManifestWorks(consumer).Get(ctx, manifestwork.Name, metav1.GetOptions{})
    if apierrors.IsNotFound(err) {
        _, err := workClient.ManifestWorks(consumer).Create(ctx, manifestwork, metav1.CreateOptions{})
        return err
    }

    if err != nil {
        return fmt.Errorf("failed to get existing work: %w", err)
    }

    // Update ResourceVersion to ensure optimistic locking
    manifestwork.ResourceVersion = existingWork.ResourceVersion

    // Work exists, update with merge patch
    patchData, err := grpcsource.ToWorkPatch(existingWork, manifestwork)
    if err != nil {
        return fmt.Errorf("failed to create patch: %w", err)
    }

    _, err = workClient.ManifestWorks(consumer).Patch(ctx, manifestwork.Name, types.MergePatchType, patchData, metav1.PatchOptions{})
    if apierrors.IsConflict(err) {
        return fmt.Errorf("version conflict - resource modified: %w", err)
    }
    return err
}
Should the CLI handle retries for conflicts, or is that the adapter's responsibility?

1243-1276: Excellent documentation of performance trade-offs.

The performance considerations section clearly articulates the design rationale for prioritizing simplicity over optimization in the job-based CLI approach. The comparison table between maestro-cli and future adapter framework is particularly helpful for understanding when these trade-offs matter.

This kind of explicit documentation of trade-off decisions is valuable for future maintainers and helps prevent premature optimization.
hyperfleet/components/adapter/maestro-cli/maestro-architecture.md (1)

196-197: The claim and explanation are accurate. Maestro correctly uses Open Cluster Management APIs (specifically the ManifestWork resource definition) without requiring the ACM operator, because it implements its own control plane with custom CloudEvents transport and PostgreSQL storage. The existing documentation already adequately explains this relationship. No changes needed.