[Merged by Bors] - Use OS file locks in validator client by michaelsproul · Pull Request #1958 · sigp/lighthouse

michaelsproul · 2020-11-24T11:52:16Z

Issue Addressed

Closes #1823

Proposed Changes

Use OS-level file locking for validator keystores, eliminating problems with lockfiles lingering after ungraceful shutdowns (SIGKILL, power outage). I'm using the fs2 crate because it's cross-platform (unlike file-lock), and it seems to have the most downloads on crates.io.
Deprecate + disable --delete-lockfiles CLI param, it's no longer necessary
Delete the validator_dir::Manager, as it was mostly dead code and was only used in the validator list command, which has been rewritten to read the validator definitions YAML instead.

Additional Info

Tested on:

michaelsproul · 2020-11-25T04:02:49Z

Confirmed working on macOS

blacktemplar

Only one small comment/question, otherwise lgtm.

common/lockfile/src/lib.rs

pawanjay176

LGTM! just a minor question

common/lockfile/src/lib.rs

michaelsproul · 2020-11-26T01:05:30Z

Ready for merging IMO. Will test Windows support once that's closer to being supported in the rest of Lighthouse.

validator_client/src/initialized_validators.rs

account_manager/src/validator/list.rs

michaelsproul · 2020-11-26T05:26:14Z

I've added a warning about existing keystores and cleaned up the ValidatorDir locks a little more.

Just planning to test under Docker (on macOS and Linux) as per @ajsutton's recommendation (thanks!) then should be good to go.

paulhauner

Great! This will make users very happy, I think.

michaelsproul · 2020-11-26T06:37:02Z

OK, tested on Docker Linux and Docker macOS. Hopefully CI succeeds with that typo fixed, so...

bors r+

## Issue Addressed Closes #1823 ## Proposed Changes * Use OS-level file locking for validator keystores, eliminating problems with lockfiles lingering after ungraceful shutdowns (`SIGKILL`, power outage). I'm using the `fs2` crate because it's cross-platform (unlike `file-lock`), and it seems to have the most downloads on crates.io. * Deprecate + disable `--delete-lockfiles` CLI param, it's no longer necessary * Delete the `validator_dir::Manager`, as it was mostly dead code and was only used in the `validator list` command, which has been rewritten to read the validator definitions YAML instead. ## Additional Info Tested on: - [x] Linux - [x] macOS - [x] Docker Linux - [x] Docker macOS - [ ] Windows

bors · 2020-11-26T07:53:14Z

Build failed:

release-tests-ubuntu

michaelsproul · 2020-11-26T10:13:52Z

I'm feeling optimistic about that fix:

bors r+

## Issue Addressed Closes #1823 ## Proposed Changes * Use OS-level file locking for validator keystores, eliminating problems with lockfiles lingering after ungraceful shutdowns (`SIGKILL`, power outage). I'm using the `fs2` crate because it's cross-platform (unlike `file-lock`), and it seems to have the most downloads on crates.io. * Deprecate + disable `--delete-lockfiles` CLI param, it's no longer necessary * Delete the `validator_dir::Manager`, as it was mostly dead code and was only used in the `validator list` command, which has been rewritten to read the validator definitions YAML instead. ## Additional Info Tested on: - [x] Linux - [x] macOS - [x] Docker Linux - [x] Docker macOS - [ ] Windows

michaelsproul · 2020-11-26T10:18:58Z

Cancelled the regular build to let Bors handle it

paulhauner · 2020-11-26T10:28:37Z

a8b187b looks good to me.

michaelsproul · 2020-11-26T11:25:28Z

Damn it, Bors crashed :(

{:timeout,
 {GenServer, :call,
  [
    BorsNG.GitHub,
    {:post_commit_status,
     {{:installation, 10233225}, 139952333},
     {"a8b187bb4c899dea0f56559a98e15d21bff8945d", :running,
      "Running",
      "https://app.bors.tech/repositories/26362/log#batch-111416"}},
    10000
  ]}}

Time to try again:

bors r-
bors r+

## Issue Addressed Closes #1823 ## Proposed Changes * Use OS-level file locking for validator keystores, eliminating problems with lockfiles lingering after ungraceful shutdowns (`SIGKILL`, power outage). I'm using the `fs2` crate because it's cross-platform (unlike `file-lock`), and it seems to have the most downloads on crates.io. * Deprecate + disable `--delete-lockfiles` CLI param, it's no longer necessary * Delete the `validator_dir::Manager`, as it was mostly dead code and was only used in the `validator list` command, which has been rewritten to read the validator definitions YAML instead. ## Additional Info Tested on: - [x] Linux - [x] macOS - [x] Docker Linux - [x] Docker macOS - [ ] Windows

bors · 2020-11-26T12:34:23Z

Pull request successfully merged into unstable.

Build succeeded:

michaelsproul added work-in-progress PR is a work-in-progress A0 labels Nov 24, 2020

michaelsproul added 2 commits November 25, 2020 14:20

Use new lockfile in validator client

9263065

Use lockfile in validator dir

f116a53

michaelsproul force-pushed the file-locks branch from 29d0a20 to f116a53 Compare November 25, 2020 03:21

Use new lockfiles in wallet

fd34a33

michaelsproul marked this pull request as ready for review November 25, 2020 05:56

michaelsproul added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Nov 25, 2020

Update doc comment

703999b

blacktemplar reviewed Nov 25, 2020

View reviewed changes

common/lockfile/src/lib.rs Show resolved Hide resolved

pawanjay176 mentioned this pull request Nov 25, 2020

Validator fails to start after hard reboot due to json.lock files #1973

Closed

pawanjay176 approved these changes Nov 25, 2020

View reviewed changes

common/lockfile/src/lib.rs Outdated Show resolved Hide resolved

Add lockfile tests

36cb7de

paulhauner reviewed Nov 26, 2020

View reviewed changes

validator_client/src/initialized_validators.rs Show resolved Hide resolved

paulhauner reviewed Nov 26, 2020

View reviewed changes

account_manager/src/validator/list.rs Show resolved Hide resolved

michaelsproul mentioned this pull request Nov 26, 2020

Clean up ValidatorDir lockfiles #1978

Open

Warn on existing lockfile, remove .lock

e1931b1

paulhauner approved these changes Nov 26, 2020

View reviewed changes

michaelsproul added 2 commits November 26, 2020 17:29

Fix dockerignore

ba692e9

Fix join error in validator dir

5b8eb3d

Fix lockfile handling in VC API

a8b187b

bors bot changed the title ~~Use OS file locks in validator client~~ [Merged by Bors] - Use OS file locks in validator client Nov 26, 2020

bors bot closed this Nov 26, 2020

michaelsproul deleted the file-locks branch December 1, 2020 01:21

michaelsproul mentioned this pull request Jun 16, 2021

Add file locking for validator_definitions.yml #2409

Open

Conversation

michaelsproul commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

michaelsproul commented Nov 25, 2020

Uh oh!

blacktemplar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pawanjay176 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelsproul commented Nov 26, 2020

Uh oh!

Uh oh!

Uh oh!

michaelsproul commented Nov 26, 2020

Uh oh!

paulhauner left a comment

Choose a reason for hiding this comment

Uh oh!

michaelsproul commented Nov 26, 2020

Uh oh!

bors bot commented Nov 26, 2020

Uh oh!

michaelsproul commented Nov 26, 2020

Uh oh!

michaelsproul commented Nov 26, 2020

Uh oh!

paulhauner commented Nov 26, 2020

Uh oh!

michaelsproul commented Nov 26, 2020

Uh oh!

bors bot commented Nov 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

michaelsproul commented Nov 24, 2020 •

edited

Loading