Revert "Don't mount shares with failed underlying storages"

[jvillafanez]

This reverts commit 692c9be. and 635267e

Description

Revert those 2 commits for now due to performance problems for large installations

Related Issue

https://github.com/owncloud/enterprise/issues/6098

Motivation and Context

There were performance problems caused by #41014 making the system too slow.

How Has This Been Tested?

• test environment:
• test case 1:
• test case 2:
• ...

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:
• Changelog item, see TEMPLATE

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[IljaN]

LGTM. Please add changelog.

[pako81]

Should this not target the release-10.13.3 branch? @jnweiger

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[phil-davis]

@jvillafanez reverting this causes problems to come back in some app tests. For example:
https://github.com/owncloud/files_lifecycle/issues/136 is a problem again.
(and see reports in issue https://github.com/owncloud/files_lifecycle/issues/555 )

Also in user_ldap owncloud/user_ldap#814

Is there likely to be another core change to address this?
Or do we sort out the tests so the failing tests do not impact the nightly CI results?

[jvillafanez]

I think the fixes for those issues were a side effect. Maybe we can try to run those tests with 10.12.0 or 10.13.0 and see what happens.

There are some skips in owncloud/user_ldap#814 so I guess the issue was already there. I'm not sure about https://github.com/owncloud/files_lifecycle/issues/555 , maybe it was merged right after the core fix was merged.

If they are side effects, I think it's fine to skip those because we'll have to analyze the issues separately anyway.

[phil-davis]

@jvillafanez after the original PR #41014 was merged in core, we adjusted the files_lifecycle tests - see PR https://github.com/owncloud/files_lifecycle/pull/544

The test scenarios started doing the right thing, so we removed the "expectation" of 500 status, and put in the correct expected behavior. That made the tests pass on against core master.

If there is some "easy" way to make these tests pass again (without causing other performance... side-effects), that would be nice. Otherwise we will revert the test changes.

[jvillafanez]

owncloud/user_ldap#814 is caused by the previous exception. Since the LDAP is down, the FS initialization for the "original" user (Alice) fails (

core/apps/files_sharing/lib/SharedStorage.php

Lines 102 to 107 in 18793a2

	Filesystem::initMountPoints($this->superShare->getShareOwner());
	$sourcePath = $this->ownerView->getPath($this->superShare->getNodeId(), false);
	list($this->sourceStorage, $sourceInternalPath) = $this->ownerView->resolvePath($sourcePath);
	$this->sourceRootInfo = $this->sourceStorage->getCache()->get($sourceInternalPath);
	// adjust jail
	$this->rootPath = $sourceInternalPath;

). The rootPath isn't set due to the bind exception that happens in the initMountPoints method.

I think running through the initMountPoints is required in order to get information from the ownerView. It isn't like we can swap the order and run the initMountPoints after those methods.
The other option we could try is to fake the value, but I don't know which one we could use. Furthermore, we could introduce new bugs in other places due to a wrong / fake rootPath. It seems too risky for a quick "fix".

[jvillafanez]

For https://github.com/owncloud/files_lifecycle/issues/555 I'll have a deep look at it tomorrow (I need a fresh installation).

I guess the problem is that the shares are "limited" to the user's files, and the archived files are outside of it, so maybe they're counted as "not available".
With the patch reverted, I guess the share file is visible for the sharee, but since it isn't available because it's on the archive storage, it's giving problems.
Anyway, it needs a deeper look. No quick solution so far.

[jvillafanez]

For https://github.com/owncloud/files_lifecycle/issues/555 I'll have a deep look at it tomorrow (I need a fresh installation).

The problem seems to be around https://github.com/owncloud/core/blob/v10.13.2/lib/private/Files/View.php#L1448-L1456
Without the patch, the mount manager returns a shared mount backed up by the shared storage, which fails to be initialized. That's why when we try to access, we have the exception shown in the logs.
However, with the patch, the mount manager returns a regular mount backed up by the home storage of the user. It doesn't give any problem in this case.

The failure in the initialization of the shared storage happens because the target file (the one shared and now archived) isn't found in the owner's filesystem. The target file is archived and the owner's view doesn't reach it (it's out of the ownCloud FS)

Taking this into account, I guess the problem was there from the beginning. We just had the patch before we noticed the problem.

[phil-davis]

@jvillafanez I think that we will revert the user_ldap and files_lifecycle tests so that they go back to passing again (demonstrating what is the "current" behavior)
issues: owncloud/user_ldap#814
https://github.com/owncloud/files_lifecycle/issues/555

If some thing happens to implement a new/different fix related to this, then the tests can easily be adjusted.