Skip to content

Improve cgroup metric management#18791

Merged
mattlord merged 8 commits intomainfrom
gracefully_handle_cgroups
Oct 24, 2025
Merged

Improve cgroup metric management#18791
mattlord merged 8 commits intomainfrom
gracefully_handle_cgroups

Conversation

@mattlord
Copy link
Member

@mattlord mattlord commented Oct 23, 2025

Description

This is a follow-up to #18282

We make the following changes here:

  1. Only support cgroup v2 as v1 is essentially dead in 2025+ (it's not easy to get an environment in place to properly test v1)
  2. Handle initialization only when the cgroup metrics are used
    • Today that's only when someone uses the non-default --grpc-enable-orca-metrics flag
  3. Properly handle cases where we failed to initialize properly
  4. Include the underlying error everywhere

We should backport this to v23 before GA as it can lead to crashes as shown in the issue.

Related Issue(s)

Checklist

  • "Backport to:" labels have been added if this change should be back-ported to release branches
  • If this change is to be back-ported to previous releases, a justification is included in the PR description
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on CI?
  • Documentation was added or is not required

Deployment Notes

AI Disclosure

@mattlord
Improve cgroup metric management
3b3a376 
1. Only support cgroup v2 as v1 is essentially dead in 2025+
2. Handle initialization only when the cgroup metrics are used
  a. Today that's only when someone uses --grpc-enable-orca-metrics
3. Properly handle cases where we failed to initialize properly

Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord mattlord added Type: Bug Component: Observability Pull requests that touch tracing/metrics/monitoring Backport to: release-23.0 Needs to be backport to release-23.0 labels Oct 23, 2025
@vitess-bot
Copy link
Contributor

vitess-bot bot commented Oct 23, 2025

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

  • Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

  • Apply the release notes (needs details) label if users need to know about this change.
  • New features should be documented.
  • There should be some code comments as to why things are implemented the way they are.
  • There should be a comment at the top of each new or modified test to explain what the test does.

New flags

  • Is this flag really necessary?
  • Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
  • vtctl command output order should be stable and awk-able.

@vitess-bot vitess-bot bot added NeedsBackportReason If backport labels have been applied to a PR, a justification is required NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Oct 23, 2025
@mattlord mattlord removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Oct 23, 2025
@github-actions github-actions bot added this to the v24.0.0 milestone Oct 23, 2025
@mattlord
Minor tweaks
ea5cbaa 
Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord
Merge remote-tracking branch 'origin' into gracefully_handle_cgroups
69a2a7c 
Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord
Run go mod tidy
68c992e 
Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord
Improve log message
3055d86 
Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord
Remove nested once usage
7d04b3f 
Signed-off-by: Matt Lord <mattalord@gmail.com>
@codecov
Copy link

codecov bot commented Oct 23, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 62.85714% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.70%. Comparing base (d6ce439) to head (89ca16c).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
go/vt/servenv/metrics_cgroup.go 64.51% 11 Missing ⚠️
go/vt/servenv/metrics.go 50.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #18791      +/-   ##
==========================================
+ Coverage   69.68%   69.70%   +0.01%     
==========================================
  Files        1605     1605              
  Lines      214485   214459      -26     
==========================================
+ Hits       149463   149485      +22     
+ Misses      65022    64974      -48

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mattlord
Add unit test
9aaba25 
Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord mattlord marked this pull request as ready for review October 24, 2025 16:47
@mattlord mattlord removed the request for review from systay October 24, 2025 16:47
@mattlord mattlord requested review from arthurschreiber and removed request for frouioui, harshit-gangal and rohit-nayak-ps October 24, 2025 16:47
dbussink
dbussink reviewed Oct 24, 2025
View reviewed changes
twthorn
twthorn approved these changes Oct 24, 2025
View reviewed changes
Copy link
Contributor

@twthorn twthorn left a comment

Choose a reason for hiding this comment

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

LGTM, swapping eager init for lazy setup (only if necessary) is the right choice.

Cgroups2 were supported by linux as early as 2019 so agree with simplifying the code and supporting what most likely all users are on (especially if they are running a new enough vitess version for this new flag)

Thanks for catching this, @mattlord

@mattlord
Wrap errors
89ca16c 
Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord mattlord merged commit d02d40c into main Oct 24, 2025
101 of 102 checks passed
@mattlord mattlord deleted the gracefully_handle_cgroups branch October 24, 2025 18:27
vitess-bot pushed a commit that referenced this pull request Oct 24, 2025
@vitess-bot @vitess-bot
Improve cgroup metric management (#18791)
3e69bd8 
Signed-off-by: Matt Lord <mattalord@gmail.com>
@vitess-bot vitess-bot bot mentioned this pull request Oct 24, 2025
Merged
systay pushed a commit that referenced this pull request Oct 27, 2025
@vitess-bot
[release-23.0] Improve cgroup metric management (#18791) (#18801)
404f58c 
Signed-off-by: Matt Lord <mattalord@gmail.com>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
@twthorn twthorn mentioned this pull request Nov 17, 2025
Closed
5 tasks
twthorn pushed a commit to slackhq/vitess that referenced this pull request Nov 18, 2025
@mattlord @twthorn
Improve cgroup metric management (vitessio#18791)
7f290c3 
Signed-off-by: twthorn <thomaswilliamthornton@gmail.com>
@twthorn twthorn mentioned this pull request Dec 11, 2025
Closed
5 tasks
twthorn pushed a commit to slackhq/vitess that referenced this pull request Jan 28, 2026
@mattlord @twthorn
Improve cgroup metric management (vitessio#18791)
5df53b4 
Signed-off-by: Matt Lord <mattalord@gmail.com>
twthorn added a commit to slackhq/vitess that referenced this pull request Jan 28, 2026
@twthorn
Improve cgroup metric management (vitessio#18791)
cf9e876 
Signed-off-by: Matt Lord <mattalord@gmail.com>

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>
@twthorn twthorn mentioned this pull request Jan 28, 2026
Merged
5 tasks
twthorn pushed a commit to slackhq/vitess that referenced this pull request Jan 29, 2026
@mattlord @twthorn
Improve cgroup metric management (vitessio#18791)
4cb9155 
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>
twthorn added a commit to slackhq/vitess that referenced this pull request Jan 29, 2026
@twthorn
Regenerate proto files after PR vitessio#18791
77d0579 
Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>
twthorn added a commit to slackhq/vitess that referenced this pull request Jan 29, 2026
@twthorn
Complete PR vitessio#18791 backport: Update metrics_cgroup.go
f76332b 
Apply missing changes from PR vitessio#18791 to metrics_cgroup.go:
- Replace cgroup1Manager and cgroup2Manager with single cgroupManager
- Add errCgroupMetricsNotAvailable error variable
- Add sync.Once for lazy initialization
- Remove cgroup v1 support, only support cgroup v2
- Simplify implementation with unified cgroup manager

This fixes compilation errors in metrics_cgroup_test.go.
tanjinx pushed a commit to slackhq/vitess that referenced this pull request Jan 30, 2026
@twthorn
V22 cdc backports round two (#784)
73edc90 
* Improve cgroup metric management (vitessio#18791)

Signed-off-by: Matt Lord <mattalord@gmail.com>

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

* VStream: Prevent buffering entire transactions (OOM risk), instead send chunks to client (vitessio#18849)

Signed-off-by: twthorn <thomaswilliamthornton@gmail.com>

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

* Run VStream copy only when VGTID requires it, use TablesToCopy in those cases (vitessio#18938)

Signed-off-by: twthorn <thomaswilliamthornton@gmail.com>
Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

* Regenerate vtgate.pb.go proto file

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

* Fix tests

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

* Complete PR vitessio#18791 backport: Update metrics_cgroup.go

Apply missing changes from PR vitessio#18791 to metrics_cgroup.go:
- Replace cgroup1Manager and cgroup2Manager with single cgroupManager
- Add errCgroupMetricsNotAvailable error variable
- Add sync.Once for lazy initialization
- Remove cgroup v1 support, only support cgroup v2
- Simplify implementation with unified cgroup manager

This fixes compilation errors in metrics_cgroup_test.go.

* Add missing github.com/containerd/cgroups dependency

Required by metrics_cgroup.go for cgroup v1/v2 support.

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

* Fix cgroups import to use v3

The v1 cgroups package is incompatible with Go 1.24.10.
Use cgroups/v3 consistently throughout the file.

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

* Fix goimports formatting

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

---------

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>
sbaker617 pushed a commit to slackhq/vitess that referenced this pull request Feb 5, 2026
@twthorn @sbaker617
V22 cdc backports round two (#784)
9b9bfdf 
* Improve cgroup metric management (vitessio#18791)

Signed-off-by: Matt Lord <mattalord@gmail.com>

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

* VStream: Prevent buffering entire transactions (OOM risk), instead send chunks to client (vitessio#18849)

Signed-off-by: twthorn <thomaswilliamthornton@gmail.com>

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

* Run VStream copy only when VGTID requires it, use TablesToCopy in those cases (vitessio#18938)

Signed-off-by: twthorn <thomaswilliamthornton@gmail.com>
Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

* Regenerate vtgate.pb.go proto file

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

* Fix tests

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

* Complete PR vitessio#18791 backport: Update metrics_cgroup.go

Apply missing changes from PR vitessio#18791 to metrics_cgroup.go:
- Replace cgroup1Manager and cgroup2Manager with single cgroupManager
- Add errCgroupMetricsNotAvailable error variable
- Add sync.Once for lazy initialization
- Remove cgroup v1 support, only support cgroup v2
- Simplify implementation with unified cgroup manager

This fixes compilation errors in metrics_cgroup_test.go.

* Add missing github.com/containerd/cgroups dependency

Required by metrics_cgroup.go for cgroup v1/v2 support.

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

* Fix cgroups import to use v3

The v1 cgroups package is incompatible with Go 1.24.10.
Use cgroups/v3 consistently throughout the file.

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

* Fix goimports formatting

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>

---------

Signed-off-by: Thomas Thornton <thomaswilliamthornton@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dbussink dbussink dbussink approved these changes

@twthorn twthorn twthorn approved these changes

@timvaillancourt timvaillancourt Awaiting requested review from timvaillancourt timvaillancourt is a code owner

@arthurschreiber arthurschreiber Awaiting requested review from arthurschreiber

Assignees

No one assigned

Labels

Backport to: release-23.0 Needs to be backport to release-23.0 Component: Observability Pull requests that touch tracing/metrics/monitoring Type: Bug

Projects

None yet

Milestone

v24.0.0

Development

Successfully merging this pull request may close these issues.

Bug Report: mysqlctld panic v23.0.0-rc1

3 participants

@mattlord @dbussink @twthorn