Add new default values to the Guardian flags `network-pool` and `max-containers` #6031

muntac · 2020-09-01T21:06:56Z

What does this PR accomplish?

Feature

Part of #5985

Changes proposed by this PR:

If --network-pool is unset Guardian defaults to 10.80.0.0/22 which allows 1024 addresses, implicitly limiting a
Guardian worker to 250 containers (4 addresses reserved per container). This is true even if max-containers is set to
a higher value.

We set network-pool to 10.80.0.0/16 to increase the allowable addresses significantly ensuring the container
count is only limited by the explicit max-containers flag. If a user passes a custom value we use that instead.

Update

Exposing both flags as options in the worker command now.
Defaulting the maximum container limit to 250 to avoid stampeding herd problem.
Defaulting containerd max-container value to do the same.

More details on reasoning in an issue comment.

Notes to reviewer:

Inspecting processes running on a Guardian worker without having set CONCOURSE_GARDEN_NETWORK_POOL shows the following.

When passing a custom value of 10.80.0.0/20:

Related PRs
concourse/concourse-chart#150
concourse/concourse-bosh-release#121

Release Note

Contributor Checklist

Followed Code of conduct, Contributing Guide & avoided Anti-patterns
Signed all commits
Added tests (Unit and/or Integration)
Updated Documentation
Added release note (Optional)

Reviewer Checklist

Code reviewed
Tests reviewed
Documentation reviewed
Release notes reviewed
PR acceptance performed
New config flags added? Ensure that they are added to the
BOSH and
Helm packaging; otherwise, ignored for
the integration
tests
(for example, if they are Garden configs that are not displayed in the
--help text).

If network-pool is unset Guardian defaults to 10.80.0.0/22 which allows 1024 addresses implicitly limiting a Guardian worker to 250 containers (4 addresses per container). This is true even if max-containers is set to a higher value. We set network-pool to 10.80.0.0/16 to increase the allowable addresses significantly ensuring the container count is only limited by the explicit max-containers flag. Signed-off-by: Muntasir Chowdhury <mchowdhury@pivotal.io>

CONCOURSE_GARDEN_ALLOW_HOST_ACCESS will have a value of "false" even without a default value being provided as it is represented as a bool in the Guardian codebase. The recommendation to have CONCOURSE_GARDEN_MAX_CONTAINERS at 250 was based off CloudFoundry's Diego which also uses Guardian. Concourse deployments have been operated at a much higher limit and do not need to be held to this limit. The binary will set CONCOURSE_GARDEN_NETWORK_POOL to a default value of 10.80.0.0/16 as of concourse/concourse#6031. concourse/concourse#5985 Signed-off-by: Muntasir Chowdhury <mchowdhury@pivotal.io>

xtreme-sameer-vohra · 2020-09-02T21:10:00Z

Would it make sense to pull this in into GuardianRuntime so its more explicit ?

chenbh · 2020-09-03T14:12:45Z

@xtreme-sameer-vohra I would say we should only move it there if we also expose max-containers for Guardian as well.

An alternative would be for us to document a list of commonly used Guardian flags like max-containers and network-pool somewhere (either in code, CLI flags, or docs site)

muntac · 2020-09-08T18:36:40Z

@xtreme-sameer-vohra @chenbh I believe Guardian specific flags were removed from the CLI listing i.e. Guardian Runtime for easier reasoning about the runtimes. The flags present under GuardianRuntime currently are Concourse specific, and all Guardian specific ones have to be looked up in the Guardian documentation. So I would prefer to note the default values in the docs site. Let me know your thoughts.

I posted a question in the BOSH release PR about determining the default value of max-containers that we would like to put here.

@xtreme-sameer-vohra I was unsure of what we should consider an average case upper limit with workload and machine variability taken into consideration.

I had thought we could have it unset initially and wait for feedback from users to see around what limits they start seeing problems. I haven't been able to find any past issues describing 250 being selected as the limit due to user feedback about performance. Mostly see stuff around the insufficient subnet problem, or hitting the max container limit.

Have you seen users run into performance problems where the issue was solved by limiting the number of containers?

muntac · 2020-09-08T20:57:33Z

I put in 250 for now, and noted in #6043 to test for new limits when we're doing drills for containerd vs guardian.

xtreme-sameer-vohra · 2020-09-08T21:47:58Z

@xtreme-sameer-vohra @chenbh I believe Guardian specific flags were removed from the CLI listing i.e. Guardian Runtime for easier reasoning about the runtimes. The flags present under GuardianRuntime currently are Concourse specific, and all Guardian specific ones have to be looked up in the Guardian documentation. So I would prefer to note the default values in the docs site. Let me know your thoughts.

I posted a question in the BOSH release PR about determining the default value of max-containers that we would like to put here.

@xtreme-sameer-vohra I was unsure of what we should consider an average case upper limit with workload and machine variability taken into consideration.
I had thought we could have it unset initially and wait for feedback from users to see around what limits they start seeing problems. I haven't been able to find any past issues describing 250 being selected as the limit due to user feedback about performance. Mostly see stuff around the insufficient subnet problem, or hitting the max container limit.
Have you seen users run into performance problems where the issue was solved by limiting the number of containers?

It would be useful to document the default values in the binary concourse worker -h and the docs site, just so a user isn't surprised. I can see it becomes simpler to reason about Concourse specific flags (in Garden Runtime) and generic Garden flags(in Garden docs) with your distinction :)

Setting default limit to 250 as this is the only default value for Guardian that has been widely tested against Concourse. This can be increased in the future with more thorough testing with production workloads. #5985 Signed-off-by: Muntasir Chowdhury <mchowdhury@pivotal.io>

CONCOURSE_GARDEN_ALLOW_HOST_ACCESS will have a value of "false" even without a default value being provided as it is represented as a bool in the Guardian codebase. The recommendation to have CONCOURSE_GARDEN_MAX_CONTAINERS at 250 was based off CloudFoundry's Diego which also uses Guardian. Concourse deployments have been operated at a much higher limit and do not need to be held to this limit. The binary will set CONCOURSE_GARDEN_NETWORK_POOL to a default value of 10.80.0.0/16 as of concourse/concourse#6031. concourse/concourse#5985 Signed-off-by: Muntasir Chowdhury <mchowdhury@pivotal.io>

concourse/concourse#6031 exposes Guardian's max-containers and network-pool as part of the worker command's options. This change keeps the chart and binary in sync. concourse/concourse#5985 Signed-off-by: Muntasir Chowdhury <mchowdhury@pivotal.io>

Limiting this to 250 similar to Guardian to avoid stampeding herd problem. With stress testing in the future this can be updated to a more accurate value. issue#5985 Signed-off-by: Muntasir Chowdhury <mchowdhury@pivotal.io>

xtreme-sameer-vohra

Hey,
Looks like the wiring logic got dropped in the refactor. The two values aren't passed to gdn at the moment. They're just exposed in Concourse.

muntac · 2020-09-17T17:26:48Z

Hey,
Looks like the wiring logic got dropped in the refactor. The two values aren't passed to gdn at the moment. They're just exposed in Concourse.

Seeing the same :( Will let you know when I have a fix.

These flags were in the GuardianRuntime struct but they weren't being detected by the detectGuardianFlags() function. Our theory is that the go-flags package is unsetting these environment variables when parsing them. Therefore when os.Environ() is called by detectGuardianFlags() the GuardianRuntime struct flags are not found. #5985 Signed-off-by: Muntasir Chowdhury <mchowdhury@pivotal.io> Co-authored-by: Taylor Silva <tsilva@pivotal.io>

muntac · 2020-09-17T20:27:07Z

@xtreme-sameer-vohra fyi @taylorsilva and I pushed a fix but all the tests are breaking because of an elm package whose signature has changed at source.

taylorsilva · 2020-09-18T15:55:30Z

The elm stuff fixed itself. I'm rerunning all test suites.

xtreme-sameer-vohra

lgtm !!

muntac added the enhancement label Sep 1, 2020

muntac mentioned this pull request Sep 1, 2020

Remove guardian flag defaults concourse/concourse-bosh-release#121

Merged

vito assigned xtreme-sameer-vohra Sep 2, 2020

muntac mentioned this pull request Sep 8, 2020

Add a runtime flag to spec files concourse/concourse-bosh-release#116

Closed

muntac mentioned this pull request Sep 8, 2020

Create BOSH Smoke Tests for Containerd #5985

Closed

muntac force-pushed the default-pool branch from 44ec35d to 846bc8f Compare September 14, 2020 20:39

muntac changed the title ~~Change default value for Guardian network-pool~~ Add new default values Guardian network-pool and max-containers Sep 14, 2020

add default for containerd max-containers

49b70b2

Limiting this to 250 similar to Guardian to avoid stampeding herd problem. With stress testing in the future this can be updated to a more accurate value. issue#5985 Signed-off-by: Muntasir Chowdhury <mchowdhury@pivotal.io>

xtreme-sameer-vohra suggested changes Sep 15, 2020

View reviewed changes

xtreme-sameer-vohra approved these changes Sep 18, 2020

View reviewed changes

muntac merged commit 64a12a7 into master Sep 21, 2020

muntac deleted the default-pool branch September 21, 2020 13:46

clarafu added the release/undocumented This didn't warrant being documented or put in release notes. label Sep 22, 2020

clarafu changed the title ~~Add new default values Guardian network-pool and max-containers~~ Add new default values to the Guardian flags network-pool and max-containers Oct 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new default values to the Guardian flags `network-pool` and `max-containers` #6031

Add new default values to the Guardian flags `network-pool` and `max-containers` #6031

muntac commented Sep 1, 2020 •

edited by taylorsilva

xtreme-sameer-vohra commented Sep 2, 2020

chenbh commented Sep 3, 2020

muntac commented Sep 8, 2020

muntac commented Sep 8, 2020

xtreme-sameer-vohra commented Sep 8, 2020

xtreme-sameer-vohra left a comment

muntac commented Sep 17, 2020

muntac commented Sep 17, 2020

taylorsilva commented Sep 18, 2020

xtreme-sameer-vohra left a comment

Add new default values to the Guardian flags network-pool and max-containers #6031

Add new default values to the Guardian flags network-pool and max-containers #6031

Conversation

muntac commented Sep 1, 2020 • edited by taylorsilva

What does this PR accomplish?

Changes proposed by this PR:

Notes to reviewer:

Release Note

Contributor Checklist

Reviewer Checklist

xtreme-sameer-vohra commented Sep 2, 2020

chenbh commented Sep 3, 2020

muntac commented Sep 8, 2020

muntac commented Sep 8, 2020

xtreme-sameer-vohra commented Sep 8, 2020

xtreme-sameer-vohra left a comment

Choose a reason for hiding this comment

muntac commented Sep 17, 2020

muntac commented Sep 17, 2020

taylorsilva commented Sep 18, 2020

xtreme-sameer-vohra left a comment

Choose a reason for hiding this comment

Add new default values to the Guardian flags `network-pool` and `max-containers` #6031

Add new default values to the Guardian flags `network-pool` and `max-containers` #6031

muntac commented Sep 1, 2020 •

edited by taylorsilva