[DPE-3049] templated config #186
Merged
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
dragomirp
force-pushed
the
dpe-3049-templated-config
branch
from
fb7d8aa
to
ca23974
Compare
dragomirp
force-pushed
the
dpe-3049-templated-config
branch
from
3fe6142
to
10b6f86
Compare
dragomirp
force-pushed
the
dpe-3049-templated-config
branch
from
10b6f86
to
496363b
Compare
dragomirp
force-pushed
the
dpe-3049-templated-config
branch
7 times, most recently
from
9e69f30
to
b248aa9
Compare
dragomirp
force-pushed
the
dpe-3049-templated-config
branch
from
b248aa9
to
8f0a651
Compare
dragomirp
commented
Feb 9, 2024
Comment on lines
+651
to
+670
| # Nothing set in the config peer data trying to regenerate based on old data in case of update. | ||
| elif not self.unit.is_leader(): | ||
| if cfg := self.get_secret(APP_SCOPE, CFG_FILE_DATABAG_KEY): | ||
| try: | ||
| parser = ConfigParser() | ||
| parser.optionxform = str | ||
| parser.read_string(cfg) | ||
| old_cfg = dict(parser) | ||
| if databases := old_cfg.get("databases"): | ||
| databases.pop("DEFAULT", None) | ||
| result = {} | ||
| i = 1 | ||
| for database in dict(databases): | ||
| if database.endswith("_standby") or database.endswith("_readonly"): | ||
| continue | ||
| result[str(i)] = {"name": database, "legacy": False} | ||
| i += 1 | ||
| return result | ||
| except Exception: | ||
| logger.exception("Unable to parse legacy config format") |
There was a problem hiding this comment.
Try to regen the old cfg, so that we can upgrade without downtime.
dragomirp
commented
Feb 9, 2024
| database = self.legacy_db_relation.get_databags(relation)[0].get("database") | ||
| if database: | ||
| databases[relation.id] = { | ||
| "name": database, |
There was a problem hiding this comment.
Is the db names sensitive info? Should this go in a secret instead?
There was a problem hiding this comment.
IMHO, it is not a sensitive info (normally) but can go through secrets as well.
@delgod ?
There was a problem hiding this comment.
I would not normally put the database name into secret.
dragomirp
commented
Feb 9, 2024
Comment on lines
+2
to
+4
| {% for name, database in databases.items() %} | ||
| {{ name }} = host={{ database.host }} dbname={{ database.dbname }} port={{ database.port }} auth_user={{ database.auth_user }} | ||
| {% endfor %} |
There was a problem hiding this comment.
We should be able to split out most (all?) of the moving parts and use %include instead to reduce the amount of rerenders. Created DPE-3549 to track.
dragomirp
commented
Feb 9, 2024
Comment on lines
+117
to
+131
| parser = ConfigParser() | ||
| parser.optionxform = str | ||
| parser.read_string( | ||
| await cat_file_from_unit(ops_test, f"{PGB_DIR}/instance_0/pgbouncer.ini", unit_name) | ||
| ) | ||
|
|
||
| cfg = dict(parser) | ||
| # Convert Section objects to dictionaries, so they can hold dictionaries themselves. | ||
| for section, data in cfg.items(): | ||
| cfg[section] = dict(data) | ||
|
|
||
| # ConfigParser object creates a DEFAULT section of an .ini file, which we don't need. | ||
| del cfg["DEFAULT"] | ||
|
|
||
| return cfg |
There was a problem hiding this comment.
Copied from the lib.
dragomirp
commented
Feb 9, 2024
Comment on lines
+162
to
+163
| self.charm.render_pgb_config(reload_pgbouncer=True) | ||
| self.charm.toggle_monitoring_layer(self.charm.backend.ready) |
There was a problem hiding this comment.
Most of the cfg updates and restarts will happen here, when pgb_dbs_config gets updated.
dragomirp
force-pushed
the
dpe-3049-templated-config
branch
from
1ae3f8c
to
19b13e1
Compare
dragomirp
commented
Feb 12, 2024
| @@ -316,7 +304,7 @@ def _on_relation_changed(self, change_event: RelationChangedEvent): | |||
| change_event.defer() | |||
| return | |||
|
|
|||
| self.update_postgres_endpoints(change_event.relation, reload_pgbouncer=True) | |||
| self.charm.render_pgb_config(reload_pgbouncer=True) | |||
There was a problem hiding this comment.
Relation errors out without this reload
taurus-forever
approved these changes
Feb 13, 2024
| database = self.legacy_db_relation.get_databags(relation)[0].get("database") | ||
| if database: | ||
| databases[relation.id] = { | ||
| "name": database, |
There was a problem hiding this comment.
IMHO, it is not a sensitive info (normally) but can go through secrets as well.
@delgod ?
dragomirp
force-pushed
the
dpe-3049-templated-config
branch
from
da87381
to
2f3443f
Compare
dragomirp
force-pushed
the
dpe-3049-templated-config
branch
from
2f3443f
to
3620aa4
Compare
marceloneppel
approved these changes
Feb 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Remove sync between on file configuration and peer data and just rerender a jinja template.