Refactor by krhubert · Pull Request #799 · mesg-foundation/engine

krhubert · 2019-03-07T18:47:26Z

No description provided.

antho1404 · 2019-03-07T20:05:43Z

I had a quick look, I have few feedback for now:

API package should accept the service as parameter for the stop, start etc... instead of the sid/hash like that we can api.StartService(api.GetService()) this will be easier to delegate to a system service
Why reverting to events/task using map and not array
I like the service manager but why not even delegating this directly to the API to simplify

krhubert · 2019-03-08T06:46:51Z

API package should accept the service as parameter for the stop, start etc... instead of the sid/hash like that we can api.StartService(api.GetService()) this will be easier to delegate to a system service

If API accepts service then who manage databases to get them?
grpc server should manage communication layer only.

The flow right now is:
grpc -> api -> service -> container

If you want to accept service struct in api package then we need something between grpc and ap

I like the service manager but why not even delegating this directly to the API to simplify

To avoid api package becoming bigger. And I felt that manager struct belongs more to service (as it only operates on service). Moving this to api means also that api imports container package so it measn it depends on service and container package rather then service itself.

Why reverting to events/task using map and not array

From discussion on discord we end up with:

The array keeps the order or parameters and it's more deterministic definition of service so the array is preferable way here. The second thing is that definitions and protobuf should be same if possible, so if we have array we will have them in mesg.yaml as well (@antho1404 please confim that what I wrote cover our discussion or add missing part)

ilgooz · 2019-03-08T09:24:55Z

This PR is too big, it needs to be split to smaller ones with explanatory commit messages.

antho1404 · 2019-03-08T11:37:21Z

The array keeps the order or parameters and it's more deterministic definition of service so the array is preferable way here. The second thing is that definitions and protobuf should be same if possible, so if we have array we will have them in mesg.yaml as well

Not exactly.

Keeping the array structure because more precise 👍
definition and protobuf similar is a nice to have
mesg.yml is a representation so don't need to have arrays, this is oversimplification that we can do if we want too but this impacts a lot existing services and reduces a lot the readability of the yaml so i suggest to keep the parsing of the yaml and convert it in the data structure with array except if we all agree to move with array only on the yaml

…to service-refactor

…ggestion Service refactor nico suggestion

…to service-refactor

NicolasMahe

Manually tested 👍

NicolasMahe · 2019-03-20T03:31:42Z

@antho1404 @ilgooz please review.

antho1404

few feedback but I didn't do a full validation yet and no testing yet.
It will take quite some time to test correctly everything. Again I strongly encourage to split this PR as many things are independent

antho1404 · 2019-03-21T06:34:11Z

+			return nil, err
+		}
+		defer resp.Body.Close()
+		if err := archive.Untar(resp.Body, contextDir, nil); err != nil {


why changing back, why not keep using the DeployService that takes a reader and will simplify the function and also put all the logic in the same place if we need to do some extra work for the tar

we wanted service to accept contextDir as arguments. So there are few cases here:

api receives io.Reader that could be read only once

api needs to formalizeContext (remove .git etc...) so it needs to extract the tar

service needs a directory to calculate the hash

service needs tar.Reader to pass it to docker deploy api

So combining all this either service will receive io.reader, extract tar, calc the hash and then archive it back, or the other way.

that's not my point here, why not use DeployService(resp.Body, ...) and have a pseudo function like that

if isGit { dir = createTmp() gitClone(arg, dir) return deploy(dir) } resp = http.Get(arg) return DeployService(resp.Body, ...)

Here we have only one function responsible for the untar, not 2 and nothing else changes and we still have the context

To send status about Service downloaded. Also it seems more readable to me.

antho1404 · 2019-03-21T06:37:26Z

-	return fmt.Sprintf(`%s Dockerfile exists
-%s mesg.yml is valid
-%s Service is valid`, pretty.SuccessSign, pretty.SuccessSign, pretty.SuccessSign), nil
+	return fmt.Sprintf(`%s mesg.yml is valid`, pretty.SuccessSign), nil


Agree with that argument but we still need to validate that the service contains a dockerfile

antho1404 · 2019-03-21T06:38:55Z

 	return string(content), err
 }

