8000 Avoid GFAL error when `exists(None)` is called. · Issue #7398 · rucio/rucio · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Avoid GFAL error when exists(None) is called. #7398

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.

Sign up for GitHub

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

Jump to bottom
Closed
Geogouz opened this issue Feb 11, 2025 · 1 comment · Fixed by #7438
Closed

Avoid GFAL error when exists(None) is called. #7398

Geogouz opened this issue Feb 11, 2025 · 1 comment · Fixed by #7438
Assignees
@Geogouz
Labels
bug Protocols
Milestone
37.0.0

Comments

@Geogouz
Copy link
Contributor
Geogouz commented Feb 11, 2025

Description

In our code (e.g., rsemanager.exists(...)), we do:

rucio/lib/rucio/rse/rsemanager.py

Lines 272 to 280 in 37cc33b

protocol = create_protocol(rse_settings, 'read', scheme=scheme, impl=impl, domain=domain, auth_token=auth_token, logger=logger)
protocol.connect()
try:
protocol.exists(None)
except NotImplementedError:
protocol = create_protocol(rse_settings, 'write', scheme=scheme, domain=domain, auth_token=auth_token, logger=logger)
protocol.connect()
except:
pass

  • With GFAL, calling exists(None) would lead to an internal ctx.stat("None") which raises "Protocol not supported or path/url invalid: None."
  • This is currently caught in rsemanager.exists(...) by except Exception: pass, so we never fall back to the “write” protocol. The rest of the flow may work, but we see messy log errors about “stat(None).”
  • For a quick solution, and if we want to preserve the current behavior (i.e. remain in the “read” protocol and skip fallback) but avoid trying with GFAL to stat None, maybe, at the beginning of gfal's def exists(self, path): we should do something like:
if path is None:
    # Ignoring exists(None) in GFAL plugin, returning False."
    return False

Steps to reproduce

Trigger the exists(...) of rsemanager.py with gfal.

Rucio Version

36.3.0

Additional Information

No response
@Geogouz Geogouz self-assigned this Feb 17, 2025
Geogouz added a commit to Geogouz/rucio that referenced this issue Feb 25, 2025
@Geogouz
Protocols: Handle exists(None) in GFAL Fix rucio#7398
7b88c62
@Geogouz Geogouz moved this from In Progress to In Review in Rucio 37 “Dungeons & Donkeys” release roadmap Feb 26, 2025
@rdimaio rdimaio linked a pull request Feb 26, 2025 that will close this issue
Protocols: Handle exists(None) in GFAL Fix #7398 #7438
Merged
@rdimaio
Copy link
Contributor
rdimaio commented Feb 26, 2025

Personally, I feel like we should fix this issue in RSEManager itself. I don't think we should even run exists(None) in the first place, as it doesn't make much sense (particularly just to check that exists is implemented)

IMO the core issue is this: we mark exists as abstractmethod in the Protocol ABC:

rucio/lib/rucio/rse/protocols/protocol.py

Lines 208 to 219 in 5dcf2f9

@abstractmethod
def exists(self, path):
"""
Checks if the requested file is known by the referred RSE.
:param path: Physical file name
:returns: True if the file exists, False if it doesn't
:raises SourceNotFound: if the source file was not found on the referred storage.
"""
raise NotImplementedError

And from https://docs.python.org/3/library/abc.html#abc.abstractmethod:

A class that has a metaclass derived from ABCMeta cannot be instantiated unless all of its abstract methods and properties are overridden.

But clearly, it seems that we are okay with having subclasses of Protocol that don't actually define exists, such as storm:

rucio/lib/rucio/rse/protocols/storm.py

Lines 71 to 80 in 5dcf2f9

def exists(self, pfn):
""" Checks if the requested file is known by the referred RSE.
:param pfn: Physical file name
:returns: True if the file exists, False if it doesn't
:raise ServiceUnavailable
"""
raise NotImplementedError

And of course based on the fact that we rely on this NotImplementedError logic here:

rucio/lib/rucio/rse/rsemanager.py

Lines 274 to 280 in 5dcf2f9

try:
protocol.exists(None)
except NotImplementedError:
protocol = create_protocol(rse_settings, 'write', scheme=scheme, domain=domain, auth_token=auth_token, logger=logger)
protocol.connect()
except:
pass

I think what we should do is:

  1. remove the abstractmethod decorator from exists - we are not treating this as a proper abstractmethod
  2. removing all the improper implementations of exists in the protocols (i.e. the ones that don't do anything other than passing or raising NotImplementedError)
  3. Refactoring that rsemanager block to use getattr to see if the method is implemented: https://stackoverflow.com/questions/5268404/what-is-the-fastest-way-to-check-if-a-class-has-a-function-defined

bari12 pushed a commit that referenced this issue Mar 10, 2025
@Geogouz @bari12
Protocols: Handle exists(None) in GFAL #7398
74433ac
bari12 pushed a commit that referenced this issue Mar 10, 2025
@Geogouz @bari12
Core & Internals: Add is_method_overridden helper to detect subclass …
9589a7f 
…method overrides #7398
bari12 pushed a commit that referenced this issue Mar 10, 2025
@Geogouz @bari12
Protocols: Remove usage of protocol.exists(None) Fix #7398
2e2b907 
Removed completely the usage of protocol.exists(None) and placed the fallback to `write` protocol when exists(..) is not overridden.
0xquark pushed a commit to 0xquark/rucio that referenced this issue Mar 10, 2025
@Geogouz @0xquark
Protocols: Handle exists(None) in GFAL rucio#7398
2b14e9c
0xquark pushed a commit to 0xquark/rucio that referenced this issue Mar 10, 2025
@Geogouz @0xquark
Core & Internals: Add is_method_overridden helper to detect subclass …
84e2e87 
…method overrides rucio#7398
0xquark pushed a commit to 0xquark/rucio that referenced this issue Mar 10, 2025
@Geogouz @0xquark
Protocols: Remove usage of protocol.exists(None) Fix rucio#7398
054c05c 
Removed completely the usage of protocol.exists(None) and placed the fallback to `write` protocol when exists(..) is not overridden.
0xquark pushed a commit to 0xquark/rucio that referenced this issue Mar 14, 2025
@Geogouz @0xquark
Protocols: Handle exists(None) in GFAL rucio#7398
a6979fd
0xquark pushed a commit to 0xquark/rucio that referenced this issue Mar 14, 2025
@Geogouz @0xquark
Core & Internals: Add is_method_overridden helper to detect subclass …
8bc8d8a 
…method overrides rucio#7398
0xquark pushed a commit to 0xquark/rucio that referenced this issue Mar 14, 2025
@Geogouz @0xquark
Protocols: Remove usage of protocol.exists(None) Fix rucio#7398
f5ec62a 
Removed completely the usage of protocol.exists(None) and placed the fallback to `write` protocol when exists(..) is not overridden.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Geogouz Geogouz

Labels
bug Protocols
Projects
No open projects
Rucio 37 “Dungeons & Donkeys” release roadmap
Status: Done
Milestone
37.0.0
Development

Successfully merging a pull request may close this issue.

Protocols: Handle exists(None) in GFAL Fix #7398 Geogouz/rucio
3 participants
@bari12 @Geogouz @rdimaio
0