Power management redesign #1891
Conversation
Fetch code from other power branch and adapt to new design
muhamadazmy
force-pushed
the
power-management-redesign
branch
from
26855b5 to
170357f
Compare
muhamadazmy
requested review from
delandtj,
mariobassem,
ashraffouda,
rawdaGastan and
xmonader
as code owners
cmds/modules/powerd/main.go
Outdated
| // Module is entry point for module | ||
| var Module cli.Command = cli.Command{ | ||
| Name: "powerd", | ||
| Usage: "reports the node total resources", |
cmds/modules/powerd/main.go
Outdated
|
|
||
| cl, err := zbus.NewRedisClient(msgBrokerCon) | ||
| if err != nil { | ||
| return errors.Wrap(err, "fail to connect to message broker server") |
| @@ -102,7 +102,7 @@ func (d *identityManager) Farm() (string, error) { | |||
| } | |||
| defer cl.Close() | |||
|
|
|||
| farm, err := cl.GetFarm(uint32(d.env.FarmerID)) | |||
| farm, err := cl.GetFarm(uint32(d.env.FarmID)) | |||
| if errors.Is(err, substrate.ErrNotFound) { | |||
| return "", fmt.Errorf("wrong farm id") | |||
| } else if err != nil { | |||
There was a problem hiding this comment.
let's do on a separate if condition no need for the else if here
pkg/power/ethtool.go
Outdated
| continue | ||
| } | ||
| if len(parts) != 2 { | ||
| return "", fmt.Errorf("invalid ethtool output format") |
There was a problem hiding this comment.
can we include the line for more debugging purposes in the error?
| if errors.Is(err, ErrFlagNotFound) { | ||
| // no support for | ||
| return false, nil | ||
| } else if err != nil { |
There was a problem hiding this comment.
let's move it outside of the else if
There was a problem hiding this comment.
I really think it's more readable with the else if (specially if someone else going to work on this code) instead of multiple if
The reason is i think it's more readable, and probably optimized, is that it works in both cases if you return or not return in the branch. Also normally if u don't return the else if optimized the code so not all of the branches are checked
so
if cond1 {
}
if cond1 {
}both will be checked in runtime
but using else if like
if cond1 {
} else if cond2 {
}the compiler knows cond1 and cond2 can't both happen at the same time, hence if cond1 is true, cond2 is not checked.
pkg/power/power.go
Outdated
|
|
||
| func (p *PowerServer) shutdown() error { | ||
| if !p.enabled { | ||
| log.Info().Msg("ignoring shutdown because power managed is not enabled") |
There was a problem hiding this comment.
power managed -> power management?
pkg/power/power.go
Outdated
| return p.events(ctx) | ||
| } | ||
|
|
||
| type Direct struct { |
There was a problem hiding this comment.
What does this struct mean/do?
There was a problem hiding this comment.
Old code, will delete
| } | ||
|
|
||
| func (p *PowerServer) syncSelf() error { | ||
| cl, err := p.sub.Substrate() |
There was a problem hiding this comment.
substrate connection was never closed?
| Uint32("node", p.node). | ||
| Msg("received power event for farm") | ||
|
|
||
| cl, err := p.sub.Substrate() |
There was a problem hiding this comment.
substrate connection never closed.
| } | ||
|
|
||
| // start processing time events. | ||
| func (p *PowerServer) events(ctx context.Context) error { |
There was a problem hiding this comment.
would opening and closing the substrate connection here help a bit??
There was a problem hiding this comment.
i'm suggesting this because if there were many power management events, with each event being processed a substrate connection is opened, which may be costly???
There was a problem hiding this comment.
while the node receive all power events, not all events reaches here, only events intended to the node farm are actually propagated to this code
There was a problem hiding this comment.
So instead of holding an open connection all the time it's actually better to only open when needed which should not be many
| } else if err != nil { | ||
| log.Error().Err(err).Msg("sending uptime failed") | ||
| } else { | ||
| // context was cancelled |
There was a problem hiding this comment.
isn't this the case where uptime is reported successfully?
I'm asking if context is really canceled here or not.
There was a problem hiding this comment.
No, if u follow the code you will see that the u.uptime() actually starts an infinite loop that only returns succefully if the ctx is cancelled and the node is shutdown. Any other error means we need to start this infinite loop again
No description provided.