Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configurable instance port pool #1096

Merged
merged 2 commits into from
Aug 1, 2017
Merged

Conversation

ejholmes
Copy link
Contributor

@ejholmes ejholmes commented Jul 29, 2017

This adds options for configuring the range of EC2 instance ports that Empire manages and allocates to applications that use legacy ELB's to prevent port conflicts.

This makes it easier to run multiple Empire daemons that schedule work into a single ECS cluster. Each Empire daemon can now specify it's own range of instance ports to allocate from to prevent overlap.

Note that, once configured, changing the flags will have no effect, so this only applies to new installations of Empire. Previous installations will continue to use the 9000-10000 range.

@ejholmes ejholmes requested a review from phobologic July 31, 2017 22:33
migrations.go Outdated
type Schema struct {
// For legacy ELB's (not ALB) Empire manages a pool of host ports to
// ensure that all applications have a unique port on the EC2 instance.
// This option specifies the beginning of that range.
Copy link
Contributor

Choose a reason for hiding this comment

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

not just the beginning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Updated.

const table = `CREATE TABLE ports (
id uuid NOT NULL DEFAULT uuid_generate_v4() primary key,
port integer,
app_id uuid references apps(id) ON DELETE SET NULL
Copy link
Contributor

Choose a reason for hiding this comment

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

should it also map to a process as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually gets updated in a later migration: https://github.com/remind101/empire/blob/master/migrations.go#L250-L251

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh, ok - makes sense.


func (s *Schema) migrations() []migrate.Migration {
ports := migrate.Migration{
ID: 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this changing an older migration? Is that going to cause any issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's explained in the PR description, but, for existing installations of Empire, this doesn't change anything. For new installations of Empire, you can set these options to pre-configure the pool when the migration runs.

db.Schema = &empire.Schema{
InstancePortPool: &empire.InstancePortPool{
Start: uint(c.Int(FlagInstancePortPoolStart)),
End: uint(c.Int(FlagInstancePortPoolEnd)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this where we'd catch it if one of the two vars isn't defined? Might be useful to have some helpful warning/error messages in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In any case where you provided invalid values here, the migration would fail, and you'd see an error in logs.

@ejholmes ejholmes force-pushed the configurable-instance-ports branch 2 times, most recently from 860d54a to 2dc38c4 Compare July 31, 2017 22:59
This commit adds new flags for configuring the instance port pool that
Empire uses to allocate instance ports to applications and processes.
@ejholmes ejholmes force-pushed the configurable-instance-ports branch from be0237c to 791f978 Compare August 1, 2017 02:35
@ejholmes ejholmes merged commit 302c7b3 into master Aug 1, 2017
@ejholmes ejholmes deleted the configurable-instance-ports branch August 1, 2017 03:09
@ejholmes ejholmes mentioned this pull request Sep 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants