Skip to content

Commit 42c2d6f

Browse files
dmartinolclaude
andauthored
Fix reconciliation loops (#1996)
* Add separate sync and API status fields to MCPRegistry - 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]> * Remove deprecated fields from MCPRegistryStatus and update sync manager 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]> * Refactor sync manager and deployment template - 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]> * Fix formatting in deployment.yaml by adding a newline at the end of the file for consistency. Signed-off-by: Daniele Martinoli <[email protected]> * Fix formatting in deployment.yaml by adding a newline at the end of the file for consistency. Signed-off-by: Daniele Martinoli <[email protected]> * Fix formatting in deployment.yaml by removing an unnecessary newline at the end of the file for consistency. Signed-off-by: Daniele Martinoli <[email protected]> * Fix formatting in deployment.yaml by ensuring consistent indentation and removing an unnecessary newline at the end of the file. Signed-off-by: Daniele Martinoli <[email protected]> * chrt version bump and updated CRD docs Signed-off-by: Daniele Martinoli <[email protected]> * increase test coverage Signed-off-by: Daniele Martinoli <[email protected]> * nil check Signed-off-by: Daniele Martinoli <[email protected]> * removed duplicate APIEndpoint Signed-off-by: Daniele Martinoli <[email protected]> * removing duplicated code Signed-off-by: Daniele Martinoli <[email protected]> --------- Signed-off-by: Daniele Martinoli <[email protected]> Co-authored-by: Claude <[email protected]>
1 parent 4af4b8d commit 42c2d6f

File tree

20 files changed

+1837
-225
lines changed

20 files changed

+1837
-225
lines changed

cmd/thv-operator/api/v1alpha1/mcpregistry_types.go

