tested with a two-zone configuration with mstart.sh. instead of testing a real upgrade, i manually injected an omap key with:

~/ceph/build $ bin/rados -c run/c2/ceph.conf -p na-2.rgw.log setomapval datalog.sync-status.shard.e4479890-0269-4dbb-9797-bb526937c491.0.retry fakebucketname ''

without this fix, radosgw-admin sync status would show recovering shards: [0] forever. it would retry sync on that key, then try to remove it with cmpomap.cmp_rm_keys. but the key comparison failed, so the key never gets removed

with this fix, this injected key is removed successfully and radosgw-admin sync status catches up as expected

the code generic and may be used in other contexts. having "" == 0 might be considered wrong depending on the usecase.
IMO, would be better to always return "0" (=false) in case of an unknown value (assuming that would be ok for the syncing case)

IMO, would be better to always return "0" (=false) in case of an unknown value (assuming that would be ok for the syncing case)

both cmp_set_vals() and cmp_rm_keys() already treat this -EIO error the same as 0, so you still wouldn't be able to update or remove empty values

    if (r == -EIO) {
      r = 0; // treat EIO as a failed comparison
    }

backward compat with multisite's error repo was a requirement for this feature, but i lost sight of it as we iterated on the design

i updated the client API comments to clarify this new behavior. does that work @yuvalif?

IMO, would be better to always return "0" (=false) in case of an unknown value (assuming that would be ok for the syncing case)

both cmp_set_vals() and cmp_rm_keys() already treat this -EIO error the same as 0, so you still wouldn't be able to update or remove empty values

    if (r == -EIO) {
      r = 0; // treat EIO as a failed comparison
    }

backward compat with multisite's error repo was a requirement for this feature, but i lost sight of it as we iterated on the design

i updated the client API comments to clarify this new behavior. does that work @yuvalif?

Is there other risk of breaking compatibility with other users? Having an explicit argument to control the changed semantics could maybe be an option?

Matt

Is there other risk of breaking compatibility with other users?

no, the multisite error repo is the only user

Having an explicit argument to control the changed semantics could maybe be an option?

it could, but i'm not sure it's worth expanding the interfaces for it. these interfaces support both U64 and String comparisons, but this change in behavior is specific to U64. the string comparisons already work as expected against empty omap values (empty strings compare less than non-empty strings), so this is essentially changing the behavior of U64 comparisons against empty values to match

Is there other risk of breaking compatibility with other users?

no, the multisite error repo is the only user

Having an explicit argument to control the changed semantics could maybe be an option?

it could, but i'm not sure it's worth expanding the interfaces for it. these interfaces support both U64 and String comparisons, but this change in behavior is specific to U64. the string comparisons already work as expected against empty omap values (empty strings compare less than non-empty strings), so this is essentially changing the behavior of U64 comparisons against empty values to match

if we're the only consumer so far, I agree, it's not needed

https://pulpito.ceph.com/cbodley-2021-08-19_16:14:55-rgw-wip-cbodley-testing-distro-basic-gibba/