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

Move towards a more modular connection manager #385

Merged
merged 5 commits into from Oct 15, 2018

Conversation

Projects
None yet
4 participants
@vkuznecovas
Copy link
Contributor

commented Sep 19, 2018

Looking for feedback for this one.

updates #382

@vkuznecovas vkuznecovas requested review from tadovas and Waldz as code owners Sep 19, 2018

@vkuznecovas vkuznecovas requested review from zolia and soffokl Sep 19, 2018

@@ -89,14 +88,14 @@ func (manager *connectionManager) Connect(consumerID, providerID identity.Identi
}
}()

err = manager.startConnection(consumerID, providerID, options)
err = manager.start(consumerID, providerID, options)

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 19, 2018

Member

Start what? because manager is already running.
manager.start() -> manager.startConnection() or manager.createConnection()

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 19, 2018

Member

Maybe wo dont need 2 manager functions for start and startConnection and we can merge them?

// ConnectOptions holds connect options
type ConnectOptions struct {
// kill switch option restricting communication only through VPN
DisableKillSwitch bool
}

// Params represents the params we need to ensure a successful connection
type Params struct {

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 19, 2018

Member

Struct looks like a duplicate of ConnectOptions.

A. Suggest to merge to ConnectOptions

B. Maybe:

  • Params -> ConnectOptions
  • ConnectOptions -> ConnectionParams -> represents plugin specific params

// Params represents the params we need to ensure a successful connection
type Params struct {
DisableKillSwitch bool

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 19, 2018

Member

Same param

type VpnClientCreator func(session.SessionDto, identity.Identity, identity.Identity, state.Callback, ConnectOptions) (openvpn.Process, error)
// Connection represents a connection
type Connection interface {
Start() error

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 19, 2018

Member

Extra line

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 20, 2018

Author Contributor

not sure what you mean by that, what extra line?

}

//this is the last state - close channel (according to best practices of go - channel writer controls channel)
if openvpnState == openvpn.ProcessExited {

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 19, 2018

Member

tests should not know about openvpn also

Show resolved Hide resolved core/connection/openvpn.go Outdated
close(channel)
}
// OpenVpnStateCallbackToConnectionState maps openvpn.State to connection.State. Returns a pointer to connection.state, or nil
func OpenVpnStateCallbackToConnectionState(input openvpn.State) *State {

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 20, 2018

Member

Should we really return a pointer here? Returning value can make the code more readable:

        switch input {
        case openvpn.ConnectedState:
                return Connected
        case openvpn.ExitingState:
		return Disconnecting
	case openvpn.ReconnectingState:
		return Reconnecting
        default:
                return Unknown
	}

And we can introduce Unknown state to avoid nil

// ErrOpenvpnProcessDied indicates that Connect method didn't reach "Connected" phase due to openvpn error
ErrOpenvpnProcessDied = errors.New("openvpn process died")
// ErrConnectionDied indicates that Connect method didn't reach "Connected" phase due to connection error
ErrConnectionDied = errors.New("connection has died")

This comment has been minimized.

Copy link
@zolia

zolia Sep 20, 2018

Member

Would use ConnectionFailed, since Died would imply that it was alive in the first place.

This comment has been minimized.

Copy link
@zolia

zolia Sep 20, 2018

Member

ConnectionFailed

better ConnectingFailed

@vkuznecovas vkuznecovas force-pushed the vkuznecovas:abstract-connection branch from f0fd72f to 9db3def Sep 20, 2018

@vkuznecovas vkuznecovas changed the title WIP - Move towards a more modular connection manager Move towards a more modular connection manager Sep 20, 2018


// VpnConnectionCreator creates new vpn client by given session,
// consumer identity, provider identity and uses state channel to report state changes
type VpnConnectionCreator func(ConnectOptions, StateChannel) (Connection, error)

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 20, 2018

Member

Where should be no VPN work in core/connection package

@vkuznecovas vkuznecovas force-pushed the vkuznecovas:abstract-connection branch 3 times, most recently from 598ad67 to 69899eb Sep 24, 2018

@vkuznecovas vkuznecovas force-pushed the vkuznecovas:abstract-connection branch from 69899eb to 6a1cfa2 Sep 27, 2018

}

// NewOpenvpnProcessBasedConnectionFactory creates a new OpenvpnProcessBasedConnectionFactory
func NewOpenvpnProcessBasedConnectionFactory(

This comment has been minimized.

Copy link
@zolia

zolia Sep 28, 2018

Member

we try to get rid of New..Factory() methods, we should proceed this this at this point.

manager.mutex.Lock()
defer manager.mutex.Unlock()

switch state {
case openvpn.ConnectedState:
manager.statsKeeper.MarkSessionStart()

This comment has been minimized.

Copy link
@zolia

zolia Sep 28, 2018

Member

I am a bit concerned with a move of manager.statsKeeper.MarkSessionStart() to openvpn package, since it should be as a general contract to implement. Specific transport should always implement statistics collection. Most basic assumption is that each connection will have a start and a finish. Any implementation details should be hidden behind those.

@zolia
Copy link
Member

left a comment

need discussion with move of stats keeper to specific transport. How those stats should relate with statistics sender? Statistics sender should always expect statistics to be available.

@vkuznecovas vkuznecovas force-pushed the vkuznecovas:abstract-connection branch 5 times, most recently from 232e8bf to 6c10259 Oct 2, 2018

@vkuznecovas

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2018

@soffokl @Waldz any thoughts about the move of stats keeper from the manager to the connection itself?

@vkuznecovas vkuznecovas force-pushed the vkuznecovas:abstract-connection branch from 6c10259 to 0421b08 Oct 11, 2018

@Waldz Waldz force-pushed the vkuznecovas:abstract-connection branch from 369aa37 to 287f010 Oct 14, 2018

vkuznecovas added some commits Sep 19, 2018

vkuznecovas added some commits Oct 2, 2018

@vkuznecovas vkuznecovas force-pushed the vkuznecovas:abstract-connection branch from 287f010 to 0439ff3 Oct 15, 2018

@Waldz

Waldz approved these changes Oct 15, 2018

Stale review (too long decisions)

@Waldz Waldz merged commit ce11163 into mysteriumnetwork:master Oct 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.