deis / builder

Git server and application builder for Deis Workflow
https://deis.com
MIT License
40 stars 41 forks source link

[Question]: Avoid simultaneous git push for the same application #168

Closed aledbf closed 8 years ago

aledbf commented 8 years ago

In V1 this is not possible. I'm not sure if this should be allowed or not. Running two simultaneous git push I see this in the builder logs:

Waiting for git-receive to run.
Waiting for deploy.
[info] Deploy complete.
[warning] Unreported error: error: Ref refs/heads/master is at 2bd4702d7ede8c417a0a070755e12b99ddbba6b7 but expected 0000000000000000000000000000000000000000
[info] Deploy complete.
arschles commented 8 years ago

Yes, we should prevent simultaneous git pushes for the same app. This was done with a lockfile prior to the builder being rewritten in Go, but I didn't port it over because at the time I was assuming that more than one builder could be running at once.

For now, we're going to assume that builder will run with replicas: 1 in its replication controller, so a lockfile may be in order. I'd prefer to see an in-memory map[string]*sync.Mutex, however :)

arschles commented 8 years ago

or, you could use the k8s API to do the locking

aledbf commented 8 years ago

@arschles I think this is the place to use the mutex

arschles commented 8 years ago

@aledbf +1

smothiki commented 8 years ago

@aledbf the place you mentioned will lock every git push request. We just have to lock the app folder

arschles commented 8 years ago

Ah, good call @smothiki. Seems like that's where you'd need to use the map[string]*sync.Mutex (note that the map would need to be protected by a lock as well).

Furthermore, I'd like to put the locking logic behind an interface, so that we can easily plug in a distributed locking mechanism later.

smothiki commented 8 years ago

I know distributed locking is a far fetched goal as of now. But if we are supporting something like this then lets design something that can be converted to a DS lock in future Also map[string]*sync.Mutex string here is folder ??

arschles commented 8 years ago

@smothiki an interface like this would allow us to plug in whatever locking we want:

type FolderLock interface {
  Lock(folderName string) error
  Unlock(folderName string) error
}

(edited to include the Unlock func)

Also, I don't think that distributed locking is a far-fetched goal as of now. The k8s API natively supports the primitives we need to build one - annotations and resource versions.

Finally, the map[string]*sync.Mutex would map a folder name to the lock that builder should take during the git push operation for that folder.

smothiki commented 8 years ago

:+1:

arschles commented 8 years ago

Also, I think we should include a timeout time.Duration parameter in the Lock and Unlock functions, so that a git push can wait for a configurable amount of time (0 is still an option) for other pushes to finish. This will allow us to swap for a distributed lock implementation later and account for network latencies, polling intervals, etc...