Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Platform] Config cleanup #6904

Open
sb-yb opened this issue Jan 18, 2021 · 1 comment
Open

[Platform] Config cleanup #6904

sb-yb opened this issue Jan 18, 2021 · 1 comment
Assignees
@sb-yb
Labels
area/platform Yugabyte Platform

Comments

@sb-yb
Copy link
Contributor

sb-yb commented Jan 18, 2021 •
edited
Loading

At present the way to set config is a mess. One has to set it in many different places.
The bug is for
1> Moving as many values from various application.conf, application.test.conf, replicated.yml, helm charts etc. to reference.conf
2> Cleaning up usages of deprecated play configuration class from java code and use typesafe config. This means move the defaults set in code and set them in reference.conf
3> There are few unsolved cases. Come up with solution for this.
e.g. While reference.conf solves it for most cases, it fails to solve it for cases where we need to override values in reference.conf for libraries. So there needs to be some crafty solution to this problem.

One way to solve it is to use system properties in build.sbt as we have done here:
https://phabricator.dev.yugabyte.com/D10386
There can be better way of solving the issue like config file inclusion.
@sb-yb sb-yb self-assigned this Jan 18, 2021
@sb-yb sb-yb added the area/platform Yugabyte Platform label Jan 18, 2021
@sb-yb
Copy link
Contributor Author

sb-yb commented Jan 18, 2021 •
edited
Loading

Another option for problem in point 3 above is to use application ids. though it has its downside.
https://www.playframework.com/documentation/1.4.x/ids

sb-yb added a commit that referenced this issue Jan 20, 2021
@sb-yb
[#6817][#6904] Platform: Allow large requests.
18a5673 
Summary:
Bumping up maxMemoryBuffer allocated by play framework to read the request payload.
Also introduce common config to tackle overriding `reference.conf` of other libraries.

Test Plan:
- Unit test added to test the limit has increased and enforced.
- Built yb_release ran yugaware and checked the logs for value of maxMemoryBuffer that is resolved:
```
$ ls conf/*.common.conf
conf/application.common.conf
$
$ grep -o maxMemoryBuffer........ logs/application.log
maxMemoryBuffer":"500k"
maxMemoryBuffer":"500k"
```

Reviewers: arnav, sanketh, spotachev, daniel

Reviewed By: daniel

Subscribers: jenkins-bot, yugaware

Differential Revision: https://phabricator.dev.yugabyte.com/D10386
sb-yb added a commit that referenced this issue Feb 23, 2021
@sb-yb
[#6904] Platform: UnitTests for application config for various produc…
6e7d330 
…tion deployments.

Summary:
Add unittests for all the deployment configuration.
This loads configs for respective deployment with all the fallbacks and compares
against expected resolved configuration.
The test covers:
    - replicated
    - helm
    - yugabyted
Note: No test added for dev application.conf in this PR though there is no
reason why it cannot be added.

On failure the test will generate a filtered view of the actual deployment config with few deployment specific environment variables and parameter values hard-coded. This config is stored in file named with prefix `candidate-` under `/tmp` folder.
An entry is added to generated file if any of these conditions are met:
      - There is existing entry for that path in expected config
      - The path is one of the special (ALWAYS_GENERATE_PATH_PREFIXES
         like "yb. or "db." )
      - The path exist in both configs but the values do not match

This should allow us to make intrusive config changes with confidence
and ensure that our changes are getting resolved correctly i.e. not
incorrectly overridden etc.

Example log for failed test:
```
[info] Test com.yugabyte.yw.ConfigTest.singleKey(test.replicated.params.conf, replicated.expected.conf, envRepl) [0] started
[info] Test com.yugabyte.yw.ConfigTest.singleKey(test.helm.params.conf, helm.expected.conf, envHelm) [1] started
----------------------------------------------------------------------------------------------------
  Actual config written to: /tmp/helm.expected.conf-5523010180921619498.candidate
----------------------------------------------------------------------------------------------------
[error] Test com.yugabyte.yw.ConfigTest.singleKey(test.helm.params.conf, helm.expected.conf, envHelm) [1] failed: junit.framework.AssertionFailedError:
[error] Expected: db.default.url = "jdbc:postgresql://localhost:5432/yugaware"
[error] Actual: db.default.url = "jdbc:postgresql://127.0.0.1:5432/yugaware" expected:<Quoted("jdbc:postgresql://localhost:5432/yugaware")> but was:<Quoted("jdbc:postgresql://127.0.0.1:5432/yugaware")>, took 0.037 sec
...
[error]     at com.yugabyte.yw.ConfigTest.singleKey(ConfigTest.java:116)
[error]     ...

$ diff /home/sbapat/code/yugabyte-db/managed/src/test/resources/helm.expected.conf   /tmp/helm.expected.conf-5523010180921619498.candidate
11a12,13
> db.default.port = 5432
> db.default.url = "jdbc:postgresql://127.0.0.1:5432/yugaware"
```

Additional changes
- This change also renames yugabyted specific config file which was named
  `application.default.conf` to `application.yugabyte.conf`

- Both helm and replicated had a bug that flywaydb plugin was added twice.
  This PR change also fixes that bug.

Test Plan: This is a testonly change (mostly)

Reviewers: sanketh, wesley, arnav, daniel

Reviewed By: daniel

Subscribers: jenkins-bot, yugaware

Differential Revision: https://phabricator.dev.yugabyte.com/D10656
polarweasel pushed a commit to lizayugabyte/yugabyte-db that referenced this issue Mar 9, 2021
@sb-yb
[yugabyte#6817][yugabyte#6904] Platform: Allow large requests.
bf8aee4 
Summary:
Bumping up maxMemoryBuffer allocated by play framework to read the request payload.
Also introduce common config to tackle overriding `reference.conf` of other libraries.

Test Plan:
- Unit test added to test the limit has increased and enforced.
- Built yb_release ran yugaware and checked the logs for value of maxMemoryBuffer that is resolved:
```
$ ls conf/*.common.conf
conf/application.common.conf
$
$ grep -o maxMemoryBuffer........ logs/application.log
maxMemoryBuffer":"500k"
maxMemoryBuffer":"500k"
```

Reviewers: arnav, sanketh, spotachev, daniel

Reviewed By: daniel

Subscribers: jenkins-bot, yugaware

Differential Revision: https://phabricator.dev.yugabyte.com/D10386
polarweasel pushed a commit to lizayugabyte/yugabyte-db that referenced this issue Mar 9, 2021
@sb-yb
[yugabyte#6904] Platform: UnitTests for application config for variou…
368a21b 
…s production deployments.

Summary:
Add unittests for all the deployment configuration.
This loads configs for respective deployment with all the fallbacks and compares
against expected resolved configuration.
The test covers:
    - replicated
    - helm
    - yugabyted
Note: No test added for dev application.conf in this PR though there is no
reason why it cannot be added.

On failure the test will generate a filtered view of the actual deployment config with few deployment specific environment variables and parameter values hard-coded. This config is stored in file named with prefix `candidate-` under `/tmp` folder.
An entry is added to generated file if any of these conditions are met:
      - There is existing entry for that path in expected config
      - The path is one of the special (ALWAYS_GENERATE_PATH_PREFIXES
         like "yb. or "db." )
      - The path exist in both configs but the values do not match

This should allow us to make intrusive config changes with confidence
and ensure that our changes are getting resolved correctly i.e. not
incorrectly overridden etc.

Example log for failed test:
```
[info] Test com.yugabyte.yw.ConfigTest.singleKey(test.replicated.params.conf, replicated.expected.conf, envRepl) [0] started
[info] Test com.yugabyte.yw.ConfigTest.singleKey(test.helm.params.conf, helm.expected.conf, envHelm) [1] started
----------------------------------------------------------------------------------------------------
  Actual config written to: /tmp/helm.expected.conf-5523010180921619498.candidate
----------------------------------------------------------------------------------------------------
[error] Test com.yugabyte.yw.ConfigTest.singleKey(test.helm.params.conf, helm.expected.conf, envHelm) [1] failed: junit.framework.AssertionFailedError:
[error] Expected: db.default.url = "jdbc:postgresql://localhost:5432/yugaware"
[error] Actual: db.default.url = "jdbc:postgresql://127.0.0.1:5432/yugaware" expected:<Quoted("jdbc:postgresql://localhost:5432/yugaware")> but was:<Quoted("jdbc:postgresql://127.0.0.1:5432/yugaware")>, took 0.037 sec
...
[error]     at com.yugabyte.yw.ConfigTest.singleKey(ConfigTest.java:116)
[error]     ...

$ diff /home/sbapat/code/yugabyte-db/managed/src/test/resources/helm.expected.conf   /tmp/helm.expected.conf-5523010180921619498.candidate
11a12,13
> db.default.port = 5432
> db.default.url = "jdbc:postgresql://127.0.0.1:5432/yugaware"
```

Additional changes
- This change also renames yugabyted specific config file which was named
  `application.default.conf` to `application.yugabyte.conf`

- Both helm and replicated had a bug that flywaydb plugin was added twice.
  This PR change also fixes that bug.

Test Plan: This is a testonly change (mostly)

Reviewers: sanketh, wesley, arnav, daniel

Reviewed By: daniel

Subscribers: jenkins-bot, yugaware

Differential Revision: https://phabricator.dev.yugabyte.com/D10656
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sb-yb sb-yb

Labels
area/platform Yugabyte Platform
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@sb-yb