Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add shutdown handler for new launcher #1314

Closed
rvl opened this issue Jan 29, 2020 · 7 comments
Closed

Add shutdown handler for new launcher #1314

rvl opened this issue Jan 29, 2020 · 7 comments
Assignees

Comments

@rvl
Copy link
Contributor

rvl commented Jan 29, 2020

Context

Part of ADP-92 (new launcher).

User story

As an API client (Javascript)
I want a method to cleanly and reliably stop cardano-wallet (and its node)
so that I can exit my application and release resources allocated to it.

Decision

Implement the cardano-wallet side of the launcher shutdown process.

DaedalusIPC can stay for the time being, but will be deleted later.

Acceptance Criteria

  • The shutdown handler must work on both Windows and POSIX.

Development

QA

  1. Unit tests in Cardano.StartupSpec
  2. Integration test in .../Scenario/CLI/Server.hs.
@rvl rvl self-assigned this Jan 29, 2020
iohk-bors bot added a commit that referenced this issue Feb 17, 2020
1315: Add shutdown handler for new launcher r=KtorZ a=rvl

Relates to #1314.

# Overview

This adds a simpler way of ensuring clean shutdown of the wallet on windows (and linux), which doesn't require DaedalusIPC. The mechanism will be used by cardano-launcher.

- Adds the shutdown handler thread.
- Needed to rearrange startup functions around a little bit.
- Unit tests and an integration test.

# Comments

