It concerns me that we have failure_reason and failed as separate fields set at different sites and conditions because it creates opportunities for inconsistent state.

Would it be possible to design an interface for updating these fields in unison? Or even, have an std::optional<string> failure_reason where we could then say bool failed() { return failure_reason.has_value(); }?

Maybe we could deduce the info from the error code - another reason to make sure that we return meaningful codes. If that were the case, we could save space because the string could be constructed from the error code at the recipient side.

If we have to share an error code for different internal reasons, we could still do something about it, like passing around a pair of the rc and the error string, or the rc and the detail code representing some error string. In either case, we could then treat the pair as an atomic result and status that would be passed into the _inc_failed_count function to unambiguously encode the state into the PeerReplayer object

UPD: Looking into this further, I realize that failed could be false while some failures could have been seen, as long as they are fewer than max_failures. That explains how we could have the fields out of sync, so failure_reason.has_value() is not a valid suggestion.

However, there is still a point about having the int error code stored instead of a formatted string, and a new point about naming: the reason field should probably be prefixed with last_ just like the boost::optional<monotime> last_failed. That would not allow for the misinterpretation of how the fields were related I had above.

I could make it a struct (union?) or make it optional, as you've suggested.

Regarding the error code: there's no error here, maybe I'll rename the variable error_str if it causes confusion. This is a special case when ceph_get_snap_info succeeds and then found an invalid metadata on the remote fs. This patch provides additional information in the peer status command output, which we've already logged.

Regarding the error code: there's no error here

There's EINVAL we return for the condition.

maybe I'll rename the variable error_str if it causes confusion.

It should be last_error_str then, to denote that it's just the last error we've encountered, which may not have been fatal. Or, it could be last_error_code: int and then you could set it in _inc_failed_count by passing the non-zero rc that you test for before calling the function.
You could also generate the error string there but only pass the code in, so you won't need a struct (a pair).

Note: union won't work.

Regarding the error code: there's no error here

There's EINVAL we return for the condition.

How does that help? The peer_status() is not implemented by checking the return value sent by build_snap_map. Also, EINVAL is not a unique error code for 'invalid metadata', it is returned at other places too. You need to check the code and understand how it works.

maybe I'll rename the variable error_str if it causes confusion.

It should be last_error_str then, to denote that it's just the last error we've encountered, which may not have been fatal.

There's no error, just a sync failure (please check the code, understand the difference). I've updated them to invalid_metadata_str and last_failed_reason, which should be more appropriate.

Or, it could be last_error_code: int and then you could set it in _inc_failed_count by passing the non-zero rc that you test for before calling the function. You could also generate the error string there but only pass the code in, so you won't need a struct (a pair).

Note: union won't work.

_inc_failed_count is not specific to 'invalid metadata' errors. The failures are incremented for what ever error comes out of do_sync_snaps(). Also EINVAL is not a unique error code for 'invalid metadata', it's returned for other errors too. The suggested change should make do_sync_snaps() return unique error codes for every error it encounters, so that _inc_failed_count() could set the values. Otherwise, it would display/log invalid metadata for all kind of failures.
It's a broader change, but it's out of scope of this PR. I'd stick with the existing implementation instead.

The last edit looks better with the last_failed_reason, thank you!

The whole point of my comment was to keep related fields coherent: we update the reason in one place, and the timestamp in another, and we use the error code to control the flow. With the new naming, it's easier to see related fields, that's an improvement. A function to set both the timestamp and the reason together could be another step forward, as would be checking for has_value on either of the fields to couple the flows that set failures and those that act upon them (i.e. _inc_failed_count)

Having that said, I'm happy to close the thread about the failure reason and how we store it, thanks for discussing it with me. More on the functional side, shouldn't we now update that failure reason in other error flows? Like the failure to fetch the snapinfo, or the failure to find PRIMARY_SNAP_ID_KEY?

Closing in favour of #56816