Support for maximum size for Output of checks #5233

pierresouchay · 2019-01-17T09:27:38Z

This PR will allow administrator to limit the size of Output produced by healthchecks,
when set at agent level, it will be the maximum output for all healthchecks produced
by agents.

It will also allow specific healthchecks to go below the limit set by administrator,
OutputMaxSize if set in check specification will now allow to go beyond agent level
setting, but will allow to reduce it further.

We had the following configuration at agent level

check_output_max_size (default: 4k) : max number for all checks of the agent
check.OutputMaxSize: (default: 4k)

orarnon · 2019-01-17T09:34:26Z

I can say that we have suffered from big health-checks. We had to use the "args" argument and wrap the health-check with a script. A more native way of reducing the output would have made a big difference while waiting for developers to change or add a param to their endpoint.

This PR will allow administrator to limit the size of Output produced by healthchecks, when set at agent level, it will be the maximum output for all healthchecks produced by agents. It will also allow specific healthchecks to go below the limit set by administrator, OutputMaxSize if set in check specification will now allow to go beyond agent level setting, but will allow to reduce it further. We had the following configuration at agent level * check_output_max_size (default: 4k) : max number for all checks of the agent * check.OutputMaxSize: (default: 4k)

freddygv

Hi Pierre,

Thanks for the PR, I have some comments inline.

Additionally, the website's docs should also be updated to reflect these changes.

You can add the CLI and config docs to: website/source/docs/agent/options.html.md, and the check docs to: website/source/api/agent/check.html.md.

agent/checks/check_test.go

agent/agent.go

agent/checks/check.go

freddygv · 2019-05-20T20:52:59Z

agent/agent.go

+		if maxOutputSize == 0 {
+			maxOutputSize = 4096
+		}
+		if chkType.OutputMaxSize > 0 && maxOutputSize > chkType.OutputMaxSize {


As you mention in your description, this will only override the agent check if the check's OutputMaxSize is less than the agent's.

Could you expand more on why we shouldn't allow the check's max to override the agent's max?

@freddygv I wanted to be as conservative as possible (meaning, that for instance, if someone is storing those results in a DB and assumed size will never be more than 4k, we won't break this assumption), but allowing large infrastructure to reduce the size of their checks since it might be hugely impacting when checks are varying a lot (for instance, the normal result if checks if 'OK', while the broken checks returns large stack traces), ask @orarnon for instance who had this issue (we did had it as well on very large clusters in the past)

Increasing this value is trivial and could be done in another patch if someone is resquesting it, but if you really want, we can remove this limit in this PR as well.

@pierresouchay Thanks. Can you please expand on the need for the check-specific max output size? Would adding a max for the agent alone be sufficient?

The implementation would be simpler if we only set this max once. Additionally, there is a UX mismatch where someone can discard output at the agent level, but at the check level the max size can only be brought down to 1.

yes, the reason for that is serialization of payload: if the user specify 0, it means no value has been specified in payload (for instance for a user not setting OutputMaxSize in the check definition and thus preserving backward compatibility.
As you pointed out in a previous comment, if the user completely want to disable checks output, it can be done by disabling completely check output.

agent/config/builder.go

* Renamed `checks.BufSize` to `checks.DefaultBufSize` * Use the constant where appropriate * Better error message when `check_output_max_size` has wrong value

pierresouchay · 2019-05-22T09:50:41Z

@freddygv all DONE

pierresouchay · 2019-06-03T23:26:31Z

@freddygv the license/cla does not pass while I agreed for a while (before this PR) the license. Is there is a way to pass this test (that used to pass) ? I have the same issue in #4548, #5455 and #4576

freddygv · 2019-06-03T23:59:00Z

@pierresouchay should be fixed now, let us know if it happens again.

Will send back some more comments about this PR tomorrow.

freddygv

@pierresouchay Thanks for the updates. Just added some more comments inline.

Also, I am not familiar with the output of gRPC health checks, but I'm seeing that here the resulting err.Error() isn't being truncated based on the check output max size. It seems like there should also be a truncation here since the error output could be large.

website/source/docs/agent/options.html.md

freddygv · 2019-06-03T17:37:32Z

website/source/docs/agent/options.html.md

@@ -117,6 +117,14 @@ will exit with an error at startup.
  [go-sockaddr](https://godoc.org/github.com/hashicorp/go-sockaddr/template)
  template

+* <a name="check_output_max_size"></a><a href="#check_output_max_size">`-check_output_max_size</a> -
+  Override the default limit of 4k for maximum size of checks, this is a value


I think this description could be simplified further to something like:

The maximum output size for health checks. Limiting check output size reduces pressure on Consul servers when many checks are producing large outputs. Defaults to 4096, and must be at least 1. To disable check output capture use discard_check_output.

freddygv · 2019-06-03T17:40:20Z

website/source/docs/agent/options.html.md

@@ -117,6 +117,14 @@ will exit with an error at startup.
  [go-sockaddr](https://godoc.org/github.com/hashicorp/go-sockaddr/template)
  template

+* <a name="check_output_max_size"></a><a href="#check_output_max_size">`-check_output_max_size</a> -
+  Override the default limit of 4k for maximum size of checks, this is a value
+  between 1 and 4096, setting it to 0 is not allowed.


I'm not seeing in the code a constraint that restricts max size to be <= 4096. This PR only checks the lower bound, which I think should be OK.

website/source/api/agent/check.html.md

agent/config/flags.go

freddygv · 2019-06-03T20:52:51Z

agent/agent.go

@@ -2239,6 +2243,13 @@ func (a *Agent) addCheck(check *structs.HealthCheck, chkType *structs.CheckType,

 	// Check if already registered
 	if chkType != nil {
+		maxOutputSize := a.config.CheckOutputMaxSize
+		if maxOutputSize == 0 {


This may be unnecessary. How could a.config.CheckOutputMaxSize end up as 0? The config builder has a default value of 4096 and returns an error if the input is < 1.

freddygv · 2019-06-03T21:04:21Z

agent/agent.go

+		if maxOutputSize == 0 {
+			maxOutputSize = 4096
+		}
+		if chkType.OutputMaxSize > 0 && maxOutputSize > chkType.OutputMaxSize {


@pierresouchay Thanks. Can you please expand on the need for the check-specific max output size? Would adding a max for the agent alone be sufficient?

The implementation would be simpler if we only set this max once. Additionally, there is a UX mismatch where someone can discard output at the agent level, but at the check level the max size can only be brought down to 1.

agent/agent_endpoint.go

@freddygv

suggestion from @freddygv Co-Authored-By: Freddy <freddygv@users.noreply.github.com>

@freddygv

suggestion from @freddygv Co-Authored-By: Freddy <freddygv@users.noreply.github.com>

hanshasselberg · 2019-06-24T09:10:16Z

@pierresouchay I just looked at your PR again and I think there are some comments left.

@freddygv

suggestion from @freddygv Co-Authored-By: Freddy <freddygv@users.noreply.github.com>

freddygv · 2019-06-25T15:19:23Z

@pierresouchay Thanks for addressing those comments, this is almost ready. The last portion is to update a test around the TTL check update, so that CI can pass.

The truncation behavior was moved from AgentUpdateCheck to updateTTLCheck but the test for AgentUpdateCheck still verifies that behavior:
https://travis-ci.org/hashicorp/consul/jobs/549970434

So the sub-test TestAgent_UpdateCheck/log_output_limit can be removed and that behavior should be tested here instead.

Check that OutputMaxSize is properly taken into account. BUGFIX: properly update local state on update of TTL

pierresouchay · 2019-06-25T22:07:31Z

@freddygv I did find actually a bug doing the feature and realizing that the agent state was not updated (I found the bug while looking at weird results for TestAgent_UpdateCheck/log_output_limit ), thus I kept the existing tests and added one.

I actually wonder if it might linked to #4460

freddygv · 2019-06-26T15:43:40Z

Looks good, thanks for working through those changes!

Regarding the issue you referenced about syncChanges, it does seem like they're right, but I'd have to dig deeper to make sure it can be removed from the API endpoints.

pierresouchay · 2019-06-26T18:41:07Z

@freddygv thank you!

pierresouchay added 2 commits May 11, 2019 00:38

Added unit test

b5717aa

pierresouchay force-pushed the buffer_size_for_checks branch from 3a9cf1b to b5717aa Compare May 10, 2019 22:38

banks requested a review from a team May 14, 2019 15:36

freddygv suggested changes May 20, 2019

View reviewed changes

pierresouchay added 2 commits May 22, 2019 11:30

Fixed code comments from @freddygv

b5f00ff

* Renamed `checks.BufSize` to `checks.DefaultBufSize` * Use the constant where appropriate * Better error message when `check_output_max_size` has wrong value

Added documentation for OutputMaxSize and check_output_max_size

79a02fa

Fixed errors in HCL for some unit tests in runtime_test.go

8dcf910

pierresouchay force-pushed the buffer_size_for_checks branch from 1e957d1 to 8dcf910 Compare May 22, 2019 13:27

freddygv suggested changes Jun 4, 2019

View reviewed changes

pierresouchay and others added 3 commits June 4, 2019 18:05

Update website/source/docs/agent/options.html.md

bc0ace6

suggestion from @freddygv Co-Authored-By: Freddy <freddygv@users.noreply.github.com>

Update agent/config/flags.go

a684019

suggestion from @freddygv Co-Authored-By: Freddy <freddygv@users.noreply.github.com>

Updated doc

89b5a23

pierresouchay and others added 2 commits June 24, 2019 17:38

Ensure TTL HealthChecks are properly truncated everywhere

0a95339

Update website/source/api/agent/check.html.md

2b3df47

suggestion from @freddygv Co-Authored-By: Freddy <freddygv@users.noreply.github.com>

Properly truncate TTL checks

a68ecd0

Check that OutputMaxSize is properly taken into account. BUGFIX: properly update local state on update of TTL

No silent error when state of check TTL does not exist in agent

acce4eb

pierresouchay force-pushed the buffer_size_for_checks branch from bc82a29 to acce4eb Compare June 25, 2019 23:12

Check that check_output_max_size is enforced properly

13710da

pierresouchay force-pushed the buffer_size_for_checks branch from bf00400 to 0eebd94 Compare June 26, 2019 08:32

Truncation of TTL check at proper place aka before notification

8478180

pierresouchay force-pushed the buffer_size_for_checks branch from 0eebd94 to 8478180 Compare June 26, 2019 09:31

freddygv approved these changes Jun 26, 2019

View reviewed changes

freddygv merged commit 0e907f5 into hashicorp:master Jun 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for maximum size for Output of checks #5233

Support for maximum size for Output of checks #5233

pierresouchay commented Jan 17, 2019

orarnon commented Jan 17, 2019

freddygv left a comment

freddygv May 20, 2019

pierresouchay May 22, 2019

freddygv Jun 3, 2019

pierresouchay Jun 5, 2019

pierresouchay commented May 22, 2019

pierresouchay commented Jun 3, 2019

freddygv commented Jun 3, 2019

freddygv left a comment

freddygv Jun 3, 2019

freddygv Jun 3, 2019

freddygv Jun 3, 2019

freddygv Jun 3, 2019

hanshasselberg commented Jun 24, 2019

freddygv commented Jun 25, 2019

pierresouchay commented Jun 25, 2019 •

edited

Loading

freddygv commented Jun 26, 2019

pierresouchay commented Jun 26, 2019

Support for maximum size for Output of checks #5233

Support for maximum size for Output of checks #5233

Conversation

pierresouchay commented Jan 17, 2019

orarnon commented Jan 17, 2019

freddygv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierresouchay commented May 22, 2019

pierresouchay commented Jun 3, 2019

freddygv commented Jun 3, 2019

freddygv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanshasselberg commented Jun 24, 2019

freddygv commented Jun 25, 2019

pierresouchay commented Jun 25, 2019 • edited Loading

freddygv commented Jun 26, 2019

pierresouchay commented Jun 26, 2019

pierresouchay commented Jun 25, 2019 •

edited

Loading