Adding support for weighted k8s service#2293
Conversation
|
|
|
Documentation has passing and warning two values for service weights. Should we also use both? |
Passing is the primary operation mode. According to last conversation with David, implementing passing is what is required for now. |
| return nil | ||
| } | ||
|
|
||
| var perAppWeight = weightI / appsCount |
There was a problem hiding this comment.
I think we can set weight to whatever value we are getting. We don't need to divide
There was a problem hiding this comment.
- One simple reason being - if a customer is setting weight - x for a service - and if they query, they will get another value from APIs which will be divided.
- Weight are proportional - if you have two services with weights 10 and 20 respectively, the service with weight 20 would receive twice as much traffic as the service with weight 10. If we divide, we will not get the correct traffic.
There was a problem hiding this comment.
yup! we should set the weight that is set on the annotation and we dont need the app count for the reasons @absolutelightning described!
| } | ||
|
|
||
| // Sets the passing service weight | ||
| func setServiceWeight(weight string, appsCount int, r *consulapi.CatalogRegistration) error { |
There was a problem hiding this comment.
id change this method to return the service weight or an error, providing the value of the annotation as an input. we should avoid sending in the consul registration into it. it will make this is a cleaner interface that is significantly easier to test.
|
I think we need to add documentation of the new annotation in |
|
|
||
| // Adding information about service weight. | ||
| // Overrides the existing weight if present | ||
| if weight, ok := svc.Annotations[annotationServiceWeight]; ok && weight != "" { |
There was a problem hiding this comment.
these lines 567 - 577 and lines 518 - 528 are exactly the same. can we take it out in a function as well?
There was a problem hiding this comment.
The code at both place is constructing the CatalogRegistration object, there is no logic which is duplicate. Read the comment #2293 (comment)
There was a problem hiding this comment.
I think this is ok to keep as is. it appears that the only behavior here is an assignment which IMO is not really duplication.
|
A reminder that the 1.2.x branch is currently under a code freeze. If this isn't a bug fix, unless it's a critical feature for the 1.2.x-rc can you please refrain from merging this until after the 1.2.0-rc1 release (Tuesday, June 13th) |
david-yu
added
pr/no-backport
|
@david-yu can you test it on latest. If thing are working as expected, I will backport the changes. |
david-yu
added
backport/1.0.x
backport/1.1.x
|
Adding backports to 1.0.x, 1.1.x and 1.2.x. Thanks @srahul3 could you follow the backport PRs to see they are back ported correctly and passing tests? |
Context
According to the documentation Consul supports setting weights for services . Unfortunately currently there is no way of doing that when using consul-k8s, as there is no annotation that can control that + there is no easy way of doing it via Consul API and it looks like the changes will be anyway overwritten.
Related link: https://hashicorp.atlassian.net/browse/NET-4266
Changes proposed in this PR:
consul.hashicorp.com/service-weight: <number>is read and then the weight is divided among underlying ingress/ips. The calculated weight is persisted as passing weight inconsulapi.CatalogRegistrationHow I've tested this PR:
Added the Junit test for different scenarios
How I expect reviewers to test this PR:
Checklist: