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

[WIP] Provisional ALB support #1045

Closed
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@ddollar
Copy link
Member

ddollar commented Aug 12, 2016

Adds provisional support for ALBs on a per-app level. To use, add a convox.router.path label to your docker-compose.yml:

web:
  labels:
    - convox.router.path=/*
  ports:
    - 80

KNOWN LIMITATIONS

  • Does not yet support SSL
  • Router priority is currently assigned by the start order of the docker-compose.yml (dependency order based on links then alphabetical by process name)
@@ -95,6 +95,11 @@ func (s Service) HasBalancer() bool {
return len(s.Ports) > 0
}

func (s Service) HasRoute() bool {

This comment has been minimized.

@houndci-bot

houndci-bot Aug 12, 2016

exported method Service.HasRoute should have comment or be unexported

return routes
}

func (rr ManifestRoutes) Listeners() []ManifestRouteListener {

This comment has been minimized.

@houndci-bot

houndci-bot Aug 12, 2016

exported method ManifestRoutes.Listeners should have comment or be unexported

Port int
}

func (m Manifest) Routes() ManifestRoutes {

This comment has been minimized.

@houndci-bot

houndci-bot Aug 12, 2016

exported method Manifest.Routes should have comment or be unexported


type ManifestRoutes []ManifestRoute

type ManifestRouteListener struct {

This comment has been minimized.

@houndci-bot

houndci-bot Aug 12, 2016

exported type ManifestRouteListener should have comment or be unexported
type name will be used as manifest.ManifestRouteListener by other packages, and that stutters; consider calling this RouteListener

ServiceName string
}

type ManifestRoutes []ManifestRoute

This comment has been minimized.

@houndci-bot

houndci-bot Aug 12, 2016

exported type ManifestRoutes should have comment or be unexported
type name will be used as manifest.ManifestRoutes by other packages, and that stutters; consider calling this Routes


import "strings"

type ManifestRoute struct {

This comment has been minimized.

@houndci-bot

houndci-bot Aug 12, 2016

exported type ManifestRoute should have comment or be unexported
type name will be used as manifest.ManifestRoute by other packages, and that stutters; consider calling this Route

@ddollar ddollar changed the title Provisional ALB support WIP: Provisional ALB support Aug 12, 2016

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 12, 2016

Current coverage is 19.11% (diff: 9.37%)

Merging #1045 into master will decrease coverage by 0.02%

@@             master      #1045   diff @@
==========================================
  Files           128        129     +1   
  Lines         11188      11219    +31   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2142       2145     +3   
- Misses         8907       8935    +28   
  Partials        139        139          

Powered by Codecov. Last update fcafc02...bf4ea36

}

// Routes returns all routes in the Manifest
func (m Manifest) Routes() manifestRoutes {

This comment has been minimized.

@houndci-bot

houndci-bot Aug 12, 2016

exported method Routes returns unexported type manifest.manifestRoutes, which can be annoying to use

return routes
}

func (rr Routes) Listeners() []RouteListener {

This comment has been minimized.

@houndci-bot

houndci-bot Aug 12, 2016

exported method Routes.Listeners should have comment or be unexported

@ddollar ddollar force-pushed the alb branch 4 times, most recently from 03ad596 to 5abd62c Aug 13, 2016

ddollar added some commits Aug 12, 2016

@ddollar ddollar changed the title WIP: Provisional ALB support [WIP] Provisional ALB support Aug 19, 2016

@dlanger

This comment has been minimized.

Copy link

dlanger commented Sep 30, 2016

For us, the most attractive feature of ALBs is that they enable running more than one copy of a container on a single instance. This greatly simplifies capacity planning and makes it easier to think of an ECS cluster as a pool of compute resources, and means I no longer have to do separate tracking to ensure that max(number of copies of a single container I'd like to run) <= number of instances.

@ddollar

This comment has been minimized.

Copy link
Member

ddollar commented Sep 30, 2016

So one thing to note is that running those processes on multiple instances is a lot more fault-tolerant. If you stack multiple processes on a single instance then it's a lot easier to take it down with the loss of a single instance.

@mwarkentin

This comment has been minimized.

Copy link
Contributor

mwarkentin commented Sep 30, 2016

@ddollar @dlanger was more talking about if we wanted to scale up to 6 processes, that we could do that at 2x3 instances or whatever, not running a whole bunch on 1 instance. Like he mentioned, it simplifies our resource calculations on one dimension.. another nice benefits is that we could see some cost savings on things like Datadog and ThreatStack where things are billed on a per-instance basis.

@ddollar ddollar added the enhancement label Oct 1, 2016

@scottmessinger

This comment has been minimized.

Copy link

scottmessinger commented Oct 11, 2016

Hi! @ddollar asked for me to leave a comment in this PR with my use case for ALB. We're deploying an Elixir app that uses websockets. From what I understand, ELB doesn't support sticky sessions with websockets. Also, there seem to be some complexity in simply setting up websockets on ELB from the scattering of blog posts I've read. ALB is attractive because, from what I understand, it would just work. (Btw, I could be off here -- I'm coming from Heroku so I haven't had to get into the weeds with AWS deployments)

@idyll

This comment has been minimized.

Copy link

idyll commented Oct 11, 2016

That's crazy @scottmessinger -- we are in the exact same position. Some Elixir (other stuff still in Ruby) and we really need the ALB support to remove the need for ProxyProtocol.

We are actively porting more and more ruby to elixir so the pain of missing ALB support is getting worse for us.

@mattmanning

This comment has been minimized.

Copy link
Contributor

mattmanning commented Nov 4, 2016

Closing in favor or #1373

@mattmanning mattmanning closed this Nov 4, 2016

@nzoschke nzoschke referenced this pull request Nov 5, 2016

Closed

ALB: Request for input #1373

@jkurz

This comment has been minimized.

Copy link

jkurz commented Nov 8, 2016

So since this PR is closed, is convox still behind this blog post? https://convox.com/blog/alb/

It points people to use this branch to get ALB features going. Maybe an update should be in order, saying that the real solution is still in the works and this is for testing and should not be used in production. Unless I am wrong and convox is saying this could be used in production?

@ddollar

This comment has been minimized.

Copy link
Member

ddollar commented Nov 8, 2016

I would not use this PR in production mainly because it has branched from master and has not received later updates, such as security-related AMI updates.

We will likely still add support for ALB but still have open questions as to the cost/benefit tradeoff that we are working through in #1373

@jkurz

This comment has been minimized.

Copy link

jkurz commented Nov 8, 2016

I understand @ddollar just wanted to clarify, as it was not clear coming from the blog post. The blog post reads like this is going to be the solution.

@ddollar

This comment has been minimized.

Copy link
Member

ddollar commented Nov 8, 2016

Thanks for asking! Hopefully these comments will help clear that up.

@ddollar ddollar deleted the alb branch Dec 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment