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

Support extra flags for mysqld_exporter #629

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Support extra flags for mysqld_exporter #629

wants to merge 8 commits into from

Conversation

stankevich
Copy link
Contributor

@stankevich stankevich commented Oct 25, 2024 •
edited
Loading

Adds a new mysqldExporter.extraFlags property that can be used to pass custom flags to mysqld_exporter to enable/configure additional collectors. Also, makes it possible to override the collect.info_schema.tables.databases value if needed. The default behaviour and collector flags remain unchanged.

This PR largely follows existing code conventions in the operator. I added a new FormatArgsConvertBoolean() method to Flags to support mysqld_exporter's flag format. Added the tests for it and for the existing FormatArgs() and Merge() methods.

Closes #470.

@stankevich stankevich force-pushed the support-extra-flags-in-mysqld-exporter branch from bc29ff4 to eac877c Compare October 25, 2024 12:04
frouioui
frouioui reviewed Nov 4, 2024
View reviewed changes
pkg/operator/vitess/flags_test.go Outdated Show resolved Hide resolved
deploy/crds/planetscale.com_vitessclusters.yaml Show resolved Hide resolved
pkg/operator/vitess/flags_test.go Show resolved Hide resolved
@stankevich stankevich force-pushed the support-extra-flags-in-mysqld-exporter branch 2 times, most recently from e395f17 to 3f26d14 Compare November 8, 2024 16:03
@stankevich
Support extra flags for mysqld_exporter
3a6997f 
Signed-off-by: Sergey Stankevich <stankevich@users.noreply.github.com>
@stankevich
Update year in license header
fdd7fc6 
Signed-off-by: Sergey Stankevich <stankevich@users.noreply.github.com>
@stankevich
Regenerate operator.yaml
364b77f 
Signed-off-by: Sergey Stankevich <stankevich@users.noreply.github.com>
@stankevich
Make mysqldExporter.resources optional
adb38e5 
It's possible that mysqldExporter.extraFlags are provided while
mysqldExporter.resources are not.

Signed-off-by: Sergey Stankevich <stankevich@users.noreply.github.com>
@stankevich
Add checkPodSpecBySelectorWithTimeout function
2553001 
Similar to checkPodStatusWithTimeout(), but uses selectors and checks
the pod spec for matches.

Signed-off-by: Sergey Stankevich <stankevich@users.noreply.github.com>
@stankevich
Add mysqld_exporter flags to the upgrade test
4f78932 
Signed-off-by: Sergey Stankevich <stankevich@users.noreply.github.com>
@stankevich stankevich force-pushed the support-extra-flags-in-mysqld-exporter branch from 3f26d14 to 4f78932 Compare November 8, 2024 16:03
@stankevich stankevich mentioned this pull request Nov 8, 2024
Open
5 tasks
@stankevich
Copy link
Contributor Author

Thanks for the helpful pointers, @frouioui! I regenerated the operator.yaml file and updated the tests to verify the exporter flags during an upgrade. I also made mysqldExporter.resources optional, otherwise it wasn't possible to provide extraFlags without also providing resources.

The upgrade test is passing locally, but is failing in CI. Unfortunately, I'm not able to see the reason. I noticed that the post-upgrade verifyVtGateVersion sometimes fails due to a race condition caused by upgrade-related pod restarts. I have ideas on improving this, but won't be able to contribute in the next couple of weeks.

frouioui
frouioui reviewed Nov 11, 2024
View reviewed changes
test/endtoend/upgrade_test.sh
@@ -251,6 +260,7 @@ checkSemiSyncSetup
verifyDurabilityPolicy "commerce" "semi_sync"
upgradeToLatest
verifyVtGateVersion "22.0.0"
verifyResourceSpec
Copy link
Member

Choose a reason for hiding this comment

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

Note to myself: once the next release (v2.15.0) is out, we should do that check before and after we upgrade.

@frouioui frouioui mentioned this pull request Nov 11, 2024
Open
frouioui
frouioui approved these changes Nov 11, 2024
View reviewed changes
Copy link
Member

@frouioui frouioui left a comment

Choose a reason for hiding this comment

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

This looks good to me! Thanks for working on that ❤️

@frouioui
Copy link
Member

frouioui commented Nov 11, 2024 •
edited
Loading

The upgrade test is passing locally, but is failing in CI. Unfortunately, I'm not able to see the reason. I noticed that the post-upgrade verifyVtGateVersion sometimes fails due to a race condition caused by upgrade-related pod restarts. I have ideas on improving this, but won't be able to contribute in the next couple of weeks.

These tests are unfortunately pretty flaky indeed 😢 Hopefully we get some time to work on this.

@frouioui frouioui force-pushed the support-extra-flags-in-mysqld-exporter branch 2 times, most recently from fdc713e to 4f78932 Compare November 11, 2024 19:10
@frouioui
Copy link
Member

Merged main into the branch on the last push, hoping this will alleviate some of the flakiness.

@frouioui
Add a retry logic when checking the vtgate version
5744194 
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
@frouioui
Copy link
Member

Still flaky/failing. I added a retry mechanism to the verify vtgate version function. Hopefully that should help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frouioui frouioui frouioui approved these changes

@shlomi-noach shlomi-noach shlomi-noach approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Allow enabling additional mysqld_exporter collectors
3 participants
@stankevich @frouioui @shlomi-noach