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.

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

[DPE-1993] Patroni COS #261

Merged
merged 10 commits into from
Oct 27, 2023
Merged

[DPE-1993] Patroni COS #261

merged 10 commits into from
Oct 27, 2023

Conversation

dragomirp
Copy link
Contributor

@dragomirp dragomirp commented Oct 23, 2023

Add Patroni COS dashboard and configuration.

@dragomirp dragomirp changed the title [DPE-1993] patroni cos [DPE-1993] Patroni COS Oct 23, 2023
@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Merging #261 (d468831) into main (ec64e16) will increase coverage by 0.06%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #261      +/-   ##
==========================================
+ Coverage   78.92%   78.98%   +0.06%     
==========================================
  Files          10       10              
  Lines        2149     2151       +2     
  Branches      349      349              
==========================================
+ Hits         1696     1699       +3     
+ Misses        383      381       -2     
- Partials       70       71       +1     
Files Coverage Δ
src/charm.py 72.48% <100.00%> (+0.07%) ⬆️

... and 2 files with indirect coverage changes

Comment on lines +148 to +150
self.on[PEER].relation_changed,
self.on.secret_changed,
self.on.secret_remove,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

certificates rel events don't guarantee that the scheme will switch, so we update on events that can potentially change or remove the certs

@@ -19,7 +19,7 @@ log:
file_size: 600

restapi:
listen: '{{ self_ip }}:8008'
listen: '0.0.0.0:8008'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that we can connect from localhost.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will produce issues with juju spaces once we are ready.
Anyway LGTM for now.

@delgod any security concerns here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like it's OK to scrape by self_ip so reverting that change.

@dragomirp dragomirp marked this pull request as ready for review October 24, 2023 13:27
@@ -19,7 +19,7 @@ log:
file_size: 600

restapi:
listen: '{{ self_ip }}:8008'
listen: '0.0.0.0:8008'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will produce issues with juju spaces once we are ready.
Anyway LGTM for now.

@delgod any security concerns here?

src/charm.py Outdated
return [
{
"metrics_path": "/metrics",
"static_configs": [{"targets": ["localhost:8008"]}],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not {{ self_ip }} here instead of localhost? :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should work, but last time we discussed that, I think COS preferred to only scrape localhost. I'll ping them on MM to recheck.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like it's OK to do that. Retested locally and all seems to be in order.

@dragomirp dragomirp force-pushed the dpe-1993-patroni-cos branch from bc75919 to 7a47678 Compare October 25, 2023 18:34
Copy link
Member

@marceloneppel marceloneppel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@dragomirp dragomirp merged commit 6790adc into main Oct 27, 2023
36 checks passed
@dragomirp dragomirp deleted the dpe-1993-patroni-cos branch October 27, 2023 12:00
BON4 pushed a commit to BON4/postgresql-operator that referenced this pull request Apr 23, 2024
* Add Patroni COS integration

* Bump libs

* Unit tests

* Scrape IP instead of localhost

* Fix unit tests

* Increase upgrade timout

* Bump libs

* Revert data_interfaces
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants