[config] Move global config alongside core configuration #30788

ctiller · 2022-08-29T22:39:32Z

This is a big rewrite of global config.

It does a few things, all somewhat intertwined:

centralize the list of configuration we have to a yaml file that can be parsed, and code generated from it
add an initialization and a reset stage so that config vars can be centrally accessed very quickly without the need for caching them
makes the syntax more C++ like (less macros!)
(optionally) adds absl flags to the OSS build

This first round of changes is intended to keep the system where it is without major changes. We pick up absl flags to match internal code and remove one point of deviation - but importantly continue to read from the environment variables. In doing so we don't force absl flags on our customers - it's possible to configure grpc without the flags - but instead allow users that do use absl flags to configure grpc using that mechanism. Importantly this lets internal customers configure grpc the same everywhere.

Future changes along this path will be two-fold:

Move documentation generation into the code generation step, so that within the source of truth yaml file we can find all documentation and data about a configuration knob - eliminating the chance of forgetting to document something in all the right places.
Provide fuzzing over configurations. Currently most config variables get stashed in static constants across the codebase. To fuzz over these we'd need a way to reset those cached values between fuzzing rounds, something that is terrifically difficult right now, but with these changes should simply be a reset on ConfigVars.

…tal-pollution

Automated fix for refs/heads/environmental-pollution

…o environmental-pollution

Automated fix for refs/heads/environmental-pollution

…o environmental-pollution

Automated fix for refs/heads/environmental-pollution

src/core/ext/filters/client_channel/resolver/dns/dns_resolver_selection.h

markdroth · 2023-03-14T00:05:48Z

src/core/ext/filters/client_channel/backup_poller.cc

@@ -61,19 +61,10 @@ static backup_poller* g_poller = nullptr;  // guarded by g_poller_mu
 static grpc_core::Duration g_poll_interval =
    grpc_core::Duration::Milliseconds(DEFAULT_POLL_INTERVAL_MS);

-GPR_GLOBAL_CONFIG_DEFINE_INT32(


One drawback of centralizing the definition of these config params is that it will be less obvious when there are no longer any users of a given parameter and it can be removed. Just as one example, prior to this PR, when we eventually remove this backup poller, this parameter would have been removed at the same time; with this PR, it might be easier to forget. Is there a reasonable way we can have some automated check for this kind of thing?

This isn't a blocker, but would be good to add some check for this if we have a reasonable way to do so.

test/core/security/security_connector_test.cc

test/cpp/end2end/rls_end2end_test.cc

test/cpp/end2end/server_load_reporting_end2end_test.cc

src/core/lib/resource_quota/memory_quota.cc

…tal-pollution

markdroth · 2023-03-15T18:57:08Z

test/core/end2end/fixtures/h2_ssl_cred_reload_tls12.cc

@@ -55,9 +55,14 @@ int main(int argc, char** argv) {

  grpc::testing::TestEnvironment env(&argc, argv);
  grpc_end2end_tests_pre_init();
+<<<<<<< HEAD


Looks like some leftover cruft from a merge.

Automated fix for refs/heads/environmental-pollution

This reverts commit e5fea5f.

…o environmental-pollution

Automated fix for refs/heads/environmental-pollution

gnossen

Correct Cython the first try? Impossible.

LGTM to the src/python/**/* changes.

jtattermusch · 2023-03-20T14:49:19Z

This very likely break the ObjC bazel test:

It started failing right after this PR got merged:
https://source.cloud.google.com/results/invocations/48b5072d-16e2-4b81-827b-c16763a8fd38/log

In file included from src/objective-c/tests/CronetTests/CoreCronetEnd2EndTests.mm:44:
In file included from ./src/core/lib/security/credentials/credentials.h:43:
In file included from ./src/core/lib/security/security_connector/security_connector.h:37:
In file included from ./src/core/lib/iomgr/closure.h:34:
In file included from ./src/core/lib/iomgr/error.h:38:
./src/core/lib/slice/slice_internal.h:82:10: warning: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]
  return absl::HashOf(grpc_core::StringViewFromSlice(s));
  ~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/objective-c/tests/CronetTests/CoreCronetEnd2EndTests.mm:122:25: error: use of undeclared identifier 'grpc_default_ssl_roots_file_path'
  GPR_GLOBAL_CONFIG_SET(grpc_default_ssl_roots_file_path, roots_filename);

In fact, this was correctly indicated by the "ObjC Bazel Test" presubmit test (which is red on this PR).
https://source.cloud.google.com/results/invocations/b1d333dd-f5aa-4d49-a0dd-4e0ef0f52923

)" This reverts commit b7a8330.