Lines changed: 142 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -160,14 +160,58 @@ type TagFilter struct {
160160

161161
// MCPRegistryStatus defines the observed state of MCPRegistry
162162
type MCPRegistryStatus struct {
163-
// Phase represents the current phase of the MCPRegistry
163+
// Phase represents the current overall phase of the MCPRegistry
164+
// Derived from sync and API status
164165
// +optional
165166
Phase MCPRegistryPhase `json:"phase,omitempty"`
166167

167168
// Message provides additional information about the current phase
168169
// +optional
169170
Message string `json:"message,omitempty"`
170171

172+
// SyncStatus provides detailed information about data synchronization
173+
// +optional
174+
SyncStatus *SyncStatus `json:"syncStatus,omitempty"`
175+
176+
// APIStatus provides detailed information about the API service
177+
// +optional
178+
APIStatus *APIStatus `json:"apiStatus,omitempty"`
179+
180+
// StorageRef is a reference to the internal storage location
181+
// +optional
182+
StorageRef *StorageReference `json:"storageRef,omitempty"`
183+
184+
// LastManualSyncTrigger tracks the last processed manual sync annotation value
185+
// Used to detect new manual sync requests via toolhive.stacklok.dev/sync-trigger annotation
186+
// +optional
187+
LastManualSyncTrigger string `json:"lastManualSyncTrigger,omitempty"`
188+
189+
// Conditions represent the latest available observations of the MCPRegistry's state
190+
// +optional
191+
// +listType=map
192+
// +listMapKey=type
193+
Conditions []metav1.Condition `json:"conditions,omitempty"`
194+
}
195+
196+
// SyncStatus provides detailed information about data synchronization
197+
type SyncStatus struct {
198+
// Phase represents the current synchronization phase
199+
// +kubebuilder:validation:Enum=Idle;Syncing;Complete;Failed
200+
Phase SyncPhase `json:"phase"`
201+
202+
// Message provides additional information about the sync status
203+
// +optional
204+
Message string `json:"message,omitempty"`
205+
206+
// LastAttempt is the timestamp of the last sync attempt
207+
// +optional
208+
LastAttempt *metav1.Time `json:"lastAttempt,omitempty"`
209+
210+
// AttemptCount is the number of sync attempts since last success
211+
// +optional
212+
// +kubebuilder:validation:Minimum=0
213+
AttemptCount int `json:"attemptCount,omitempty"`
214+
171215
// LastSyncTime is the timestamp of the last successful sync
172216
// +optional
173217
LastSyncTime *metav1.Time `json:"lastSyncTime,omitempty"`
@@ -181,32 +225,66 @@ type MCPRegistryStatus struct {
181225
// +optional
182226
// +kubebuilder:validation:Minimum=0
183227
ServerCount int `json:"serverCount,omitempty"`
228+
}
184229

185-
// SyncAttempts is the number of sync attempts since last success
186-
// +optional
187-
// +kubebuilder:validation:Minimum=0
188-
SyncAttempts int `json:"syncAttempts,omitempty"`
189-
190-
// APIEndpoint is the URL of the registry API service
191-
// +optional
192-
APIEndpoint string `json:"apiEndpoint,omitempty"`
230+
// APIStatus provides detailed information about the API service
231+
type APIStatus struct {
232+
// Phase represents the current API service phase
233+
// +kubebuilder:validation:Enum=NotStarted;Deploying;Ready;Unhealthy;Error
234+
Phase APIPhase `json:"phase"`
193235

194-
// StorageRef is a reference to the internal storage location
236+
// Message provides additional information about the API status
195237
// +optional
196-
StorageRef *StorageReference `json:"storageRef,omitempty"`
238+
Message string `json:"message,omitempty"`
197239

198-
// LastManualSyncTrigger tracks the last processed manual sync annotation value
199-
// Used to detect new manual sync requests via toolhive.stacklok.dev/sync-trigger annotation
240+
// Endpoint is the URL where the API is accessible
200241
// +optional
201-
LastManualSyncTrigger string `json:"lastManualSyncTrigger,omitempty"`
242+
Endpoint string `json:"endpoint,omitempty"`
202243

203-
// Conditions represent the latest available observations of the MCPRegistry's state
244+
// ReadySince is the timestamp when the API became ready
204245
// +optional
205-
// +listType=map
206-
// +listMapKey=type
207-
Conditions []metav1.Condition `json:"conditions,omitempty"`
246+
ReadySince *metav1.Time `json:"readySince,omitempty"`
208247
}
209248

249+
// SyncPhase represents the data synchronization state
250+
// +kubebuilder:validation:Enum=Idle;Syncing;Complete;Failed
251+
type SyncPhase string
252+
253+
const (
254+
// SyncPhaseIdle means no sync is needed or scheduled
255+
SyncPhaseIdle SyncPhase = "Idle"
256+
257+
// SyncPhaseSyncing means sync is currently in progress
258+
SyncPhaseSyncing SyncPhase = "Syncing"
259+
260+
// SyncPhaseComplete means sync completed successfully
261+
SyncPhaseComplete SyncPhase = "Complete"
262+
263+
// SyncPhaseFailed means sync failed
264+
SyncPhaseFailed SyncPhase = "Failed"
265+
)
266+
267+
// APIPhase represents the API service state
268+
// +kubebuilder:validation:Enum=NotStarted;Deploying;Ready;Unhealthy;Error
269+
type APIPhase string
270+
271+
const (
272+
// APIPhaseNotStarted means API deployment has not been created
273+
APIPhaseNotStarted APIPhase = "NotStarted"
274+
275+
// APIPhaseDeploying means API is being deployed
276+
APIPhaseDeploying APIPhase = "Deploying"
277+
278+
// APIPhaseReady means API is ready to serve requests
279+
APIPhaseReady APIPhase = "Ready"
280+
281+
// APIPhaseUnhealthy means API is deployed but not healthy
282+
APIPhaseUnhealthy APIPhase = "Unhealthy"
283+
284+
// APIPhaseError means API deployment failed
285+
APIPhaseError APIPhase = "Error"
286+
)
287+
210288
// StorageReference defines a reference to internal storage
211289
type StorageReference struct {
212290
// Type is the storage type (configmap)
@@ -258,8 +336,10 @@ const (
258336
//+kubebuilder:object:root=true
259337
//+kubebuilder:subresource:status
260338
//+kubebuilder:printcolumn:name="Phase",type="string",JSONPath=".status.phase"
261-
//+kubebuilder:printcolumn:name="Servers",type="integer",JSONPath=".status.serverCount"
262-
//+kubebuilder:printcolumn:name="Last Sync",type="date",JSONPath=".status.lastSyncTime"
339+
//+kubebuilder:printcolumn:name="Sync",type="string",JSONPath=".status.syncStatus.phase"
340+
//+kubebuilder:printcolumn:name="API",type="string",JSONPath=".status.apiStatus.phase"
341+
//+kubebuilder:printcolumn:name="Servers",type="integer",JSONPath=".status.syncStatus.serverCount"
342+
//+kubebuilder:printcolumn:name="Last Sync",type="date",JSONPath=".status.syncStatus.lastSyncTime"
263343
//+kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"
264344
//+kubebuilder:resource:scope=Namespaced,categories=toolhive
265345
//nolint:lll
@@ -294,6 +374,48 @@ func (r *MCPRegistry) GetAPIResourceName() string {
294374
return fmt.Sprintf("%s-api", r.Name)
295375
}
296376

377+
// DeriveOverallPhase determines the overall MCPRegistry phase based on sync and API status
378+
func (r *MCPRegistry) DeriveOverallPhase() MCPRegistryPhase {
379+
syncStatus := r.Status.SyncStatus
380+
apiStatus := r.Status.APIStatus
381+
382+
// Default phases if status not set
383+
syncPhase := SyncPhaseIdle
384+
if syncStatus != nil {
385+
syncPhase = syncStatus.Phase
386+
}
387+
388+
apiPhase := APIPhaseNotStarted
389+
if apiStatus != nil {
390+
apiPhase = apiStatus.Phase
391+
}
392+
393+
// If sync failed, overall is Failed
394+
if syncPhase == SyncPhaseFailed {
395+
return MCPRegistryPhaseFailed
396+
}
397+
398+
// If sync in progress, overall is Syncing
399+
if syncPhase == SyncPhaseSyncing {
400+
return MCPRegistryPhaseSyncing
401+
}
402+
403+
// If sync is complete or idle (no sync needed), check API status
404+
if syncPhase == SyncPhaseComplete || syncPhase == SyncPhaseIdle {
405+
switch apiPhase {
406+
case APIPhaseReady:
407+
return MCPRegistryPhaseReady
408+
case APIPhaseError:
409+
return MCPRegistryPhaseFailed
410+
case APIPhaseNotStarted, APIPhaseDeploying, APIPhaseUnhealthy:
411+
return MCPRegistryPhasePending // API still starting/not healthy
412+
}
413+
}
414+
415+
// Default to pending for initial states
416+
return MCPRegistryPhasePending
417+
}
418+
297419
func init() {
298420
SchemeBuilder.Register(&MCPRegistry{}, &MCPRegistryList{})
299421
}

0 commit comments

Comments
 (0)