Harmonize reconciler logic and improve logging #244
Conversation
github-actions
bot
added
api-change
documentation
afritzler
force-pushed
the
enh/reconciler
branch
from
e8a14ea to
3883e25
Compare
hardikdr
left a comment
hardikdr
left a comment
There was a problem hiding this comment.
Thanks for all the polishing @afritzler
| if ignitionErr != nil { | ||
| patchError := r.patchStatus(ctx, HTTPBootConfig, state) | ||
| if patchError != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to patch status %w %w", ignitionErr, patchError) |
There was a problem hiding this comment.
While we loose reporting the IgnitionErr when Patch fails, I do like simplistic error model here, thanks.
hardikdr
left a comment
hardikdr
left a comment
There was a problem hiding this comment.
Could you please rebase and it's good to go in.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR updates Kubernetes dependencies from v1.31 to v1.34, upgrades golangci-lint tooling from v2.1 to v2.6, and refactors boot configuration controller reconciliation logic with standardized parameter naming, context-based logging, and new SetupWithManager methods for resource integration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. 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: 0
🧹 Nitpick comments (3)
internal/controller/ipxebootconfig_controller.go (1)
49-68: Inconsistent error handling compared to HTTPBootConfigReconciler.This reconciler returns an error after
ensureIgnitionfails (line 58), whileHTTPBootConfigReconcilerlogs and returnsnil(httpbootconfig_controller.go:58-59). The approach here is preferable as it enables retries, but consider aligning behavior across controllers.internal/controller/httpbootconfig_controller.go (1)
119-129: Inefficient cross-namespace listing.Line 120 lists all
HTTPBootConfigresources across all namespaces withclient.InNamespace(""), then filters by namespace match on lines 127-129. TheIPXEBootConfigReconciler(ipxebootconfig_controller.go:119) usesclient.InNamespace(secretObj.Namespace)directly, which is more efficient.Consider aligning with the IPXE controller approach:
♻️ Suggested fix
- if err := r.List(ctx, configList, client.InNamespace("")); err != nil { + if err := r.List(ctx, configList, client.InNamespace(secretObj.Namespace)); err != nil { log.Error(err, "failed to list HTTPBootConfig for Secret", "Secret", client.ObjectKeyFromObject(secretObj)) return nil } var requests []reconcile.Request for _, config := range configList.Items { - if config.Spec.IgnitionSecretRef != nil && - config.Spec.IgnitionSecretRef.Name == secretObj.Name && - config.Namespace == secretObj.Namespace { + if config.Spec.IgnitionSecretRef != nil && + config.Spec.IgnitionSecretRef.Name == secretObj.Name {internal/controller/serverbootconfiguration_pxe_controller.go (1)
196-208: Suboptimal slice capacity.Line 202 pre-allocates capacity of 1, but servers typically have multiple network interfaces. The HTTP controller (serverbootconfiguration_http_controller.go:174) uses
2*len(server.Status.NetworkInterfaces).♻️ Suggested fix
- systemIPs := make([]string, 0, 1) + systemIPs := make([]string, 0, len(server.Status.NetworkInterfaces)) for _, nic := range server.Status.NetworkInterfaces { systemIPs = append(systemIPs, nic.IP.String()) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/lint.ymlMakefilecmdutils/suite_test.godocs/api-reference/api.mdhack/api-reference/config.jsoninternal/controller/httpbootconfig_controller.gointernal/controller/ipxebootconfig_controller.gointernal/controller/serverbootconfiguration_http_controller.gointernal/controller/serverbootconfiguration_pxe_controller.gointernal/controller/suite_test.go
🧰 Additional context used
🧬 Code graph analysis (4)
internal/controller/serverbootconfiguration_http_controller.go (2)
cmd/bootctl/app/app.go (1)
Name(15-15)api/v1alpha1/httpbootconfig_types.go (1)
HTTPBootConfig(54-60)
internal/controller/httpbootconfig_controller.go (2)
api/v1alpha1/httpbootconfig_types.go (4)
HTTPBootConfig(54-60)HTTPBootConfigState(34-34)HTTPBootConfigStateError(44-44)HTTPBootConfigList(65-69)api/v1alpha1/constants.go (1)
DefaultIgnitionKey(7-7)
internal/controller/serverbootconfiguration_pxe_controller.go (2)
api/v1alpha1/ipxebootconfig_types.go (3)
IPXEBootConfig(73-79)IPXEBootConfigStateReady(48-48)IPXEBootConfigStateError(54-54)server/bootserver.go (1)
SetStatusCondition(446-474)
internal/controller/ipxebootconfig_controller.go (1)
api/v1alpha1/ipxebootconfig_types.go (3)
IPXEBootConfig(73-79)IPXEBootConfigState(44-44)IPXEBootConfigList(84-88)
🔇 Additional comments (16)
cmdutils/suite_test.go (1)
62-62: LGTM! Consistent Kubernetes test binary version upgrade.The BinaryAssetsDirectory paths have been correctly updated from 1.31.0 to 1.34.0 for both source and target test environments, aligning with the PR objective to bump envtest to Kubernetes 1.34.
Also applies to: 92-92
hack/api-reference/config.json (1)
24-24: LGTM! API reference documentation URL aligned with Kubernetes 1.34.The docsURLTemplate has been correctly updated to reference v1.34 Kubernetes API documentation, ensuring generated API docs link to the appropriate version.
internal/controller/suite_test.go (1)
78-78: LGTM! Kubernetes test binary version upgraded to 1.34.0.The BinaryAssetsDirectory path correctly references the new Kubernetes 1.34.0 test binaries, consistent with other test files in this PR.
docs/api-reference/api.md (1)
51-51: LGTM! Kubernetes API reference links consistently updated to v1.34.All Kubernetes API documentation hyperlinks (ObjectMeta, LocalObjectReference, Condition) have been correctly updated from v1.31 to v1.34, ensuring users reference the appropriate Kubernetes API version.
Also applies to: 89-89, 171-171, 275-275, 288-288, 345-345, 435-435, 543-543, 556-556, 625-625
.github/workflows/lint.yml (1)
20-20: LGTM! golangci-lint binary version updated.The update from v2.5 to v2.6 is a straightforward tooling upgrade. The
versionfield specifies the golangci-lint binary version, not the GitHub action version itself.Likely an incorrect or invalid review comment.
internal/controller/serverbootconfiguration_http_controller.go (2)
254-280: LGTM on the refactored enqueue function.The switch to
ctrl.LoggerFrom(ctx)and standardizedbootConfig/bootConfigListnaming aligns well with the harmonization effort across controllers.
282-292: SetupWithManager implementation follows established patterns.The wiring correctly manages
ServerBootConfiguration, ownsHTTPBootConfig, and watchesSecretsfor ignition secret changes.internal/controller/ipxebootconfig_controller.go (2)
33-40: LGTM on the Reconcile method refactoring.The switch to
ctrl.LoggerFrom(ctx)andconfigvariable naming is consistent with the harmonization effort.
137-147: SetupWithManager implementation is correct.Properly registers the controller for
IPXEBootConfigand watchesSecretsfor ignition changes.internal/controller/httpbootconfig_controller.go (3)
33-40: LGTM on the Reconcile method refactoring.Consistent use of
ctrl.LoggerFrom(ctx)andconfigvariable naming.
54-59: Error swallowing on ignition failure is intentional.The reconciler logs the error and returns
nilrather than propagating it. This prevents retries when ignition setup fails, which may be the desired behavior based on prior review discussions.
140-150: SetupWithManager implementation is correct.Follows the established pattern and properly registers Secret watches.
internal/controller/serverbootconfiguration_pxe_controller.go (3)
57-62: LGTM on the reconciler struct declaration.The struct fields are properly defined with consistent naming.
94-160: Reconcile logic is well-structured.The distinction between
bootConfig(ServerBootConfiguration) andconfig(IPXEBootConfig) clearly communicates the parent-child relationship. Controller reference setup and apply logic are correct.
373-384: SetupWithManager implementation is correct.Properly manages
ServerBootConfiguration, ownsIPXEBootConfig, and watchesSecretsfor ignition changes. Consistent with the HTTP controller's setup pattern.Makefile (1)
204-204: Version bump to v2.6 is valid.golangci-lint v2.6.0 was released in October 2025 with patch releases available. The update aligns well with the CI workflow changes.
Proposed Changes
k8s.ioenvtest setup to 1.34Summary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.