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

Add slow-start support in vs/vsr #655

Merged
merged 1 commit into from
Aug 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/virtualserver-and-virtualserverroute.md
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ tls:
| `client-max-body-size` | Sets the maximum allowed size of the client request body. See the [client_max_body_size](https://nginx.org/en/docs/http/ngx_http_core_module.html#client_max_body_size) directive. The default is set in the `client-max-body-size` ConfigMap key. | `string` | No |
| `tls` | The TLS configuration for the Upstream. | [`tls`](#UpstreamTLS) | No |
| `healthCheck` | The health check configuration for the Upstream. See the [health_check](http://nginx.org/en/docs/http/ngx_http_upstream_hc_module.html#health_check) directive. Note: this feature is supported only in NGINX Plus. | [`healthcheck`](#UpstreamHealthcheck) | No |
| `slow-start` | The slow start allows an upstream server to gradually recover its weight from 0 to its nominal value after it has been recovered or became available or when the server becomes available after a period of time it was considered unavailable. By default, the slow start is disabled. See the [slow_start](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#slow_start) parameter of the server directive. Note: The parameter cannot be used along with the `random`, `hash` or `ip_hash` load balancing methods and will be ignored. | `string` | No |

### Upstream.TLS

Expand Down
1 change: 1 addition & 0 deletions internal/configs/version2/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ type UpstreamServer struct {
MaxConns int
FailTimeout string
Resolve bool
SlowStart string
}

// Server defines a server.
Expand Down
2 changes: 1 addition & 1 deletion internal/configs/version2/nginx-plus.virtualserver.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ upstream {{ $u.Name }} {
{{ if $u.LBMethod }}{{ $u.LBMethod }};{{ end }}

{{ range $s := $u.Servers }}
server {{ $s.Address }} max_fails={{ $s.MaxFails }} fail_timeout={{ $s.FailTimeout }} max_conns={{ $s.MaxConns }}{{ if $s.Resolve }} resolve{{ end }};
server {{ $s.Address }} max_fails={{ $s.MaxFails }} fail_timeout={{ $s.FailTimeout }}{{ if $s.SlowStart }} slow_start={{ $s.SlowStart }}{{ end }} max_conns={{ $s.MaxConns }}{{ if $s.Resolve }} resolve{{ end }};
{{ end }}

{{ if $u.Keepalive }}
Expand Down
1 change: 1 addition & 0 deletions internal/configs/version2/templates_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ var virtualServerCfg = VirtualServerConfig{
MaxFails: 5,
FailTimeout: "10s",
MaxConns: 31,
SlowStart: "10s",
},
},
LBMethod: "random",
Expand Down
41 changes: 37 additions & 4 deletions internal/configs/virtualserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,15 @@ import (

const nginx502Server = "unix:/var/run/nginx-502-server.sock"

var incompatibleLBMethodsForSlowStart = map[string]bool{
"random": true,
"ip_hash": true,
"random two": true,
"random two least_conn": true,
"random two least_time=header": true,
"random two least_time=last_byte": true,
}

// VirtualServerEx holds a VirtualServer along with the resources that are referenced in this VirtualServer.
type VirtualServerEx struct {
VirtualServer *conf_v1alpha1.VirtualServer
Expand Down Expand Up @@ -211,7 +220,7 @@ func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileNam
endpoints := generateEndpointsForUpstream(upstreamNamespace, u, virtualServerEx, isResolverConfigured, isPlus)
// isExternalNameSvc is always false for OSS
_, isExternalNameSvc := virtualServerEx.ExternalNameSvcs[GenerateExternalNameSvcKey(upstreamNamespace, u.Service)]
ups := generateUpstream(upstreamName, u, isExternalNameSvc, endpoints, baseCfgParams)
ups := generateUpstream(upstreamName, u, isExternalNameSvc, endpoints, baseCfgParams, isPlus)
upstreams = append(upstreams, ups)
crUpstreams[upstreamName] = u

Expand All @@ -231,7 +240,7 @@ func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileNam
endpoints := generateEndpointsForUpstream(upstreamNamespace, u, virtualServerEx, isResolverConfigured, isPlus)
// isExternalNameSvc is always false for OSS
_, isExternalNameSvc := virtualServerEx.ExternalNameSvcs[GenerateExternalNameSvcKey(upstreamNamespace, u.Service)]
ups := generateUpstream(upstreamName, u, isExternalNameSvc, endpoints, baseCfgParams)
ups := generateUpstream(upstreamName, u, isExternalNameSvc, endpoints, baseCfgParams, isPlus)
upstreams = append(upstreams, ups)
crUpstreams[upstreamName] = u

Expand Down Expand Up @@ -332,8 +341,9 @@ func generateVirtualServerConfig(virtualServerEx *VirtualServerEx, tlsPemFileNam
}
}

func generateUpstream(upstreamName string, upstream conf_v1alpha1.Upstream, isExternalNameSvc bool, endpoints []string, cfgParams *ConfigParams) version2.Upstream {
func generateUpstream(upstreamName string, upstream conf_v1alpha1.Upstream, isExternalNameSvc bool, endpoints []string, cfgParams *ConfigParams, isPlus bool) version2.Upstream {
var upsServers []version2.UpstreamServer
lbMethod := generateLBMethod(upstream.LBMethod, cfgParams.LBMethod)

for _, e := range endpoints {
s := version2.UpstreamServer{
Expand All @@ -343,18 +353,41 @@ func generateUpstream(upstreamName string, upstream conf_v1alpha1.Upstream, isEx
MaxConns: generateIntFromPointer(upstream.MaxConns, cfgParams.MaxConns),
Resolve: isExternalNameSvc,
}

if isPlus {
s.SlowStart = generateSlowStartForPlus(upstream, lbMethod)
}

upsServers = append(upsServers, s)
}

return version2.Upstream{
Name: upstreamName,
Servers: upsServers,
LBMethod: generateLBMethod(upstream.LBMethod, cfgParams.LBMethod),
LBMethod: lbMethod,
Keepalive: generateIntFromPointer(upstream.Keepalive, cfgParams.Keepalive),
UpstreamZoneSize: cfgParams.UpstreamZoneSize,
}
}

func generateSlowStartForPlus(upstream conf_v1alpha1.Upstream, lbMethod string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

this function needs to be rewritten as it doesn't implement the slow start logic completely. The logic is below:

  • if slowStart is "" , return ""
  • if the lbMethod is either random, ip_hash or hash, print a warning and return "" . Note that the hash method is defined as hash + some varialble(s), for example $hash $request_id. The current implementation incorrectly allows methods like $hash $request_id. Also, the random method is similar to the hash, but has a finite number of possible values. Please see them here -- https://github.com/nginxinc/kubernetes-ingress/blob/master/internal/configs/parsing_helpers.go#L122-L126 Those should be excluded as well.
  • return slowStart

Also, the corresponding unit test must be rewritten as well. For the test, consider the following test cases:

Positive cases:

  • slow start is "" and the method is "least_conn", expects ""
  • slow_start is "10s" and the method is "least_conn", expects "10s"

Negative cases:

Please add any additional cases if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks better now! Great job. I am not 100% sure about the incompatibleLBMethodsForSlowStart so let's wait til @pleshakov has another look 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Normally we have a preference for using a map like: https://github.com/nginxinc/kubernetes-ingress/blob/master/pkg/apis/configuration/validation/validation.go#L343-L357 which I think should be possible.

If sticking with a slice is necessary, you should be able to make it const , right?

if upstream.SlowStart == "" {
return ""
}

if strings.HasPrefix(lbMethod, "hash") {
glog.Warningf("slow-start will be disabled for the Upstream %v", upstream.Name)
return ""
}

if _, exists := incompatibleLBMethodsForSlowStart[lbMethod]; exists {
glog.Warningf("slow-start will be disabled for the Upstream %v", upstream.Name)
return ""
}

return upstream.SlowStart
}

func generateLBMethod(method string, defaultMethod string) string {
if method == "" {
return defaultMethod
Expand Down
62 changes: 58 additions & 4 deletions internal/configs/virtualserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -767,7 +767,7 @@ func TestGenerateVirtualServerConfigForVirtualServerWithRules(t *testing.T) {

func TestGenerateUpstream(t *testing.T) {
name := "test-upstream"
upstream := conf_v1alpha1.Upstream{Service: name, Port: 80}
upstream := conf_v1alpha1.Upstream{Service: name, Port: 80, SlowStart: "10s"}
endpoints := []string{
"192.168.10.10:8080",
}
Expand All @@ -788,14 +788,15 @@ func TestGenerateUpstream(t *testing.T) {
MaxFails: 1,
MaxConns: 0,
FailTimeout: "10s",
SlowStart: "",
},
},
LBMethod: "random",
Keepalive: 21,
UpstreamZoneSize: "256k",
}

result := generateUpstream(name, upstream, false, endpoints, &cfgParams)
result := generateUpstream(name, upstream, false, endpoints, &cfgParams, true)
if !reflect.DeepEqual(result, expected) {
t.Errorf("generateUpstream() returned %v but expected %v", result, expected)
}
Expand Down Expand Up @@ -859,7 +860,7 @@ func TestGenerateUpstreamWithKeepalive(t *testing.T) {
}

for _, test := range tests {
result := generateUpstream(name, test.upstream, false, endpoints, test.cfgParams)
result := generateUpstream(name, test.upstream, false, endpoints, test.cfgParams, false)
if !reflect.DeepEqual(result, test.expected) {
t.Errorf("generateUpstream() returned %v but expected %v for the case of %v", result, test.expected, test.msg)
}
Expand All @@ -882,7 +883,7 @@ func TestGenerateUpstreamForExternalNameService(t *testing.T) {
},
}

result := generateUpstream(name, upstream, true, endpoints, &cfgParams)
result := generateUpstream(name, upstream, true, endpoints, &cfgParams, false)
if !reflect.DeepEqual(result, expected) {
t.Errorf("generateUpstream() returned %v but expected %v", result, expected)
}
Expand Down Expand Up @@ -1962,3 +1963,56 @@ func TestGenerateEndpointsForUpstream(t *testing.T) {
}
}
}

func TestGenerateSlowStartForPlusWithInCompatibleLBMethods(t *testing.T) {
serviceName := "test-slowstart-with-incompatible-LBMethods"
upstream := conf_v1alpha1.Upstream{Service: serviceName, Port: 80, SlowStart: "10s"}
expected := ""

var tests = []string{
"random",
"ip_hash",
"hash 123",
"random two",
"random two least_conn",
"random two least_time=header",
"random two least_time=last_byte",
}

for _, lbMethod := range tests {
result := generateSlowStartForPlus(upstream, lbMethod)

if !reflect.DeepEqual(result, expected) {
t.Errorf("generateSlowStartForPlus returned %v, but expected %v for lbMethod %v", result, expected, lbMethod)
}
}

}

func TestGenerateSlowStartForPlus(t *testing.T) {
serviceName := "test-slowstart"

tests := []struct {
upstream conf_v1alpha1.Upstream
lbMethod string
expected string
}{
{
upstream: conf_v1alpha1.Upstream{Service: serviceName, Port: 80, SlowStart: "", LBMethod: "least_conn"},
lbMethod: "least_conn",
expected: "",
},
{
upstream: conf_v1alpha1.Upstream{Service: serviceName, Port: 80, SlowStart: "10s", LBMethod: "least_conn"},
lbMethod: "least_conn",
expected: "10s",
},
}

for _, test := range tests {
result := generateSlowStartForPlus(test.upstream, test.lbMethod)
if !reflect.DeepEqual(result, test.expected) {
t.Errorf("generateSlowStartForPlus returned %v, but expected %v", result, test.expected)
}
}
}
1 change: 1 addition & 0 deletions pkg/apis/configuration/v1alpha1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ type Upstream struct {
ClientMaxBodySize string `json:"client-max-body-size"`
TLS UpstreamTLS `json:"tls"`
HealthCheck *HealthCheck `json:"healthCheck"`
SlowStart string `json:"slow-start"`
}

// UpstreamTLS defines a TLS configuration for an Upstream.
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/configuration/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,7 @@ func validateUpstreams(upstreams []v1alpha1.Upstream, fieldPath *field.Path, isP
allErrs = append(allErrs, validatePositiveIntOrZeroFromPointer(u.MaxConns, idxPath.Child("max-conns"))...)
allErrs = append(allErrs, validateSize(u.ClientMaxBodySize, idxPath.Child("client-max-body-size"))...)
allErrs = append(allErrs, validateUpstreamHealthCheck(u.HealthCheck, idxPath.Child("healthCheck"))...)
allErrs = append(allErrs, validateTime(u.SlowStart, idxPath.Child("slow-start"))...)

for _, msg := range validation.IsValidPortNum(int(u.Port)) {
allErrs = append(allErrs, field.Invalid(idxPath.Child("port"), u.Port, msg))
Expand Down