Multiple services #541

vkuznecovas · 2018-11-15T07:24:06Z

myst service now runs all the supported services(openvpn, noop). Adjusted the cli accordingly, we're now able to connect to a provider by specifying the service type as well. It's also possible to start a single service via myst service openvpn or myst service noop

vkuznecovas · 2018-11-15T08:11:11Z

closes #483

cmd/commands/cli/command.go

core/connection/manager_test.go

core/service/runner.go

services/noop/bootstrap.go

services/noop/connection.go

soffokl · 2018-11-15T08:22:04Z

services/noop/service.go

 }

 // Manager represents entrypoint for Noop service
 type Manager struct {
-	fakeProcess sync.WaitGroup
+	fakeProcess      sync.WaitGroup
+	locationResolver location.Resolver


Why do we need locationResolver here?

I guess to lookup country in which service is provided. But country is hardcoded below :(

correct, somehow forgot it - implemented.

tequilapi/endpoints/connection.go

services/noop/bootstrap.go

tadovas · 2018-11-15T09:41:23Z

services/noop/service.go

 }

 // Manager represents entrypoint for Noop service
 type Manager struct {
-	fakeProcess sync.WaitGroup
+	fakeProcess      sync.WaitGroup
+	locationResolver location.Resolver


I guess to lookup country in which service is provided. But country is hardcoded below :(

new one please

zolia · 2018-11-15T15:07:49Z

cmd/commands/service/command.go

+		os.Exit(2)
+	}
+
+	errorChannel := make(chan error, 2+len(serviceTypes))


Why we need 2+len here? Short comment would be good here.

zolia · 2018-11-15T15:49:04Z

core/service/manager.go

@@ -46,10 +46,11 @@ type Service interface {
 	Start(providerID identity.Identity) (dto_discovery.ServiceProposal, session.ConfigProvider, error)
 	Wait() error
 	Stop() error
+	GetType() string


This looks artificial. Maybe its better to get type from serviceProposal returned by manager.service.Start(providerID) if needed? We should know what we are running.

I would like this manager to manage multiple services instead of one. This way it can have method Register(Service, serviceType), Start(list of services), or maybe I don't get the purpose of this component. It can also hide a map from di, which is leaky details

The way I see it, we'll still need the manager(maybe the naming needs a change, but naming is hard). I like that it's responsible for

Registering/pinging the proposal

Starting the service it's given

Starting to listen for the relevant dialogs

We'll need these for every service, so in my head the component makes perfect sense.

In that case yes - better naming is needed. I didn't expect to see multiple managers :)

zolia · 2018-11-15T15:55:57Z

core/service/runner.go

+	return nil
+}
+
+// KillAll kills all the service managers


Suggested change

// KillAll kills all the service managers

// KillAll kills all service managers

zolia · 2018-11-15T16:27:03Z

server/mysterium_api_fake.go

-		if filterMatched {
+		var serviceMatched = true
+		if serviceType != "" {
+			serviceMatched = serviceMatched && (serviceType == proposal.ServiceType)


Suggested change

serviceMatched = serviceMatched && (serviceType == proposal.ServiceType)

serviceMatched = (serviceType == proposal.ServiceType)

zolia · 2018-11-15T16:33:46Z

services/noop/service.go

+	}
+
+	if manager.isStarted {
+		return dto_discovery.ServiceProposal{}, sessionConfigProvider, nil


Should it be "ErrAlreadyStarted" or return the same instance?

zolia · 2018-11-15T16:35:01Z

services/noop/service.go

+	if manager.isStarted {
+		return dto_discovery.ServiceProposal{}, sessionConfigProvider, nil
+	}
+
 	manager.fakeProcess.Add(1)


We prob should not use 'fake' wording anymore here.

zolia · 2018-11-15T16:40:26Z

tequilapi/endpoints/connection.go

+	// service type
+	// required: false
+	// default: openvpn
+	// example: openvpn


We should see somewhere what available options there are: openvpn, noop

zolia

small issues

tadovas · 2018-11-15T20:47:41Z

cmd/commands/service/command.go

+		if err != nil {
+			return err
+		}
+		err = di.ServiceRunner.StartServiceByType(serviceType, options, errorChannel)


What is the purpose of error channel here? method it self returns error

the error here tells us if the startup of services was successful. The channel is then used to report runtime errors that occur in the underlying service.

Hmm. Do we really need to report those errors upstairs? Startup errors are pretty much ok, but runtime errors - well I am not sure, what happens in case of that error - we shutdown whole node? What if other services run perfectly? Maybe some kind of retry mechanism is needed? Don't forget that service will be managed through api in the future - not only by args on startup.

tadovas · 2018-11-15T20:49:27Z

cmd/di.go

-		if err := di.ServiceManager.Kill(); err != nil {
-			errs = append(errs, err)
+	if di.ServiceRunner != nil {
+		if runnerErrs := di.ServiceRunner.KillAll(); len(runnerErrs) > 0 {


Do you really need a len > 0 here ? Append of empty slice elements shouldn't append anything

tadovas · 2018-11-15T20:52:47Z

cmd/di.go

-		discoveryService,
-	)
+	if len(nodeOptions.ServiceTypes) > 0 {
+		serviceManagerMap := make(map[string]service.RunnableService, len(nodeOptions.ServiceTypes))


Interface should be defined by caller or user. Anyway this block looks very complicated and hard to understand. I guess we can maintain a map of self registered services provides always ready, and runner only picks which ones to start according to options.

tadovas · 2018-11-15T20:54:25Z

core/connection/interface.go

@@ -49,7 +49,7 @@ type PromiseIssuerCreator func(issuerID identity.Identity, dialog communication.
 // Manager interface provides methods to manage connection
 type Manager interface {
 	// Connect creates new connection from given consumer to provider, reports error if connection already exists
-	Connect(consumerID identity.Identity, providerID identity.Identity, params ConnectParams) error
+	Connect(consumerID identity.Identity, providerID identity.Identity, serviceType string, params ConnectParams) error


Maybe it's time to gather proposal related information in it's own params structure. In the long term connection manager shouldn't do a proposal lookup but get one as a parameter from outside.

tadovas · 2018-11-15T20:55:11Z

core/connection/manager.go

@@ -141,7 +141,7 @@ func (manager *connectionManager) startConnection(consumerID, providerID identit
 		}
 	}()

-	proposal, err := manager.findProposalByProviderID(providerID)
+	proposal, err := manager.findProposalByProviderID(providerID, serviceType)


This parameter leaks from Connect only to be passed as a filter to discovery

tadovas · 2018-11-15T20:58:15Z

core/service/manager.go

@@ -46,10 +46,11 @@ type Service interface {
 	Start(providerID identity.Identity) (dto_discovery.ServiceProposal, session.ConfigProvider, error)
 	Wait() error
 	Stop() error
+	GetType() string


I would like this manager to manage multiple services instead of one. This way it can have method Register(Service, serviceType), Start(list of services), or maybe I don't get the purpose of this component. It can also hide a map from di, which is leaky details

new one please

vkuznecovas · 2018-11-16T12:17:16Z

Did a rework where a proposal is now passed to the connect method, and the manager no longer fetches it. Fixed most of the comments.

soffokl · 2018-11-19T05:40:05Z

e2e/connection_test.go

@@ -114,7 +114,7 @@ func consumerConnectFlow(t *testing.T, tequilapi *tequilapi_client.Client, consu
 	})
 	assert.NoError(t, err)

-	connectionStatus, err = tequilapi.Connect(consumerID, proposal.ProviderID, endpoints.ConnectOptions{true})
+	connectionStatus, err = tequilapi.Connect(consumerID, proposal.ProviderID, "openvpn", endpoints.ConnectOptions{true})


Shouldn't we pass here a proposal too?

out of scope, we'll need to fix the api part though, to avoid having to query for the service type in general. I've created #549 for this.

Awesome 👍

vkuznecovas · 2018-11-19T08:48:56Z

The runners "startServiceByType" now blocks.

tadovas · 2018-11-20T07:10:26Z

server/interface.go

@@ -28,7 +28,7 @@ import (
 type Client interface {
 	RegisterIdentity(id identity.Identity, signer identity.Signer) (err error)

-	FindProposals(providerID string) (proposals []dto_discovery.ServiceProposal, err error)
+	FindProposals(providerID string, serviceType string) (proposals []dto_discovery.ServiceProposal, err error)


Maybe it's time to add method for single proposal fetch by it's identification

tadovas

Looks OK. Lets move on

vkuznecovas requested review from tadovas, soffokl and zolia November 15, 2018 07:24

vkuznecovas added 3 commits November 15, 2018 09:24

running myst service will now run openvpn and noop

c8397dc

multiple services now do start. Need to adjust nats next

b77e6f4

Multiple services seem to work now

cf53bc8

vkuznecovas force-pushed the multiple-services branch from d1f3899 to cf53bc8 Compare November 15, 2018 07:24

soffokl reviewed Nov 15, 2018

View reviewed changes

tadovas previously requested changes Nov 15, 2018

View reviewed changes

vkuznecovas added 2 commits November 15, 2018 13:22

added the missing country resolution

e20bc7d

Merge branch 'master' into multiple-services

b0dac46

vkuznecovas force-pushed the multiple-services branch from 4dd1ca6 to b0dac46 Compare November 15, 2018 11:48

vkuznecovas requested review from soffokl and tadovas November 15, 2018 11:50

zolia reviewed Nov 15, 2018

View reviewed changes

zolia previously requested changes Nov 15, 2018

View reviewed changes

tadovas previously requested changes Nov 15, 2018

View reviewed changes

refactored connection/service flow

5ca4cd3

vkuznecovas requested review from tadovas and zolia November 16, 2018 12:12

soffokl reviewed Nov 19, 2018

View reviewed changes

the runners start method now blocks

3d070bc

vkuznecovas mentioned this pull request Nov 19, 2018

Get rid of the discovery call in tequilaAPI connection/create #549

Closed

vkuznecovas requested a review from soffokl November 19, 2018 08:49

soffokl approved these changes Nov 20, 2018

View reviewed changes

tadovas reviewed Nov 20, 2018

View reviewed changes

tadovas approved these changes Nov 20, 2018

View reviewed changes

zolia approved these changes Nov 20, 2018

View reviewed changes

tadovas merged commit 52942b3 into master Nov 20, 2018

tadovas deleted the multiple-services branch November 20, 2018 07:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple services #541

Multiple services #541

vkuznecovas commented Nov 15, 2018

vkuznecovas commented Nov 15, 2018

soffokl Nov 15, 2018

tadovas Nov 15, 2018

vkuznecovas Nov 15, 2018

tadovas Nov 15, 2018

zolia Nov 15, 2018 •

edited

Loading

zolia Nov 15, 2018

tadovas Nov 15, 2018

vkuznecovas Nov 16, 2018

tadovas Nov 19, 2018

zolia Nov 15, 2018

zolia Nov 15, 2018

zolia Nov 15, 2018

zolia Nov 15, 2018

tadovas Nov 15, 2018

zolia Nov 15, 2018

zolia left a comment

tadovas Nov 15, 2018

vkuznecovas Nov 16, 2018

tadovas Nov 19, 2018

tadovas Nov 15, 2018

tadovas Nov 15, 2018

tadovas Nov 15, 2018

tadovas Nov 15, 2018

tadovas Nov 15, 2018

vkuznecovas commented Nov 16, 2018

soffokl Nov 19, 2018

vkuznecovas Nov 19, 2018

tadovas Nov 20, 2018

vkuznecovas commented Nov 19, 2018

tadovas Nov 20, 2018

tadovas left a comment

	// KillAll kills all the service managers
	// KillAll kills all service managers

	serviceMatched = serviceMatched && (serviceType == proposal.ServiceType)
	serviceMatched = (serviceType == proposal.ServiceType)

Multiple services #541

Multiple services #541

Conversation

vkuznecovas commented Nov 15, 2018

vkuznecovas commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zolia Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zolia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkuznecovas commented Nov 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkuznecovas commented Nov 19, 2018

Choose a reason for hiding this comment

tadovas left a comment

Choose a reason for hiding this comment

zolia Nov 15, 2018 •

edited

Loading