Skip to content

osd: use an hex-only format for snap_id when creating snapmapper entries#59012

Closed
ronen-fr wants to merge 1 commit intoceph:mainfrom
ronen-fr:wip-rf-hex-snapid
Closed

osd: use an hex-only format for snap_id when creating snapmapper entries#59012
ronen-fr wants to merge 1 commit intoceph:mainfrom
ronen-fr:wip-rf-hex-snapid

Conversation

@ronen-fr
Copy link
Contributor

@ronen-fr ronen-fr commented Aug 5, 2024

i.e. - without the optional text representation of NOSNAP & SNAPDIR

@ronen-fr ronen-fr requested a review from a team as a code owner August 5, 2024 05:40
@github-actions github-actions bot added the core label Aug 5, 2024
@ronen-fr ronen-fr requested a review from Matan-B August 5, 2024 05:40
Comment on lines +212 to +213
// note: the snap_id is to be formatted as a 64-bit hex number,
// and not according to the text representation of snapid_t
Copy link
Contributor

Choose a reason for hiding this comment

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

If we create snapmapper entries only for clone objects (which I think is true), the faulty formatting shouldn't have any real impact and the fix should be ok.

It looks like OSDMonitor also uses the same casting, so that might help to rule out other objects rather than clone ones:

string OSDMonitor::make_purged_snap_key(int64_t pool, snapid_t snap)
{
  char k[80];
  snprintf(k, sizeof(k), "purged_snap_%llu_%016llx",
	   (unsigned long long)pool, (unsigned long long)snap);
  return k;
}

Perhaps we should assert that snap != NOSNAP && snap != SNAPDIR before/while formatting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assert() suggestion makes a lot of sense.
I'll try to verify the other points.
Thanks

@ronen-fr ronen-fr marked this pull request as draft August 5, 2024 13:00
i.e. - without the optional text representation of NOSNAP & SNAPDIR

Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
@ronen-fr ronen-fr force-pushed the wip-rf-hex-snapid branch from 58d208c to 547b67f Compare August 5, 2024 14:19
@ronen-fr ronen-fr marked this pull request as ready for review August 7, 2024 04:42
@ronen-fr ronen-fr requested a review from Matan-B August 7, 2024 04:42
@ronen-fr
Copy link
Contributor Author

ronen-fr commented Aug 7, 2024

@Matan-B
Copy link
Contributor

Matan-B commented Aug 7, 2024

Looking at Teu test failure that may be related: https://pulpito.ceph.com/rfriedma-2024-08-06_16:20:07-smoke-wip-rf-hex-snapid-distro-default-smithi/7840165/

From first glance that failure look unrelated to me, it's a s3 test test_schema_definition.

2024-08-06T17:13:35.815 INFO:teuthology.orchestra.run.smithi190.stdout:E        +    where <built-in method find of str object at 0x7fe8cc05d4d0> = 'An error occurred (s3select-engine-error) when calling the SelectObjectContent operation: s3select-ProcessingTime-Error :[int failed : alias {c11} or column not exist in schema] :resourcse-id'.find
2024-08-06T17:13:35.816 INFO:teuthology.orchestra.run.smithi190.stdout:
2024-08-06T17:13:35.816 INFO:teuthology.orchestra.run.smithi190.stdout:s3tests_boto3/functional/test_s3select.py:1331: AssertionError

@Matan-B
Copy link
Contributor

Matan-B commented Aug 8, 2024

We can close this one, cherry-picked here #58990

@ronen-fr
Copy link
Contributor Author

ronen-fr commented Aug 8, 2024

closed for #58990

@ronen-fr ronen-fr closed this Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants