[16.0][IMP] fs_storage: invalidate filesystem cache when connection fails

[JordiMForgeFlow]

Fixes #319

CC @LoisRForgeFlow

[lmignon]

@JordiMForgeFlow Thank you for this improvement. A few comments... In the same time can you add a file named 320.feature into the fs_storage/readme/newsframent with a description of this new feature. The content will be used at merge time to fill the addon's changelog.
Could you also update the USAGE.rst file to describe the new parameters?

[LoisRForgeFlow]

LGTM

invalidate_cache_on_error maybe is not even needed as a setting and what it adds should be just the standard behavior, but not a big deal.

[JordiMForgeFlow]

@lmignon after trying using fs.exists and looking into more detail, I have put back using fs.ls.

The fs.exists does not raise an error, but simply returns true or false. Also, it basically calls the method info which ends up calling the method fs.ls but with detail=True, which is worse than what we had. (code can be seen here: https://filesystem-spec.readthedocs.io/en/latest/_modules/fsspec/spec.html#AbstractFileSystem)

[lmignon]

@lmignon after trying using fs.exists and looking into more detail, I have put back using fs.ls.

The fs.exists does not raise an error, but simply returns true or false. Also, it basically calls the method info which ends up calling the method fs.ls but with detail=True, which is worse than what we had. (code can be seen here: https://filesystem-spec.readthedocs.io/en/latest/_modules/fsspec/spec.html#AbstractFileSystem)

My concern is about the performance cost when the root directory contains hundred of thousand files... To limit the scope of the ls command, it could be performed on a specific path to a marker file. What do you thing about?

[simahawk]

@lmignon after trying using fs.exists and looking into more detail, I have put back using fs.ls.
The fs.exists does not raise an error, but simply returns true or false. Also, it basically calls the method info which ends up calling the method fs.ls but with detail=True, which is worse than what we had. (code can be seen here: filesystem-spec.readthedocs.io/en/latest/_modules/fsspec/spec.html#AbstractFileSystem)

My concern is about the performance cost when the root directory contains hundred of thousand files... To limit the scope of the ls command, it could be performed on a specific path to a marker file. What do you thing about?

I agree. We should probably create a file on 1st check (eg: .odoo_fs_storage_$id.marker).

[simahawk]

@sebalix can you try by installing it from https://github.com/fsspec/filesystem_spec ?

[sebalix]

Regarding connection failure, I replaced the use of the deprecated functions in edi_storage_oca there: OCA/edi-framework#65 (comment)
Copying my comment:

I'm suspecting (but I'm not sure) it was the use of these deprecated methods that triggers connection issues over time. These methods are decorated, and it could be that the decorator is keeping an old reference to self.__fs having a socket that gets closed after some time. That could explain why the "Test connection" button of fs_storage provides the expected result, while the EDI jobs are failing even when requeued.
This will be tested anyway.

Maybe it's not related at all.

[lmignon]

/ocabot merge minor

[OCA-git-bot]

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-320-by-lmignon-bump-minor, awaiting test results.

[OCA-git-bot]

Congratulations, your PR was merged at 916060f. Thanks a lot for contributing to OCA. ❤️

[sebalix]

After some tests, we are still facing connections issues and the current implementation doesn't cover it: OSError: Socket is closed
This errors arrives only after some time (several hours, workers have a high lifespan here and FS object is never recreated meanwhile).

[JordiMForgeFlow]

@sebalix maybe then we should not store the FS object at all in the slot, and always call to _get_filesystem

[lmignon]

The module can also be used in contexts of heavy traffic between odoo and external filesystems. This is particularly the case when it is used to manage Odoo's default attachment storage. In such circumstances, the cost of the reconnection required each time the fs proxy would be recreated is not negligible. Perhaps it would be a good idea to set up a 'keep-alive' mechanism via a cron running every 'X' minutes...

[JordiMForgeFlow]

@lmignon we could do that or putting it as a configuration in the FS storage to decide if you want to check the connection everytime or not. The cron approach however seems to be the solution covering both cases, and one can decide to run it more/less frequently. What do you think @sebalix ?

[sebalix]

Not sure to understand the cron approach regarding this in-memory object?
But yes, the only fix that is working for us right now is to return a new FS object every time.

[john-herholz-dt]

@JordiBForgeFlow thanks for your answer! I checked again.
This PR was not forward ported to 17.0 branch that we were using. Any change to get this done quickly? I will check tomorrow if I can prepare a forward port myself but i am not so familiar with it.

[JordiMForgeFlow]

@john-herholz-dt all FWs were done. You can see it in v17: https://github.com/OCA/storage/blob/17.0/fs_storage/models/fs_storage.py#L149

[john-herholz-dt]

@JordiMForgeFlow okay you are right. I was confused because the def _check_connection method looks different but that is maybe due to different PR later.

So this brings me back to: We still have the issue with closed Socket and we have a test connection method selected for all FS Storage entries.

But I will further investigate it. Thanks for helping!

[JordiMForgeFlow]

@john-herholz-dt unfortunately I am not running this in any V17 project, so I cannot assure you that the changes they did in V17 are still handling correctly the issue.

Maybe you could try changing the _check_connection method back to how it is in V16 to see if it works

[lmignon]

@john-herholz-dt you could be interested by #487 In this PR I add the possibility to disable the cache of the fsspec client in odoo since fsspec already manage it's own cache. IMO, the cache into Odoo should be removed into the next version.

@JordiMForgeFlow @john-herholz-dt AFAIK 16, 17 and 18 branches are aligned. I regularly do back and forward ports between branches and every time I merge a PR. (But I'm not infallible...)