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

Appserver improvements. #357

Merged
merged 19 commits into from
May 15, 2020
Merged

Conversation

evanlinjin
Copy link
Contributor

@evanlinjin evanlinjin commented May 1, 2020

Fixes #338

Changes:

  • Combined the appserver.Server and appserver.ProcManager concepts.
  • Have one rpc.Server per app proc.
  • Simplified app.Client initialization.
    • Visor to App initialization is done via a single env defined for the app PROC_CONFIG which contains a JSON object.
    • App to Visor initialization is done by App connecting the Visor via app server address. The first 16 bytes that the App sends to Visor is the appcommon.ProcKey.
    • Changed appcommon.ProcKey format so that it is a deterministic length.
  • Improved app logging and removed any possibilities of log database race conditions with bbolt.
  • Various logging improvements.

志宇 added 2 commits May 1, 2020 23:15
* Combined the appserver.Server and appserver.ProcManager concepts.

* Have one rpc.Server per proc.

* Various refactoring and renamings.
@evanlinjin evanlinjin changed the title WIP: appserver improvements WIP: appserver improvements. May 1, 2020
@evanlinjin evanlinjin changed the title WIP: appserver improvements. Appserver improvements. May 6, 2020
@evanlinjin evanlinjin marked this pull request as ready for review May 6, 2020 12:35
@jdknives jdknives requested review from Darkren and nkryuchkov May 7, 2020 11:43
Copy link
Contributor

@nkryuchkov nkryuchkov 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!

cmd/apps/helloworld/helloworld.go Outdated Show resolved Hide resolved
cmd/apps/skychat/chat.go Outdated Show resolved Hide resolved
cmd/apps/skychat/chat.go Show resolved Hide resolved
cmd/apps/skysocks-client/skysocks-client.go Outdated Show resolved Hide resolved
cmd/apps/vpn-client/vpn-client.go Outdated Show resolved Hide resolved
internal/vpn/server.go Outdated Show resolved Hide resolved
pkg/app/appcommon/log_store.go Outdated Show resolved Hide resolved
pkg/app/appserver/proc.go Show resolved Hide resolved
pkg/app/client_test.go Outdated Show resolved Hide resolved
pkg/hypervisor/hypervisor.go Outdated Show resolved Hide resolved
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.

Just a few minor comments, overall - great job! btw, I think we're mixing app and proc terms here. probably we should consider moving on to just a single one of it as it may get confusing at some point

func (c *ProcConfig) encodeJSON() []byte {
b, err := json.Marshal(c)
if err != nil {
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about getting rid of panic and just returning an error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like panic is suited here because json.Marshal will only return error if the ProcConfig definition is problematic (which should never happen).

pkg/app/appdisc/const.go Outdated Show resolved Hide resolved
@evanlinjin
Copy link
Contributor Author

Just a few minor comments, overall - great job! btw, I think we're mixing app and proc terms here. probably we should consider moving on to just a single one of it as it may get confusing at some point

As mentioned on Discord:

Proc is an instance of an app
My app2 specs stated to have the ability to run multiple instances of a single app
But seems we were rushed so I didn't push for that feature
The current layout edges us closes to that vision (hopefully).

@evanlinjin
Copy link
Contributor Author

https://discordapp.com/channels/466450852098605059/552769767904116737/709679913309044759

@Darkren wrote:

@evanlinjin#8888 I see thank you. could you please just update the comment for Proc struct to mention this? it's now just a wrapper for skywire app

志宇 added 6 commits May 15, 2020 17:21
* Separated app-associated functionality within /pkg/visor into /pkg/app/launcher.
* Added the concept of visor.BaseConfig() to simplify obtaining default values.
* Made startup/shutdown logic modular and introduced init and shutdown stacks.
* Shutdown now uses module-based timeouts and wait groups.
Improve visor config and startup/shutdown logic.
@evanlinjin evanlinjin merged commit 6c4c0d5 into develop May 15, 2020
@jdknives jdknives deleted the feature/appserver-improvements-#338 branch June 15, 2020 17:35
jdknives pushed a commit that referenced this pull request Oct 19, 2020
Appserver improvements:
* Combined the appserver.Server and appserver.ProcManager concepts.
* Have one rpc.Server per app proc.
* Simplified app.Client initialization.
* Visor to App initialization is done via a single env defined for the app PROC_CONFIG which contains a JSON object.
* App to Visor initialization is done by App connecting the Visor via app server address. The first 16 bytes that the App sends to Visor is the appcommon.ProcKey.
* Changed appcommon.ProcKey format so that it is a deterministic length.
* Improved app logging and removed any possibilities of log database race conditions with bbolt.
* Various logging improvements.

App management, visor config, startup and shutdown improvements:
* Separated app-associated functionality within /pkg/visor into /pkg/app/launcher.
* Added the concept of visor.BaseConfig() to simplify obtaining default values.
* Made startup/shutdown logic modular and introduced init and shutdown stacks.
* Shutdown now uses module-based timeouts and wait groups.

Changes as suggested by @Darkren and @nkryuchkov

Former-commit-id: 6c4c0d5
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.

3 participants