Add POSIX mirror lifecycle outline & witness support#989
Conversation
AlCutter
force-pushed
the
posix_mirror_outline_witness
branch
from
ebdf08d to
34b51b2
Compare
AlCutter
force-pushed
the
posix_mirror_outline_witness
branch
2 times, most recently
from
d5fe3cf to
96f5aab
Compare
AlCutter
force-pushed
the
posix_mirror_outline_witness
branch
from
96f5aab to
dd9d61a
Compare
| // Oh dear, panic Mr Mainwaring! | ||
| panic(fmt.Errorf("failed to unlock tree state: %v", err)) | ||
| } | ||
| m.s.mu.Unlock() |
There was a problem hiding this comment.
There is a chance of mutex deadlock. The mutex unlock will not trigger when the err is not nil on L1151.
Defer the mutex unlock immediately after m.s.mu.Lock().
There was a problem hiding this comment.
Good catch!
I've done two things:
- changed all "double lock" locations to defer the unlocks separately (because that's a more correct pattern)
- changed this location to
panic()on failure to lock to match all the others - this also "fixes" the deadlock issue, but makes behaviour consistent across lifecycles (we can later decide if we want to modify the behaviour).
| r, err := os.ReadFile(path) | ||
| if err != nil { | ||
| slog.ErrorContext(ctx, "Failed to read witness cosigner file", slog.String("path", *witnessSignerPath), slog.Any("error", err)) | ||
| slog.ErrorContext(ctx, "Failed to read cosigner file", slog.String("path", path), slog.Any("error", err)) |
There was a problem hiding this comment.
I'm not too familiar with the terminology. Should we say signer instead of cosigner in the error log in this function?
There was a problem hiding this comment.
It's a bit dependent on context, sadly - these are cosigners from the PoV of tlog-* and MTC spec PoV, and the type of key these are expected to be are Cosignature/v1, but from the note library PoV they're note.Signers.
I've renamed anything to do with them as ...Cosigner... in this file for consistency, as the operator is more likely to be familiar with.
| // with the `tlog-mirror` spec, allowing the root of the storage directory to be exported directly to read-only clients. | ||
| func newMirrorTarget(ctx context.Context, w *witness.Witness, origin string, mirrorSigner note.Signer) (*tessera.MirrorTarget, error) { | ||
| targetDir := filepath.Join(*storageDir, url.PathEscape(origin)) | ||
| if err := os.MkdirAll(targetDir, 0o700); err != nil && !errors.Is(err, os.ErrExist) { |
There was a problem hiding this comment.
It should be safe to remove !errors.Is(err, os.ErrExist) as no error will be returned if the directory is already created.
If path is already a directory, MkdirAll does nothing and returns nil.
| // with the `tlog-mirror` spec, allowing the root of the storage directory to be exported directly to read-only clients. | ||
| func newMirrorTarget(ctx context.Context, w *witness.Witness, origin string, mirrorSigner note.Signer) (*tessera.MirrorTarget, error) { | ||
| targetDir := filepath.Join(*storageDir, url.PathEscape(origin)) | ||
| if err := os.MkdirAll(targetDir, 0o700); err != nil && !errors.Is(err, os.ErrExist) { |
There was a problem hiding this comment.
0o700 seems to be contradicted to the godoc comment "allowing the root of the storage directory to be exported directly to read-only clients".
2 participants
This PR adds an skeleton POSIX storage implementation for the mirror lifecycle, and connects it up to the POSIX mirror binary.
Towards #945