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

Improve app configurability via hypervisor and make it persistent #111

Merged
merged 18 commits into from
Jan 22, 2020

Conversation

nkryuchkov
Copy link
Contributor

@nkryuchkov nkryuchkov commented Jan 7, 2020

Fixes #66
Fixes #86
Fixes #121

Depends on https://github.com/SkycoinPro/skywire-services/pull/56

#66

Changes:

  • If the autostart value of an application is changed by calling the PUT /api/visors/{pk}/apps/{app} API endpoint, the new value is restored after stopping the visor and starting it again.

How to test this PR:

  1. Start some nodes using the make integration-run-generic command of the skywire-services repository.
  2. Call GET /api/nodes/024ec47420176680816e0406250e7156465e4531f5b26057c9f6297bb0303558c7. The autostart property of the skychat will be true
  3. Call PUT /api/nodes/024ec47420176680816e0406250e7156465e4531f5b26057c9f6297bb0303558c7/apps/skychat with {autostart: false} as content, to stop the skychat app.
  4. Call GET /api/nodes/024ec47420176680816e0406250e7156465e4531f5b26057c9f6297bb0303558c7 again. The autostart property of the skychat will be false
  5. Call make integration-teardown; tmux kill-server in the command window that is running the skywire-services test enviroment, to stop it. Then call make integration-run-generic to start it again.
  6. Call GET /api/nodes/024ec47420176680816e0406250e7156465e4531f5b26057c9f6297bb0303558c7 again. The autostart property of the skychat will be false.

#86

Changes:

  • Implemented changing skysocks app password and skysocks-client app public key of a server via the PUT /api/visors/{pk}/apps/{app} API endpoint.

How to test this PR:

  1. Make sure https://github.com/SkycoinPro/skywire-services/pull/56 is merged. If not, you may also copy the proxy apps configs to the generic environment and try it instead of the proxy environment.
  2. Start some nodes using the make integration-run-proxy command of the skywire-services repository.
  3. Check skysocks app passcode in integration/proxy/nodeA.json file.
  4. Call PUT /api/nodes/024ec47420176680816e0406250e7156465e4531f5b26057c9f6297bb0303558c7/apps/skysocks with {"passcode": "test"} as content, to update the passcode in the config. This should also restart the skysocks app.
  5. Check if skysocks app passcode in integration/proxy/nodeA.json file is changed to test.
  6. Check the NodeA pane in tmux and make sure that no error happened while restarting skysocks.
  7. Run ps aux | grep skysocks and check if skysocks is running.
  8. Repeat 1-6 for skysocks-client PK (argument is named pk) on node 031b80cd5773143a39d940dc0710b93dcccc262a85108018a7a95ab9af734f8055

#121

Changes:

  • Fixed the bug when visor could not run more than one app
  • Fixed the bug when apps could not be restarted

How to test this PR:

  1. Add the skysocks app config from integration/proxy/nodeA.json to integration/generic/nodeA.json as the second app.
  2. Start some nodes using the make integration-run-generic command of the skywire-services repository.
  3. Check NodeA pane in tmux.
  4. Both apps should be started, the output of ps aux should show them.
  5. Call PUT /api/nodes/024ec47420176680816e0406250e7156465e4531f5b26057c9f6297bb0303558c7/apps/skysocks with {"passcode": "test"} as content, to update the passcode in the config. This should also restart the skysocks app.
  6. Check logs, there should be no error while stopping and starting skysocks, it also should present in the output of ps aux | grep skysocks.

@evanlinjin
Copy link
Contributor

@nkryuchkov This PR looks good. What is the status of it?

@nkryuchkov
Copy link
Contributor Author

@evanlinjin It's implemented. Visor restarts apps to update their config. However, I've found out that restarting apps doesn't work right now, so I'm fixing this.

# Conflicts:
#	cmd/skywire-visor/commands/root.go
#	pkg/visor/visor.go
- Fix app stop bug
- Fix app restart bug
- Fix start of 2+ apps bug
- Improve linter
@nkryuchkov nkryuchkov marked this pull request as ready for review January 18, 2020 13:42
pkg/app/appserver/proc_manager.go Show resolved Hide resolved
pkg/app/appserver/proc_manager.go Show resolved Hide resolved
pkg/visor/visor.go Outdated Show resolved Hide resolved
pkg/visor/visor.go Outdated Show resolved Hide resolved
@jdknives
Copy link
Member

@nkryuchkov great work overall.

  1. For Improve app configurability from Hypervisor #86, I followed instructions and on tmux pane for NodeA I got:
INFO [skywire]: Updated skysocks password, restarting it
INFO [skywire]: Restarting app skysocks
INFO [skywire]: Stopping app skysocks and closing ports
INFO [skywire]: Starting skysocks.v1.0
ERROR[rpc_server_025c7ffea1c9f99328fbfb5a7a38c100bc15cb2272d2c80dc47647c696a762ec17]: Error accepting conn on RPC Accept server side: listening on closed connection
WARN [skywire]: App skysocks stopped working: no such app

Despite these errors, everything worked as expected, so not sure we need to worry about it.

@nkryuchkov
Copy link
Contributor Author

nkryuchkov commented Jan 22, 2020

@jdknives These are expected errors happening because of stopped app and lost connection. However, I think the log level needs to be decreased from error to warning. Fixed.

Copy link
Contributor

@Darkren Darkren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

@nkryuchkov nkryuchkov merged commit 6dd75b9 into skycoin:milestone2 Jan 22, 2020
@nkryuchkov nkryuchkov deleted the feature/persistent-settings branch March 13, 2020 17:38
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

Successfully merging this pull request may close these issues.

4 participants