metallb controller: improve visibility on metallb status, add logs, and fix bug #362
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
liornoy
changed the title
liornoy
force-pushed
the
add-controller-logs
branch
from
0ef7d81 to
a77519b
Compare
fedepaol
reviewed
Jul 24, 2023
controllers/metallb_controller.go
Outdated
| @@ -111,9 +113,11 @@ func (r *MetalLBReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct | |||
| } | |||
| if err := status.Update(context.TODO(), r.Client, instance, condition, errorMsg, wrappedErrMsg); err != nil { | |||
| logger.Error(err, "Failed to update metallb status", "Desired status", status.ConditionAvailable) | |||
| return result, err | |||
There was a problem hiding this comment.
does it make sense to return ctrl.Result{} here?
liornoy
force-pushed
the
add-controller-logs
branch
from
a77519b to
d753420
Compare
IsMetalLBAvailable is the function that checks if metallb is deployed and available, which results in the update of the metallb resource condition. Currently, we only check the CurrentNumberScheduled, but this is doesn't take into account a situation when the speaker pods are scheduled but are not in the "Ready" state. Therefore, here we add a check against ds.Status.NumberReady. Signed-off-by: liornoy <[email protected]>
Add log to a successfuly metallb resource condition update, example of output:
1.690101009667401e+09 INFO controllers.MetalLB updated metallb status successfully
{"metallb": "metallb-system/metallb", "condition": "Available", "resource name": "metallb"}
Signed-off-by: liornoy <[email protected]>
Change errorMsg value to "internal error" because previously, assigning it the error message string resulted in failure to update the condition having the following error: status.conditions[3].reason in body should match '^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$'", "errorVerbose": "MetalLB.metallb.io \"metallb\" is invalid: status.conditions[3].reason: Invalid value: i.e. the error message includes invalid characters. we do print the error message under the status.conditions.message. Example of the error message that includes invalid characters: "FailedToSyncMetalLBResources: could not apply (apps/v1, Kind=Deployment) openshift-metallb-system/controller: could not update object (apps/v1, Kind=Deployment) openshift-metallb-system/controller: Operation cannot be fulfilled on deployments.apps \\\"controller\\\": the object has been modified; please apply your changes to the latest version and try again\" Signed-off-by: liornoy <[email protected]>
liornoy
force-pushed
the
add-controller-logs
branch
from
d753420 to
1769567
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR batches together 3 small commits to solve issues with the metallb-operator controller.