…2659) Reverts #30788 (it breaks grpc_objc_bazel_test (see #30788 (comment)) and also seems to be breaking some other internal stuff).

This is a big rewrite of global config. It does a few things, all somewhat intertwined: 1. centralize the list of configuration we have to a yaml file that can be parsed, and code generated from it 2. add an initialization and a reset stage so that config vars can be centrally accessed very quickly without the need for caching them 3. makes the syntax more C++ like (less macros!) 4. (optionally) adds absl flags to the OSS build This first round of changes is intended to keep the system where it is without major changes. We pick up absl flags to match internal code and remove one point of deviation - but importantly continue to read from the environment variables. In doing so we don't force absl flags on our customers - it's possible to configure grpc without the flags - but instead allow users that do use absl flags to configure grpc using that mechanism. Importantly this lets internal customers configure grpc the same everywhere. Future changes along this path will be two-fold: 1. Move documentation generation into the code generation step, so that within the source of truth yaml file we can find all documentation and data about a configuration knob - eliminating the chance of forgetting to document something in all the right places. 2. Provide fuzzing over configurations. Currently most config variables get stashed in static constants across the codebase. To fuzz over these we'd need a way to reset those cached values between fuzzing rounds, something that is terrifically difficult right now, but with these changes should simply be a reset on `ConfigVars`.  --------- Co-authored-by: ctiller <ctiller@users.noreply.github.com>

…pc#32659) Reverts grpc#30788 (it breaks grpc_objc_bazel_test (see grpc#30788 (comment)) and also seems to be breaking some other internal stuff).

This is a big rewrite of global config. It does a few things, all somewhat intertwined: 1. centralize the list of configuration we have to a yaml file that can be parsed, and code generated from it 2. add an initialization and a reset stage so that config vars can be centrally accessed very quickly without the need for caching them 3. makes the syntax more C++ like (less macros!) 4. (optionally) adds absl flags to the OSS build This first round of changes is intended to keep the system where it is without major changes. We pick up absl flags to match internal code and remove one point of deviation - but importantly continue to read from the environment variables. In doing so we don't force absl flags on our customers - it's possible to configure grpc without the flags - but instead allow users that do use absl flags to configure grpc using that mechanism. Importantly this lets internal customers configure grpc the same everywhere. Future changes along this path will be two-fold: 1. Move documentation generation into the code generation step, so that within the source of truth yaml file we can find all documentation and data about a configuration knob - eliminating the chance of forgetting to document something in all the right places. 2. Provide fuzzing over configurations. Currently most config variables get stashed in static constants across the codebase. To fuzz over these we'd need a way to reset those cached values between fuzzing rounds, something that is terrifically difficult right now, but with these changes should simply be a reset on `ConfigVars`.  --------- Co-authored-by: ctiller <ctiller@users.noreply.github.com>

…2659) Reverts #30788 (it breaks grpc_objc_bazel_test (see #30788 (comment)) and also seems to be breaking some other internal stuff).

wip

12982ee

github-actions bot added the lang/core label Aug 29, 2022

ctiller and others added 13 commits August 30, 2022 07:24

grpc compiles

0d465f7

progress

8af5620

Merge branch 'master' of https://github.com/grpc/grpc into environmen…

08b5cd5

…tal-pollution

delete experiments

4c9e175

Merge https://github.com/grpc/grpc into environmental-pollution

9e863dc

fix

d388e1b

fix

d1a4f7d

x

7cc3d01

fix

78c15f4

progress

631621d

Automated change: Fix sanity tests

7202cd8

Merge pull request #886 from ctiller/create-pull-request/patch-631621d

e42008b

Automated fix for refs/heads/environmental-pollution

Automated change: Fix sanity tests

743b75e

grpc-checks bot added per-call-memory/neutral per-channel-memory/neutral labels Sep 18, 2022

Merge pull request #887 from ctiller/create-pull-request/patch-e42008b

b5895b8

Automated fix for refs/heads/environmental-pollution

grpc-checks bot added the bloat/improvement label Sep 18, 2022

ctiller and others added 8 commits September 18, 2022 08:20

fix

82f038c

Merge branch 'environmental-pollution' of github.com:ctiller/grpc int…

2e90fda

…o environmental-pollution

fix

1713285

Automated change: Fix sanity tests

ae6886a

Merge pull request #888 from ctiller/create-pull-request/patch-1713285

c9a6c8e

Automated fix for refs/heads/environmental-pollution

Merge https://github.com/grpc/grpc into environmental-pollution

b27f10d

Merge branch 'environmental-pollution' of github.com:ctiller/grpc int…

0f19bb6

…o environmental-pollution

fix

15c8b48

ctiller added the release notes: no Indicates if PR should not be in release notes label Sep 19, 2022

ctiller added 2 commits September 18, 2022 22:10

convert stuff

69830e6

fix

78e3e95

Merge pull request #1144 from ctiller/create-pull-request/patch-2cb652d

902cd0e

Automated fix for refs/heads/environmental-pollution

markdroth reviewed Mar 14, 2023

View reviewed changes

src/core/lib/resource_quota/memory_quota.cc Outdated Show resolved Hide resolved

ctiller added 2 commits March 15, 2023 11:40

Merge branch 'master' of https://github.com/grpc/grpc into environmen…

71ecd98

…tal-pollution

review feedback

59d9caa

markdroth approved these changes Mar 15, 2023

View reviewed changes

ctiller and others added 11 commits March 15, 2023 12:50

x

9d5b3db

fix

22e8113

Automated change: Fix sanity tests

722fa5c

Merge pull request #1151 from ctiller/create-pull-request/patch-22e8113

a4f3fbf

Automated fix for refs/heads/environmental-pollution

fix

e5fea5f

Revert "fix"

0960d8b

This reverts commit e5fea5f.

fix

041c1e5

Merge https://github.com/grpc/grpc into environmental-pollution

3b853fb

Merge branch 'environmental-pollution' of github.com:ctiller/grpc int…

6c18425

…o environmental-pollution

Automated change: Fix sanity tests

2aee083

Merge pull request #1152 from ctiller/create-pull-request/patch-6c18425

cca7adf

Automated fix for refs/heads/environmental-pollution

gnossen approved these changes Mar 17, 2023

View reviewed changes

Merge https://github.com/grpc/grpc into environmental-pollution

420eddd

ctiller enabled auto-merge (squash) March 17, 2023 22:56

ctiller merged commit b7a8330 into grpc:master Mar 17, 2023

jtattermusch added a commit that referenced this pull request Mar 20, 2023

Revert "[config] Move global config alongside core configuration (#30788

5a84f18

)" This reverts commit b7a8330.

jtattermusch mentioned this pull request Mar 20, 2023

Revert "[config] Move global config alongside core configuration" #32659

Merged

ctiller pushed a commit that referenced this pull request Mar 20, 2023

Revert "[config] Move global config alongside core configuration" (#3…

0c1797c

…2659) Reverts #30788 (it breaks grpc_objc_bazel_test (see #30788 (comment)) and also seems to be breaking some other internal stuff).

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Mar 20, 2023

wanlin31 pushed a commit that referenced this pull request May 18, 2023

Revert "[config] Move global config alongside core configuration" (#3…

f0ae73a

…2659) Reverts #30788 (it breaks grpc_objc_bazel_test (see #30788 (comment)) and also seems to be breaking some other internal stuff).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[config] Move global config alongside core configuration #30788

[config] Move global config alongside core configuration #30788

ctiller commented Aug 29, 2022 •

edited

markdroth Mar 14, 2023

markdroth Mar 15, 2023

gnossen left a comment

jtattermusch commented Mar 20, 2023

[config] Move global config alongside core configuration #30788

[config] Move global config alongside core configuration #30788

Conversation

ctiller commented Aug 29, 2022 • edited

markdroth Mar 14, 2023

Choose a reason for hiding this comment

markdroth Mar 15, 2023

Choose a reason for hiding this comment

gnossen left a comment

Choose a reason for hiding this comment

jtattermusch commented Mar 20, 2023

ctiller commented Aug 29, 2022 •

edited