provider/gce: Fix updates for http_health_check (set defaults) #1894
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mixture of hard and soft tabs, which isn't picked up by `go fmt` because it's inside a string. Standardise on hard-tabs since that is what's used in the rest of the code.
By first creating a very simple resource that mostly uses the default
values and then changing the two thresholds from their computed defaults.
This currently fails with the following error and will be fixed in a
subsequent commit:
--- FAIL: TestAccComputeHttpHealthCheck_update (5.58s)
testing.go:131: Step 1 error: Error applying: 1 error(s) occurred:
* 1 error(s) occurred:
* 1 error(s) occurred:
* Error patching HttpHealthCheck: googleapi: Error 400: Invalid value for field 'resource.port': '0'. Must be greater than or equal to 1
More details:
Reason: invalid, Message: Invalid value for field 'resource.port': '0'. Must be greater than or equal to 1
Reason: invalid, Message: Invalid value for field 'resource.checkIntervalSec': '0'. Must be greater than or equal to 1
Reason: invalid, Message: Invalid value for field 'resource.timeoutSec': '0'. Must be greater than or equal to 1
In order to fix the failing test in the preceding commit when optional params are changed from their default "computed" values. These weren't working well with `HttpHealthCheck.Patch()` because it was attempting to set all unspecified params to Go's type defaults (eg. 0 for int64) which the API rejected. Changing the call to `HttpHealthCheck.Update()` seemed to fix this but it still didn't allow you to reset a param back to it's default by no longer specifying it. Settings defaults like this, which match the Terraform docs, seems like the best all round solution. Includes two additional tests for the acceptance tests which verify the params are really getting set correctly.
dcarley
added a commit
to alphagov/paas-alpha-tsuru-terraform
that referenced
this pull request
May 12, 2015
For all load balancer health checks on AWS and GCE. Using variables so that they are always the same. Most of these values have been reduced because it was taking a very long time for new instances to come into service. Change the following: - interval to 5s which is the minimum supported by AWS. This has reduced AWS from 30s and increased GCE from 1s. - timeout to 2s which is the minimum supported by AWS. This has reduced AWS from 5s and increased GCE from 1s. - healthy threshold to 2 requests. This has not changed AWS or GCE. - unhealthy threshold to 2 requests. This has changed AWS from 10 and not changed GCE. I'm not 100% confident about the values. They weren't thoroughly tested when we first introduced them for GCE and I suspect we might want to experiment with them in the future, but this is a good start. I've changed the target for API on AWS from the default of `TCP:8080` to `HTTP:8080/info` in order to match GCE and give a more accurate check. Other targets remain as their defaults but we have to pass them because they're mandatory. They don't match GCE because GCE can't do TCP health checks. This will *not* apply cleanly to existing GCE environments due to a bug in Terraform. This should be fixed in the future by hashicorp/terraform#1894. But for the timebeing I think it's important enough that we should delete existing forwarding rules, target pools, and health checks, then let Terraform recreate them with the correct config.
|
Thanks, and this is interesting... It seems the lesson here is that the APIs don't allow an easy way to return a resource back to its default values. Terraform needs that unless it is driving the defaults itself. So I guess Terraform will have to drive all default values. That applies to more GCP resources than just this one. I am less certain about whether it is a good idea to change the defaults away from the Google defaults. Won't this change behavior for existing users? Do we need a state transfer function? Personally I think it's probably better to keep the Google defaults as the Terraform defaults. @phinze what do you think? |
|
Ah sorry, I misread the chain of commits -- you are using the google defaults after all so ignore that last paragraph in my previous comment :) |
sparkprime
added a commit
that referenced
this pull request
May 12, 2015
provider/gce: Fix updates for http_health_check (set defaults)
|
@sparkprime don't forget to update the CHANGELOG too after merging :) |
|
Ah yes, will do from now on :) |
|
Thanks! |
dcarley
added a commit
to alphagov/paas-alpha-tsuru-terraform
that referenced
this pull request
May 13, 2015
For all load balancer health checks on AWS and GCE. Using variables so that they are always the same. Most of these values have been reduced because it was taking a very long time for new instances to come into service. Change the following: - interval to 5s which is the minimum supported by AWS. This has reduced AWS from 30s and increased GCE from 1s. - timeout to 2s which is the minimum supported by AWS. This has reduced AWS from 5s and increased GCE from 1s. - healthy threshold to 2 requests. This has not changed AWS or GCE. - unhealthy threshold to 2 requests. This has changed AWS from 10 and not changed GCE. I'm not 100% confident about the values. They weren't thoroughly tested when we first introduced them for GCE and I suspect we might want to experiment with them in the future, but this is a good start. I've changed the target for API on AWS from the default of `TCP:8080` to `HTTP:8080/info` in order to match GCE and give a more accurate check. Other targets remain as their defaults but we have to pass them because they're mandatory. They don't match GCE because GCE can't do TCP health checks. This will *not* apply cleanly to existing GCE environments due to a bug in Terraform. This should be fixed in the future by hashicorp/terraform#1894. But for the timebeing I think it's important enough that we should delete existing forwarding rules, target pools, and health checks, then let Terraform recreate them with the correct config.
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
provider/gce: Test updates to http_health_check
By first creating a very simple resource that mostly uses the default
values and then changing the two thresholds from their computed defaults.
This currently fails with the following error and will be fixed in a
subsequent commit:
provider/gce: Set defaults for http_health_check
In order to fix the failing test in the preceding commit when optional
params are changed from their default "computed" values.
These weren't working well with
HttpHealthCheck.Patch()because it wasattempting to set all unspecified params to Go's type defaults (eg. 0 for
int64) which the API rejected.
Changing the call to
HttpHealthCheck.Update()seemed to fix this but itstill didn't allow you to reset a param back to it's default by no longer
specifying it.
Settings defaults like this, which match the Terraform docs, seems like the
best all round solution. Includes two additional tests for the acceptance
tests which verify the params are really getting set correctly.