-
Notifications
You must be signed in to change notification settings - Fork 45
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
Integrate m3 dsmgpty with hypervisor and various fixes. #166
Integrate m3 dsmgpty with hypervisor and various fixes. #166
Conversation
Please add some documentation for @Senyoret1 before merging so that he can integrate these changes with the frontend. |
The following has been commented out: * WIP: Visor endpoints exposed for hypervisor switched to RESTful. * WIP: Various new helper functions for having http over dmsg.
* Updated hypervisor config format. * Added hypervisor /pty/{pk} endpoint. * Added logic to prepare unix file. * Improved logging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
Regarding the services repo, we also need to adjust integration/hypervisor.json
for hypervisor config changes. The script for testing the /exec
endpoint is to be modified.
I'm getting the following error on attempt to test the PR:
[2020-02-27T21:15:59+01:00] INFO [snet.dmsgC]: ClientSession.acceptStream() temporary error, continuing... error="dmsg error 306 - request has no associated listener" session=035915c609f71d0c7df27df85ec698ceca0cb262590a54f732e3bbd0cc68d89282
Did I set up anything wrong?
@nkryuchkov Did you test this with |
* managedTransport.close() now optionally calls mt.wg.Wait() to avoid hangs if called in mt.Serve(). * transport.Manager deletes mTp entry on SaveTransport if mTp has already stopped serving. * Improved various logs. * Added some comments.
@evanlinjin Yes. I'll have a look again, thanks. |
I've tested again. The "interfaces": {
"http_address": ":8080",
"rpc_addr": ":7080"
} we need "http_addr": ":8080" An error related to accessing the removed Testing against #160, got the following:
Reproduced. Is it intended behavior now?
Could not reproduce. It seems to be fixed.
Could not reproduce.
Could not reproduce. |
Note that this PR should pass manual integration tests specified in #152 as well. |
* transport.Manager now deletes from 'tps' when the associated mTp stops serving. * The initiating settlement handshake no longer updates transport discovery status. This logic is now moved to ManagedTransport. * ManagedTransport now has a better mechanism to update statuses. * Only the least-significant edge of a transport can redial the underlying connection.
I think I fixed these. Please retest! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Are these logs supposed to remain on [Info] level:
[2020-03-02T22:53:37+01:00] INFO [router]: Removed 0 rules
They pop up every 2 seconds it seems.
-
Fixes [M2] DMSG transports fail to deliver packets after visor restart #171 in integration env.
-
Does not fix Routes are not being created again #161 for me. But I saw that there is another PR from @Darkren trying to fix that.
Confirm fix of #171 . Both STCP and DMSG transports work with all of my test scenarios |
This comment has been minimized.
This comment has been minimized.
* mTp.redial exponential backoff. * mTp.updateStatuses looks at discovery response codes to see if tp is still registered. * Various logging improvements. * tpDisc.Client: Made errors analyzable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job overall!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
This is meant to be proof that this PR in fact does fix #160, #161 and #171. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, really great job!
httpAddr = config.Interfaces.HTTPAddr | ||
rpcAddr = config.Interfaces.RPCAddr | ||
) | ||
conf := prepareConfig(args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function looks nice! 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall a really great job! All of the issues seem fixed in integration env. Will test in deployment after merge. Only small issues that we may fix:
- The terminal art or previous bash history is disappearing bit by bit when using arrow keys to scroll through history. (Note where the cursor is)
- There does not seem to be an attempt to actually redial transport upon remote shutdown. When starting remote visor though, the transport is initialized properly by the remote, which means there is no problem with overall functionality right now. Also, we still get this error log upon redial from remote after restart.
[2020-03-04T10:52:57+01:00] WARN [tp:031b80]: Failed to read packet. error="EOF" src="read_loop"
[2020-03-04T10:53:09+01:00] INFO [tp_manager]: recv transport connection request: type(dmsg) remote(031b80cd5773143a39d940dc0710b93dcccc262a85108018a7a95ab9af734f8055)
[2020-03-04T10:53:09+01:00] DEBUG [tp_manager]: TP found, accepting...
[2020-03-04T10:53:09+01:00] DEBUG [tp:031b80]: Performing settlement handshake...
[2020-03-04T10:53:09+01:00] ERROR [transport]: Failed to register transports error="(500)Internal Server Error: {"error":"ID already registered"}
Overall however, great work.
Did you run
make format && make check
?Changes (closes #155):
RPCClientDialer
logic to not return on hypervisor disconnect.How to test this:
skywire-services@feature/m3-dmsgpty-hypervisor
replace
statements ingo.mod
forskywire-services
andskywire-mainnet
repos.Changes (fixes #160, fixes #161, fixes #171):
managedTransport.close()
now optionally callsmt.wg.Wait()
to avoid hangs if called inmt.Serve()
.transport.Manager
deletesmTp
entry onSaveTransport
ifmTp
has already stopped serving.How to test this:
skywire-services@feature/m3-dmsgpty-hypervisor
replace
statements ingo.mod
forskywire-services
andskywire-mainnet
repos.Changes (closes #68):