@kchheda3 thanks for the fix.
not sure about the approach with the 2nd commit...
maybe we can detect the version of https://github.com/ceph/ceph/blob/main/src/cls/2pc_queue/cls_2pc_queue_types.h#L63 and decide whether we can trust the reserved_size or not? and do the full calculation accordingly?

@kchheda3 thanks for the fix. not sure about the approach with the 2nd commit... maybe we can detect the version of https://github.com/ceph/ceph/blob/main/src/cls/2pc_queue/cls_2pc_queue_types.h#L63 and decide whether we can trust the reserved_size or not? and do the full calculation accordingly?

@yuvalif depending on version we decide on trusting reserved_size ?
at this point trusting reserved_size is not possible because in real world that value is incorrect.
also as mentioned the calculation shouldn't be that much overhead as at any point of time we going to have not more than 1k entries ( thats the max reservation we currently support). looping 1k entries should not have major latency issues?

@kchheda3 thanks for the fix. not sure about the approach with the 2nd commit... maybe we can detect the version of https://github.com/ceph/ceph/blob/main/src/cls/2pc_queue/cls_2pc_queue_types.h#L63 and decide whether we can trust the reserved_size or not? and do the full calculation accordingly?

@yuvalif depending on version we decide on trusting reserved_size ? at this point trusting reserved_size is not possible because in real world that value is incorrect. also as mentioned the calculation shouldn't be that much overhead as at any point of time we going to have not more than 1k entries ( thats the max reservation we currently support). looping 1k entries should not have major latency issues?

was thinking that we can bump the version of the urgent_data as part of 00ad83d adding a flag indicating that the queue was created with the fix in it - this would indicate that we can trust the reserved size in that queue.
if the queue does not have the flag, we ignore the value and recalculate for each reservation

@kchheda3 thanks for the fix. not sure about the approach with the 2nd commit... maybe we can detect the version of https://github.com/ceph/ceph/blob/main/src/cls/2pc_queue/cls_2pc_queue_types.h#L63 and decide whether we can trust the reserved_size or not? and do the full calculation accordingly?

@yuvalif depending on version we decide on trusting reserved_size ? at this point trusting reserved_size is not possible because in real world that value is incorrect. also as mentioned the calculation shouldn't be that much overhead as at any point of time we going to have not more than 1k entries ( thats the max reservation we currently support). looping 1k entries should not have major latency issues?

was thinking that we can bump the version of the urgent_data as part of 00ad83d adding a flag indicating that the queue was created with the fix in it - this would indicate that we can trust the reserved size in that queue. if the queue does not have the flag, we ignore the value and recalculate for each reservation

yeah i did thought about it originally, but really how do you know that someone has started using brand new notifications and they can rely on flag.
other option was to add a NEW bool flag deciding whether to re-calculate and is defaulted to true.
so first time someone upgrades or starts with new notification, it will be true and that will calculate the value first time and update the reserve_size value and then going forward we can use that calculated value, but that would require to first lock the flag and then calculate and then update the osd, so other rgw do not race to calculate.
this was getting more and more messy
so i decided to calculate always as the size was not that big (1k entries max)

so first time someone upgrades or starts with new notification, it will be true and that will calculate the value first time and update the reserve_size value and then going forward we can use that calculated value, but that would require to first lock the flag and then calculate and then update the osd, so other rgw do not race to calculate.

isn't this all happening in an atomic cls call? i'm not sure where locking is necessary. if radosgws issue racing reservations on an un-upgraded queue, the first call will recalculate the size and update the flag atomically - the second call will see that flag and increment as usual

so first time someone upgrades or starts with new notification, it will be true and that will calculate the value first time and update the reserve_size value and then going forward we can use that calculated value, but that would require to first lock the flag and then calculate and then update the osd, so other rgw do not race to calculate.

isn't this all happening in an atomic cls call? i'm not sure where locking is necessary. if radosgws issue racing reservations on an un-upgraded queue, the first call will recalculate the size and update the flag atomically - the second call will see that flag and increment as usual

agree that there should not be a race here. i think that the issue would be that in a mixed version cluster, old cls code writing to a new queue (or to an existing queue on which recalculation was performed) would still leak.
however, assuming the upgrade process is limited in time the amount of leak would be bounded

so first time someone upgrades or starts with new notification, it will be true and that will calculate the value first time and update the reserve_size value and then going forward we can use that calculated value, but that would require to first lock the flag and then calculate and then update the osd, so other rgw do not race to calculate.

isn't this all happening in an atomic cls call? i'm not sure where locking is necessary. if radosgws issue racing reservations on an un-upgraded queue, the first call will recalculate the size and update the flag atomically - the second call will see that flag and increment as usual

agree that there should not be a race here. i think that the issue would be that in a mixed version cluster, old cls code writing to a new queue (or to an existing queue on which recalculation was performed) would still leak. however, assuming the upgrade process is limited in time the amount of leak would be bounded

actually i have now updated the commit to only one time re calculate the value and persist with correct value in the queue.
this addresses both upgrade and new cluster scenario and also mix mode scenario.
Since this code change is in cls_queue, it will run on OSD. so even on mix mode, as long as OSD is upgraded that is handling the calculation would re-calc and then store the correct value and hence subsequent reads will read correct value and we do not have to worry about leak.

i originally thought it was rgw code, but then realized its cls code and writes will be atomic and serialize, so leveraging that i do the calculation first time and second callers will automatically get the correct value as @cbodley pointed.

jenkins test make check

just to make sure that we covered all cases:

• new queue with new osd: osd will detect v3 on queue creation and trust the counter
• old queue with new osd: will detect v2, recalculate and write back as v3
• old queue with old osd (in a mixed cluster during upgrade): reservations are still leaking during upgrade, this will be fixed when old osd is upgraded and recalculates
• new queue with old osd: reservations are still leaking during upgrade, when old osd writes back the it is done as v2. this will be fixed when old osd is upgraded and recalculates

jenkins test make check

jenkins test make check

Execution complete, tracker approved by @ivancich. tracker detail: https://tracker.ceph.com/issues/75124
@kchheda3 , If no further testing needed, pr can be merged

jenkins test make check