+func taskByKey(s *coreapi.Service, key string) *coreapi.Task {


should return an error if not found to avoid risk of null pointer

we can but we can check for the null as well, it's just a helper function only used inside this file.

just seems better to adopt this pattern all the time. Also we are returning the error anyway just after so let's get the error from the function itself

I feel it's not a really good pattern in Go to return a pointer without error.
I speak about this with Hubert about should we keep pointer in the data structure of service or not. We didn't find a good solution.
In both TypeScript and in Swift, they force to check if the variable / pointer is nil / undefined before accessing a value in it. This is not something that the Go compiler enforce. I feel it opens the place for many nil pointer error...

What about removing all pointers in the struct of the service? Like this, for example, this function taskByKey, will need to have the following signature: taskByKey(s coreapi.Service, key string) (coreapi.Task, error).
It will consume more memory, but we are forced to follow a good pattern.

the pointer discussion is something else, the problem will be the same. Either we test something based on the value returned or based on an error.
After I kind of agree if it's pure data we don't necessary need pointers as long as it's not huge data and not really important that are present everywhere in all the modules. For this one I don't mind keeping the pointers. I don't see any really good reason to change it

we can change to error, it's really small change (but error won't ever happen here) but it's ok

antho1404 · 2019-03-21T06:40:55Z

 	}
-	if err := tx.Delete([]byte(hashKeyPrefix+s.Hash), nil); err != nil {
-		return err
+	if s.Sid != "" {


Sid should never be "" in the database. Services without Sid should be rejected as it is a mandatory data (at least if we have volumes)

So they are not mandatory right now (they are marked as optional).

What about volumes ? 2 services with no sid will share the same volume.
Right now they are not mandatory in the mesg.yml but generated with a value by default (_HASH if i remember correctly) so the definition file is optional but in the core mandatory.

This change related to Sid breaks volumes for services

In service manager, if sid is empty it uses the hash.
The behavior is exactly like before.

great I didn't see that. Also behavior is not exactly, we are taking the hash, before we where appending an character in front of the hash to make sure that we cannot create a service with the hash of another one as sid and then overwrite the previous service/volume. But anyway that was not a strong security anyway.

I still feel more confident to make sure that we can rely on a service to have a sid (that is generate when we deploy it like the hash) instead of having to check all the time we want to use the sid. I'm not necessary against it either it's just something I would like to have agreement to see the pro and con on that and not change something that impact the data.

In that case, we have to generate different sid because one we generated one is an invalid domain. Also, sid should be used only for local development and should not be mandatory.

sid is the base for the markeplace too so it's not only about local development. We put sid as optional with generation in order not to break previous services but every services needs to have a sid to identify this service and "group" the different version to the same service.

How do you manage services without sid on the marketplace for example ?

antho1404 · 2019-03-21T06:43:45Z

-		return nil, err
-	}
+// New creates an event eventKey with eventData for given service.
+func New(s *service.Service, eventKey string, eventData map[string]interface{}) *Event {


Any good reason to move the validation from this package ? It ensure consistency in the data. Agree that it doesn't impact the data but in that case maybe the data should change and instead have a reference of the event maybe.

It was moved to api package (so api handles all validation) and in that case event and execution packages depend only on service structure (and not the way how to validate them). So both package can take care only of it's logic (like update complete status for execution)

I saw that this was moved on the API but what happen if we use the event/execution packages directly without the API ? We might introduce invalid data ? Also this doesn't reduce any dependencies as event/execution packages already depends on the service package.

antho1404 · 2019-03-21T06:44:08Z

-	if err := task.RequireInputs(inputs); err != nil {
-		return nil, err
-	}
+func New(service *service.Service, eventID string, taskKey string, inputs map[string]interface{}, tags []string) *Execution {


antho1404 · 2019-03-21T06:57:05Z

+  string image = 1;                 // Image's name of the container.
  repeated string volumes = 2;      // List of volumes.
-  repeated string volumesfrom = 3;  // List of volumes mounted from other dependencies.
+  repeated string volumesFrom = 3;  // List of volumes mounted from other dependencies.


this is breaking change, not critical and i'm ok with it but just that everyone acknowledge this

It's fine here. The mesg.yml still have the volumesfrom.

And no, it's not a breaking change, gRPC uses the number (in this case 3) to decode the data. So this is compatible with previous version.

We are changing the data so it is a breaking change if any client like cli for example are reading this property which is quite unlikely but still this is a breaking change

antho1404 · 2019-03-21T07:04:50Z

+}
+
+// Delete see Manager.Delete.
+func (m *ContainerManager) Delete(service *Service) error {


Maybe DeleteVolumes. This doesn't delete the service so that's confusing

So this is the implementation of the Manager interface

// Delete deletes all persistent data used by service. Delete(service *Service) error

So for Container, it happens to delete volumes but for other deploy methods, it could be something else.

Instead of having the stop and delete logic in API, it could be done at the service manager.
Stop is just stopping the service.
Delete: stop + delete persistent data.

No, it's manager part to have deleted and stop function separated. so api (or any other packge) can use them, and there they should be splited.

* service.Service type is now logicless and its relationship with Docker Containers will be managed by any implementation of manager.Manager. * Manager is responsible for managing Docker Containers of MESG Services. it can be implemented for any container orchestration tool. right now, Docker Service is the only one we have. related to #799.

service.Service type is now logicless and its relationship with Docker Containers will be managed by any implementation of manager.Manager. Manager is responsible for managing Docker Containers of MESG Services. it can be implemented for any container orchestration tool. right now, Docker Service is the only one we have. * service: Service.GetDependency(), Service.Namespace() & Dependency.Namespace() made public. related to #799.

service.Service type is now logicless and its relationship with Docker Containers will be managed by any implementation of manager.Manager. Manager is responsible for managing Docker Containers of MESG Services. it can be implemented for any container orchestration tool. right now, Docker Service is the only one we have. * service: Service.GetDependency(), Service.Namespace() & Dependency.Namespace() are made public. related to #799.

service.Service type is now logicless and its relationship with Docker Containers will be managed by any implementation of manager.Manager. Manager is responsible for managing Docker Containers of MESG Services. it can be implemented for any container orchestration tool. right now, Docker Service is the only one we have. * service: Service.GetDependency(), Service.Namespace() & Dependency.Namespace() func are made public. related to #799.

service.Service type is now logicless and its relationship with Docker Containers will be managed by any implementation of manager.Manager. Manager is responsible for managing Docker Containers of MESG Services. it can be implemented for any container orchestration tool. right now, Docker Service is the only one we have. * new manager pkg put under service/manager. * new dockermanager pkg put under service/manager/dockermanager. its logic is consist of the code split from service pkg. * service: Service.GetDependency(), Service.Namespace() & Dependency.Namespace() func are made public. * rename Manager.DeleteVolumes() as Manager.Delete(). related to #799.

service.Service type is now logicless and its relationship with Docker Containers will be managed by any implementation of manager.Manager. Manager is responsible for managing Docker Containers of MESG Services. it can be implemented for any container orchestration tool. right now, Docker Service is the only one we have. * new manager pkg put under service/manager. * new dockermanager pkg put under service/manager/dockermanager. its logic is consist of the code split from service pkg. * service: Service.GetDependency(), Service.Namespace() & Dependency.Namespace() funcs are made public. * service/manager: rename/have Manager.DeleteVolumes() as Manager.Delete(). related to #799.

NicolasMahe · 2019-06-03T11:13:17Z

Closing this PR as it's not relevant anymore

krhubert force-pushed the service-refactor branch 3 times, most recently from 2dd3260 to 1f8c203 Compare March 7, 2019 19:50

krhubert force-pushed the service-refactor branch from 1f8c203 to 3a24759 Compare March 7, 2019 20:06

krhubert mentioned this pull request Mar 8, 2019

Deterministic hash #731

Merged

krhubert force-pushed the service-refactor branch 2 times, most recently from f72dd7b to b9e1cb6 Compare March 8, 2019 06:25

krhubert mentioned this pull request Mar 8, 2019

Add Hash to reply of Deploy API #801

Merged

krhubert force-pushed the service-refactor branch from b9e1cb6 to 28482ce Compare March 8, 2019 06:36

Refactor

d015140

krhubert force-pushed the service-refactor branch from 28482ce to d015140 Compare March 8, 2019 08:12

krhubert added 2 commits March 8, 2019 12:20

Fix formalize context

9b9322c

Use sha256 for service hash

600a010

krhubert mentioned this pull request Mar 12, 2019

Make sure generated SID is valid #806

Closed

Merge branch 'dev' into service-refactor

bc406e0

krhubert force-pushed the service-refactor branch 2 times, most recently from d898cd3 to 2bf586d Compare March 12, 2019 17:14

Fixes

4db9b62

krhubert force-pushed the service-refactor branch from 2bf586d to 4db9b62 Compare March 12, 2019 18:33

antho1404 reviewed Mar 12, 2019

View reviewed changes

Comment thread core/main.go

krhubert and others added 2 commits March 13, 2019 08:52

Set system service hash and sid

ed3c871

Merge branch 'dev' into service-refactor

47d93b7

NicolasMahe reviewed Mar 14, 2019

View reviewed changes

Comment thread database/service_db.go

NicolasMahe reviewed Mar 14, 2019

View reviewed changes

Comment thread execution/execution.go Outdated

krhubert added 2 commits March 14, 2019 07:27

Revert to github.com/cnf/structhash

27830ee

Merge branch 'service-refactor' of github.com:mesg-foundation/core in…

9a92b41

…to service-refactor

NicolasMahe and others added 8 commits March 19, 2019 13:22

Rename in proto volumesfrom to volumesFrom

ea1bab1

Fix test about volumesfrom

c163059

Merge pull request #825 from mesg-foundation/service-refactor-nico-su…

e62f408

…ggestion Service refactor nico suggestion

Remove pointers from service definition

0ddf601

Merge branch 'service-refactor' of github.com:mesg-foundation/core in…

6944c1f

…to service-refactor

Merge branch 'dev' into service-refactor

348c2de

Merge branch 'dev' into service-refactor

4e720d1

Merge branch 'dev' into service-refactor

f6ee341

NicolasMahe previously approved these changes Mar 19, 2019

View reviewed changes

Merge branch 'dev' into service-refactor

a61fe23

antho1404 requested changes Mar 21, 2019

View reviewed changes

Start dependencies before service

58dfe13

krhubert dismissed NicolasMahe’s stale review via 58dfe13 March 21, 2019 16:01

This was referenced Apr 2, 2019

api: simplify the code & switch to container mocks for testing #847

Merged

simplify service pkg & make types logicless #855

Merged

antho1404 added the on hold label Apr 8, 2019

ilgooz mentioned this pull request Apr 16, 2019

service: simplify DeleteVolumes() #881

Merged

krhubert mentioned this pull request May 17, 2019

Move service's status to manager #958

Closed

NicolasMahe closed this Jun 3, 2019

NicolasMahe deleted the service-refactor branch June 3, 2019 11:13

Conversation

krhubert commented Mar 7, 2019 • edited by NicolasMahe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antho1404 commented Mar 7, 2019

Uh oh!

krhubert commented Mar 8, 2019

Uh oh!

ilgooz commented Mar 8, 2019

Uh oh!

antho1404 commented Mar 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NicolasMahe left a comment

Choose a reason for hiding this comment

Uh oh!

NicolasMahe commented Mar 20, 2019

Uh oh!

antho1404 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krhubert Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasMahe Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antho1404 Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

krhubert commented Mar 7, 2019 •

edited by NicolasMahe

Loading

krhubert Mar 21, 2019 •

edited

Loading

NicolasMahe Mar 21, 2019 •

edited

Loading

antho1404 Mar 21, 2019 •

edited

Loading