implement new service start style #1024
Conversation
ilgooz
force-pushed
the
feature/new-start
branch
7 times, most recently
from
dc896a9
to
6881295
Compare
while using new instance db.
| @@ -6,9 +6,11 @@ import ( | |||
|
|
|||
| // Manager is responsible for managing Docker Containers of MESG services. | |||
| // it can be implemented for any container orchestration tool. | |||
| // TODO(ilgooz): discuss if these should accept service.Instance instead of service.Service. | |||
There was a problem hiding this comment.
@mesg-foundation/core please provide feedbacks about this.
There was a problem hiding this comment.
Yes it should accept instance and I think this function can be moved directly to the sdk under the instance sdk
There was a problem hiding this comment.
I'll do this improvement in an another PR
There was a problem hiding this comment.
Yes it should accept instance instead of service.
Agree also to move the manager package directly inside sdk/instance.
| @@ -60,6 +60,16 @@ type Service struct { | |||
| DeployedAt time.Time `hash:"-"` | |||
| } | |||
|
|
|||
| // Instance represents a running instance of service. | |||
| type Instance struct { | |||
| Sid string | |||
There was a problem hiding this comment.
What's the purpose of this sid? is it to be able to run an instance based on the sid? If this is the case it should be removed and the api/sdk should do the resolution and Instance only have a service hash
There was a problem hiding this comment.
It's for extra doc that we can send to cli on an instance create or delete action. I can rm this and fetch it from service db. let's do it an another PR
There was a problem hiding this comment.
Also move the struct instance in its own package instance or in the instanceDB package?
| @@ -6,9 +6,11 @@ import ( | |||
|
|
|||
| // Manager is responsible for managing Docker Containers of MESG services. | |||
| // it can be implemented for any container orchestration tool. | |||
| // TODO(ilgooz): discuss if these should accept service.Instance instead of service.Service. | |||
There was a problem hiding this comment.
Yes it should accept instance and I think this function can be moved directly to the sdk under the instance sdk
Any reason why? To me this should be handled by implementations of Manager. That way, the chosen Manager implementation can provide its own info easily. Otherwise we have to switch check to determine underlying orchestration tool used and maybe create different type of info for each (if in sdk/instance). |
| } | ||
|
|
||
| // Create creates a new service instance for service with id(sid/hash) and applies given env vars. | ||
| func (i *Instance) Create(id string, env []string) (*service.Instance, error) { |
There was a problem hiding this comment.
| func (i *Instance) Create(id string, env []string) (*service.Instance, error) { | |
| func (i *Instance) Create(serviceHash string, env []string) (*service.Instance, error) { |
Not blocking but I would rename the id variable to something way more explicit.
There was a problem hiding this comment.
Discussed with @antho1404, I'll change the name but still resolve the sid inside to service hash when sid is given in the argument.
|
|
||
| srv.Hash = instanceHash | ||
| srv.Configuration.Image = imageHash | ||
| srv.Configuration.Env = xos.EnvMapToSlice(instanceEnv) |
There was a problem hiding this comment.
The above 3 lines have to me removed.
The service should not be modified when an instance is created.
The goal is actually to be able to create multiple instance for one service.
The instance have to directly contains those 3 info: instanceHash, imageHash and merge envs
There was a problem hiding this comment.
The instanceHash and imageHash are already saved in the instance struct.
The env could also be saved in the instance struct OR directly send to the function start of the container. Up to you.
Not saving the customs envs feel a bit saffer. It will be very hard to expose them on a public api if they are not saved in a db
The orchestration tool used should be managed by the container package directly and the manager should anyway not be aware of that. Also, lint and test are failing |
while using new instance db.
depends on #1023.
@mesg-foundation/core please keep in mind that, since
listenTask()only accepts service hashes, an instance created byinstancesdk.Create()cannot start listening for tasks yet. I can makelistenTask()to both accept service hash and instance hash if necessary but in another PR. Please request this feature if you think it's needed right now, otherwise i'll eventually do it after we merge everything related to new deployment/start style.