feat/Check that just bootstrapped SN is available #2542

carpawell · 2023-09-01T16:41:37Z

No description provided.

carpawell · 2023-09-01T16:44:08Z

pkg/innerring/processors/netmap/process_peers.go

+		return
+	}
+
+	_, err = c.EndpointInfo(context.Background(), client.PrmEndpointInfo{})


@roman-khimov, @cthulhu-rider, @AliceInHunterland, ping me if you think we need to check returned values (IMO, that is useless)

I'd check them against the data for the map.

added some checks

codecov · 2023-09-01T16:45:16Z

Codecov Report

Merging #2542 (ccf8b5c) into master (9224e9f) will decrease coverage by 0.01%.
Report is 2 commits behind head on master.
The diff coverage is 0.00%.

❗ Current head ccf8b5c differs from pull request most recent head 757b90e. Consider uploading reports for the commit 757b90e to get more accurate results

@@            Coverage Diff             @@
##           master    #2542      +/-   ##
==========================================
- Coverage   29.67%   29.66%   -0.01%     
==========================================
  Files         405      405              
  Lines       30756    30758       +2     
==========================================
- Hits         9126     9124       -2     
- Misses      20860    20864       +4     
  Partials      770      770

Files Changed	Coverage Δ
pkg/innerring/innerring.go	`0.00% <0.00%> (ø)`
pkg/innerring/processors/netmap/process_peers.go	`0.00% <0.00%> (ø)`

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

📢 Have feedback on the report? Share it here.

roman-khimov

What if we have some temporary networking error/inconsistent behavior across IR nodes? Can this be retried once in a while until the notary request expires?

pkg/innerring/processors/netmap/process_peers.go

carpawell · 2023-09-04T22:46:07Z

What if we have some temporary networking error/inconsistent behavior across IR nodes?

Every IR node? More than 5/7?

Can this be retried once in a while until the notary request expires?

Do you mean some cache of "loser" SN nodes that will be retried some times after some period? Need to think about it. Maybe it would be better to do it on the SN side? It may not find itself as a candidate for the next epoch and retry request (but to whom if the IR (local morph consensus) is down?).

roman-khimov · 2023-09-05T06:25:04Z

What if we have some temporary networking error/inconsistent behavior across IR nodes?

Every IR node? More than 5/7?

Can be anything.

Maybe it would be better to do it on the SN side?

That depends on notary request expiration parameters mostly, although nodes can retry even without waiting for request to expire.

cthulhu-rider · 2023-09-05T12:21:06Z

pkg/innerring/processors/netmap/process_peers.go

+	var info clientcore.NodeInfo
+	err = clientcore.NodeInfoFromRawNetmapElement(&info, netmapCore.Node(nodeInfo))
+	if err != nil {
+		np.log.Warn("could not convert node info: %w", zap.Error(err))


Suggested change

np.log.Warn("could not convert node info: %w", zap.Error(err))

np.log.Warn("could not convert node info", zap.Error(err))

same below

cthulhu-rider · 2023-09-05T12:23:04Z

pkg/innerring/processors/netmap/process_peers.go

@@ -37,6 +44,37 @@ func (np *Processor) processAddPeer(ev netmapEvent.AddPeer) {
 		return
 	}

+	var info clientcore.NodeInfo


why don't we extend np.nodeValidator.VerifyAndUpdate procedure below with new check?

have not thought about that. it does not seem to me like a node info structure validator. do we need a separate pkg per every validation, a separate interface for it (and a separate composite interface implementation) for checks that should be done in only one place and be done always?

ok, moved to a separate validator

cthulhu-rider · 2023-09-05T12:24:57Z

pkg/innerring/processors/netmap/process_peers.go

+		return
+	}
+
+	res, err := c.EndpointInfo(context.Background(), client.PrmEndpointInfo{})


i'd add some sane deadline

added 15 sec

cthulhu-rider · 2023-09-05T12:28:31Z

pkg/innerring/processors/netmap/process_peers.go

@@ -116,3 +154,60 @@ func (np *Processor) processUpdatePeer(ev netmapEvent.UpdatePeer) {
 		np.log.Error("can't invoke netmap.UpdatePeer", zap.Error(err))
 	}
 }
+
+var errUnknownDifference = errors.New("announced and received information differ but not critical")


in current code makes no sense

dont mind dropping it

cthulhu-rider · 2023-09-05T12:29:46Z

pkg/innerring/processors/netmap/process_peers.go

+func compareNodeInfos(niExp, niGot netmap.NodeInfo) error {
+	// if a node bootstraps the first time it is OK
+	// to be OFFLINE but send ONLINE bootstrap requests
+	niGot.SetOnline()


states will be almost definitely different, maybe SetOnline() for both instances?

states will be almost definitely different

do not agree, health check pings should be the majority

maybe SetOnline() for both instances?

agree that it should allow maintenance state transfer, fixed

cthulhu-rider · 2023-09-05T12:31:55Z

pkg/innerring/processors/netmap/process_peers.go

+		return fmt.Errorf("hash: got %d, expect %d", got, exp)
+	}
+
+	niExp.SortAttributes()


really needed?

hm, strange, not needed at all, dropped

cthulhu-rider · 2023-09-05T12:35:39Z

pkg/innerring/processors/netmap/process_peers.go

+		return fmt.Errorf("attr number: got %d, expect %d", got, exp)
+	}
+
+	niExp.IterateAttributes(func(key, value string) {


btw do we check the uniqueness of attributes and endpoints somewhere? if so, i'd add a comment here, otherwise, lets cover this too

https://github.com/nspcc-dev/neofs-node/pull/2542/files#r1315818528 perhaps reduce this ambiguity

btw do we check the uniqueness of attributes and endpoints somewhere?

not found that check on the IR side. but our SN will not accept such config

added a validator for it (extended the existing one) but found out that it is impossible to parse such node info so i can't even add a test for it

carpawell · 2023-09-05T19:26:35Z

That depends on notary request expiration parameters mostly, although nodes can retry even without waiting for request to expire.

So we may do smth, I am not sure I have a strict opinion on that. @cthulhu-rider, @AliceInHunterland

Closes nspcc-dev#2475. Signed-off-by: Pavel Karpy <[email protected]>

Improves debug process. Signed-off-by: Pavel Karpy <[email protected]>

That is the API restriction. Add this check to the `maddress` validator and rename in to `structure` validator. Signed-off-by: Pavel Karpy <[email protected]>

cthulhu-rider · 2023-09-08T07:57:39Z

pkg/innerring/processors/netmap/nodevalidation/availability/validator.go

+
+	c, err := client.New(prmInit)
+	if err != nil {
+		return nil, fmt.Errorf("can't create SDK client: %w", err)


general: SDK client always seems stange to me, it's actully NeoFS API clent

carpawell requested a review from AliceInHunterland September 1, 2023 16:41

carpawell self-assigned this Sep 1, 2023

carpawell force-pushed the feat/validate-SN-availability-by-IR branch 2 times, most recently from 2bea24e to 7fa4a5a Compare September 1, 2023 16:43

carpawell commented Sep 1, 2023

View reviewed changes

carpawell marked this pull request as ready for review September 1, 2023 16:44

carpawell requested review from roman-khimov and cthulhu-rider as code owners September 1, 2023 16:44

roman-khimov reviewed Sep 1, 2023

View reviewed changes

pkg/innerring/processors/netmap/process_peers.go Outdated Show resolved Hide resolved

carpawell force-pushed the feat/validate-SN-availability-by-IR branch 2 times, most recently from acb6354 to 30fba81 Compare September 4, 2023 22:51

cthulhu-rider reviewed Sep 5, 2023

View reviewed changes

carpawell force-pushed the feat/validate-SN-availability-by-IR branch from 30fba81 to b2e5c32 Compare September 5, 2023 19:26

carpawell force-pushed the feat/validate-SN-availability-by-IR branch from b2e5c32 to 85ca87e Compare September 5, 2023 19:46

carpawell added 3 commits September 7, 2023 22:54

ir: Check that just bootstrapped SN is available

1877325

Closes nspcc-dev#2475. Signed-off-by: Pavel Karpy <[email protected]>

ir: Add public key hex to the logs if deny network candidate

de58dd3

Improves debug process. Signed-off-by: Pavel Karpy <[email protected]>

ir: Add node info attribute uniqueness validation

757b90e

That is the API restriction. Add this check to the `maddress` validator and rename in to `structure` validator. Signed-off-by: Pavel Karpy <[email protected]>

carpawell force-pushed the feat/validate-SN-availability-by-IR branch from 85ca87e to 757b90e Compare September 7, 2023 20:07

roman-khimov approved these changes Sep 7, 2023

View reviewed changes

cthulhu-rider approved these changes Sep 8, 2023

View reviewed changes

roman-khimov merged commit 0a13794 into nspcc-dev:master Sep 8, 2023

carpawell deleted the feat/validate-SN-availability-by-IR branch September 8, 2023 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat/Check that just bootstrapped SN is available #2542

feat/Check that just bootstrapped SN is available #2542

carpawell commented Sep 1, 2023

carpawell Sep 1, 2023

roman-khimov Sep 1, 2023

carpawell Sep 4, 2023

codecov bot commented Sep 1, 2023 •

edited

Loading

roman-khimov left a comment

carpawell commented Sep 4, 2023

roman-khimov commented Sep 5, 2023

cthulhu-rider Sep 5, 2023

cthulhu-rider Sep 5, 2023

carpawell Sep 5, 2023 •

edited

Loading

carpawell Sep 7, 2023

cthulhu-rider Sep 5, 2023

carpawell Sep 5, 2023

cthulhu-rider Sep 5, 2023

carpawell Sep 5, 2023

cthulhu-rider Sep 5, 2023 •

edited

Loading

carpawell Sep 5, 2023

cthulhu-rider Sep 5, 2023

carpawell Sep 5, 2023

cthulhu-rider Sep 5, 2023 •

edited

Loading

carpawell Sep 5, 2023

carpawell Sep 7, 2023

carpawell commented Sep 5, 2023

cthulhu-rider Sep 8, 2023

	np.log.Warn("could not convert node info: %w", zap.Error(err))
	np.log.Warn("could not convert node info", zap.Error(err))

feat/Check that just bootstrapped SN is available #2542

feat/Check that just bootstrapped SN is available #2542

Conversation

carpawell commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 1, 2023 • edited Loading

Codecov Report

roman-khimov left a comment

Choose a reason for hiding this comment

carpawell commented Sep 4, 2023

roman-khimov commented Sep 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cthulhu-rider Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cthulhu-rider Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell commented Sep 5, 2023

Choose a reason for hiding this comment

codecov bot commented Sep 1, 2023 •

edited

Loading

carpawell Sep 5, 2023 •

edited

Loading

cthulhu-rider Sep 5, 2023 •

edited

Loading

cthulhu-rider Sep 5, 2023 •

edited

Loading