cephfs: Create the subvolume with --case-insensitive

[anoopcs9]

ceph/ceph#62105 is now merged which introduces the --case-insensitive subvolume option to make it case insensitive to suite SMB use cases.

[xhernandez]

Shouldn't we make it configurable ?

[anoopcs9]

Shouldn't we make it configurable ?

We only access those subvolumes via Samba configured shares. That's why I made it mandatory. Or do you intend to extend the matrix by defining another set of shares with case insensitive subvolumes?

[anoopcs9]

There is an interesting failure with kernel client..

=================================== FAILURES ===================================
________ test_consistency[192.168.123.10-share-cephfs-default-kclient] _________

self = <testhelper.smbclient.SMBClient object at 0x7f96cef0c850>
fpath = '/test_consistency'
teststr = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz'

    def write_text(self, fpath: str, teststr: str) -> None:
        self._check_connected("write_text")
        try:
>           with smbclient.open_file(
                self._path(fpath), mode="w", **self.client_params
            ) as fd:

testhelper/smbclient.py:130: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.tox/sanity/lib/python3.9/site-packages/smbclient/_os.py:533: in open_file
    raw_fd.open()
.tox/sanity/lib/python3.9/site-packages/smbclient/_io.py:463: in open
    transaction.commit()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <smbclient._io.SMBFileTransaction object at 0x7f96cee34400>

    def commit(self):
        """
        Sends a compound request to the server. Optionally opens and closes a handle in the same request if the handle
        is not already opened.
        """
        remove_index = []
        if self.raw.closed and (not self._actions or not isinstance(self._actions[0][0], SMB2CreateRequest)):
            self.raw.open(transaction=self)
            self._actions.insert(0, self._actions.pop(-1))  # Need to move the create to the start
            remove_index.insert(0, 0)
    
            self.raw.close(transaction=self)
            remove_index.insert(0, len(self._actions) - 1)
    
        send_msgs = []
        unpack_functions = []
        for action in self._actions:
            send_msgs.append(action[0])
            unpack_functions.append(action[1])
    
        sid = self.raw.fd.tree_connect.session.session_id
        tid = self.raw.fd.tree_connect.tree_connect_id
        requests = self.raw.fd.connection.send_compound(send_msgs, sid, tid, related=True)
    
        # Due to a wonderful threading issue we need to ensure we call .receive() for each message we sent so cannot
        # just enumerate the list in 1 line in case it throws an exception.
        failures = []
        responses = []
        try_again = False
        for idx, func in enumerate(unpack_functions):
            res = None
    
            try:
                try:
                    res = func(requests[idx])
    
                except (PathNotCovered, ObjectNameNotFound, ObjectPathNotFound):
                    # The MS-DFSC docs state that STATUS_PATH_NOT_COVERED is used when encountering a link to a
                    # different server but Samba seems to return the generic name or path not found.
    
                    # If the first action is not a CreateRequest then we can't resolve the DFS path in the transaction.
                    if not (idx == 0 and isinstance(send_msgs[0], SMB2CreateRequest)):
                        raise
    
                    for smb_open in _resolve_dfs(self.raw):
                        if smb_open.tree_connect.share_name == self.raw.fd.tree_connect.share_name:
                            continue
    
                        # Ensure we don't continuously try the same DFS referral targets if it's already been attempted.
                        # https://github.com/jborean93/smbprotocol/issues/228
                        tested_path = f"{smb_open.tree_connect.share_name}{smb_open.file_name}".lower()
                        if tested_path in self._attempted_dfs_paths:
                            continue
    
                        self._attempted_dfs_paths.add(tested_path)
    
                        self.raw.fd = smb_open
    
                        # In case this is a transaction with an explicit open we want to reopen it with the new params
                        # before trying it again.
                        self.raw.open(transaction=self)
                        self._actions[0] = self._actions.pop(-1)
    
                        try_again = True
                        break
    
                    else:
                        # Either there wasn't any DFS referrals or none of them worked, just reraise the error.
                        raise
    
            except SMBResponseException as exc:
                failures.append(SMBOSError(exc.status, self.raw.name))
    
            finally:
                responses.append(res)
    
        # Remove the existing open/close actions this transaction added internally.
        for idx in remove_index:
            del self._actions[idx]
            del responses[idx]
    
        if try_again:
            # If we updated the SMB Tree due to a DFS referral hit then try again.
            self.commit()
    
        elif failures:
            # If there was a failure, just raise the first exception found.
>           raise failures[0]
E           smbprotocol.exceptions.SMBOSError: [Error 0] [NtStatus 0xc0000022] Unknown NtStatus error returned 'STATUS_ACCESS_DENIED': '\\192.168.123.10\share-cephfs-default-kclient\/test_consistency'

.tox/sanity/lib/python3.9/site-packages/smbclient/_io.py:349: SMBOSError

During handling of the above exception, another exception occurred:

hostname = '192.168.123.10', share_name = 'share-cephfs-default-kclient'

    @pytest.mark.parametrize("hostname,share_name", generate_consistency_check())
    def test_consistency(hostname: str, share_name: str) -> None:
>       consistency_check(hostname, share_name)

testcases/consistency/test_consistency.py:58: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
testcases/consistency/test_consistency.py:31: in consistency_check
    smbclient.write_text(test_filename, test_string)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <testhelper.smbclient.SMBClient object at 0x7f96cef0c850>
fpath = '/test_consistency'
teststr = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz'

    def write_text(self, fpath: str, teststr: str) -> None:
        self._check_connected("write_text")
        try:
            with smbclient.open_file(
                self._path(fpath), mode="w", **self.client_params
            ) as fd:
                fd.write(teststr)
        except SMBException as error:
>           raise IOError(f"write: {error}")
E           OSError: write: [Error 0] [NtStatus 0xc0000022] Unknown NtStatus error returned 'STATUS_ACCESS_DENIED': '\\192.168.123.10\share-cephfs-default-kclient\/test_consistency'

testhelper/smbclient.py:135: OSError

I'm wondering how the case sensitivity configuration can affect just kernel client mounts !

[xhernandez]

Shouldn't we make it configurable ?

We only access those subvolumes via Samba configured shares. That's why I made it mandatory. Or do you intend to extend the matrix by defining another set of shares with case insensitive subvolumes?

No, I don't think we need to extend the test matrix with a new option, but having it configurable makes it more flexible for debugging/testing, if needed. On the other side, it's relatively easy to reconfigure it once created, so it's not that important to have an option. As you prefer.

[xhernandez]

I'm wondering how the case sensitivity configuration can affect just kernel client mounts !

I don't see how it could be related, specially if we are not using different names that could be seen by the kernel as different entries. It's a bit weird.

I'll trigger another run to see if it happens consistently.

[xhernandez]

/retest centos-ci/cephfs

[anoopcs9]

I'll trigger another run to see if it happens consistently.

Re-triggered run also failed. Out of curiosity I manually kicked a normal run without this change and it passed.

[anoopcs9]

I'll trigger another run to see if it happens consistently.

Re-triggered run also failed. Out of curiosity I manually kicked a normal run without this change and it passed.

.. and it fails after rebase for shares out of kernel client mounts. There's something going on !!

[xhernandez]

I've seen this in the CephFS documentation for the case folding feature (https://docs.ceph.com/en/squid/cephfs/charmap/#restricting-incompatible-client-access):

The MDS protects access to directory trees with a charmap via a new client feature bit. The MDS will not allow a client that does not understand the charmap feature to modify a directory with a charmap configuration except to unlink files or remove subdirectories.

and it also says later that the kernel client doesn't support charmap:

The kernel driver does not understand the charmap feature and probably will not because existing kernel libraries have opinionated case folding and normalization forms.

This basically means that the kernel client cannot create a file, which is what the sanity test tries to do.

I only see two possibilities to adapt for this issue:

1. Create a configuration option to enable or not case-insensitive. If it's enabled, the kernel client share should not be created.
2. Remove support for kernel client in all configurations.

I'm more inclined for the first option, but given that most likely we'll always run tests with case-insensitive enabled, it doesn't make much sense to keep it if we'll never use it.

[anoopcs9]

I've seen this in the CephFS documentation for the case folding feature (https://docs.ceph.com/en/squid/cephfs/charmap/#restricting-incompatible-client-access):

The MDS protects access to directory trees with a charmap via a new client feature bit. The MDS will not allow a client that does not understand the charmap feature to modify a directory with a charmap configuration except to unlink files or remove subdirectories.

and it also says later that the kernel client doesn't support charmap:

The kernel driver does not understand the charmap feature and probably will not because existing kernel libraries have opinionated case folding and normalization forms.

This basically means that the kernel client cannot create a file, which is what the sanity test tries to do.

I only see two possibilities to adapt for this issue:

1. Create a configuration option to enable or not case-insensitive. If it's enabled, the kernel client share should not be created.

2. Remove support for kernel client in all configurations.

I'm more inclined for the first option, but given that most likely we'll always run tests with case-insensitive enabled, it doesn't make much sense to keep it if we'll never use it.

I agree.

@spuiuk What do you think? Should we make a configurable option for deciding the case sensitivity of the underlying CephFS subvolume? Or right away get rid of shares based on kernel client mounts?

[spuiuk]

There is a third option. Create a subvolume which is case sensitive and export that with the kernel mount. This will involve some code change as we then set the shares exported based on the volume type and it is an extension of option 1 but instead of a global option, a per volume setting. But we can then test the Ceph kernel module as well.

I would like option 1 - ie. use an option to enable case insensitive and set it on by default. That way, if we ever want to test kernel ceph mount, then we can go ahead and disable it for our one off test.

[xhernandez]

There is a third option. Create a subvolume which is case sensitive and export that with the kernel mount. This will involve some code change as we then set the shares exported based on the volume type and it is an extension of option 1. But we can then test the Ceph kernel module as well.

Is it useful to test the kernel client on a different subvolume than the other shares ? specially if it's using a different configuration. I think we wouldn't be comparing apples to apples.

[spuiuk]

Maybe we should continue testing with case-sensitive option enabled for vfs_ceph_new for one of the subvolumes along with the kernel-ceph client. One of the reasons for this could be new requirements which require us to have case-sensitivity enabled for vfs_ceph_new.