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-1796 Implement support for general/slowquery logs and logrotation of text logs #329

Merged
merged 33 commits into from Sep 29, 2023

Conversation

shayancanonical
Copy link
Contributor

Issue

  1. We are rendering the custom mysqld config as a string (making it very difficult to maintain and trace)
  2. We do not have general and slow query logs enabled
  3. We are not rotating mysql text logs (error, general and slowquery)

Solution

  1. Use python's in-built configparser to generate the mysqld custom config (an ini file) -> this has the added benefit of validation
  2. Enable general and slow query logs
  3. Use cron to rotate logs every minute. The logrotate's postrotate script will invoke a custom event in the charm (using juju-run in v2.9.X and juju-exec in >= v3.1.X) to execute the FLUSH statement. This is because we would like to avoid keeping database credentials on the machine

@carlcsaposs-canonical
Copy link
Contributor

Issue
We do not have general and slow query logs enabled

I might be misremembering—I thought we didn't want to enable slow query logs but we did want to rotate them if they were enabled (cc @delgod)

lib/charms/data_platform_libs/v0/upgrade.py Show resolved Hide resolved
lib/charms/mysql/v0/mysql.py Outdated Show resolved Hide resolved
lib/charms/mysql/v0/mysql.py Outdated Show resolved Hide resolved
lib/charms/mysql/v0/mysql.py Outdated Show resolved Hide resolved
poetry.lock Outdated Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
src/mysql_vm_helpers.py Show resolved Hide resolved
src/mysql_vm_helpers.py Outdated Show resolved Hide resolved
src/mysql_vm_helpers.py Outdated Show resolved Hide resolved
templates/logrotate.j2 Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Merging #329 (d44da0e) into main (f0abc40) will decrease coverage by 0.09%.
The diff coverage is 63.88%.

@@            Coverage Diff             @@
##             main     #329      +/-   ##
==========================================
- Coverage   64.04%   63.96%   -0.09%     
==========================================
  Files          15       16       +1     
  Lines        2940     2997      +57     
  Branches      385      390       +5     
==========================================
+ Hits         1883     1917      +34     
- Misses        934      956      +22     
- Partials      123      124       +1     
Files Coverage Δ
src/charm.py 51.60% <80.00%> (+0.57%) ⬆️
lib/charms/mysql/v0/mysql.py 72.74% <73.07%> (-0.14%) ⬇️
src/flush_mysql_logs.py 65.38% <65.38%> (ø)
src/mysql_vm_helpers.py 61.73% <40.00%> (-1.53%) ⬇️

src/charm.py Outdated Show resolved Hide resolved
lib/charms/mysql/v0/mysql.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@paulomach paulomach left a comment

Choose a reason for hiding this comment

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

Left some non-blockers - want to see your opinion before approving

lib/charms/mysql/v0/mysql.py Outdated Show resolved Hide resolved
src/flush_mysql_logs.py Outdated Show resolved Hide resolved
src/mysql_vm_helpers.py Show resolved Hide resolved
templates/logrotate.j2 Outdated Show resolved Hide resolved
tests/integration/test_log_rotation.py Outdated Show resolved Hide resolved
src/flush_mysql_logs.py Outdated Show resolved Hide resolved
paulomach
paulomach previously approved these changes Sep 26, 2023
Copy link
Contributor

@paulomach paulomach left a comment

Choose a reason for hiding this comment

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

nice work

dragomirp
dragomirp previously approved these changes Sep 26, 2023
taurus-forever
taurus-forever previously approved these changes Sep 26, 2023
Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

LGTM.

delete_file_or_directory_in_unit() could play us bad story... today it is in tests only, but tomorrow someone will use it in production. File removal "rm -f" and folder removal "rm -rf" have significantly different risks.

boolean indicating success
"""
try:
return_code, _, _ = await ops_test.juju("ssh", unit_name, "sudo", "rm", "-rf", path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it worth to ensure path is not "/" or "."? Otherwise: sudo rm -rf /

In general I would separate file removal, and avoid recursive call in this case.
I prefer find /path/ -mindepth 1 -mtime +5 -delete for the deep cleanup.
In this particular case, I would separate file and folder removal.
P.S. Better safe then sorry (c) this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, it would be terrible to have rm -rf run by mistake. changed to using find in a0052bf (with maxdepth=0 to match exact paths, and no mtime as the files the test deletes will be recently created)

@delgod
Copy link
Member

delgod commented Sep 26, 2023

Issue
We do not have general and slow query logs enabled

I might be misremembering—I thought we didn't want to enable slow query logs but we did want to rotate them if they were enabled (cc @delgod)

In my personal opinion:

  • general log should be enabled, rotated and streamed to COS.
  • slow log should be disabled by default in Charm because it is disabled by default in MySQL (maybe we should have the option to enable it in future). but when the slow log is enabled - it should be rotated.

in case of disagreement, the decision maker here is @taurus-forever

@taurus-forever
Copy link
Contributor

taurus-forever commented Sep 27, 2023

Issue
We do not have general and slow query logs enabled

I might be misremembering—I thought we didn't want to enable slow query logs but we did want to rotate them if they were enabled (cc @delgod)

In my personal opinion:

  • general log should be enabled, rotated and streamed to COS.
  • slow log should be disabled by default in Charm because it is disabled by default in MySQL (maybe we should have the option to enable it in future). but when the slow log is enabled - it should be rotated.

in case of disagreement, the decision maker here is @taurus-forever

My opinion:

  • slow query log is useful application level tool. DBA enables it to improve DB performance (indexes, caches, ...).
  • IMHO, slow query log is disabled in production by default, but enabled on X% of servers to monitor X% of "traffic".
    IMHO2, Queries longer then X seconds have to be killed (and not reported to slow query log).
    This is a DBA level decisions...
  • Canonical Data is an operator provider, not a DBA => we should "ship MySQL defaults"(c) unless we need to change them.
  • Enabling "slow_query_log=ON" is a premature optimization we are granting to Application/DBA without the request and (what is most important) without the ability to overwrite us (as we ship it as mysql.conf.d/z-custom-mysqld.cnf).

Mohamed, let's agree to keep slow query log disabled for now, by default (at least until we provide configuration options to disable it => mysql tuning). Tnx!

@shayancanonical shayancanonical merged commit e4532cf into main Sep 29, 2023
20 of 22 checks passed
@shayancanonical shayancanonical deleted the feature/text_log_rotation branch September 29, 2023 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants