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

Conversation

bvighnesha
Copy link

@bvighnesha bvighnesha commented Aug 13, 2019

Proposed changes

Describe the use case and detail of the change. If this PR addresses an issue on GitHub, make sure to include a link to that issue here in this description (not in the title of the PR).

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

@bvighnesha bvighnesha self-assigned this Aug 13, 2019
@bvighnesha bvighnesha requested a review from Rulox August 13, 2019 11:21
@bvighnesha bvighnesha force-pushed the feature/support_slow_start_in_Plus branch 3 times, most recently from 3a004c1 to c15e1a4 Compare August 14, 2019 14:35
Copy link
Contributor

@Rulox Rulox left a comment

Choose a reason for hiding this comment

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

Hey @b-v-r thanks for the PR!

I've found 2 small issues and made some suggestion to fix them. Let me know what you think

@@ -342,6 +347,12 @@ func generateUpstream(upstreamName string, upstream conf_v1alpha1.Upstream, isEx
FailTimeout: generateString(upstream.FailTimeout, cfgParams.FailTimeout),
MaxConns: generateIntFromPointer(upstream.MaxConns, cfgParams.MaxConns),
Resolve: isExternalNameSvc,
SlowStart: upstream.SlowStart,
}
if incompatibleLBMethods[upstream.LBMethod] {
Copy link
Contributor

Choose a reason for hiding this comment

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

If upstream.LBMethod is not defined, the cfgParams.LBMethod will be used (check line 363 of this file: generateLBMethod(upstream.LBMethod, cfgParams.LBMethod),). That's why using the upstream.LBMethod here is not the good option.

I suggest create a variable lbMethod = generateLBMethod(upstream.LBMethod, cfgParams.LBMethod), and use that variable here incompatibleLBMethods[lbMethod] and in line 363.

Copy link
Contributor

Choose a reason for hiding this comment

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

Apart from that, I would move this check outside the for loop. All the servers for the upstream will work the same way. This way we only do the check once, and also we can create a variable for slowStart and make everything more explicit (instead of changing the s.SlowStart afterwards)

I mean this:

	incompatibleLBMethods := map[string]bool{
		"random":  true,
		"ip-hash": true,
		"hash":    true,
	}
	lbMethod := generateLBMethod(upstream.LBMethod, cfgParams.LBMethod)
	slowStart := upstream.SlowStart
	if upstream.SlowStart != "" &&  incompatibleLBMethods[lbMethod] {
		// do the warning
		slowStart = ""
	}

	for _, e := range endpoints {
		s := version2.UpstreamServer{
			Address:     e,
			MaxFails:    generateIntFromPointer(upstream.MaxFails, cfgParams.MaxFails),
			FailTimeout: generateString(upstream.FailTimeout, cfgParams.FailTimeout),
			MaxConns:    generateIntFromPointer(upstream.MaxConns, cfgParams.MaxConns),
			Resolve:     isExternalNameSvc,
                         SlowStart: slowStart,
		}
		upsServers = append(upsServers, s)
	}


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

Let me know your thoughts!
PS: Code is not tested, just to point the idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would further suggest putting the logic for checking for incompatible method in a separate function and writing a unit test for that function.

	lbMethod := generateLBMethod(upstream.LBMethod, cfgParams.LBMethod)
	slowStart := 	for _, e := range endpoints {
		s := version2.UpstreamServer{
			Address:     e,
			MaxFails:    generateIntFromPointer(upstream.MaxFails, cfgParams.MaxFails),
			FailTimeout: generateString(upstream.FailTimeout, cfgParams.FailTimeout),
			MaxConns:    generateIntFromPointer(upstream.MaxConns, cfgParams.MaxConns),
			Resolve:     isExternalNameSvc,
                         SlowStart:  generateSlowStart(lbMethod, upstream.SlowStart, isPlus)

,
		}
		upsServers = append(upsServers, s)
	}


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

Note that isPlus will have to be added to the list of generateUpstream arguments. We need it because slow start only works for NGINX Plus.

Additionally, also note that hash method is defined by hash + some varialble(s), for example $hash $request_id. As a result, the check incompatibleLBMethods[lbMethod] for hash method won't work. We need to check for the prefix, not the exact match here. Please also see the implementation of ParseLBMethod function https://github.com/nginxinc/kubernetes-ingress/blob/master/internal/configs/parsing_helpers.go#L92

Also, I suggest renaming incompatibleLBMethods to incompatibleLBMethodsForSlowStart and making it a global variable similar to https://github.com/nginxinc/kubernetes-ingress/blob/master/internal/configs/parsing_helpers.go#L119 so that the map is only created once.

}
if incompatibleLBMethods[upstream.LBMethod] {
//TODO trigger warning
glog.Warningf("The parameter slow-start cannot be used along with %s, load balancing method", upstream.LBMethod)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can add also, at the end of the message, something like: "slow-start will be disabled for the Upstream %v", Upstream.Name, to make sure the user understands that slow-start is not working even if they set it due to conflicts. What do you think?

Also, the warning doesn't make sense when SlowStart == "", so we should check that (eg, if slowStart = "" it means the user didn't set it, so there's no point of checking and warning the user)

PS: @pleshakov I think is better if we use a warning for now, because emitting events from virtualserver.go can be tricky. Maybe we can do that in a future PR? Revisiting the warnings/errors and make sure to update them to Events when necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Rulox warningf for now works OK. we can add a PR later to support emitting events for warnings.

Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

@b-v-r please see my comments.

Additionally, I suggest renaming the commit message and PR name to Add slow-start support in vs/vsr so that it reflects that we're adding the slow-start support to our VirtualServer/VirtualServerRoutes resources. This will also be consistent with the previous similar commits https://github.com/nginxinc/kubernetes-ingress/commits/master

@@ -212,6 +212,7 @@ tls:
| `next-upstream-tries` | The number of possible tries for passing a request to the next upstream server. See the [proxy_next_upstream_tries](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_next_upstream_tries) directive. The `0` value turns off this limit. The default is `0`. | `int` | 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. Default value is zero seconds ("0s"), i.e. 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 hash and ip_hash load balancing methods. | `string` | No |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `slow-start` | The slowstart 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. Default value is zero seconds ("0s"), i.e. 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 hash and ip_hash load balancing methods. | `string` | 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 |

@@ -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 }};
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to use the curly brackets in a consistent manner.

