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

osd: restoring timely collection of PG stats #55478

Merged
merged 2 commits into from Feb 11, 2024

Conversation

ronen-fr
Copy link
Contributor

@ronen-fr ronen-fr commented Feb 7, 2024

Fixing the regression introduced by PR #54491.
Also - fixing review comments that were not addressed in the original PR.

Fixes tracker issue 53342 note 5 (a specific scenario leading to 'not all pgs scrubbed' failures)

Copy link
Contributor

@Matan-B Matan-B left a comment

Choose a reason for hiding this comment

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

LGTM overall, does this "Fixes" https://tracker.ceph.com/issues/53342?

with_legacy: true
default: 5
Copy link
Contributor

Choose a reason for hiding this comment

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

How did we get away with 500 before #54491? Was it the removed usage of need_publish?


default configuration option for the manager collection of the OSD data.

Can you please share the option name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How did we get away with 500 before #54491? Was it the removed usage of need_publish?

So it seems. I am still comparing logs for both versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please share the option name?

mgr_stats_period

bool is_time_expired = cutoff_time > info.stats.last_fresh ? true : false;
cutoff_time -=
cct->_conf.get_val<int64_t>("osd_pg_stat_report_interval_max_seconds");
const bool is_time_expired = cutoff_time > info.stats.last_fresh;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing 👍

@ronen-fr
Copy link
Contributor Author

ronen-fr commented Feb 7, 2024

LGTM overall, does this "Fixes" https://tracker.ceph.com/issues/53342?

Seems to. I do not think I've missed any other side effects.

@ronen-fr ronen-fr requested a review from Matan-B February 7, 2024 14:50
Copy link
Contributor

@Matan-B Matan-B left a comment

Choose a reason for hiding this comment

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

Seems to. I do not think I've missed any other side effects.

Can you add "Fixes" to the commit message?

@ronen-fr
Copy link
Contributor Author

ronen-fr commented Feb 7, 2024

Seems to. I do not think I've missed any other side effects.

Can you add "Fixes" to the commit message?

Sorry - I've answered the wrong question here.
Regarding 53342 - I will add a 'fixes' line, with a clarification that it only fixes one specific scenario leading to that type of reports.

500 seconds is way too long, e.g. when compared to the 5s
default configuration option for the manager collection of the OSD data.

Fixes tracker issue 53342 note 5 (a specific scenario leading
to 'not all pgs scrubbed')

Fixes: https://tracker.ceph.com/issues/53342 - partial fix

Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
also - fixing review comments not addressed in the original PR.

Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
@ronen-fr
Copy link
Contributor Author

ronen-fr commented Feb 9, 2024

jenkins test api

@ronen-fr
Copy link
Contributor Author

jenkins test windows

@ronen-fr ronen-fr removed the needs-qa label Feb 11, 2024
@ronen-fr
Copy link
Contributor Author

Merging based on my Teuthology runs

@ronen-fr ronen-fr merged commit f912e90 into ceph:main Feb 11, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants