[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.

[JordiMForgeFlow]

I understand it would be to have a cron that invalidates the __fs slot after some time

[sebalix]

The cron process could updates memory objects of other Odoo process (workers)? Not sure it's possible 🤔

[sebalix]

The raise e is an issue here (testing the code during a workshop), it is blocking somehow the queue-job.
Clearing the fsspec cache should be enough here, so a new connection will be established in case of error.

[sebalix]

Ok, after one day of testing, we encountered no connection issues with the help of the skip_instance_cache parameter of fsspec (set in the options). This will add some overhead but at least we are safe... dealing with this cache in Odoo seems error prone (connection stability, configuration of the SFTP server...).

I don't know how huge is this overhead, load testing will tell us I guess.