noobaa-core icon indicating copy to clipboard operation
noobaa-core copied to clipboard

NSFS dir attrs should be taken from the dir itself and use a .folder subfile only if content is provided at all

Open guymguym opened this issue 2 years ago • 13 comments

FYI @eshelmarc @troppens - This is a follow up to the discussion in #6412 and https://github.com/noobaa/noobaa-core/pull/6947/files#r898130803.

Today there is a difference in how keys are interpreted w/o a final slash:

No-Slash-Case - HEAD dir:

  • We open the path dir.
  • If it doesn't exist we return NoSuchKey.
  • If it does, but is a directory, then we return NoSuchKey - which was introduced in #6410.
  • If it's a file (that was named dir for spite), we return the x/attrs of it.

Yes-Slash-Case- HEAD dir/:

  • We open the path dir/.folder.
  • If it doesn't exist we return NoSuchKey.
  • If it does, we return the x/attrs of that .folder subfile.
  • This same applies also to GET dir/ and PUT dir/, which means that directory x/attrs will be set on the subfile.
  • The rational behind this flow is that we must have an option to store contents with PUT dir/, although perhaps it's a rare use case to send read content with final slash.

We want to change this so that the x/attrs will get/set from/to the dir itself, and only if there's content it will be written to a .folder subfile (maybe this file name doesn't fit anymore so we can rename). But to complement this we also need LIST to be able to differentiate between a folder which was created in S3 and one that wasn't, so we might need to put some xattr on dirs that get created through s3.

The reason behind this change is to improve protocol interoperability and make attributes of directories exposed to S3, and vice versa.

guymguym avatar Jun 16 '22 11:06 guymguym

The problem that I saw was related to the different behavior of AFM/S3 It has an option to support directories as object so we can save xattr like ACLs that all files in the directory can inherent. Without this option every thing is working fine but with it there is comparability with directories/files that already existed or bucket that was created without this option and than it was turned on. The problem is with the missing .folder file that fails HEAD request . So, if like suggested above, we are going to eliminate the .folder file and add the xattr to the directory files that will solve the problem for AFM. I am not sure how complicated this change is but if it is maybe we can for now just not fail HEAD of directory just because the .folder is missin.

eshelmarc avatar Jun 17 '22 02:06 eshelmarc

@guymguym @eshelmarc I started working on this bug and I have a question, AFAIU the issue is that when heading a directory that was not created by NooBaa, the .folder file is missing. So why not create and populate .folder file when we observe this situation?

romayalon avatar Jul 04 '22 12:07 romayalon

From a multi-protocol access perspective, we do not want to have a directory .folder in each directory.

Where is this .folder directory coming from? Why it is required?

troppens avatar Jul 04 '22 12:07 troppens

@romayalon I don't think we want to preserve this, we want to get rid of it.

guymguym avatar Jul 04 '22 13:07 guymguym

The case where a directory path is used to store content in S3 is esoteric anyway.

guymguym avatar Jul 04 '22 13:07 guymguym

okay, np! Should I handle upgrade as well, right?

romayalon avatar Jul 04 '22 14:07 romayalon

Should I handle upgrade as well, right?

Yes, but first let's focus on implementing how we want this to function, and then we can evaluate the upgrade between the previous behavior and location of dir xattr+data.

guymguym avatar Jul 05 '22 12:07 guymguym

@eshelmarc @troppens @guymguym and I had a meeting about this change and some questions/doubts were raised, would be glad to hear your opinions about the following:

  1. By moving out the xattr from the .folder file to the directory, the atomicity between the metadata and the data will no longer exist.
  2. A flow in which a user creates a directory object (similarly to PUT a/ object) through the file system is expected to be used?
  3. HEAD a vs HEAD a/ - according to a discussion in @eshelmarc PR it was expected that HEAD a will return the same response as HEAD a/ but I tested both calls on AWS and HEAD a will throw Not Found error while HEAD a/ will return the wanted response. Since we want to be compatible with AWS, we would prefer to stick with the current result.

romayalon avatar Jul 06 '22 11:07 romayalon

  1. Which data are we talking about ? .folder and now the directory file we not have data, just xattr.
  2. yes
  3. with xattr in the directory only a/ should return found if a is a directory, this is the only way the caller can tell that a is a directory.

eshelmarc avatar Jul 06 '22 17:07 eshelmarc

@eshelmarc let me be more specific here -

  1. In S3 PUT a/ can be called with content of size > 0, in this situation, we will have data that will be stored in .folder file, while the xattr will be stored on the dir itself.
  2. Same here, I meant to create a directory object with content of size > 0, is it still an existing flow we should support?

romayalon avatar Jul 07 '22 08:07 romayalon

about point 2 - the question is different I think - since a dir-object (regardless of having content or not) can only be created from S3 because we would need to differentiate between an actual PUT a/ which will create a dir-object with key a/, vs. PUT a/file which "accidentally" creates dir a/ but not as an S3 object, and therefore will not be listed in list_objects etc. The question remaining is - do we need a way to create a dir-object from the FS directly? Our direction is to use an xattr to mark dir-objects so it should be possible with the right xattr name, but it won't be "natural" for FS users.

guymguym avatar Jul 07 '22 08:07 guymguym

about point 3 - @eshelmarc your recent fix attempt was to allow HEAD a (without a trailing slash!) to detect that a is a directory on the filesystem and then return its xattrs instead of returning NotFound, which seems non S3 compliant because there is no such dir-object without a trailing slash, and only HEAD a/ should succeed. Is that satisfying your client needs?

guymguym avatar Jul 07 '22 08:07 guymguym

point 2: the fs (our AFM S3 client) will not create a/file without first create a/ but for compatibility it will be required that HEAD of a/ will return xattr even if it was not created with PUT a/

point 3: my fix attempt is not needed with this new code that stores xattr in the a/ directory and not in .folder

eshelmarc avatar Jul 07 '22 16:07 eshelmarc

Fixed with #7020

nimrod-becker avatar Sep 13 '23 08:09 nimrod-becker