go/control/api: Improve node registration status clarity #5256

abukosek · 2023-04-26T11:01:24Z

This PR adds three new fields to the node's control status output under the registration status section:

last_attempt_successful - true if the last registration attempt succeeded.
last_attempt_error_message - error message if the last registration attempt failed.
last_attempt - time of the last registration attempt.

Also, if the registration descriptor is expired, it is no longer shown in the output.

codecov · 2023-04-26T13:59:39Z

Codecov Report

Merging #5256 (d243d97) into master (139111d) will increase coverage by 0.22%.
The diff coverage is 30.64%.

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

@@            Coverage Diff             @@
##           master    #5256      +/-   ##
==========================================
+ Coverage   66.87%   67.09%   +0.22%     
==========================================
  Files         515      516       +1     
  Lines       54757    54814      +57     
==========================================
+ Hits        36620    36780     +160     
+ Misses      13663    13574      -89     
+ Partials     4474     4460      -14

Impacted Files	Coverage Δ
go/worker/registration/worker.go	`61.29% <20.45%> (-3.04%)`	⬇️
go/runtime/host/sgx/metrics.go	`42.85% <42.85%> (ø)`
go/runtime/host/sgx/sgx.go	`70.13% <100.00%> (+1.32%)`	⬆️

... and 58 files with indirect coverage changes

go/worker/registration/worker.go

peternose

Tested and works. Left some comments.

  "registration": {
    "successful": false,
    "error": "registry: no such entity",
    "last_registration": "0001-01-01T00:00:00Z"
  },

go/worker/registration/worker.go

peternose · 2023-04-27T16:26:53Z

go/control/api/api.go

+	// Successful is true if the current registration has been successful.
+	Successful bool `json:"successful"`
+
+	// Error contains the error message if the current registration has not been successful.


Suggested change

// Error contains the error message if the current registration has not been successful.

// Error contains the error message if the last registration has not been successful.

To be concise with last successful registration. Same for Successful.

The last registration can be successful, but the current one can fail -- this field only shows the status of the current one.

For example, the node successfully registers and sets Successful to true, LastRegistration contains the timestamp and Descriptor the descriptor. Then before the existing registration expires, the node can try to re-register and fail, in which case, its registration would still be valid (as well as the Descriptor), but Successful would be false.

peternose · 2023-04-27T16:44:44Z

go/control/api/api.go

+	Successful bool `json:"successful"`
+
+	// Error contains the error message if the current registration has not been successful.
+	Error string `json:"error,omitempty"`


Suggested change

Error string `json:"error,omitempty"`

ErrorMessage string `json:"error_message,omitempty"`

Maybe I would change this, as I expect that Error has error type.

I initially had this as the error type, but apparently it can't be serialized properly, so this is the most straightforward way to do it.

Type error is an interface and will marshal to {}, so you cannot use it, unfortunately.

peternose · 2023-04-27T16:48:56Z

go/control/api/api.go

@@ -124,6 +124,12 @@ type IdentityStatus struct {

 // RegistrationStatus is the node registration status.
 type RegistrationStatus struct {
+	// Successful is true if the current registration has been successful.
+	Successful bool `json:"successful"`


I have mixed fillings about this field as it duplicates data (e.g currently a bug could return true and error) and is ambiguous (looking only at the json without reading the comment).

I prefer having the following data: Registered (bool), LastRegistration (timestamp), LastAttempt (timestamp) and ErrorMessage (string) if last attempt failed. I think that when we delete the descriptor we say that we are not registered anymore, so the Registration field could be optional/removed or maybe not, as timestamp is kept.

Hmm, I'm not sure... Registered in your case is basically the same as checking if Descriptor is not nil. Adding a timestamp for the last registration attempt is a good idea, I'll do that. And I need to add better comments for the existing fields.

How about this, I:

add the field LastAttempt (timestamp)

rename Successful to LastAttemptSuccessful

rename Error to LastAtemptError

This will make it more clear what the new fields actually mean. I think that having the bool and the error separate is still useful for quickly checking if the node has problems (especially when using automation) and once I change the status updating as you've suggested above, there's really no way that the bool and the error could be out of sync :)

Looks good, have no better suggestions. Putting this under last_attempt with fields timestamp, successful and error_message would probably be an overkill.

peternose · 2023-04-28T15:56:29Z

.changelog/5256.feature.md

+under the registration status section:
+
+- `successful` - true if the registration succeeded.
+- `error` - error message if the registration failed.


Needs to be updated.

Thanks, fixed! :)

go/control/api/api.go

go/worker/registration/worker.go

peternose

👌

"registration": {
    "last_attempt_successful": false,
    "last_attempt_error_message": "registry: no such entity",
    "last_attempt": "2023-05-03T11:37:29+02:00",
    "last_registration": "2023-05-03T11:36:37+02:00",
}

peternose · 2023-05-03T09:44:49Z

go/worker/registration/worker.go

-		addrs, err := w.gatherConsensusAddresses(sentryConsensusAddrs)
-		if err != nil {
-			return fmt.Errorf("error gathering consensus addresses: %w", err)
+		addrs, grr := w.gatherConsensusAddresses(sentryConsensusAddrs)


Suggested change

addrs, grr := w.gatherConsensusAddresses(sentryConsensusAddrs)

addrs, err := w.gatherConsensusAddresses(sentryConsensusAddrs)

Linter complains about this, so I'll revert this change :)

peternose · 2023-05-03T09:44:59Z

go/worker/registration/worker.go

@@ -941,28 +965,22 @@ func (w *Worker) registerNode(epoch beacon.EpochTime, hook RegisterNodeHook) err
 		nodeSigners = append([]signature.Signer{w.identity.NodeSigner}, nodeSigners...)
 	}

-	sigNode, err := node.MultiSignNode(nodeSigners, registry.RegisterNodeSignatureContext, &nodeDesc)
-	if err != nil {
+	sigNode, grr := node.MultiSignNode(nodeSigners, registry.RegisterNodeSignatureContext, &nodeDesc)


Suggested change

sigNode, grr := node.MultiSignNode(nodeSigners, registry.RegisterNodeSignatureContext, &nodeDesc)

sigNode, err := node.MultiSignNode(nodeSigners, registry.RegisterNodeSignatureContext, &nodeDesc)

peternose · 2023-05-03T09:53:40Z

go/worker/registration/worker.go

 		w.logger.Error("failed to register node",
 			"err", err,
 		)
 		return err
 	}

-	// Update the registration status on successful registration.
-	w.RLock()


It looks like this has been a bug for 3 years.

abukosek force-pushed the andrej/feature/node-reg-status branch 3 times, most recently from 56faf51 to d243d97 Compare April 26, 2023 13:11

abukosek force-pushed the andrej/feature/node-reg-status branch 2 times, most recently from 0328a4f to 5bab05a Compare April 27, 2023 13:14

abukosek marked this pull request as ready for review April 27, 2023 14:03

abukosek requested review from kostko, peterjgilbert, pro-wh, ptrus and peternose as code owners April 27, 2023 14:03

pro-wh reviewed Apr 27, 2023

View reviewed changes

go/worker/registration/worker.go Outdated Show resolved Hide resolved

peternose requested changes Apr 27, 2023

View reviewed changes

abukosek force-pushed the andrej/feature/node-reg-status branch from 5bab05a to 2922d29 Compare April 28, 2023 15:05

peternose reviewed Apr 28, 2023

View reviewed changes

abukosek force-pushed the andrej/feature/node-reg-status branch 2 times, most recently from 2ea8ba4 to a8edce8 Compare May 3, 2023 07:38

peternose approved these changes May 3, 2023

View reviewed changes

abukosek force-pushed the andrej/feature/node-reg-status branch from a8edce8 to 97d1487 Compare May 3, 2023 10:46

peternose approved these changes May 3, 2023

View reviewed changes

go/control/api: Improve node registration status clarity

b8a572a

abukosek force-pushed the andrej/feature/node-reg-status branch from 97d1487 to b8a572a Compare May 3, 2023 10:50

abukosek merged commit f642890 into master May 3, 2023
3 checks passed

abukosek deleted the andrej/feature/node-reg-status branch May 3, 2023 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go/control/api: Improve node registration status clarity #5256

go/control/api: Improve node registration status clarity #5256

abukosek commented Apr 26, 2023 •

edited

codecov bot commented Apr 26, 2023 •

edited

peternose left a comment

peternose Apr 27, 2023

abukosek Apr 28, 2023

peternose Apr 27, 2023

abukosek Apr 28, 2023

peternose Apr 28, 2023

peternose Apr 27, 2023

abukosek Apr 28, 2023

abukosek Apr 28, 2023

peternose Apr 28, 2023

peternose Apr 28, 2023

abukosek May 3, 2023

peternose left a comment

peternose May 3, 2023

abukosek May 3, 2023

peternose May 3, 2023

peternose May 3, 2023

	// Error contains the error message if the current registration has not been successful.
	// Error contains the error message if the last registration has not been successful.

	Error string `json:"error,omitempty"`
	ErrorMessage string `json:"error_message,omitempty"`

	addrs, grr := w.gatherConsensusAddresses(sentryConsensusAddrs)
	addrs, err := w.gatherConsensusAddresses(sentryConsensusAddrs)

	sigNode, grr := node.MultiSignNode(nodeSigners, registry.RegisterNodeSignatureContext, &nodeDesc)
	sigNode, err := node.MultiSignNode(nodeSigners, registry.RegisterNodeSignatureContext, &nodeDesc)

go/control/api: Improve node registration status clarity #5256

go/control/api: Improve node registration status clarity #5256

Conversation

abukosek commented Apr 26, 2023 • edited

codecov bot commented Apr 26, 2023 • edited

Codecov Report

peternose left a 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

Choose a reason for hiding this comment

peternose left a 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

abukosek commented Apr 26, 2023 •

edited

codecov bot commented Apr 26, 2023 •

edited