Merge pull request #43405 from azat/fs/use-access

Simply filesystem helpers to check is-readable/writable/executable

src/Common/filesystemHelpers.cpp

@@ -312,39 +312,32 @@ bool exists(const std::string & path)
 
 bool canRead(const std::string & path)
 {
-    struct stat st;
-    if (stat(path.c_str(), &st) == 0)
-    {
-        if (st.st_uid == geteuid())
-            return (st.st_mode & S_IRUSR) != 0;
-        else if (st.st_gid == getegid())
-            return (st.st_mode & S_IRGRP) != 0;
-        else
-            return (st.st_mode & S_IROTH) != 0 || geteuid() == 0;
-    }
+    int err = faccessat(AT_FDCWD, path.c_str(), R_OK, AT_EACCESS);
+    if (err == 0)
+        return true;
+    if (errno == EACCES)
+        return false;
     DB::throwFromErrnoWithPath("Cannot check read access to file: " + path, path, DB::ErrorCodes::PATH_ACCESS_DENIED);
 }
 
 bool canWrite(const std::string & path)
 {
-    struct stat st;
-    if (stat(path.c_str(), &st) == 0)
-    {
-        if (st.st_uid == geteuid())
-            return (st.st_mode & S_IWUSR) != 0;
-        else if (st.st_gid == getegid())
-            return (st.st_mode & S_IWGRP) != 0;
-        else
-            return (st.st_mode & S_IWOTH) != 0 || geteuid() == 0;
-    }
+    int err = faccessat(AT_FDCWD, path.c_str(), W_OK, AT_EACCESS);
+    if (err == 0)
+        return true;
+    if (errno == EACCES)
+        return false;
     DB::throwFromErrnoWithPath("Cannot check write access to file: " + path, path, DB::ErrorCodes::PATH_ACCESS_DENIED);
 }
 
 bool canExecute(const std::string & path)
 {
-    if (exists(path))
-        return faccessat(AT_FDCWD, path.c_str(), X_OK, AT_EACCESS) == 0;
-    DB::throwFromErrnoWithPath("Cannot check execute access to file: " + path, path, DB::ErrorCodes::PATH_ACCESS_DENIED);
+    int err = faccessat(AT_FDCWD, path.c_str(), X_OK, AT_EACCESS);
+    if (err == 0)
+        return true;
+    if (errno == EACCES)
+        return false;
+    DB::throwFromErrnoWithPath("Cannot check write access to file: " + path, path, DB::ErrorCodes::PATH_ACCESS_DENIED);
 }
 
 time_t getModificationTime(const std::string & path)

src/Disks/DiskLocalCheckThread.cpp

@@ -54,8 +54,11 @@ void DiskLocalCheckThread::run()
     else
     {
         retry = 0;
+        if (!disk->broken)
+            LOG_ERROR(log, "Disk {} marked as broken", disk->getName());
+        else
+            LOG_INFO(log, "Disk {} is still broken", disk->getName());
         disk->broken = true;
-        LOG_INFO(log, "Disk {} is broken", disk->getName());
         task->scheduleAfter(check_period_ms);
     }
 }

tests/integration/helpers/cluster.py

@@ -1854,6 +1854,8 @@ class ClickHouseCluster:
             exec_cmd = ["docker", "exec"]
             if "user" in kwargs:
                 exec_cmd += ["-u", kwargs["user"]]
+            if "privileged" in kwargs:
+                exec_cmd += ["--privileged"]
             result = subprocess_check_call(
                 exec_cmd + [container_id] + cmd, detach=detach, nothrow=nothrow
             )
@@ -3003,6 +3005,8 @@ services:
             - NET_ADMIN
             - IPC_LOCK
             - SYS_NICE
+            # for umount/mount on fly
+            - SYS_ADMIN
         depends_on: {depends_on}
         user: '{user}'
         env_file:

tests/integration/test_jbod_ha/test.py

@@ -72,9 +72,21 @@ def test_jbod_ha(start_cluster):
 
         node2.query("SYSTEM SYNC REPLICA tbl", timeout=10)
 
-        # mimic disk failure
+        # Mimic disk failure
+        #
+        # NOTE: you cannot do one of the following:
+        # - chmod 000 - this will not block access to the owner of the namespace,
+        #   and running clickhouse from non-root user is very tricky in this
+        #   sandbox.
+        # - unmount it, to replace with something else because in this case you
+        #   will loose tmpfs and besides clickhouse works from root, so it will
+        #   still be able to write/read from/to it.
+        #
+        # So it simply mounts over tmpfs, proc, and this will throw exception
+        # for read, because there is no such file and does not allows writes
+        # either.
         node1.exec_in_container(
-            ["bash", "-c", "chmod -R 000 /jbod1"], privileged=True, user="root"
+            ["bash", "-c", "mount -t proc proc /jbod1"], privileged=True, user="root"
         )
 
         time.sleep(3)
@@ -91,9 +103,11 @@ def test_jbod_ha(start_cluster):
 
         assert int(node1.query("select count(p) from tbl")) == 2500
 
-        # mimic disk recovery
+        # Mimic disk recovery
+        #
+        # NOTE: this will unmount only proc from /jbod1 and leave tmpfs
         node1.exec_in_container(
-            ["bash", "-c", "chmod -R 755 /jbod1"],
+            ["bash", "-c", "umount  /jbod1"],
             privileged=True,
             user="root",
         )