Suggested change
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 }};
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 }};

@@ -238,7 +238,7 @@ func TestGenerateVirtualServerConfig(t *testing.T) {
Name: "vs_default_cafe_tea",
Copy link
Contributor

Choose a reason for hiding this comment

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

please fix the formatting in this file using gofmt.

@@ -342,6 +347,12 @@ func generateUpstream(upstreamName string, upstream conf_v1alpha1.Upstream, isEx
FailTimeout: generateString(upstream.FailTimeout, cfgParams.FailTimeout),
MaxConns: generateIntFromPointer(upstream.MaxConns, cfgParams.MaxConns),
Resolve: isExternalNameSvc,
SlowStart: upstream.SlowStart,
}
if incompatibleLBMethods[upstream.LBMethod] {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would further suggest putting the logic for checking for incompatible method in a separate function and writing a unit test for that function.

	lbMethod := generateLBMethod(upstream.LBMethod, cfgParams.LBMethod)
	slowStart := 	for _, e := range endpoints {
		s := version2.UpstreamServer{
			Address:     e,
			MaxFails:    generateIntFromPointer(upstream.MaxFails, cfgParams.MaxFails),
			FailTimeout: generateString(upstream.FailTimeout, cfgParams.FailTimeout),
			MaxConns:    generateIntFromPointer(upstream.MaxConns, cfgParams.MaxConns),
			Resolve:     isExternalNameSvc,
                         SlowStart:  generateSlowStart(lbMethod, upstream.SlowStart, isPlus)

,
		}
		upsServers = append(upsServers, s)
	}


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

Note that isPlus will have to be added to the list of generateUpstream arguments. We need it because slow start only works for NGINX Plus.

Additionally, also note that hash method is defined by hash + some varialble(s), for example $hash $request_id. As a result, the check incompatibleLBMethods[lbMethod] for hash method won't work. We need to check for the prefix, not the exact match here. Please also see the implementation of ParseLBMethod function https://github.com/nginxinc/kubernetes-ingress/blob/master/internal/configs/parsing_helpers.go#L92

Also, I suggest renaming incompatibleLBMethods to incompatibleLBMethodsForSlowStart and making it a global variable similar to https://github.com/nginxinc/kubernetes-ingress/blob/master/internal/configs/parsing_helpers.go#L119 so that the map is only created once.

}
if incompatibleLBMethods[upstream.LBMethod] {
//TODO trigger warning
glog.Warningf("The parameter slow-start cannot be used along with %s, load balancing method", upstream.LBMethod)
Copy link
Contributor

Choose a reason for hiding this comment

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

@Rulox warningf for now works OK. we can add a PR later to support emitting events for warnings.

@@ -41,6 +41,7 @@ type Upstream struct {
ProxyNextUpstreamTries int `json:"next-upstream-tries"`
TLS UpstreamTLS `json:"tls"`
HealthCheck *HealthCheck `json:"healthCheck"`
SlowStart string `json:"slow-start,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

omitempty is not necessary here

@bvighnesha bvighnesha force-pushed the feature/support_slow_start_in_Plus branch from c15e1a4 to 29e22eb Compare August 19, 2019 05:12
@bvighnesha bvighnesha changed the title Add slow-start in plus feature/support_slow_start_in_Plus Aug 19, 2019
@bvighnesha bvighnesha changed the title feature/support_slow_start_in_Plus support_slow_start_in_Plus Aug 19, 2019
@bvighnesha bvighnesha force-pushed the feature/support_slow_start_in_Plus branch from 29e22eb to ce8439c Compare August 19, 2019 05:20
@bvighnesha bvighnesha changed the title support_slow_start_in_Plus dd slow-start support in vs/vsr Aug 19, 2019
@bvighnesha bvighnesha changed the title dd slow-start support in vs/vsr Add slow-start support in vs/vsr Aug 19, 2019
@bvighnesha bvighnesha force-pushed the feature/support_slow_start_in_Plus branch from ce8439c to 849b35c Compare August 19, 2019 06:26
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

Please see my comments.

Additionally, multiple files are not properly formatted. Make sure the code is formatted using gofmt tool. You can also configure your editor to do that when saving a file.

docs/virtualserver-and-virtualserverroute.md Outdated Show resolved Hide resolved
var upsServers []version2.UpstreamServer
lbMethod := generateLBMethod(upstream.LBMethod, cfgParams.LBMethod)
slowStart := generateSlowStartForPlus(upstream, lbMethod)
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of

		if isPlus {
			s.SlowStart = slowStart
		}

I suggest to do the following:

slowStart := ""
if isPlus {
  slowStart := generateSlowStartForPlus(upstream, lbMethod)
}

this way generateSlowStartForPlus will only be invoked for Plus.

@@ -354,6 +366,23 @@ func generateUpstream(upstreamName string, upstream conf_v1alpha1.Upstream, isEx
}
}

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.

the empty line is not necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

this wasn't addressed. we don't put empty lines at the beginning of the function

Copy link
Contributor

Choose a reason for hiding this comment

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

the empty line is back :) please remove

@@ -354,6 +366,23 @@ func generateUpstream(upstreamName string, upstream conf_v1alpha1.Upstream, isEx
}
}

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:

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?

Copy link
Contributor

@Rulox Rulox left a comment

Choose a reason for hiding this comment

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

Looks good. 👍

Just small comments in the TestCases only for consistency. Overall looks good, waiting on @pleshakov final review too as I wasn't sure of 1 thing.

@@ -1960,3 +1961,51 @@ func TestGenerateEndpointsForUpstream(t *testing.T) {
}
}
}

func TestGenerateSlowStartForPlus(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

TestGenerateSlowStartForPlus doesn't follow the rest of the file consistency.

  1. Why do we need the { } blocks delimiters? I think they are redundant
  2. If there are more than 1 test case, normally we define the test cases in a slice of structs tests := []struct {...} and iterate over them. For instance, we could have 2 different Test functions. 1 for the incompatible methods and another one for compatible ones. That way you can still use the for loop in one of the functions (iterating over incompatibleLBMethodsForSlowStart) and another function using a []struct {...} for the other 2 cases with valid lbMethods, to keep consistency with the rest of the file.
  3. Comments with all the cases are not needed as code should be auto explained (unless there's a weird edge case there)

Let me know if this makes sense to you and your thoughts!


for _, nLBMethod := range incompatibleLBMethodsForSlowStart {
if strings.Contains(lbMethod, nLBMethod) {
//TODO trigger warning
Copy link
Contributor

Choose a reason for hiding this comment

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

The //TODO comment is not needed. I think we should avoid things like TODO or FIXME when merging to master.

@@ -354,6 +366,23 @@ func generateUpstream(upstreamName string, upstream conf_v1alpha1.Upstream, isEx
}
}

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.

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

@Dean-Coakley Dean-Coakley left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Just some code style to fix from @Rulox and my feedback and I'll approve.

@@ -354,6 +366,23 @@ func generateUpstream(upstreamName string, upstream conf_v1alpha1.Upstream, isEx
}
}

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.

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?

Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

Please see my suggestions +1 to @Rulox and @Dean-Coakley 's comments

return ""
}

for _, nLBMethod := range incompatibleLBMethodsForSlowStart {
Copy link
Contributor

Choose a reason for hiding this comment

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

to check for the incompatible method, I suggest using approach similar to https://github.com/nginxinc/kubernetes-ingress/blob/master/internal/configs/parsing_helpers.go#L141-L148 :

  1. first check if the method starts with hash
  2. check if the method is in the map. This will require changing incompatibleLBMethodsForSlowStart to a map.

Copy link
Contributor

@Dean-Coakley Dean-Coakley left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@@ -1960,3 +1961,44 @@ func TestGenerateEndpointsForUpstream(t *testing.T) {
}
}
}

func TestGenerateSlowStartForPlusWithInCompatibleLBMethods(t *testing.T) {
name := "test-slowstart-with-incompatible-LBMethods"
Copy link
Contributor

Choose a reason for hiding this comment

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

Completely optional: but personally I'd name this var serviceName or change the value to include the suffix -svc

At first glance I thought this was a name of a test case.

(Same applies to TestGenerateSlowStartForPlus)

Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

@b-v-r thanks for the updates. Please see my comments

if slowStart == "" {
return ""
}
method := strings.TrimSpace(lbMethod)
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no need for TrimSpace, as lbMethod in this function has already been parsed.

}
method := strings.TrimSpace(lbMethod)
if strings.HasPrefix(method, "hash") {
method = "hash"
Copy link
Contributor

Choose a reason for hiding this comment

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

for readability and clarity, it is better to return "" and print a warning in this block right way, rather than assigning it to the method variable.

@@ -14,6 +14,16 @@ import (

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

var incompatibleLBMethodsForSlowStart = map[string]bool{
"random": true,
"ip-hash": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

the correct method is ip_hash

}

for _, test := range tests {
lbMethod := generateLBMethod(test.upstream.LBMethod, "least_conn")
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no need to call generateLBMethod. I suggest making the method the third field of the struct you use in tests array.

upstream := conf_v1alpha1.Upstream{Service: name, Port: 80, SlowStart: "10s"}
expected := ""

for lbMethod, _ := range incompatibleLBMethodsForSlowStart {
Copy link
Contributor

Choose a reason for hiding this comment

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

it is better to have an array of the input parameters like you have in the TestGenerateSlowStartForPlus. this will make the tests much more readable.

expected := ""

for lbMethod, _ := range incompatibleLBMethodsForSlowStart {
nlbMethod := generateLBMethod(upstream.LBMethod, lbMethod)
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no need to call generateLBMethod. this is the same comment as for line 1998.

@bvighnesha bvighnesha force-pushed the feature/support_slow_start_in_Plus branch 2 times, most recently from cb5cc3d to 8423006 Compare August 22, 2019 03:21
@bvighnesha bvighnesha requested a review from ampant August 22, 2019 04:02
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

@b-v-r thanks for the changes. looks like we're almost there. please see my suggestions.

var incompatibleLBMethodsForSlowStart = map[string]bool{
"random": true,
"ip_hash": true,
"hash": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

}

for _, test := range tests {
for lbMethod, _ := range incompatibleLBMethodsForSlowStart {
Copy link
Contributor

Choose a reason for hiding this comment

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

this suggestion wasn't implemented #655 (comment) please have a list of input parameters rather than taking lb methods from incompatibleLBMethodsForSlowStart

Keepalive: generateIntFromPointer(upstream.Keepalive, cfgParams.Keepalive),
}
}

func generateSlowStartForPlus(upstream conf_v1alpha1.Upstream, lbMethod string) string {
slowStart := upstream.SlowStart
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't usually create auxiliary variables like this one. Note that is only used twice. Can we just use upstream.SlowStart instead?

@ampant ampant requested a review from pleshakov August 23, 2019 21:02
@bvighnesha bvighnesha force-pushed the feature/support_slow_start_in_Plus branch from 8423006 to 6e18d66 Compare August 25, 2019 02:28
@bvighnesha
Copy link
Author

@pleshakov can you please review changes

Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

@b-v-r please see the comments about the new changes

internal/configs/virtualserver_test.go Outdated Show resolved Hide resolved
@@ -354,6 +366,23 @@ func generateUpstream(upstreamName string, upstream conf_v1alpha1.Upstream, isEx
}
}

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.

the empty line is back :) please remove

internal/configs/virtualserver_test.go Outdated Show resolved Hide resolved
@pleshakov pleshakov added the enhancement Pull requests for new features/feature enhancements label Aug 27, 2019
@bvighnesha bvighnesha force-pushed the feature/support_slow_start_in_Plus branch from 4546260 to facb764 Compare August 28, 2019 03:41
Copy link
Contributor

@Rulox Rulox left a comment

Choose a reason for hiding this comment

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

lgtm :shipit: 🚀

@bvighnesha bvighnesha merged commit 924726b into master Aug 28, 2019
@Rulox Rulox deleted the feature/support_slow_start_in_Plus branch September 18, 2019 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants