-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update thresholds logic in the farmerbot and create independent checks for each cloud unit utilization #819
Conversation
…ting the demand and filtering nodes that can be powered on or turned off to reduce the waste of resources as long as they meet the defined threshold
…into development_farmerbot_usageunits
…d high resources, return error if nodes length is zero in high usage case
farmerbot/internal/power.go
Outdated
return f.powerOn(sub, nodeID) | ||
log.Info().Uint32("nodeID", uint32(node.ID)).Msg("Too much resource usage. Turning on node") | ||
if err := f.powerOn(sub, uint32(node.ID)); err != nil { | ||
return fmt.Errorf("couldn't power on node %v with error: %w", node.ID, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return? Or continue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should continue indeed you're right
Anything blocking here? Would be nice to get this merged, since the current implementation means that in many cases new nodes won't be woken when they are needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, just some small comments.
} | ||
|
||
// Check if this node can contribute to the remaining demand | ||
contribute := false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think it is a good idea to find the least number of nodes to wakeup?
i am thinking of in some cases maybe node1 satisfy cru only and it will be added to selectedNodes to power on, then node2 will satisfy the rest of resources and it also can satisfy the initial demand of cru, so instead of powering on both, node2 will be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discussed it with @muhamadazmy. I think it is ok (your suggestion is right) to power on the next node that fits requirements. Of course, we don't guarantee if it will be used or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, agree with Omar comments :)
…ldtech/tfgrid-sdk-go into development_farmerbot_usageunits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
update thresholds logic in the farmerbot and create independent checks for each cloud unit utilization.
Description
update thresholds logic in the farmerbot and create independent checks for each cloud unit utilization.
Changes
Related Issues
Checklist
Updated report