Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

UB-1622 provisioner long time for start in the installation phase #291

Merged

Conversation

27149chen
Copy link
Contributor

@27149chen 27149chen commented Jan 16, 2019

helm install takes ~3.5 mins, while provisioner activation takes ~2 mins. The root cause is that the default timeout interval for a http request is very long. This PR:

  1. set a short timeout for every request (10 seconds)
  2. set a shorter timeout for activation (2 seconds) and retry 30 times if it fails.

This change is Reviewable

@27149chen 27149chen added the bug label Jan 16, 2019
@27149chen 27149chen self-assigned this Jan 16, 2019
@27149chen 27149chen requested a review from shay-berman January 16, 2019 13:25
@coveralls
Copy link

coveralls commented Jan 16, 2019

Coverage Status

Coverage decreased (-0.1%) to 58.662% when pulling 8f8a750 on feature/UB-1622_provisioner_timeout_during_installation into bf9a572 on dev.

Copy link
Contributor

@shay-berman shay-berman left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @27149chen and @shay-berman)


remote/client.go, line 54 at r1 (raw file):

	clientWithShortTimeout := new(http.Client)
	*clientWithShortTimeout = *s.httpClient
	clientWithShortTimeout.Timeout = time.Second * 2

2 seconds sound to low. i think the default is 2 minutes, so maybe set it for 10 seconds. (and also consider to reduce the retires from 30 to 5)


remote/client_init.go, line 72 at r1 (raw file):

	s.storageApiURL = fmt.Sprintf(storageAPIURL, protocol, s.config.UbiquityServer.Address, s.config.UbiquityServer.Port)
	s.httpClient = &http.Client{
		Timeout: time.Second * 10,

since its a global change - make sure QA know about it and check if it breaks something.
If its the global one and the default is 2 minutes consider to set it to 30s instead of 120s.
up to you

Copy link
Contributor Author

@27149chen 27149chen left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @shay-berman and @27149chen)


remote/client.go, line 54 at r1 (raw file):

Previously, shay-berman wrote…

2 seconds sound to low. i think the default is 2 minutes, so maybe set it for 10 seconds. (and also consider to reduce the retires from 30 to 5)

activation is a very fast action, take less then 1 second, so I think 2 second is enough. 2 * 30 = 60 while 10 * 5 = 50, so if I keep 2 seconds timeout, 30 times is reasonable, is it?


remote/client_init.go, line 72 at r1 (raw file):

Previously, shay-berman wrote…

since its a global change - make sure QA know about it and check if it breaks something.
If its the global one and the default is 2 minutes consider to set it to 30s instead of 120s.
up to you

ok, 30 is acceptable

Copy link
Contributor

@shay-berman shay-berman left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @27149chen and @olgashtivelman)


remote/client.go, line 54 at r1 (raw file):

Previously, 27149chen wrote…

activation is a very fast action, take less then 1 second, so I think 2 second is enough. 2 * 30 = 60 while 10 * 5 = 50, so if I keep 2 seconds timeout, 30 times is reasonable, is it?

I still think that 2 seconds is low, what if you have low network for a while, in that case it will never succeed.
I would set it to 5~10 seconds.
please decide and update.

BTW make sure you run staging before merge to dev.

Copy link
Contributor Author

@27149chen 27149chen left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @olgashtivelman and @shay-berman)


remote/client.go, line 54 at r1 (raw file):

Previously, shay-berman wrote…

I still think that 2 seconds is low, what if you have low network for a while, in that case it will never succeed.
I would set it to 5~10 seconds.
please decide and update.

BTW make sure you run staging before merge to dev.

ok, set to 5 seconds now, please approve this PR

@27149chen 27149chen merged commit 5d52b33 into dev Feb 10, 2019
@27149chen 27149chen deleted the feature/UB-1622_provisioner_timeout_during_installation branch March 26, 2019 08:52
@shay-berman shay-berman added this to the Helm chart support milestone Mar 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants