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

Enable loadbalancer.sticky for ECS #1925

Merged
merged 6 commits into from Aug 25, 2017

Conversation

mmatur
Copy link
Member

@mmatur mmatur commented Aug 7, 2017

Description

The goal of this PR is to add support of sticky session for ECS.
Low balancer configuration will be always present with default values:
traefik.backend.loadbalancer.method=wrr
traefik.backend.loadbalancer.sticky=false

To override these values you just to add labels

Fixes #1522

@ldez
Copy link
Member

ldez commented Aug 7, 2017

Could you fix the build?

Errors from golint:
provider/ecs/ecs.go:435:1: exported method Provider.LoadBalancerSticky should have comment or be unexported
provider/ecs/ecs.go:444:1: exported method Provider.LoadBalancerMethod should have comment or be unexported

Please fix the above errors. You can test via "golint" and commit the result.

@mmatur
Copy link
Member Author

mmatur commented Aug 8, 2017

This PR is the implementation for feature requested on #1522

Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

Thanks! 👍


func (p *Provider) LoadBalancerMethod(instances []ecsInstance) string {
for instances != nil && len(instances) > 0 {
if label := instances[0].label(types.LabelBackendLoadbalancerMethod); label != "" {
Copy link
Member

Choose a reason for hiding this comment

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

could you split in two lines?

@@ -420,6 +432,24 @@ func (p *Provider) getFrontendRule(i ecsInstance) string {
return "Host:" + strings.ToLower(strings.Replace(i.Name, "_", "-", -1)) + "." + p.Domain
}

func (p *Provider) LoadBalancerSticky(instances []ecsInstance) string {
for instances != nil && len(instances) > 0 {
if label := instances[0].label(types.LabelBackendLoadbalancerSticky); label != "" {
Copy link
Member

Choose a reason for hiding this comment

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

could you split in two lines?

@@ -420,6 +432,24 @@ func (p *Provider) getFrontendRule(i ecsInstance) string {
return "Host:" + strings.ToLower(strings.Replace(i.Name, "_", "-", -1)) + "." + p.Domain
}

func (p *Provider) LoadBalancerSticky(instances []ecsInstance) string {
for instances != nil && len(instances) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

instances != nil is not needed

}

func (p *Provider) LoadBalancerMethod(instances []ecsInstance) string {
for instances != nil && len(instances) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

instances != nil is not needed

@@ -420,6 +432,24 @@ func (p *Provider) getFrontendRule(i ecsInstance) string {
return "Host:" + strings.ToLower(strings.Replace(i.Name, "_", "-", -1)) + "." + p.Domain
}

func (p *Provider) getLoadBalancerSticky(instances []ecsInstance) string {
for instances != nil && len(instances) > 0 {
if label := instances[0].label(types.LabelBackendLoadbalancerSticky); label != "" {
Copy link
Member

Choose a reason for hiding this comment

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

could you split in two lines?


func (p *Provider) getLoadBalancerMethod(instances []ecsInstance) string {
for instances != nil && len(instances) > 0 {
if label := instances[0].label(types.LabelBackendLoadbalancerMethod); label != "" {
Copy link
Member

Choose a reason for hiding this comment

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

could you split in two lines?

@@ -420,6 +432,24 @@ func (p *Provider) getFrontendRule(i ecsInstance) string {
return "Host:" + strings.ToLower(strings.Replace(i.Name, "_", "-", -1)) + "." + p.Domain
}

func (p *Provider) getLoadBalancerSticky(instances []ecsInstance) string {
for instances != nil && len(instances) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

instances != nil is unnecessary.

func ExampleEmptySlice() {
	var foo []string
	fii := bar()
	fmt.Println(foo, fii)
	// output: [] []
}
func bar() []string{
	return nil
}

}

func (p *Provider) getLoadBalancerMethod(instances []ecsInstance) string {
for instances != nil && len(instances) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

instances != nil is unnecessary.

func ExampleEmptySlice() {
	var foo []string
	fii := bar()
	fmt.Println(foo, fii)
	// output: [] []
}
func bar() []string{
	return nil
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the go course. I will fix that asap

@@ -189,10 +191,20 @@ func (p *Provider) loadECSConfig(ctx context.Context, client *awsClient) (*types

instances = fun.Filter(p.filterInstance, instances).([]ecsInstance)

services := make(map[string][]ecsInstance)

for _, i := range instances {
Copy link
Member

Choose a reason for hiding this comment

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

could you rename i to instance ?

I know it's already use but I think i is an ambiguous name in a for.

@ldez ldez changed the title [ecs] - Enable loadbalancer.sticky Enable loadbalancer.sticky Aug 8, 2017
@ldez ldez modified the milestone: 1.4 Aug 10, 2017
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

@ldez ldez added the size/S label Aug 16, 2017
Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM 👏

@ldez ldez changed the title Enable loadbalancer.sticky Enable loadbalancer.sticky for ECS Aug 22, 2017
@traefiker traefiker merged commit 086a85d into traefik:master Aug 25, 2017
@mmatur mmatur deleted the session-stickiness-ecs branch August 25, 2017 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants