kaap-834: Introduce heartbeat mechanism for byohosts. #66
Conversation
Changelist by BitoThis pull request implements the following key changes.
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #443e41
Actionable Suggestions - 3
-
main.go - 1
- Incorrect initialization order for HeartbeatTimeoutPeriod variable · Line 116-116
-
agent/reconciler/host_reconciler.go - 2
- Missing update to Connected status field · Line 71-73
- Overly restrictive controller reconciliation predicate · Line 226-226
Additional Suggestions - 1
-
apis/infrastructure/v1beta1/condition_consts.go - 1
-
Inconsistent constant type declaration pattern · Line 92-98The new constants `HeartbeatReceivedReason` and `HeartbeatTimeoutReason` are defined as string type, but other similar reason constants in the file are untyped. For consistency, consider removing the explicit string type.
code suggestion
@@ -94,6 +94,6 @@ - // HeartbeatReceivedReason is used when a heartbeat is received within the timeout. - HeartbeatReceivedReason string = "HeartbeatReceived" - - // HeartbeatTimeoutReason is used when a heartbeat is not received within the timeout. - HeartbeatTimeoutReason string = "HeartbeatTimeout" + // HeartbeatReceivedReason is used when a heartbeat is received within the timeout. + HeartbeatReceivedReason = "HeartbeatReceived" + + // HeartbeatTimeoutReason is used when a heartbeat is not received within the timeout. + HeartbeatTimeoutReason = "HeartbeatTimeout"
-
Review Details
-
Files reviewed - 9 · Commit Range:
3ca6981..3ca6981- agent/reconciler/host_reconciler.go
- apis/infrastructure/v1beta1/byohost_types.go
- apis/infrastructure/v1beta1/condition_consts.go
- chart-generator/byoh-chart/manager_extraconfig_patch.yaml
- chart-generator/sample-values.yaml
- cmd/byohctl/service/constants.go
- config/crd/bases/infrastructure.cluster.x-k8s.io_byohosts.yaml
- controllers/infrastructure/byohost_controller.go
- main.go
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
| return ctrl.NewControllerManagedBy(mgr). | ||
| For(&infrastructurev1beta1.ByoHost{}). | ||
| WithEventFilter(predicates.ResourceNotPaused(ctrl.LoggerFrom(ctx))). | ||
| WithEventFilter(predicate.ResourceVersionChangedPredicate{}). |
There was a problem hiding this comment.
Adding ResourceVersionChangedPredicate will cause the controller to only reconcile when the resource version changes, potentially missing important reconciliation events. This predicate should be used with caution as it filters out many reconciliation triggers.
Code suggestion
Check the AI-generated fix before applying
| WithEventFilter(predicate.ResourceVersionChangedPredicate{}). |
Code Review Run #443e41
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Whenever byohost_controller makes changes in the status of byohost, there is version change and we want it to be considered as a reconcile even for host_reconciler.
vaibhavd21
requested review from
cruizen,
hsri-pf9,
mithilarun,
mridulgain,
psarwate,
sebastian-pf9 and
srm6867
and removed request for
hsri-pf9
cruizen
left a comment
cruizen
left a comment
There was a problem hiding this comment.
Can you resolve the feedback from Bito with appropriate comments?
|
|
||
| // ByohAgentDebPackageURL is the URL to download the agent package | ||
| ByohAgentDebPackageURL = "quay.io/platform9/byoh-agent-deb:0.1.200" | ||
| ByohAgentDebPackageURL = "quay.io/platform9/byoh-agent-deb:0.1.276" |
There was a problem hiding this comment.
Can we read this tag from a config file / parameter so that we can update it as metadata without changing code?
There was a problem hiding this comment.
Yes, We should do this through the byohctl.
Which version of agent-service this host should run.
Code Review Agent Run #6ca147Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
3 participants
kaap-834
byohost-controller reconciles after every
ByohHostReconcilePeriodwhich is 60 seconds - validates iflastHeartbeatTimeis within configurableHeartbeatTimeoutPeriod( defaults to 120 seconds ) and accordingly updates byohost status conditions, connected field andlastHeartbeatCheckTime.Updated agent side host_reconciler to reconcile every time with the change in the status field of respective byohost by adding
ResourceVersionChangedPredicatepredicate. At every reconciliation, host_reconciler updateslastHeartbeatTime.Testing:
If either the agent service is stopped or the machine is down ( which will anyway stop the service )
Starting service again -
Summary by Bito
This PR implements and enhances a heartbeat mechanism for monitoring byohost connectivity with a configurable timeout period and refined validation logic. The changes include CRD schema updates, configuration improvements through chart updates, improved logging, and condition checks for better host connectivity monitoring. Updates to controller and main application components ensure proper timeout value usage and system reliability.