diff --git a/README.md b/README.md index 94aaa80..0a8a761 100644 --- a/README.md +++ b/README.md @@ -300,8 +300,8 @@ a user whom EOS does not take for root, they leave out owners, and the tests that need root on EOS (changing a file's layout) say that they are skipped; with `EOSMIRROR_EOS_AS_ROOT=1`, as `scripts/integration-test.sh` sets it, they fail instead. The test of listings with find limits needs -the limits for nobody that `instance.sh` sets, and a caller that EOS lets -become nobody. +the limits for nobody that `instance.sh` sets, and a caller that is neither +nobody nor a sudoer. Two longer tests (about a minute each) run only when asked for, as in CI's on-demand job: `ctest --test-dir build -C long -L long`. One kills diff --git a/docs/architecture.md b/docs/architecture.md index 87bd747..6fa7986 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -463,8 +463,8 @@ of it, not an independent model of EOS. (read back with stripes dropped, since a normal read cannot tell), the EOS-specific failure paths, the self-test, a sudoer's identity. The EOS tests run as root on EOS, and check that they do; on another instance as - another user, they leave out owners and skip, with a message, what needs - root there. + another user, they leave out owners and skip, with a message, what that + user cannot test there. - Long (`ctest -C long -L long`): the randomized differential test (random trees, random changes, injected faults; the journal must record what was left behind before a clean run repairs it, and the target must then equal diff --git a/docs/design.md b/docs/design.md index 8bbe042..791b90e 100644 --- a/docs/design.md +++ b/docs/design.md @@ -669,21 +669,23 @@ endpoint uses the MGM's commands, sent the way the `eos` client sends them read the storage node is asked for the replica's stat again (a request on the open file, which XrdCl sends to the storage node): another size or mtime means that the file changed while it was read, and the copy is - retried. That stat is of the replica opened: a file replaced during the - read is not seen there, its copy gets the old, listed mtime, and the next - run copies it again. A file with another size than listed has changed - since the listing: it is stated through the MGM before and after reading, - so that it is still copied in this run, with its current metadata. Those - stats must find the file opened under its name, known by its id from the - redirect (`mgm.id`, as for uploads): a file replaced once more since the - open has another id, which fails the copy as changed (it is retried). A - file rewritten with the same size after the listing shows up as a checksum - that differs from the listed one. A stat through the MGM then tells a - change (the copy fails as changed, after its retries, and the next run - copies it) from corrupt data (the MGM still has the listed size and - mtime). Where the listing has no checksum (a directory without - `sys.forced.checksum`), such a file is copied with the listed, older - mtime, and the next run copies it again. + retried. That stat is of the replica opened. It does not show a file + replaced during the read, and of a file with several replicas, it may not + show an append made during the read (seen with replicas on different + storage nodes). The copy is then of the listed version, with the listed + mtime, and the next run copies the file again. A file with another size + than listed has changed since the listing: it is stated through the MGM + before and after reading, so that it is still copied in this run, with its + current metadata. Those stats must find the file opened under its name, + known by its id from the redirect (`mgm.id`, as for uploads): a file + replaced once more since the open has another id, which fails the copy as + changed (it is retried). A file rewritten with the same size after the + listing shows up as a checksum that differs from the listed one. A stat + through the MGM then tells a change (the copy fails as changed, after its + retries, and the next run copies it) from corrupt data (the MGM still has + the listed size and mtime). Where the listing has no checksum (a directory + without `sys.forced.checksum`), such a file is copied with the listed, + older mtime, and the next run copies it again. - EOS's own hidden entries (atomic temporaries `.sys.a#.`, version directories `.sys.v#.`) are never treated as entries of the tree. An atomic temporary gets the `eos.mtime` of its open when EOS creates it, diff --git a/src/eosmirror/eos_endpoint.cc b/src/eosmirror/eos_endpoint.cc index f93850d..234e0c3 100644 --- a/src/eosmirror/eos_endpoint.cc +++ b/src/eosmirror/eos_endpoint.cc @@ -234,9 +234,10 @@ std::string format_timespec(const Timespec& t) { // replica has the listed size. The storage node's stat of the replica, which // comes with the open, has the file's size (the logical size of erasure-coded // files) but the replica's own mtime, in seconds (fst/XrdFstOfsFile.cc stat): -// a change during the copy shows in its stat after the read. A file whose -// size differs from the listing has changed since. It is stated through the -// MGM before and after reading, and each stat must find the opened file under +// a change to that replica during the copy shows in its stat after the read +// (see docs/design.md for the changes it does not show). A file whose size +// differs from the listing has changed since. It is stated through the MGM +// before and after reading, and each stat must find the opened file under // its name (a file replaced since has another id). class EosReader : public XrdReader { public: diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 57e6783..606116f 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -74,9 +74,9 @@ if(EOSMIRROR_WITH_XROOTD) # that may be where the compiler's sanitizer runtimes are found (a # compiler outside the system's, as in an LCG view). Leaks in XRootD's and # OpenSSL's global state are not ours (lsan.supp), nor are the races TSan - # sees in glibc's time zone setup and in XrdCl's file state handler - # (tsan.supp). The unit tests load the - # client library too. UBSan stops at its first finding, with a stack trace. + # sees in glibc's time zone setup, XrdCl's file state handler and XrdCl's + # response parsing (tsan.supp). The unit tests load the client library + # too. UBSan stops at its first finding, with a stack trace. if(EOSMIRROR_SANITIZER AND EOSMIRROR_XROOTD_LIBDIR) set(sanitized_tests unit unit_unprivileged integration integration_sudoer cli_smoke differential crash_consistency) set_tests_properties(${sanitized_tests} diff --git a/tests/integration/test_eos.cc b/tests/integration/test_eos.cc index d3053e3..f4dc705 100644 --- a/tests/integration/test_eos.cc +++ b/tests/integration/test_eos.cc @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -178,8 +179,9 @@ bool as_root(EosEndpoint& ep) { } // Whether asking for nobody's identity (eos.ruid, eos.rgid) makes this -// caller nobody, and no sudoer, as find limits need. EOS grants it to a -// sudoer, and to another caller that holds nobody's ids (common/Mapping.cc). +// caller nobody, and no sudoer, as find limits need. EOS makes every caller +// but a sudoer nobody then: one that does not hold the ids it asks for gets +// nobody's. A sudoer gets them, but stays a sudoer (common/Mapping.cc). bool becomes_nobody(const std::string& url) { auto ep = EosEndpoint::create(url + "?eos.ruid=65534&eos.rgid=65534"); REQUIRE(ep.ok()); @@ -869,6 +871,26 @@ void append(const std::string& url, std::string_view data) { REQUIRE(file.Close().IsOK()); } +// The storage nodes that hold a file's replicas, from the hosts in the JSON +// of fileinfo. +std::set replica_hosts(EosEndpoint& ep, const RelPath& path) { + auto info = ep.proc("mgm.cmd=fileinfo&mgm.format=json&mgm.path=" + ep.absolute(path)); + REQUIRE(info.ok()); + const std::string& json = info.value().out; + INFO("fileinfo: ", json); + std::set hosts; + for (size_t at = json.find("\"host\""); at != std::string::npos; + at = json.find("\"host\"", at + 1)) { + size_t begin = json.find('"', json.find(':', at)); + REQUIRE(begin != std::string::npos); + size_t end = json.find('"', ++begin); + REQUIRE(end != std::string::npos); + hosts.insert(json.substr(begin, end - begin)); + } + REQUIRE_FALSE(hosts.empty()); + return hosts; +} + } // namespace TEST_CASE("EOS to FS: files that changed since the listing or while they are read") { @@ -924,10 +946,13 @@ TEST_CASE("EOS to FS: files that changed since the listing or while they are rea CHECK(tmp.read_file("dst/f") == "replaced with a longer content"); // Appended to while it is read: changed, and copied as it is now when the - // copy is retried with the same listed entry. + // copy is retried with the same listed entry. Of a file with replicas on + // several storage nodes, the one read may not show the append + // (docs/design.md): the copy is then of the listed version. std::string content = pattern(300000); put(ep, "g", content); old = listed(ep, "g"); + size_t nodes = replica_hosts(ep, "g").size(); CopyOptions appending = options; bool appended = false; appending.on_chunk = [&](uint64_t) { @@ -935,10 +960,18 @@ TEST_CASE("EOS to FS: files that changed since the listing or while they are rea }; auto grown = copy_file(ep, dst, "g", old, appending, pool, cancel); REQUIRE(appended); - REQUIRE_FALSE(grown.ok()); - INFO("error: ", grown.error().describe()); - CHECK(grown.error().kind == ErrorKind::Changed); - CHECK_FALSE(dst.stat("g").ok()); + if (grown.ok() && nodes > 1) { + MESSAGE("the replica read does not show the append: copied as listed"); + CHECK(tmp.read_file("dst/g") == content); + REQUIRE(dst.flush().ok()); + CHECK(must(dst.stat("g")).mtime == old.mtime); + } else { + INFO("storage nodes: ", nodes); + REQUIRE_FALSE(grown.ok()); + INFO("error: ", grown.error().describe()); + CHECK(grown.error().kind == ErrorKind::Changed); + CHECK_FALSE(dst.stat("g").ok()); + } auto retried = copy_file(ep, dst, "g", old, options, pool, cancel); REQUIRE(retried.ok()); CHECK(tmp.read_file("dst/g") == content + "more"); @@ -1046,8 +1079,9 @@ TEST_CASE("eos endpoint: listings for an identity with find limits") { if (!base_url()) return; RemoteDir dir("limits"); EosEndpoint& ep = *dir.endpoint; - if (!becomes_nobody(dir.url)) { - MESSAGE("this caller cannot become nobody: listings with find limits not tested"); + // A caller that is nobody would own the directory that nobody may not read. + if (eos_id(ep).uid == 65534 || !becomes_nobody(dir.url)) { + MESSAGE("the caller is nobody, or a sudoer as nobody: listings with find limits not tested"); return; } // The instance limits nobody to 20 files and 10 directories per find, diff --git a/tests/tsan.supp b/tests/tsan.supp index 52c6ee0..8f242dd 100644 --- a/tests/tsan.supp +++ b/tests/tsan.supp @@ -7,3 +7,8 @@ race:XrdSysTimer::TimeZone # uninstrumented library: TSan takes the handler's destruction by the last # owner, an XrdCl thread, for a race with the File's earlier calls. race:XrdCl::FileStateHandler +# Now and then, with Ubuntu 24.04's XRootD 5.6, TSan reports XrdCl's poller +# thread freeing a response buffer that a job thread copied before in +# XRootDMsgHandler::ParseResponse: a race, if any, between XrdCl's own +# threads inside the uninstrumented library. +race:XrdCl::XRootDMsgHandler::ParseResponse