sandbox_controller: improve error handling#185
Conversation
✅ Deploy Preview for agent-sandbox canceled.
|
k8s-ci-robot
added
cncf-cla: yes
|
Hi @Iceber. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/M
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
Signed-off-by: Iceber Gu <caiwei95@hotmail.com>
Iceber
force-pushed
the
improve_error_handling
branch
from
7c95570 to
dbfac36
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
barney-s
left a comment
barney-s
left a comment
There was a problem hiding this comment.
The refactoring in reconcilePVCs to un-nest the logic is a good improvement for readability. Several error messages could be made more specific by including the names of the resources being acted upon, which would greatly help with debugging.
|
|
||
| if !k8serrors.IsNotFound(err) { | ||
| log.Error(err, "Failed to get PVC") | ||
| return fmt.Errorf("PVC Get Failed: %w", err) |
There was a problem hiding this comment.
The error message "PVC Get Failed" is generic. For better debuggability, please include the name of the PVC that could not be retrieved. For example: return fmt.Errorf("failed to get PVC %s: %w", pvcName, err)
| return fmt.Errorf("SetControllerReference for PVC failed: %w", err) | ||
| } | ||
| if err := r.Create(ctx, pvc, client.FieldOwner(sandboxControllerFieldOwner)); err != nil { | ||
| log.Error(err, "Failed to create PVC", "PVC.Namespace", sandbox.Namespace, "PVC.Name", pvcName) |
There was a problem hiding this comment.
While the error is logged, the returned error lacks context. Consider wrapping the error to make it clearer where it originated, for instance: return fmt.Errorf("failed to create PVC %s: %w", pvcName, err).
| if podReady && svcReady { | ||
| readyCondition.Status = metav1.ConditionTrue | ||
| readyCondition.Reason = "DependenciesReady" | ||
| return readyCondition |
There was a problem hiding this comment.
While removing the early return is a valid stylistic choice for creating a single exit point, the condition for Ready=False is now less explicit. Consider adding an else block to set a Reason such as "DependenciesNotReady". This would make the sandbox's status clearer during inspection.
| log.Error(err, "Failed to get PVC") | ||
| return fmt.Errorf("PVC Get Failed: %w", err) | ||
| } | ||
| if err == nil { |
There was a problem hiding this comment.
This continue statement handles the case where the PVC already exists. This is correct, but adding a comment here explaining that this signifies the "pvc already exists" case would improve clarity for future readers.
| Spec: pvcTemplate.Spec, | ||
| } | ||
| if err := ctrl.SetControllerReference(sandbox, pvc, r.Scheme); err != nil { | ||
| return fmt.Errorf("SetControllerReference for PVC failed: %w", err) |
There was a problem hiding this comment.
When setting the controller reference, the error message SetControllerReference for PVC failed is helpful, but it would be even better to include the name of the PVC for which it failed.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Iceber, janetkuo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
4 participants
No description provided.