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

Limit length of load balancer names #6812

Closed
a-robinson opened this issue Apr 14, 2015 · 16 comments · Fixed by #7609
Closed

Limit length of load balancer names #6812

a-robinson opened this issue Apr 14, 2015 · 16 comments · Fixed by #7609
Labels
priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Milestone

Comments

@a-robinson
Copy link
Contributor

GCE has a length limit of 63 characters for its load balancer components (target pools and forwarding rules), and AWS has a length limit of 32 characters. Our current load balancer name construction of clustername-namespace-servicename can exceed both limits, especially AWS's. We should use a shorter name construction method less likely to hit these limits.

@a-robinson a-robinson added priority/backlog Higher priority than priority/awaiting-more-evidence. team/cluster labels Apr 14, 2015
@ghost
Copy link

ghost commented Apr 14, 2015

Thanks for filing this Alex. My suggestion would be to use a simple UID, as per utils.NewUID(). We'll need to store the mapping from {clustername, namespace, servicename} -> UID somewhere stable like etcd. Using anything based directly on {cluster, namespace, name}, for example a stable hash of each, has the problem that the aforementioned tuple is not unique over time. This raises the possibility of e.g. the following scenario:

  1. user creates mycluster/mynamespace/myservicename, which creates external load balancer named stablehash(mycluster,mynamespace,myservicename)
  2. user deletes mycluster/mynamespace/myservicename, but at the time MyCloudProvider is down, so kubernetes can't delete ELB named stablehash(mycluster,mynamespace,myservicename). Kubernetes keeps trying in the background to delete the ELB.
  3. In the mean time user creates a new service named the same as her old one mycluster/mynamespace/myservicename. Kubernetes tries to create an associated ELB named stablehash(mycluster,mynamespace,myservicename), but this fails, because the ELB exists already due to 2 above.
    ... etc. Badness ensues.

@ghost ghost added this to the v1.0 milestone Apr 14, 2015
@ghost
Copy link

ghost commented Apr 14, 2015

Assigning to Chao Xu, as this makes for a good starter bug.

@ghost
Copy link

ghost commented Apr 14, 2015

Hmmmm.... can't seem to assign to caesarxuchao yet. Not sure why.

@a-robinson
Copy link
Contributor Author

He has to have write privileges on the repo before issues/PRs can be assigned to him.

@ghost
Copy link

ghost commented Apr 14, 2015

But we have many contributors with issues assigned to them that are not in the kubernetes-write group (that only has 48 members). I must be missing something. How do I grant him write privileges on the repo?

@ghost ghost self-assigned this Apr 14, 2015
@a-robinson
Copy link
Contributor Author

We do? That's not how I understand it to work: https://help.github.com/articles/permission-levels-for-an-organization-repository/

If you want to add him, go here.

@a-robinson
Copy link
Contributor Author

@caesarxuchao: #7145 has been causing issues for a user because the load balancer name changes if the service's UID changes (e.g. if it's deleted and recreated). Think we can change this?

A hash of cluster name, namespace, and service name should get around it, but there may be better ideas as well.

@a-robinson a-robinson reopened this Apr 28, 2015
@a-robinson a-robinson added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed priority/backlog Higher priority than priority/awaiting-more-evidence. labels Apr 28, 2015
@caesarxuchao
Copy link
Member

Sure. I'm not confident that I can fix this quickly. If it's urgent, maybe you can assign some one more experienced to fix it?

One question:
I guess the ELB's name should stay unchanged as long as the "zone+namespace+service name" doesn't change, am I right?

@ghost
Copy link

ghost commented Apr 28, 2015

I think I'm the guilty party here. I was unaware that when we create a new
service with the same name as a previously existing service, that it needed
to keep the same load balancer name and IP. This seems broken to me, but
I may be missing the larger context.

We can quite safely roll back the change while we decide on the best
alternative solution.

Q

On Mon, Apr 27, 2015 at 6:46 PM, Chao Xu notifications@github.com wrote:

Sure. I'm not confident that I can fix this quickly. If it's urgent, maybe
you can assign some one more experienced to fix it?

One question:
I guess the ELB's name should stay unchanged as long as the
"zone+namespace+service name" doesn't change, am I right?


Reply to this email directly or view it on GitHub
#6812 (comment)
.

@thockin
Copy link
Member

thockin commented Apr 28, 2015

I think the real breakage is that we don't have any way to claim an LB-IP
at a larger lifespan than a single service. Alex, can you go back to OP
and get a sense of what's happening there to trigger this?

On Mon, Apr 27, 2015 at 7:09 PM, Quinton Hoole notifications@github.com
wrote:

I think I'm the guilty party here. I was unaware that when we create a new
service with the same name as a previously existing service, that it needed
to keep the same load balancer name and IP. This seems broken to me, but
I may be missing the larger context.

We can quite safely roll back the change while we decide on the best
alternative solution.

Q

On Mon, Apr 27, 2015 at 6:46 PM, Chao Xu notifications@github.com wrote:

Sure. I'm not confident that I can fix this quickly. If it's urgent,
maybe
you can assign some one more experienced to fix it?

One question:
I guess the ELB's name should stay unchanged as long as the
"zone+namespace+service name" doesn't change, am I right?


Reply to this email directly or view it on GitHub
<
#6812 (comment)

.


Reply to this email directly or view it on GitHub
#6812 (comment)
.

@a-robinson
Copy link
Contributor Author

Will do. Even if the name is kept consistent as it was before, with the way everything currently works the IP won't be consistent.

@brendandburns
Copy link
Contributor

yeah, I'm not positive that the name has to be consistent. What we need is a way to claim the same public IP address, and we already have that (at least for GCE)

Assigning to me, and I'll validate the length requirements, make sure public ip assignment works for AWS and close out this issue.

@alex-mohr
Copy link
Contributor

Note there are two separate things here: (1) a purely-name-based construction can exceed length limits given a single service and (2) some form of across-service consistency is apparently needed.

(1) is trivially solvable by either always using a hash or alternatively using a two-stage process where you generate a desired name and only hashify if it's longer than the length limits by truncating it and appending a hash of said name.

@a-robinson
Copy link
Contributor Author

AWS's load balancer hasn't been merged yet, so there's not much to check out there.

I'm going to be reducing the churn of load balancers later today, which should help prevent IP address changes for services that are modified.

@thockin
Copy link
Member

thockin commented May 1, 2015

I think we need to be able to figure out which Service an LB is attached to
from the Service, so that LBs can't get orphaned.

@justinsb regarding being able to claim an LB IP. I don't think that is
covered in revamp...

On Thu, Apr 30, 2015 at 10:11 AM, Brendan Burns notifications@github.com
wrote:

yeah, I'm not positive that the name has to be consistent. What we need is
a way to claim the same public IP address, and we already have that (at
least for GCE)

Assigning to me, and I'll validate the length requirements, make sure
public ip assignment works for AWS and close out this issue.


Reply to this email directly or view it on GitHub
#6812 (comment)
.

@ghost
Copy link

ghost commented May 3, 2015

I think that we need to roll back #7609 and think this issue through a bit better. See comment on that PR.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@caesarxuchao @thockin @brendandburns @a-robinson @alex-mohr and others