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

Default multihop tombstone is effectively None #5110

Closed
nsmith- opened this issue Dec 13, 2021 · 3 comments
Closed

Default multihop tombstone is effectively None #5110

nsmith- opened this issue Dec 13, 2021 · 3 comments
Assignees
Milestone

Comments

@nsmith-
Copy link
Contributor

nsmith- commented Dec 13, 2021

Motivation

In

DEFAULT_MULTIHOP_TOMBSTONE_DELAY = datetime.timedelta(hours=2)

a DEFAULT_MULTIHOP_TOMBSTONE_DELAY is set, and then in
default_tombstone_delay = core_config_get('transfers', 'multihop_tombstone_delay', default=DEFAULT_MULTIHOP_TOMBSTONE_DELAY, expiration_time=600)

the default value is passed if no runtime configuration is set. The problem is, all cache values are converted to string in
write_to_cache(value_key, str(value)) # Also write default to cache

which means that the actual default value ends up being the string '2:00:00', and later on in tombstone_from_delay we get a ValueError which is converted to None
if not isinstance(tombstone_delay, timedelta):
try:
tombstone_delay = timedelta(seconds=int(tombstone_delay))
except ValueError:
return None

hence no tombstone unless transfers.multihop_tombstone_delay is explicitly set (or if the RSE has a multihop_tombstone_delay attribute)

Modification

Set

DEFAULT_MULTIHOP_TOMBSTONE_DELAY = int(datetime.timedelta(hours=2).total_seconds())
@rcarpa
Copy link
Contributor

rcarpa commented Dec 14, 2021

thank you for the investigation. I think the real problem is that we write the default to cache. Also, setting to none on conversion error isn't the best idea. @bari12 , WDYT?

@rcarpa rcarpa self-assigned this Dec 14, 2021
@bari12
Copy link
Member

bari12 commented Dec 14, 2021

Writing the default to cache is good in principle, this way we still minimise having to do database interaction in case the default is used. In principle dogpile supports writing the actual value (with type) to the cache and also retrieve it like that. I am not exactly sure why we cast it to string here. But I am a little bit concerned of changing this now and what side-effects this could result in.
For now I think @nsmith- suggestion is good. In a second step we should perhaps evaluate writing the right types to the cache.

@nsmith-
Copy link
Contributor Author

nsmith- commented Dec 16, 2021

A workaround is to set rucio-admin config set --section transfers --option multihop_tombstone_delay --value 7200

@bari12 bari12 closed this as completed in 00144a4 Jan 10, 2022
bari12 added a commit that referenced this issue Jan 10, 2022
…_cache

Transfers: don't allow timedelta obj in tombstone_from_delay. Fix #5110
bari12 pushed a commit that referenced this issue Jan 10, 2022
It is an additional case to handle, which already had a nasty bug.
Rather than trying to handle the special case, simplify the code.
@bari12 bari12 added this to the 1.27.3 milestone Jan 10, 2022
@bari12 bari12 changed the title [conveyor] Default multihop tombstone is effectively None Default multihop tombstone is effectively None Jan 17, 2022
piperov pushed a commit to piperov/rucio that referenced this issue Feb 25, 2022
…io#5110

It is an additional case to handle, which already had a nasty bug.
Rather than trying to handle the special case, simplify the code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants