Consider exporting node netmap status #217

New Issue

Open

opened 2024-04-27 11:09:56 +00:00 by fyrchik · 0 comments

fyrchik commented

2024-04-27 11:09:56 +00:00

Owner

Currently we have IsOnline()/IsOffline()/IsMaintenance().
All they do is fetch status as a number and perform some function on it.
We could return state directly.
Supposedly it should've been more convenient, but when I grep the node code, most of the time (5 out of 7 matches) we use them in switches. It prevents us from accidentally setting invalid status, but also leads to somewhat verbose code.

The sugestion is to export GetStatus, deprecate current methods and, possibly, make Is* methods on status type, though I think they are useless.

cmd/frostfs-node/netmap.go
62-             s.nodeInfo.Store(*ni)
63-
64-             switch {
65:             case ni.IsOnline():
66-                     ctrlNetSt = control.NetmapStatus_ONLINE
67-             case ni.IsOffline():
68-                     ctrlNetSt = control.NetmapStatus_OFFLINE
--
276-func nodeState(ni *netmapSDK.NodeInfo) string {
277-    if ni != nil {
278-            switch {
279:            case ni.IsOnline():
280-                    return "online"
281-            case ni.IsOffline():
282-                    return "offline"

cmd/internal/common/netmap.go
17-     switch {
18-     default:
19-             strState = "STATE_UNSUPPORTED"
20:     case node.IsOnline():
21-             strState = "ONLINE"
22-     case node.IsOffline():
23-             strState = "OFFLINE"

cmd/frostfs-cli/modules/netmap/nodeinfo.go
52-     switch {
53-     default:
54-             stateWord = "<undefined>"
55:     case i.IsOnline():
56-             stateWord = "online"
57-     case i.IsOffline():
58-             stateWord = "offline"

pkg/innerring/processors/netmap/process_peers.go
62-     // But there is no guarantee that code will be executed in the same order.
63-     // That is why we need to perform `addPeerIR` only in case when node is online,
64-     // because in scope of this method, contract set state `ONLINE` for the node.
65:     if updated && nodeInfo.IsOnline() {
66-             np.log.Info(logs.NetmapApprovingNetworkMapCandidate,
67-                     zap.String("key", keyString))
68-

pkg/innerring/processors/netmap/nodevalidation/state/validator.go
54-// VerifyAndUpdate does not mutate the parameter in a binary format.
55-// MUST NOT be called before SetNetworkSettings.
56-//
57:// See also netmap.NodeInfo.IsOnline/SetOnline and other similar methods.
58-func (x *NetMapCandidateValidator) VerifyAndUpdate(node *netmap.NodeInfo) error {
59:     if node.IsOnline() {
60-             return nil
61-     }
62-

pkg/morph/client/netmap/netmap_test.go
39-             var state int64
40-
41-             switch {
42:             case expected[i].IsOnline():
43-                     state = int64(netmapcontract.NodeStateOnline)
44-             case expected[i].IsOffline():
45-                     state = int64(netmapcontract.NodeStateOffline)

Currently we have `IsOnline()`/`IsOffline()`/`IsMaintenance()`. All they do is fetch status as a number and perform some function on it. We could return state directly. Supposedly it should've been more convenient, but when I grep the node code, most of the time (5 out of 7 matches) we use them in switches. It prevents us from accidentally setting invalid status, but also leads to somewhat verbose code. The sugestion is to export `GetStatus`, deprecate current methods and, possibly, make `Is*` methods on status type, though I think they are useless. ``` cmd/frostfs-node/netmap.go 62- s.nodeInfo.Store(*ni) 63- 64- switch { 65: case ni.IsOnline(): 66- ctrlNetSt = control.NetmapStatus_ONLINE 67- case ni.IsOffline(): 68- ctrlNetSt = control.NetmapStatus_OFFLINE -- 276-func nodeState(ni *netmapSDK.NodeInfo) string { 277- if ni != nil { 278- switch { 279: case ni.IsOnline(): 280- return "online" 281- case ni.IsOffline(): 282- return "offline" cmd/internal/common/netmap.go 17- switch { 18- default: 19- strState = "STATE_UNSUPPORTED" 20: case node.IsOnline(): 21- strState = "ONLINE" 22- case node.IsOffline(): 23- strState = "OFFLINE" cmd/frostfs-cli/modules/netmap/nodeinfo.go 52- switch { 53- default: 54- stateWord = "<undefined>" 55: case i.IsOnline(): 56- stateWord = "online" 57- case i.IsOffline(): 58- stateWord = "offline" pkg/innerring/processors/netmap/process_peers.go 62- // But there is no guarantee that code will be executed in the same order. 63- // That is why we need to perform `addPeerIR` only in case when node is online, 64- // because in scope of this method, contract set state `ONLINE` for the node. 65: if updated && nodeInfo.IsOnline() { 66- np.log.Info(logs.NetmapApprovingNetworkMapCandidate, 67- zap.String("key", keyString)) 68- pkg/innerring/processors/netmap/nodevalidation/state/validator.go 54-// VerifyAndUpdate does not mutate the parameter in a binary format. 55-// MUST NOT be called before SetNetworkSettings. 56-// 57:// See also netmap.NodeInfo.IsOnline/SetOnline and other similar methods. 58-func (x *NetMapCandidateValidator) VerifyAndUpdate(node *netmap.NodeInfo) error { 59: if node.IsOnline() { 60- return nil 61- } 62- pkg/morph/client/netmap/netmap_test.go 39- var state int64 40- 41- switch { 42: case expected[i].IsOnline(): 43- state = int64(netmapcontract.NodeStateOnline) 44- case expected[i].IsOffline(): 45- state = int64(netmapcontract.NodeStateOffline) ```