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

fix idp schema, add virtual setup #2219

Merged
merged 8 commits into from
Nov 3, 2021

Conversation

butonic
Copy link
Contributor

@butonic butonic commented Oct 29, 2021

This just adds a config for the virtual views. The local integration tests now have two storage providers mounted at /virtual with the local driver. They can be accessed via the frontent-global service.

@update-docs
Copy link

update-docs bot commented Oct 29, 2021

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.

@butonic
Copy link
Contributor Author

butonic commented Oct 29, 2021

@ishank011 @labkode what kind of tests should we run on the /virtual namespace? We want to make sure

  1. the etag for /virtual changes when either /virtual/aaaaa or /virtual/zzzzz is touched.
  2. the mtime taken from the youngest node in either storage provider
  3. the size is the sum of both storage providers
  4. what else?

@phil-davis can you recommend a set of features we can use in drone to test etag and size propagation?

cc @C0rby @aduffeck

For now I have a basic test working: https://drone.cernbox.cern.ch/cs3org/reva/3916/8/5

It sets up two storage providers with the local driver and mounts them at /virtual/[a-k] and /virtual/[l-z].
The tests then create /virtual/(a|b|c|k|l|z) and do some PROPFIND sanity checks with depth: (0|1|infinity).

  • we need to set the mtime for folders ... does the localfs propagate the etag?

@butonic butonic force-pushed the add-vitual-view-tests branch from 177259c to 924682d Compare October 29, 2021 14:41
@butonic butonic self-assigned this Nov 1, 2021
@butonic butonic force-pushed the add-vitual-view-tests branch 5 times, most recently from 15b3f76 to 3606c52 Compare November 1, 2021 15:38
@butonic
Copy link
Contributor Author

butonic commented Nov 1, 2021

ok, I am now checking if the etage of /, /virtual and /virtual/b changes when the folder /virtual/b/bar is created.

Unfortunately, not even this basic test works, I assume becaus I am using the local driver which

  1. did not do a propagation when creating a dir (fixed in this PR)
  2. does not seem provide an etag for the root of the storage ... or I am not fully understanding how the stat and list across providers code works...

when doing a PROPFIND against the virtual folder I should see the etag thet corresponds to the later provider entry, like this:

diff --git a/internal/grpc/services/gateway/storageprovider.go b/internal/grpc/services/gateway/storageprovider.go
index dcfd7be3..5292ac30 100644
--- a/internal/grpc/services/gateway/storageprovider.go
+++ b/internal/grpc/services/gateway/storageprovider.go
@@ -1334,7 +1334,14 @@ func (s *svc) statAcrossProviders(ctx context.Context, req *provider.StatRequest
                }
                if resp.Info != nil {
                        info.Size += resp.Info.Size
-                       info.Mtime = utils.LaterTS(info.Mtime, resp.Info.Mtime)
+                       if utils.TSToUnixNano(resp.Info.Mtime) > utils.TSToUnixNano(info.Mtime) {
+                               info.Mtime = resp.Info.Mtime
+                               info.Etag = resp.Info.Etag
+                               info.Checksum = resp.Info.Checksum
+                       }
+                       if info.Etag == "" && info.Etag != resp.Info.Etag {
+                               info.Etag = resp.Info.Etag
+                       }
                }
        }
 
@@ -1705,7 +1712,14 @@ func (s *svc) listContainerAcrossProviders(ctx context.Context, req *provider.Li
                                }
                                // TODO(ishank011): aggregrate properties such as etag, checksum, etc.
                                p.Size += info.Size
-                               p.Mtime = utils.LaterTS(p.Mtime, info.Mtime)
+                               if utils.TSToUnixNano(info.Mtime) > utils.TSToUnixNano(p.Mtime) {
+                                       p.Mtime = info.Mtime
+                                       p.Etag = info.Etag
+                                       p.Checksum = info.Checksum
+                               }
+                               if p.Etag == "" && p.Etag != info.Etag {
+                                       p.Etag = info.Etag
+                               }
                                p.Type = provider.ResourceType_RESOURCE_TYPE_CONTAINER
                                p.MimeType = "httpd/unix-directory"
                        } else {

right @ishank011 ?

Anyway, we should add the expectations to this PRs feature. Hopefully, it can be mimicked using the localfs.

@butonic
Copy link
Contributor Author

butonic commented Nov 1, 2021

without the above diff I get

      WebDavPropertiesContext::theseEtagsShouldHaveChanged
      The etag '' of element '/' of user 'einstein' did not change.
      The etag '' of element '/virtual' of user 'einstein' did not change.
      The etag '"0330f2019a2524fc51bc62964a186ac4"' of element '/virtual/b' of user 'einstein' did not change.
      Failed asserting that 3 matches expected 0.

with it I can get rid of the last etag not changing:

      WebDavPropertiesContext::theseEtagsShouldHaveChanged
      The etag '' of element '/' of user 'einstein' did not change.
      The etag '' of element '/virtual' of user 'einstein' did not change.
      Failed asserting that 2 matches expected 0.

In any case, I was under the impression that / as well as /virtual should change as well, right @ishank011 @labkode ?

Copy link
Contributor

@individual-it individual-it left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these tests reva specific or should they be moved/copied to ocis?

@butonic
Copy link
Contributor Author

butonic commented Nov 2, 2021

are these tests reva specific or should they be moved/copied to ocis?

They are reva specific. The 'virtual views' feature allows merging multiple storage providers to serve a single folder. Not something owncloud 10 can do.

@butonic butonic requested a review from a team as a code owner November 3, 2021 09:40
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic
Copy link
Contributor Author

butonic commented Nov 3, 2021

@ishank011 I think we have a passing test! 🤞

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
ishank011
ishank011 previously approved these changes Nov 3, 2021
C0rby
C0rby previously approved these changes Nov 3, 2021
Copy link
Contributor

@individual-it individual-it left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest not to use "einstein" as user in the tests but create a new user as we do everywhere else

@individual-it individual-it self-requested a review November 3, 2021 11:20
@butonic butonic dismissed stale reviews from C0rby and ishank011 via 5c6c36b November 3, 2021 11:26
@butonic
Copy link
Contributor Author

butonic commented Nov 3, 2021

I would suggest not to use "einstein" as user in the tests but create a new user as we do everywhere else

@individual-it I did not want to make an ldap server necessary as I don't want to test managing users. I would actually like to reduce this test to a point where we only register the storage providers and don't even have users ... but I would have to test further if disabling user home creation on login is enough.

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic butonic requested a review from individual-it November 3, 2021 11:32
@butonic
Copy link
Contributor Author

butonic commented Nov 3, 2021

hm ... we could change the config of the default test suite to start two storage providers for /users .... but that would make the setup even more complex ...

@ishank011 ishank011 merged commit a4fb51f into cs3org:master Nov 3, 2021
@butonic butonic deleted the add-vitual-view-tests branch November 3, 2021 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants