Handle bootc last, and always apply configuration changes#17
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors bootc upgrade handling into per-host processing, integrates it with the global retry flow, and extends the SSH HostManager interface with bootc-aware methods. Adds a retry helper, replaces a processing function, removes a pre-check function, and updates apply logic to respect bootc upgrade status. Notes potential duplicate method blocks in ssh.go. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Sync as SyncExporterHosts
participant Host as processExporterInstancesAndBootc
participant SSH as HostManager (SSH)
participant Retry as Global Retry Loop
Sync->>Host: For each host: process(exporterInstances, hostSsh, hostName, retryQueue)
Host->>SSH: GetBootcStatus()
alt Bootc WILL_UPDATE
Host->>SSH: HandleBootcUpgrade(dryRun=false)
alt Error triggering upgrade
Host->>Host: enqueue RetryItem{ExporterInstance=nil}
end
else Bootc UPDATING
Note over Host: Skip exporter restarts/updates due to active upgrade
end
Host->>Host: Process non-nil exporter instances (apply/update)
loop For each retry item
Retry->>Retry: Check item.ExporterInstance
alt item.ExporterInstance == nil (bootc)
Retry->>SSH: HandleBootcUpgrade(dryRun=false)
alt Error persists
Retry->>Retry: Re-enqueue (increment attempts)
end
else Exporter instance retry
Retry->>Host: process exporter instance
alt Error persists
Retry->>Retry: Re-enqueue (increment attempts)
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
internal/exporter/ssh/ssh_test.go
Outdated
| func TestBootcStatusConstants(t *testing.T) { | ||
| // Test that all BootcStatus constants are defined correctly | ||
| assert.Equal(t, BootcStatus(0), BOOTC_UP_TO_DATE) | ||
| assert.Equal(t, BootcStatus(1), BOOTC_UPDATING) | ||
| assert.Equal(t, BootcStatus(2), BOOTC_WILL_UPDATE) | ||
| assert.Equal(t, BootcStatus(3), BOOTC_NOT_MANAGED) | ||
| } |
There was a problem hiding this comment.
Worst test ever, will remove
mangelajo
force-pushed
the
bootc-order
branch
from
6d9ce80 to
8a7ea44
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/exporter/host/host.go (2)
238-241: Fix nil deref when max attempts exceeded for bootc retry items
retryItem.ExporterInstancecan be nil for bootc entries, causing a panic here. Handle both cases.- fmt.Printf("💀 Max retry attempts exceeded for %s on %s, giving up: %v\n", - retryItem.ExporterInstance.Name, retryItem.HostName, retryItem.LastError) - finalErrors = append(finalErrors, fmt.Sprintf("%s on %s: %v", retryItem.ExporterInstance.Name, retryItem.HostName, retryItem.LastError)) - continue + if retryItem.ExporterInstance == nil { + fmt.Printf("💀 Max retry attempts exceeded for bootc upgrade on %s, giving up: %v\n", + retryItem.HostName, retryItem.LastError) + finalErrors = append(finalErrors, fmt.Sprintf("bootc upgrade on %s: %v", retryItem.HostName, retryItem.LastError)) + } else { + fmt.Printf("💀 Max retry attempts exceeded for %s on %s, giving up: %v\n", + retryItem.ExporterInstance.Name, retryItem.HostName, retryItem.LastError) + finalErrors = append(finalErrors, fmt.Sprintf("%s on %s: %v", retryItem.ExporterInstance.Name, retryItem.HostName, retryItem.LastError)) + } + continue
329-332: Bootc never runs when a host has zero exporter instancesEarly-continue skips bootc and host-level template apply, contradicting “always apply configuration changes” and “handle bootc last.” Remove the guard and let
processExporterInstancesAndBootcrun even with an empty slice.- // Skip the host if no viable exporter instances remain - if len(exporterInstances) == 0 { - continue - }Note:
processExporterInstancesAndBootcsafely ranges over a nil slice and still performs bootc handling.
🧹 Nitpick comments (4)
internal/exporter/host/host.go (4)
187-194: Avoid address-of-range variable; pass RetryItem by valueTaking
&retryItemwhereretryItemis the range variable is a classic Go pitfall. Make the helper take a value and append it.-// addToRetryQueue increments attempts and adds a retry item to the next retry queue -func (e *ExporterHostSyncer) addToRetryQueue(retryItem *RetryItem, err error, nextRetryQueue *[]RetryItem) { - retryItem.Attempts++ - retryItem.LastError = err - retryItem.LastAttemptTime = time.Now() - *nextRetryQueue = append(*nextRetryQueue, *retryItem) -} +// addToRetryQueue increments attempts and adds a retry item to the next retry queue +func (e *ExporterHostSyncer) addToRetryQueue(retryItem RetryItem, err error, nextRetryQueue *[]RetryItem) { + retryItem.Attempts++ + retryItem.LastError = err + retryItem.LastAttemptTime = time.Now() + *nextRetryQueue = append(*nextRetryQueue, retryItem) +}
259-267: Update helper call to pass value (bootc branch)Align with the safer
addToRetryQueuesignature.- e.addToRetryQueue(&retryItem, err, &nextRetryQueue) + e.addToRetryQueue(retryItem, err, &nextRetryQueue)
269-278: Update helper call to pass value (instance branch)Align with the safer
addToRetryQueuesignature.- e.addToRetryQueue(&retryItem, err, &nextRetryQueue) + e.addToRetryQueue(retryItem, err, &nextRetryQueue)
360-360: Wording nit: queue contains bootc items tooMessage says “failed instances” but queue may have bootc entries.
- fmt.Printf("\n🔄 Processing retry queue (%d failed instances) ===========================\n", len(retryQueue)) + fmt.Printf("\n🔄 Processing retry queue (%d failed items) ===========================\n", len(retryQueue))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/exporter/host/host.go(4 hunks)internal/exporter/ssh/ssh.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exporter/ssh/ssh.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursorrules)
**/*.go: Run golangci-lint viamake lintand fix all linting errors before proceeding
Runmake buildto ensure the binary compiles successfully and fix any build errors
Runmake fmtto ensure code formatting (go fmt) before finalizing changes
Files:
internal/exporter/host/host.go
🧬 Code graph analysis (1)
internal/exporter/host/host.go (2)
api/v1alpha1/exporterinstance_types.go (1)
ExporterInstance(71-77)internal/exporter/ssh/ssh.go (1)
HostManager(32-40)
🔇 Additional comments (2)
internal/exporter/host/host.go (2)
195-224: Bootc-after-instances flow looks goodBootc is handled after instance processing and enqueued for retries distinctly. Nice separation.
355-356: Ready after CI verification
No stale callers ofprocessExporterInstancesremain; please confirm build, vet, test, and lint all pass before merging.
* Moves bootc handling into the ssh module, since a different module/manager like flightctl would do things differently than what we do via SSH * Apply bootc changes at the end, after configuration changes and services have been handled on the exporter hosts * If bootc update is in progress don't restart services, since that will happen naturally on the host after bootc reboot, and it means too much pressure for some sidekicks.
mangelajo
force-pushed
the
bootc-order
branch
from
931c1f3 to
a9e6222
Compare
1 participant
Moves bootc handling into the ssh module, since a different module/manager like flightctl would do things differently than what we do via SSH
Apply bootc changes at the end, after configuration changes and services have been handled on the exporter hosts
If bootc update is in progress don't restart services, since that will happen naturally on the host after bootc reboot, and it means too much pressure for some sidekicks.
Summary by CodeRabbit