[Hydra jobset](https://hydra.iohk.io/jobset/Cardano/cardano-wallet-pr-1315)


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
iohk-bors bot added a commit that referenced this issue Feb 26, 2020
1315: Add shutdown handler for new launcher r=KtorZ a=rvl

Relates to #1314.

# Overview

This adds a simpler way of ensuring clean shutdown of the wallet on windows (and linux), which doesn't require DaedalusIPC. The mechanism will be used by cardano-launcher.

- Adds the shutdown handler thread.
- Needed to rearrange startup functions around a little bit.
- Unit tests and an integration test.

# Comments

[Hydra jobset](https://hydra.iohk.io/jobset/Cardano/cardano-wallet-pr-1315)


1321: Add CLI command for extracting root xprvs r=Anviking a=Anviking

# Issue Number

#1316 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have added `cardano-wallet-jormungandr key root --type random <mnemonic words>`
- [x] I added unit tests for help-text and actual usage.
- [x] I added a *pending* test making sure keys are compatible with jcli (which would fail)

# Comments

```bash
$ cardano-wallet-jormungandr key --help
Usage: cardano-wallet-jormungandr key COMMAND
  Derive keys from mnemonics.

Available options:
  -h,--help                Show this help text

Available commands:
  root                     Extract root xprv as hex (64 bytes private key + 32
                           bytes chain code)
$ cardano-wallet-jormungandr key root --help
Usage: cardano-wallet-jormungandr key root --type KEYTYPE MNEMONIC_WORDS...
  Extract root xprv as hex (64 bytes private key + 32 bytes chain code)

Available options:
  -h,--help                Show this help text
  --type KEYTYPE           Any of the following:
                             random (Daedalus, 12 words)
                             icarus (15 words)
                             trezor (12, 15, 18, 21, or 24 words)
                             ledger (12, 15, 18, 21, or 24 words)
$ cardano-wallet-jormungandr key root --type random flock advance execute country leader exotic mix twenty six margin orient meat
68a0f29e6bd5d8af7ffd00a55006afa8af6fbdbded07984ddf7fb1c31c66f7460685e5d1016553fccc9724f5ee95dd8d66facd2ac1bb2f6fcd7fa5e53c97a57f50c592fcd18b67bf3393a16184d009fb25450b2de8079f870222874e804584a8
```

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
iohk-bors bot added a commit that referenced this issue Feb 26, 2020
1315: Add shutdown handler for new launcher r=KtorZ a=rvl

Relates to #1314.

# Overview

This adds a simpler way of ensuring clean shutdown of the wallet on windows (and linux), which doesn't require DaedalusIPC. The mechanism will be used by cardano-launcher.

- Adds the shutdown handler thread.
- Needed to rearrange startup functions around a little bit.
- Unit tests and an integration test.

# Comments

[Hydra jobset](https://hydra.iohk.io/jobset/Cardano/cardano-wallet-pr-1315)


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this issue Feb 27, 2020
1315: Add shutdown handler for new launcher r=KtorZ a=rvl

Relates to #1314.

# Overview

This adds a simpler way of ensuring clean shutdown of the wallet on windows (and linux), which doesn't require DaedalusIPC. The mechanism will be used by cardano-launcher.

- Adds the shutdown handler thread.
- Needed to rearrange startup functions around a little bit.
- Unit tests and an integration test.

# Comments

[Hydra jobset](https://hydra.iohk.io/jobset/Cardano/cardano-wallet-pr-1315)


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this issue Feb 27, 2020
1315: Add shutdown handler for new launcher r=KtorZ a=rvl

Relates to #1314.

# Overview

This adds a simpler way of ensuring clean shutdown of the wallet on windows (and linux), which doesn't require DaedalusIPC. The mechanism will be used by cardano-launcher.

- Adds the shutdown handler thread.
- Needed to rearrange startup functions around a little bit.
- Unit tests and an integration test.

# Comments

[Hydra jobset](https://hydra.iohk.io/jobset/Cardano/cardano-wallet-pr-1315)


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
@piotr-iohk
Copy link
Contributor

hm, one of the integration tests fails for me locally. (btw. thanks for handy PATATE handler ;) https://github.com/input-output-hk/cardano-wallet/blob/master/lib/jormungandr/test/integration/Test/Integration/Jormungandr/Scenario/CLI/Server.hs#L72)

Failures:

  test/integration/Test/Integration/Jormungandr/Scenario/CLI/Server.hs:162:17: 
  1) CLI Specifications, PATATE, LOGGING - cardano-wallet serve logging [SERIAL], LOGGING - Serve debug logs for one component
       not expected: 0

  To rerun use: --match "/CLI Specifications/PATATE/LOGGING - cardano-wallet serve logging [SERIAL]/LOGGING - Serve debug logs for one component/"

Randomized with seed 831114914

Finished in 3.8773 seconds
13 examples, 1 failure

@KtorZ
Copy link
Member

KtorZ commented Mar 3, 2020

Also fails for me locally for a reason I can't explain, but does not on CI 🤔

@piotr-iohk
Copy link
Contributor

@rvl, @KtorZ:

1. I want to start cardano-wallet (and its node)
2. I want to cardano-wallet to use a free TCP port for its server
3. I want to receive events when the wallet backend is started/stopped
4. I want to receive events when the node is started/stopped
5. I want to receive an event when the wallet API is ready
6. I want a method to get the wallet backend API base URL and connection parameters
7. I want a method to cleanly and reliably stop cardano-wallet (and its node)
8. I want a documented Javascript module with typed interface files
9. I want the cardano-launcher to be well-tested on Windows
10. I want the module to output detailed logging

Does this ticket cover all points or part (which part if so)?

  • Also in particular re pt. 9 from AC - do you recommend any manual tests to perform under Windows (or any further manual or automated tests) to be performed or added

@rvl
Copy link
Contributor Author

rvl commented Mar 9, 2020

Sorry @piotr-iohk I lost track of your message.

Yes the failure is due to withShutdownHandler and I am working on fixing it.

Those items from ADP-92 are user stories and each has its own AC. It's a bit difficult to track because the user stories are implemented in two git repos - cardano-launcher and cardano-wallet.

This ticket covers "7. I want a method to cleanly and reliably stop cardano-wallet (and its node)"

Regarding point 9 from the user stories - there are some automated tests for windows in the cardano-launcher. The end-to-end test is still pending on IntersectMBO/cardano-launcher#12.

iohk-bors bot added a commit that referenced this issue Mar 10, 2020
1420: Fix test failure by making clean shutdown handler optional r=rvl a=rvl

# Issue Number

#1314 

# Overview

- Fixes integration test failure which occurs locally - #1314 (comment)


1422: Disable running integration tests on Hydra CI r=rvl a=rvl

# Overview

Reduces the load on Hydra by not running integration tests for PR builds. The test executables are still built however.

This uses the `pr` input which was added to [ci-ops/jobsets/default.nix](https://github.com/input-output-hk/ci-ops/blob/30d4685beded3e54caff4a22718aa6acde48d367/jobsets/default.nix#L326).

# Comments

Tested with:

    rodney@blue:~/iohk/cardano-wallet % nix-build --argstr pr 1234 release.nix -A native.checks.cardano-wallet-jormungandr.integration.x86_64-linux
    error: attribute 'integration' in selection path 'native.checks.cardano-wallet-jormungandr.integration.x86_64-linux' not found

    rodney@blue:~/iohk/cardano-wallet % nix-build release.nix -A native.checks.cardano-wallet-jormungandr.integration.x86_64-linux
    ...

[Hydra jobset](https://hydra.iohk.io/jobset/Cardano/cardano-wallet-pr-1422#tabs-jobs)

Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
@piotr-iohk
Copy link
Contributor

Ok, the failing test is fixed but there is now also this opened PR related to this task #1424. (Not sure than, shall we move it back to "in progress" or...?)

@rvl
Copy link
Contributor Author

rvl commented Mar 13, 2020

I don't know ... the code removal is not part of the epic, it's just a clean up which I wanted to do early because its tests are fragile. I'm not sure when to make a github issue and when not, because we have jira there, and also multiple github repos, ...

iohk-bors bot added a commit that referenced this issue Mar 13, 2020
1437: Add cardano-wallet-byron serve --shutdown-handler option r=rvl a=rvl

### Issue Number

Relates to #1314.

### Overview

Adds the same flag that cardano-wallet-jormungandr has.

Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
@piotr-iohk
Copy link
Contributor

All right. It seems the original scope for this particular GH task is addressed, so I will close this one.
Then maybe if there are further parts related to ADP-92 that need QA review, let's add them as separate GH tasks moving forward.

iohk-bors bot added a commit that referenced this issue Apr 1, 2020
1424: Remove DaedalusIPC r=rvl a=rvl

# Issue Number

Relates to [ADP-92](https://jira.iohk.io/browse/ADP-92).

# Overview

The new [cardano-launcher](https://github.com/input-output-hk/cardano-launcher) code pre-allocates TCP ports for the jormungandr backend, uses unix sockets/named pipes for the byron backend, and has a simpler method of ensuring clean shutdown of the cardano-wallet server on Windows (#1314).

# Comments

Can be merged once cardano-launcher integration into Daedalus is complete.


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants