control: add network settings management via ir control #3059

End-rey · 2024-12-19T19:20:46Z

Closes #2088, #1866.

codecov · 2024-12-19T19:28:21Z

Codecov Report

Attention: Patch coverage is 1.80587% with 1305 lines in your changes missing coverage. Please review.

Project coverage is 21.95%. Comparing base (08bd8d3) to head (3f32aae).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/services/control/ir/service.pb.go	4.14%	463 Missing ⚠️
pkg/services/control/ir/service_neofs.pb.go	0.00%	192 Missing ⚠️
pkg/innerring/network.go	0.00%	130 Missing ⚠️
pkg/services/control/ir/service_grpc.pb.go	0.00%	88 Missing ⚠️
cmd/neofs-cli/modules/control/notary_request.go	0.00%	71 Missing ⚠️
pkg/services/control/ir/server/calls.go	0.00%	65 Missing ⚠️
pkg/services/control/ir/service.go	0.00%	44 Missing ⚠️
cmd/neofs-cli/modules/control/notary_list.go	0.00%	43 Missing ⚠️
cmd/neofs-cli/modules/control/notary_sign.go	0.00%	43 Missing ⚠️
pkg/services/control/ir/rpc.go	0.00%	42 Missing ⚠️
... and 13 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3059      +/-   ##
==========================================
- Coverage   22.43%   21.95%   -0.48%     
==========================================
  Files         792      797       +5     
  Lines       58538    59823    +1285     
==========================================
+ Hits        13132    13135       +3     
- Misses      44506    45787    +1281     
- Partials      900      901       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

End-rey · 2024-12-20T15:02:42Z

I have implemented the following methods: newEpoch, setConfig and removeNode. What else needs to be done?

When calling the list command, only hashes are given. Is it necessary to display the information in more detail?

carpawell · 2024-12-23T12:40:11Z

docs/cli-commands/neofs-cli_control_notary.md

+* [neofs-cli control](neofs-cli_control.md)	 - Operations with storage node
+* [neofs-cli control notary list](neofs-cli_control_notary_list.md)	 - Get list of all notary requests in network
+* [neofs-cli control notary request](neofs-cli_control_notary_request.md)	 - Create and send a notary request
+* [neofs-cli control notary sign](neofs-cli_control_notary_sign.md)	 - Sign notary request with it hash


Suggested change

* [neofs-cli control notary sign](neofs-cli_control_notary_sign.md) - Sign notary request with it hash

* [neofs-cli control notary sign](neofs-cli_control_notary_sign.md) - Sign notary request by its hash

?

carpawell · 2024-12-23T12:42:24Z

pkg/innerring/network.go

+func (s *Server) RequestNotary(method string, args ...string) (string, error) {
+	if !s.IsAlphabet() {
+		s.log.Info("non alphabet mode, ignore request")
+		return "", nil


we can return the error, otherwise, it is not clear for the client what happened

carpawell · 2024-12-23T12:45:06Z

pkg/innerring/network.go

+		nonce uint32 = 1
+		vubP  *uint32


could be dropped (ok, nonce can be a const if needed). this code does have its meaning in the morph client code, but is useless here

carpawell · 2024-12-23T12:47:17Z

pkg/innerring/network.go

+
+		hash, err = s.netmapClient.Morph().NotaryInvoke(s.netmapClient.ContractAddress(), 0, nonce, vubP, method, epoch+1)
+		if err != nil {
+			s.log.Warn("can't invoke newEpoch method in alphabet contract",


this is not the alphabet contract here and below. alphabet contract is smth different. just netmap. also, i would add info that it is an external request to understand logs better

carpawell · 2024-12-23T12:48:45Z

pkg/innerring/network.go

+		err  error
+	)
+
+	switch method {


what do we do in default? it is also an error if we are requested to do things we cannot do. can be returned to client too

carpawell · 2024-12-23T13:06:06Z

pkg/services/control/ir/service.proto

+message NotaryRequestRequest {
+    // Request body structure.
+    message Body {
+        string method = 1;


maybe it would be better to have a more generic contractHash, method, args or maybe not @roman-khimov, @cthulhu-rider

but then admin needs to know contract hash of course

Let's keep it simple for now, can be extended in future.

carpawell · 2024-12-23T13:07:09Z

pkg/services/control/ir/service.proto

+    // Request body structure.
+    message Body {
+        // Hash of transaction that need to be signed.
+        string hash = 1;


i think raw representation would be better as we usually prefer in our protobuf protocol cc @roman-khimov

carpawell · 2024-12-23T13:07:50Z

pkg/services/control/ir/types.proto

+
+    // Hash of transaction.
+    string hash = 2;
+}


add empty line before EOF, please

carpawell · 2024-12-23T13:08:40Z

pkg/services/control/ir/types.proto

+// Info about transaction.
+message TransactionInfo {
+    // Script that contains in the transaction.
+    bytes script = 1;


it is not used now, right? do we need it?

carpawell · 2024-12-23T13:09:07Z

pkg/services/control/ir/types.proto

+    bytes script = 1;
+
+    // Hash of transaction.
+    string hash = 2;


i would also prefer raw hash here

carpawell · 2024-12-23T13:12:57Z

Is it necessary to display the information in more detail?

I think it will be useful in the future, we may leave an issue or do this now. notaryPreparator can simplify it significantly.

pkg/innerring/network.go

cmd/neofs-cli/modules/control/notary_request.go

pkg/innerring/network.go

pkg/morph/client/notary.go

pkg/services/control/ir/server/calls.go

pkg/innerring/network.go

roman-khimov · 2024-12-25T20:04:15Z

pkg/services/control/ir/service.proto

+message NotaryRequestRequest {
+    // Request body structure.
+    message Body {
+        string method = 1;


Let's keep it simple for now, can be extended in future.

pkg/innerring/network.go

cmd/neofs-cli/modules/control/notary_request.go

pkg/innerring/network.go

New `neofs-cli control notary` with `list`, `request` and `sign` commands. - `list` - get list of all notary requests from notary pool. - `request` - send a notary request with one of the following methods: `newEpoch`, `setConfig` and `removeNode`. - `sign` - sign the notary request using a hash. Closes #2088, #1866. Signed-off-by: Andrey Butusov <[email protected]>

End-rey self-assigned this Dec 19, 2024

End-rey requested review from roman-khimov, carpawell and cthulhu-rider as code owners December 19, 2024 19:20

End-rey marked this pull request as draft December 19, 2024 19:21

End-rey force-pushed the 2088-allow-ir-to-change-network-parameters branch 3 times, most recently from f8477d6 to 9a98964 Compare December 20, 2024 15:00

End-rey marked this pull request as ready for review December 20, 2024 15:02

carpawell reviewed Dec 23, 2024

View reviewed changes

End-rey force-pushed the 2088-allow-ir-to-change-network-parameters branch from 9a98964 to 742d111 Compare December 23, 2024 15:09

roman-khimov reviewed Dec 25, 2024

View reviewed changes

End-rey force-pushed the 2088-allow-ir-to-change-network-parameters branch from 742d111 to 9e19c9e Compare December 26, 2024 14:53

End-rey requested review from carpawell and roman-khimov December 26, 2024 15:20

roman-khimov reviewed Dec 26, 2024

View reviewed changes

pkg/innerring/network.go Show resolved Hide resolved

cmd/neofs-cli/modules/control/notary_request.go Outdated Show resolved Hide resolved

cmd/neofs-cli/modules/control/notary_request.go Outdated Show resolved Hide resolved

pkg/innerring/network.go Outdated Show resolved Hide resolved

End-rey force-pushed the 2088-allow-ir-to-change-network-parameters branch from 9e19c9e to 3f32aae Compare December 27, 2024 11:44

roman-khimov approved these changes Dec 27, 2024

View reviewed changes

roman-khimov mentioned this pull request Dec 27, 2024

Test IR controls for setting management nspcc-dev/neofs-testcases#924

Closed

roman-khimov merged commit 6749956 into master Dec 28, 2024
20 of 22 checks passed

roman-khimov deleted the 2088-allow-ir-to-change-network-parameters branch December 28, 2024 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

control: add network settings management via ir control #3059

control: add network settings management via ir control #3059

End-rey commented Dec 19, 2024

codecov bot commented Dec 19, 2024 •

edited

Loading

End-rey commented Dec 20, 2024

carpawell Dec 23, 2024

carpawell Dec 23, 2024

carpawell Dec 23, 2024

carpawell Dec 23, 2024

carpawell Dec 23, 2024

carpawell Dec 23, 2024

carpawell Dec 23, 2024

roman-khimov Dec 25, 2024

carpawell Dec 23, 2024

carpawell Dec 23, 2024

carpawell Dec 23, 2024

carpawell Dec 23, 2024

carpawell commented Dec 23, 2024

roman-khimov Dec 25, 2024

	* [neofs-cli control notary sign](neofs-cli_control_notary_sign.md) - Sign notary request with it hash
	* [neofs-cli control notary sign](neofs-cli_control_notary_sign.md) - Sign notary request by its hash

control: add network settings management via ir control #3059

control: add network settings management via ir control #3059

Conversation

End-rey commented Dec 19, 2024

codecov bot commented Dec 19, 2024 • edited Loading

Codecov Report

End-rey commented Dec 20, 2024

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

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

carpawell commented Dec 23, 2024

Choose a reason for hiding this comment

codecov bot commented Dec 19, 2024 •

edited

Loading