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

Fix vpn status #1208

Merged
merged 2 commits into from
May 17, 2022
Merged

Fix vpn status #1208

merged 2 commits into from
May 17, 2022

Conversation

ersonp
Copy link
Contributor

@ersonp ersonp commented May 17, 2022

Did you run make format && make check?
yes

Fixes #1205

Changes:

  • Added App in API interface
  • Updated AppState and AppStates methods of Launcher
  • Fixed putApp method of Hypervisor

How to test this PR:

  1. Start visor ./skywire-visor -c skywire-config.json
  2. Open postman and send a PUT request to http://localhost:8000/api/visors/<visor-pk>/apps/vpn-client with body
    {"status":1}
    
  3. Check response

ersonp added 2 commits May 17, 2022 13:05
This commit updates the Launcher struct's method AppState by moving all the appstate logic into it and using the same method inside AppStates to get the app states of all apps instead of having the same code in two places.
This commit fixes the response of putApp where first the respose used to be disconnected but now we set the appstate as connected in the method itself and then retrive the latest appstate and send that as the response.
@@ -174,39 +174,30 @@ func (l *Launcher) AppState(name string) (*AppState, bool) {
return nil, false
}
state := &AppState{AppConfig: ac, Status: AppStatusStopped}
if _, ok := l.procM.ErrorByName(ac.Name); ok { //nolint:errcheck
state.Status = AppStatusErrored
if err, ok := l.procM.ErrorByName(ac.Name); ok { //nolint:errcheck
Copy link
Member

Choose a reason for hiding this comment

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

Not implemented by you, but would it be more elegant to actually return an error here instead of a string and bool?

Copy link
Member

Choose a reason for hiding this comment

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

Do we need the //nolint:errcheck here still?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

//nolint:errcheck can be taken out. l.procM.ErrorByName(ac.Name) was actually created by me, and I think there was some trouble sending the error through RPC, I will check it once again if it's possible.

@@ -710,7 +716,18 @@ func (hv *Hypervisor) putApp() http.HandlerFunc {
}
}

httputil.WriteJSON(w, r, http.StatusOK, ctx.App)
if err := ctx.API.SetAppDetailedStatus(ctx.App.Name, vpn.ClientStatusConnecting); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we setting the status for the app from the hypervisor? Sounds counterintuitive to me that the frontend would be able to manipulate the state of the application thats running directly. It should be able to call certain actions and then based on whether or not they succeed, the app changes its state, which the frontend then can poll.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a small delay when we send back the response of the API putApp and the app starting and changing the the detailed_status by itself. That used to cause the vpn-ui to show disconnecting first and then connecting after we started it.
Another way would be to send a starting detailed status regardless of the apps state and it will update to the correct status after a while. But that makes the other apps appear slow so I went with this approach.

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