Allow to decide which tasks are going to be executed per playground #218
Conversation
| @@ -52,7 +52,7 @@ func main() { | |||
| log.Fatalf("Cannot parse duration %s. Got: %v", config.DefaultSessionDuration, err) | |||
| } | |||
|
|
|||
| playground := types.Playground{Domain: config.PlaygroundDomain, DefaultDinDInstanceImage: config.DefaultDinDImage, AllowWindowsInstances: config.NoWindows, DefaultSessionDuration: d, AvailableDinDInstanceImages: []string{config.DefaultDinDImage}} | |||
| playground := types.Playground{Domain: config.PlaygroundDomain, DefaultDinDInstanceImage: config.DefaultDinDImage, AllowWindowsInstances: config.NoWindows, DefaultSessionDuration: d, AvailableDinDInstanceImages: []string{config.DefaultDinDImage}, Tasks: []string{".*"}} | |||
There was a problem hiding this comment.
Didn't we say that the default should be "don't run any tasks unless specified" ?
There was a problem hiding this comment.
The default of the Playground struct is an empty string slice, which means don't run anything. But when you run the playground locally (for development for example) you want the scheduler to do something, right? Like show the ports that you open, etc. Do you prefer it not to show anything and change the current behavior?
There was a problem hiding this comment.
But if the config will be retrieved from the DB shoudn't we populate the local db with that config instead of hardcoding it in the code?
There was a problem hiding this comment.
How do you populate the DB with the playground for development if you don't do it here?
|
|
||
| storage storage.StorageApi | ||
| event event.EventApi | ||
| pwd pwd.PWDApi | ||
| mx sync.Mutex |
There was a problem hiding this comment.
For what I see in the a PR, a single scheduler can handle multiple plagrounds, so shouldn't this mutex be per-playground?. The way it's implemented, every time it's used it'll hold on all the playgrounds operations.
There was a problem hiding this comment.
How are you going to do it? This is to update the managed playgrounds and matched tasks. Send away your suggestions
There was a problem hiding this comment.
Add a mutex per playground instead one mutex for all?
There was a problem hiding this comment.
And how do you update the playground list in a way that is safe?
| func (s *scheduler) schedulePlaygroundsUpdate() { | ||
| s.updatePlaygrounds() | ||
| s.ticker = time.NewTicker(time.Minute * 5) | ||
| go func() { |
There was a problem hiding this comment.
running the update in a goroutine doesn't make too much sense as they will get executed sequentially because of the mutex. right?
There was a problem hiding this comment.
Not sure I understand. The idea is that the schedulePlaygroundUpdate returns and doesn't block forever.
| matchedTasks = append(matchedTasks, task) | ||
| continue | ||
| } | ||
| matched, err := regexp.MatchString(expr, task.Name()) |
There was a problem hiding this comment.
This will compile the regex every time. Shouldn't we make expr a *Regexp and compile them when updating the playground config?
There was a problem hiding this comment.
Yes. Good idea. I will change it!
| } | ||
|
|
||
| func (s *scheduler) getTasks(playgroundId string) []Task { | ||
| s.mx.Lock() |
There was a problem hiding this comment.
We're blocking everything (all the playgrounds) just to retrieve the tasks for only one playground. I'm aware that this should be extremely fast.. but still IDK if it's the best approach.
There was a problem hiding this comment.
You are more than welcome to propose a better approach :) As long as it is simple.
| log.Printf("EVENT: Session New %s\n", sessionId) | ||
| session, err := s.storage.SessionGet(sessionId) | ||
| if err != nil { | ||
| log.Printf("Session [%s] was not found in storage. Got %s\n", sessionId, err) | ||
| return | ||
| } | ||
| if _, found := s.playgrounds[session.PlaygroundId]; !found { |
There was a problem hiding this comment.
Do we need to do this here also?. Isn't this be populated here: https://github.com/play-with-docker/play-with-docker/pull/218/files#diff-9667cfa1ad6b8b445794f7c2469f069fR244 before in Start function?
There was a problem hiding this comment.
We populate on Start for all sessions we retrieve from the DB. But then new sessions could be created, on different playgrounds, so we need to populate for the new playgrounds.
@marcosnils 🔔