mgr/snap_schedule: restore yearly spec to lowercase y by mchangir · Pull Request #56732 · ceph/ceph

mchangir · 2024-04-05T15:35:16Z

Fixes: https://tracker.ceph.com/issues/65350
Signed-off-by: Milind Changire mchangir@redhat.com

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e

joscollin

Looks Good

vshankar · 2024-04-10T11:46:35Z

@mchangir what happens to existing shedules (iff Y got used to set a schedule)?

mchangir · 2024-04-10T12:16:57Z

@mchangir what happens to existing shedules (iff Y got used to set a schedule)?

haven't explicitly tested this aspect
but as long as the mgr isn't restarted, the timer would be set for 1 year and the retention would continue to collect snapshots

as soon as the mgr is started with the new bits to restore the yearly spec back to lowercase 'y', then the timer arming would fail since there would be no multiplier found for uppercase 'Y'
if uppercase 'Y' was the only retention spec, then the new bits which restores the yearly spec back to lowercase 'y' would result in no valid retention spec and all collected yearly snapshots would be purged

All of this also depends on the start time from which the year starts. With no explicit start time, the yearly snapshot would fall on Jan 01 00:00 of every year.

vshankar · 2024-04-10T12:22:32Z

@mchangir what happens to existing shedules (iff Y got used to set a schedule)?

haven't explicitly tested this aspect but as long as the mgr isn't restarted, the timer would be set for 1 year and the retention would continue to collect snapshots

as soon as the mgr is started with the new bits to restore the yearly spec back to lowercase 'y', then the timer arming would fail since there would be no multiplier found for uppercase 'Y'

if uppercase 'Y' was the only retention spec, then the new bits which restores the yearly spec back to lowercase 'y' would result in no valid retention spec and all collected yearly snapshots would be purged

All of this also depends on the start time from which the year starts. With no explicit start time, the yearly snapshot would fall on Jan 01 00:00 of every year.

So, if the mgr is updated and a spec using Y exist in the DB, then that's not a valid spec, right?

mchangir · 2024-04-10T12:31:43Z

So, if the mgr is updated and a spec using Y exist in the DB, then that's not a valid spec, right?

that's right

vshankar · 2024-04-12T08:29:05Z

So, if the mgr is updated and a spec using Y exist in the DB, then that's not a valid spec, right?

that's right

Did that change make it to a release? We have to do something about it if the change made it to a release.

mchangir · 2024-04-12T09:19:00Z

So, if the mgr is updated and a spec using Y exist in the DB, then that's not a valid spec, right?

that's right

Did that change make it to a release? We have to do something about it if the change made it to a release.

yes, the change has made it to:

reef - 18.2.2
squid - 19.0.0

mchangir · 2024-04-12T09:52:18Z

The change has been backported to quincy as well, but so far there's been no quincy release with the change.

vshankar · 2024-04-12T11:33:16Z

So, if the mgr is updated and a spec using Y exist in the DB, then that's not a valid spec, right?

that's right

Did that change make it to a release? We have to do something about it if the change made it to a release.

yes, the change has made it to:

reef - 18.2.2

squid - 19.0.0

We can fix squid as it's still RC. We need a to get this fixed in reef.

mchangir · 2024-04-12T13:08:48Z

So, if the mgr is updated and a spec using Y exist in the DB, then that's not a valid spec, right?

that's right

Did that change make it to a release? We have to do something about it if the change made it to a release.

yes, the change has made it to:

reef - 18.2.2

squid - 19.0.0

We can fix squid as it's still RC. We need a to get this fixed in reef.

IMO this PR along with relevant Release Notes to bring this to users' attention and requesting users to abstain from using 'Y' and instead reverting to use 'y' if it has been used in active retention spec would be sufficient.

chrisphoffman · 2024-04-15T17:41:13Z

src/pybind/mgr/snap_schedule/fs/schedule.py

Don't forget to change formatting for year here

ceph/src/pybind/mgr/snap_schedule/fs/schedule.py

Line 111 in 2685bc0

# spec is empty or other than n,m,h,d,w,M,Y

and

ceph/src/pybind/mgr/snap_schedule/fs/schedule.py

Lines 413 to 415 in 2685bc0

raise ValueError('invalid schedule specified - period should be '

'non-zero positive value and multiplier should '

'be one of h,d,w,M,Y e.g. 1h or 4d etc.')

@chrisphoffman thanks for catching this

nit: And here too :https://github.com/ceph/ceph/pull/56732/files#diff-5e7ce71223ed90d5fd19cdbe2b185501768a556ffa3e8dba1e6f876f1e078550R417

@neesingh-rh thanks .. this is fixed now

rishabh-d-dave

This PR branch has been merged in the testing branch now.

Introduced-by: ceph#53070 Fixes: https://tracker.ceph.com/issues/65350 Signed-off-by: Milind Changire <mchangir@redhat.com>

rishabh-d-dave

QA run was successful for this PR. QA run tooks some time due infra issues (specifically CentOS 9 issue) that were persistent in Sepia lab. Besides, there were new failues in the QA run, both related and unrelated to PRs on the testing branch.

However, since this PR has gone through change since it was merged in testing branch, it needs to be tested again.

I have collected few PRs and will start next QA in little time. @mchangir If current changes are final, shall I add this one to it right away?

rishabh-d-dave · 2024-04-26T10:24:51Z

I've applied my other label, let me know if it shouldn't be put through testing.

* refs/pull/56732/head: mgr/snap_schedule: restore yearly spec to lowercase y Reviewed-by: Jos Collin <jcollin@redhat.com> Reviewed-by: Venky Shankar <vshankar@redhat.com> Reviewed-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave

QA run was successful - https://tracker.ceph.com/projects/cephfs/wiki/main#3-May-2024.

Testing took more time than expected because there were 25-30 new failures. Most of them caused by a PR in the testing branch but these were resolved on removing that PR.

rishabh-d-dave · 2024-05-03T07:14:54Z

make check result has been auto-deleted - https://jenkins.ceph.com/job/ceph-pull-requests/133444/. Relaunching make check job since merging is blocked without it.

rishabh-d-dave · 2024-05-03T07:15:20Z

jenkins test make check

rishabh-d-dave · 2024-05-03T07:18:04Z

Waiting for make check to finish so that I can proceed to merge.

rishabh-d-dave · 2024-05-03T08:33:38Z

make check ran successfully - https://jenkins.ceph.com/job/ceph-pull-requests/134377/. Proceeding to merge.

github-actions bot added the pybind label Apr 5, 2024

mchangir force-pushed the mgr-snap_schedule-restore-yearly-spec-from-Y-to-y branch from 3f7cc0d to c2cf05b Compare April 5, 2024 15:37

github-actions bot added cephfs Ceph File System tests labels Apr 5, 2024

mchangir force-pushed the mgr-snap_schedule-restore-yearly-spec-from-Y-to-y branch from c2cf05b to 3cb7111 Compare April 5, 2024 15:43

mchangir requested a review from a team as a code owner April 5, 2024 15:43

github-actions bot added the documentation label Apr 5, 2024

mchangir force-pushed the mgr-snap_schedule-restore-yearly-spec-from-Y-to-y branch from 3cb7111 to 2685bc0 Compare April 5, 2024 15:44

joscollin approved these changes Apr 8, 2024

View reviewed changes

rishabh-d-dave approved these changes Apr 12, 2024

View reviewed changes

rishabh-d-dave added the wip-rishabh-testing Rishabh's testing label label Apr 12, 2024

chrisphoffman reviewed Apr 15, 2024

View reviewed changes

mchangir force-pushed the mgr-snap_schedule-restore-yearly-spec-from-Y-to-y branch from 2685bc0 to e3fce09 Compare April 16, 2024 00:50

vshankar approved these changes Apr 16, 2024

View reviewed changes

rishabh-d-dave reviewed Apr 16, 2024

View reviewed changes

mgr/snap_schedule: restore yearly spec to lowercase y

7151415

Introduced-by: ceph#53070 Fixes: https://tracker.ceph.com/issues/65350 Signed-off-by: Milind Changire <mchangir@redhat.com>

mchangir force-pushed the mgr-snap_schedule-restore-yearly-spec-from-Y-to-y branch from e3fce09 to 7151415 Compare April 17, 2024 11:16

rishabh-d-dave reviewed Apr 26, 2024

View reviewed changes

rishabh-d-dave added wip-rishabh-testing2 and removed wip-rishabh-testing Rishabh's testing label labels Apr 26, 2024

rishabh-d-dave reviewed May 3, 2024

View reviewed changes

rishabh-d-dave merged commit 35b6248 into ceph:main May 3, 2024

This was referenced May 13, 2024

quincy: mgr/snap_schedule: restore yearly spec to lowercase y #57445

Merged

reef: mgr/snap_schedule: restore yearly spec to lowercase y #57446

Merged

squid: mgr/snap_schedule: restore yearly spec to lowercase y #57447

Merged

	raise ValueError('invalid schedule specified - period should be '
	'non-zero positive value and multiplier should '
	'be one of h,d,w,M,Y e.g. 1h or 4d etc.')

Conversation

mchangir commented Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

joscollin left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar commented Apr 10, 2024

Uh oh!

mchangir commented Apr 10, 2024

Uh oh!

vshankar commented Apr 10, 2024

Uh oh!

mchangir commented Apr 10, 2024

Uh oh!

vshankar commented Apr 12, 2024

Uh oh!

mchangir commented Apr 12, 2024

Uh oh!

mchangir commented Apr 12, 2024

Uh oh!

vshankar commented Apr 12, 2024

Uh oh!

mchangir commented Apr 12, 2024

Uh oh!

chrisphoffman Apr 15, 2024

Choose a reason for hiding this comment

Uh oh!

mchangir Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

vshankar Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

neesingh-rh Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

mchangir Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave left a comment

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave commented Apr 26, 2024

Uh oh!

rishabh-d-dave left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rishabh-d-dave commented May 3, 2024

Uh oh!

rishabh-d-dave commented May 3, 2024

Uh oh!

rishabh-d-dave commented May 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mchangir commented Apr 5, 2024 •

edited

Loading

rishabh-d-dave left a comment •

edited

Loading

rishabh-d-dave left a comment •

edited

Loading

rishabh-d-dave commented May 3, 2024 •

edited

Loading