Skip to content

Conversation

dmartinol
Copy link
Collaborator

Fix #1916

Note: The issue became even more relevant with the introduction of the Registry API, because a single phase field was trying con condense the reconciliation state of both the sync and API deployment processes. By dividing the phase in 2 distinct fields we can ensure consistent execution of the needed steps and skip those already completed.

  • Added separate SyncStatus and APIStatus with dedicated phase enums
  • Implemented phase derivation logic
  • Update status collector to handle nested status fields
  • Fix infinite reconciliation loops by making sync manager status-aware
  • Add conditional status updates to prevent unnecessary Kubernetes API calls (e.g. when the K8s Service spec does not change)
  • Update printer columns to show separate sync and API phases

🤖 Generated with Claude Code

dmartinol and others added 2 commits September 22, 2025 15:58
- Add SyncStatus and APIStatus structs with dedicated phase enums
- Implement phase derivation logic treating SyncPhaseIdle as valid for Ready state
- Update status collector to handle nested status fields with batched updates
- Fix infinite reconciliation loops by making sync manager status-aware
- Add conditional status updates to prevent unnecessary Kubernetes API calls
- Include comprehensive test suite covering all phase derivation scenarios
- Maintain backward compatibility with deprecated top-level sync fields
- Update printer columns to show separate sync and API phases

🤖 Generated with [Claude Code](https://claude.ai/code)

Signed-off-by: Daniele Martinoli <[email protected]>
Co-authored-by: Claude <[email protected]>
…er to use new SyncStatus structure

- Removed deprecated fields: LastSyncTime, LastSyncHash, and ServerCount from MCPRegistryStatus for cleaner API.
- Updated the sync manager to return a new Result struct containing sync details.
- Adjusted status collector and controller logic to utilize the new SyncStatus fields.
- Ensured backward compatibility by removing deprecated fields from CRD definitions.

This change enhances the clarity of the MCPRegistry status management and aligns with the recent restructuring of status fields.

Signed-off-by: Daniele Martinoli <[email protected]>
Copy link

codecov bot commented Sep 22, 2025

Codecov Report

❌ Patch coverage is 32.78237% with 244 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.27%. Comparing base (040f5cf) to head (a69eef5).

Files with missing lines Patch % Lines
...thv-operator/controllers/mcpregistry_controller.go 0.00% 144 Missing ⚠️
...thv-operator/api/v1alpha1/zz_generated.deepcopy.go 0.00% 39 Missing ⚠️
...md/thv-operator/pkg/mcpregistrystatus/collector.go 63.82% 17 Missing ⚠️
...ator/pkg/mcpregistrystatus/mocks/mock_collector.go 0.00% 16 Missing ⚠️
cmd/thv-operator/pkg/registryapi/manager.go 64.28% 9 Missing and 1 partial ⚠️
cmd/thv-operator/pkg/registryapi/service.go 0.00% 10 Missing ⚠️
cmd/thv-operator/pkg/sync/manager.go 81.08% 5 Missing and 2 partials ⚠️
cmd/thv-operator/api/v1alpha1/mcpregistry_types.go 96.42% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1996      +/-   ##
==========================================
- Coverage   47.42%   47.27%   -0.15%     
==========================================
  Files         232      232              
  Lines       28644    28915     +271     
==========================================
+ Hits        13583    13671      +88     
- Misses      14039    14226     +187     
+ Partials     1022     1018       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Removed commented-out success reason constants from the sync manager for clarity.
- Cleaned up deployment.yaml by removing unnecessary conditional blocks.

These changes streamline the codebase and improve readability.

Signed-off-by: Daniele Martinoli <[email protected]>
…he file for consistency.

Signed-off-by: Daniele Martinoli <[email protected]>
…he file for consistency.

Signed-off-by: Daniele Martinoli <[email protected]>
…at the end of the file for consistency.

Signed-off-by: Daniele Martinoli <[email protected]>
…and removing an unnecessary newline at the end of the file.

Signed-off-by: Daniele Martinoli <[email protected]>
@dmartinol dmartinol marked this pull request as ready for review September 22, 2025 15:06
Signed-off-by: Daniele Martinoli <[email protected]>
Signed-off-by: Daniele Martinoli <[email protected]>
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only place where I feel like we should improve the code is the nil dereference, the others are nits that can be fixed later.

@jhrozek jhrozek merged commit 42c2d6f into stacklok:main Sep 24, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Kubernetes MCPRegistry] Syncing status in case of failure
2 participants