Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

feat(publisher): Check if app port is open before publishing it. Fixes 3115 #3156

Merged
merged 1 commit into from Mar 4, 2015

Conversation

glogiotatidis
Copy link
Contributor

fixes #3115

@aledbf
Copy link
Contributor

aledbf commented Feb 27, 2015

@glogiotatidis why use ExecStartPost like in deis-builder?.
With that it will appear as start-post in fleetctl list-units and not as started.
https://github.com/deis/deis/blob/master/deisctl/units/deis-builder.service#L12

@@ -113,12 +115,29 @@ func (s *Server) IsPublishableApp(name string) bool {
log.Println(err)
return false
}
if version >= latestRunningVersion(s.EtcdClient, appName) {

if version >= latestRunningVersion(s.EtcdClient, appName) && isPortOpen(hostAndPort) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's separate this call out on line 96. IsPublishableApp determines if the container name follows a regex, where IsPortOpen would be useful on its own.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@glogiotatidis
Copy link
Contributor Author

@aledbf I'm not aware of this fleet functionality. I'd prefer thought that this goes to publisher so that Deis gets to have the final call and it's fleetd independent. Thanks for the suggestion though :)

@mboersma
Copy link
Member

https://get.docker.io is currently down, so this may fail until we can merge #3162 and rebase it off that.

…s 3115

Check if the port that container exposes is open before adding to Etcd.
@glogiotatidis
Copy link
Contributor Author

Rebased

@mboersma
Copy link
Member

mboersma commented Mar 3, 2015

Having publisher check that the app port is open before publishing it seems like a good idea. Code LGTM.

@bacongobbler
Copy link
Member

ooh, tests and everything! LGTM!

bacongobbler pushed a commit that referenced this pull request Mar 4, 2015
feat(publisher): Check if app port is open before publishing it. Fixes 3115
@bacongobbler bacongobbler merged commit ee07221 into deis:master Mar 4, 2015
@jgmize
Copy link
Contributor

jgmize commented Mar 4, 2015

nice work, @glogiotatidis

@lorieri
Copy link
Contributor

lorieri commented Mar 5, 2015

Great!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deis should check if container's port is accepting connections before adding it to the pool
7 participants