diff --git a/src/control/lib/control/system.go b/src/control/lib/control/system.go index 314890204dc..c1019c2ed60 100644 --- a/src/control/lib/control/system.go +++ b/src/control/lib/control/system.go @@ -697,8 +697,14 @@ func (resp *SystemEraseResp) Errors() error { // checkSystemErase queries system to interrogate membership before deciding // whether a system erase is appropriate. -func checkSystemErase(ctx context.Context, rpcClient UnaryInvoker) error { - resp, err := SystemQuery(ctx, rpcClient, &SystemQueryReq{FailOnUnavailable: true}) +func checkSystemErase(ctx context.Context, rpcClient UnaryInvoker, sys string) error { + // sys is the request's raw (unversioned) system name; pass it through unmodified so + // SystemQuery's own getSystem() resolution (req.Sys, falling back to rpcClient's + // configured system name, then the build default) appends the version suffix exactly + // once, matching the resolution SystemErase itself performs below. + queryReq := &SystemQueryReq{FailOnUnavailable: true} + queryReq.SetSystem(sys) + resp, err := SystemQuery(ctx, rpcClient, queryReq) if err != nil { // If the AP hasn't been started, it will respond as if it // is not a replica. @@ -737,7 +743,11 @@ func SystemErase(ctx context.Context, rpcClient UnaryInvoker, req *SystemEraseRe return nil, errors.Errorf("nil %T request", req) } - if err := checkSystemErase(ctx, rpcClient); err != nil { + // Pass through the request's raw system name (not the version-suffixed form) so + // checkSystemErase's internal query resolves it through the same fallback chain + // (request -> rpcClient config -> build default) without double-appending the + // version suffix. + if err := checkSystemErase(ctx, rpcClient, req.Sys); err != nil { return nil, err } diff --git a/src/control/lib/control/system_test.go b/src/control/lib/control/system_test.go index d43db611704..329ee8a54ad 100644 --- a/src/control/lib/control/system_test.go +++ b/src/control/lib/control/system_test.go @@ -1314,6 +1314,7 @@ func TestControl_SystemCleanup(t *testing.T) { func TestControl_System_checkSystemErase(t *testing.T) { for name, tc := range map[string]struct { + sys string uErr, expErr error members []*mgmtpb.SystemMember }{ @@ -1328,6 +1329,9 @@ func TestControl_System_checkSystemErase(t *testing.T) { uErr: system.ErrRaftUnavail, }, "empty membership": {}, + "non-empty system name propagated to query": { + sys: "daos_nonstd", + }, "rank not stopped": { members: []*mgmtpb.SystemMember{ {Rank: 0, State: system.MemberStateStopped.String()}, @@ -1358,8 +1362,21 @@ func TestControl_System_checkSystemErase(t *testing.T) { &mgmtpb.SystemQueryResp{Members: tc.members}), }) - err := checkSystemErase(test.Context(t), mi) + err := checkSystemErase(test.Context(t), mi, tc.sys) test.CmpErr(t, tc.expErr, err) + + if tc.uErr == nil { + if len(mi.SentReqs) != 1 { + t.Fatalf("expected 1 request to be sent, got %d", len(mi.SentReqs)) + } + queryReq, ok := mi.SentReqs[0].(*SystemQueryReq) + if !ok { + t.Fatalf("expected *SystemQueryReq, got %T", mi.SentReqs[0]) + } + if diff := cmp.Diff(tc.sys, queryReq.Sys); diff != "" { + t.Fatalf("unexpected system name in query request (-want, +got):\n%s\n", diff) + } + } }) } } diff --git a/src/control/security/grpc_authorization.go b/src/control/security/grpc_authorization.go index ac6a0a30412..f4228d0b9c9 100644 --- a/src/control/security/grpc_authorization.go +++ b/src/control/security/grpc_authorization.go @@ -48,7 +48,7 @@ var methodAuthorizations = map[string][]Component{ "/mgmt.MgmtSvc/Join": {ComponentServer}, "/mgmt.MgmtSvc/ClusterEvent": {ComponentServer}, "/mgmt.MgmtSvc/LeaderQuery": {ComponentAdmin}, - "/mgmt.MgmtSvc/SystemQuery": {ComponentAdmin}, + "/mgmt.MgmtSvc/SystemQuery": {ComponentAdmin, ComponentServer}, "/mgmt.MgmtSvc/SystemErase": {ComponentAdmin, ComponentServer}, "/mgmt.MgmtSvc/SystemStart": {ComponentAdmin}, "/mgmt.MgmtSvc/SystemStop": {ComponentAdmin}, diff --git a/src/control/security/grpc_authorization_test.go b/src/control/security/grpc_authorization_test.go index 1a3ac71d82a..ce31f936ede 100644 --- a/src/control/security/grpc_authorization_test.go +++ b/src/control/security/grpc_authorization_test.go @@ -65,7 +65,7 @@ func TestSecurity_ComponentHasAccess(t *testing.T) { "/mgmt.MgmtSvc/Join": {ComponentServer}, "/mgmt.MgmtSvc/ClusterEvent": {ComponentServer}, "/mgmt.MgmtSvc/LeaderQuery": {ComponentAdmin}, - "/mgmt.MgmtSvc/SystemQuery": {ComponentAdmin}, + "/mgmt.MgmtSvc/SystemQuery": {ComponentAdmin, ComponentServer}, "/mgmt.MgmtSvc/SystemStop": {ComponentAdmin}, "/mgmt.MgmtSvc/SystemErase": {ComponentAdmin, ComponentServer}, "/mgmt.MgmtSvc/SystemStart": {ComponentAdmin}, diff --git a/src/control/server/mgmt_system.go b/src/control/server/mgmt_system.go index f04afb1d236..827a6b3df65 100644 --- a/src/control/server/mgmt_system.go +++ b/src/control/server/mgmt_system.go @@ -1823,6 +1823,10 @@ func (svc *mgmtSvc) eraseReplicas(ctx context.Context, peers []*net.TCPAddr) err svc.log.Tracef("sending erase RPC to peer %s", peer.String()) peerReq := new(control.SystemEraseReq) peerReq.AddHost(peer.String()) + // Explicitly set the system name rather than relying on svc.rpcClient's + // configured default, so the forwarded request is correctly scoped even if + // that default is ever left unset. + peerReq.SetSystem(svc.sysdb.SystemName()) if _, err := control.SystemErase(ctx, svc.rpcClient, peerReq); err != nil { // A connection error here can't be distinguished from "peer never @@ -1934,9 +1938,34 @@ func (svc *mgmtSvc) scheduleControlPlaneRestart(leaderStr string) error { return nil } -// execControlPlaneRestart drains the gRPC server via GracefulStop() (falling back to a -// forced Stop() after svc.gracefulStopTimeout), then exec()s to restart the control -// plane process. +// dumpGoroutineStacks logs the stacks of all running goroutines, for diagnosing hangs in +// gRPC/raft shutdown paths that have no other visibility into what is blocking progress. +func (svc *mgmtSvc) dumpGoroutineStacks(reason string) { + buf := make([]byte, 1<<20) + for { + n := runtime.Stack(buf, true) + if n < len(buf) { + buf = buf[:n] + break + } + buf = make([]byte, 2*len(buf)) + } + svc.log.Errorf("System Erase: dumping goroutine stacks (%s):\n%s", reason, buf) +} + +// execControlPlaneRestart drains the gRPC server via GracefulStop(), falling back to +// exec()ing to restart the control plane process after svc.gracefulStopTimeout if the +// drain hasn't completed by then. +// +// A forced grpcServer.Stop() is deliberately *not* attempted as a fallback here: Stop() +// and GracefulStop() share the same internal server mutex/condvar/connection bookkeeping +// (see grpc's Server.stop()), so calling Stop() while GracefulStop() is still draining +// just queues behind the same lock and blocks on the same "all connections closed" +// condition rather than forcing anything independently. If GracefulStop() is stuck (e.g. +// a peer keeps a connection open rather than closing it in response to drain), Stop() +// will be stuck too, so it only adds a wasted extra wait before exec() proceeds anyway. +// exec() itself terminates this process image and the kernel closes all of its sockets, +// which is sufficient to tear down the gRPC server's listeners/connections. func (svc *mgmtSvc) execControlPlaneRestart(myPath, leaderStr string) { scheduledAt := time.Now() @@ -1951,9 +1980,9 @@ func (svc *mgmtSvc) execControlPlaneRestart(myPath, leaderStr string) { svc.log.Debugf("System Erase: gRPC server drained cleanly after %s [role=%s]", time.Since(scheduledAt), leaderStr) case <-time.After(svc.gracefulStopTimeout): - svc.log.Errorf("System Erase: timed out after %s waiting for gRPC server to drain, forcing stop [role=%s]", + svc.log.Errorf("System Erase: timed out after %s waiting for gRPC server to drain, proceeding to exec anyway [role=%s]", svc.gracefulStopTimeout, leaderStr) - svc.grpcServer.Stop() + svc.dumpGoroutineStacks("GracefulStop() timed out") } svc.log.Infof("System Erase: exec'ing %s to restart control plane after %s total drain [role=%s]", diff --git a/src/control/server/server.go b/src/control/server/server.go index 78aa7bc7037..63b8e7090ba 100644 --- a/src/control/server/server.go +++ b/src/control/server/server.go @@ -244,6 +244,7 @@ func (srv *server) createServices(ctx context.Context) (err error) { // Create rpcClient for inter-server communication. cliCfg := control.DefaultConfig() cliCfg.TransportConfig = srv.cfg.TransportConfig + cliCfg.SystemName = srv.cfg.SystemName rpcClient := control.NewClient( control.WithClientComponent(build.ComponentServer), control.WithConfig(cliCfg), diff --git a/src/tests/ftest/control/dmg_system_reformat.py b/src/tests/ftest/control/dmg_system_reformat.py index 4bf8c87297e..b8259059be6 100644 --- a/src/tests/ftest/control/dmg_system_reformat.py +++ b/src/tests/ftest/control/dmg_system_reformat.py @@ -16,8 +16,11 @@ class DmgSystemReformatTest(TestWithServers): """Test Class Description: - Test to verify dmg storage format reformat option works as expected on a - DAOS system after a controlled shutdown. + Test to verify dmg system erase and storage format reformat work as expected on a + DAOS system after a controlled shutdown. Exercised across PMem and MD-on-SSD tier-0 + storage configurations (selected via launch.py --nvme) and single/multiple + management service (MS) replica configurations (selected via the + mgmt_svc_replicas_qty test yaml setup parameter). :avocado: recursive """ @@ -28,19 +31,43 @@ def setUp(self): self.update_log_file_names() super().setUp() + def get_replicas(self, dmg): + """Get the current set of management service (MS) replica ranks. + + Args: + dmg (DmgCommand): dmg command object to use to query the system. + + Returns: + list: sorted list of MS replica ranks reported by the leader. + + """ + leader_info = dmg.system_leader_query() + return sorted(leader_info["response"]["replicas"]) + def test_dmg_system_reformat(self): """ JIRA ID: DAOS-5415 - Test Description: Test dmg system reformat functionality. + Test Description: Test dmg system erase/reformat functionality across both + PMem and MD-on-SSD storage tier-0 configurations as well as single and + multiple management service (MS) replica configurations. The storage mode + (PMem vs. MD-on-SSD) is selected at launch time (e.g. launch.py --nvme=auto + vs. --nvme=auto_md_on_ssd); the MS replica count is varied via the + mgmt_svc_replicas_qty test yaml setup parameter. :avocado: tags=all,daily_regression :avocado: tags=hw,medium - :avocado: tags=control,dmg,system_reformat + :avocado: tags=control,dmg,system_reformat,system_erase :avocado: tags=DmgSystemReformatTest,test_dmg_system_reformat """ dmg = self.get_dmg_command().copy() + self.log.info( + "Testing with %s configured MS replica(s): %s", + len(self.mgmt_svc_replicas), self.mgmt_svc_replicas) + pre_erase_replicas = self.get_replicas(dmg) + self.log.info("MS replica ranks before erase: %s", pre_erase_replicas) + # Create pool using 90% of the available NVMe capacity pools = [self.get_pool(dmg=dmg)] @@ -56,7 +83,7 @@ def test_dmg_system_reformat(self): dmg.exit_status_exception = True self.log.info("Stop running engine instances: 'dmg system stop'") - dmg.system_stop(force=True) + dmg.system_stop() if dmg.result.exit_status != 0: self.fail("Detected issues performing a system stop: {}".format( dmg.result.stderr_text)) @@ -75,26 +102,54 @@ def test_dmg_system_reformat(self): self.log.info("Perform dmg storage format on all system ranks:") - # Calling storage format after system stop too soon would fail, so - # wait 10 sec and retry up to 4 times. + # Record the start time before the first format attempt so that engine-start + # detection below covers engines that joined during an earlier retry, even if + # that attempt's dmg client call locally timed out before the server-side + # format/join sequence actually completed (see MD-on-SSD timing note below). + start_time = journalctl_time() + + # Calling storage format after system stop too soon would fail, so wait 10 sec + # and retry up to 4 times. Use a generous per-call client timeout since + # MD-on-SSD NVMe format plus engine startup/join can legitimately take longer + # than the previous 30 sec default, especially under loaded CI hardware; a + # client-side timeout here does not necessarily mean the server-side format + # failed, it may just still be in progress. count = 0 while count < 4: try: - dmg.storage_format(force=True) + dmg.storage_format(timeout=90) if dmg.result.exit_status != 0: self.fail( - "Issues performing storage format --force: {}".format( + "Issues performing storage format: {}".format( dmg.result.stderr_text)) break except CommandFailure as error: + if "code = 509" in str(error) or "invoked on a running system" in str(error): + # A previous attempt's client-side call timed out locally but the + # server-side format/join actually completed in the background; + # this later retry is correctly rejected because the system is + # already up. Treat this as success rather than exhausting retries. + self.log.info( + "Storage format already completed by a prior attempt: %s", error) + break self.log.info("Storage format failed. Wait 10 sec and retry. %s", error) count += 1 time.sleep(10) - # Check that engine starts up again + # Check that engine starts up again on every host, not just the last one, since + # a multi-replica MS configuration spans more than one host. self.log.info(" Waiting for the engines to start") - self.server_managers[-1].manager.timestamps["start"] = journalctl_time() - self.server_managers[-1].detect_engine_start() + for server_manager in self.server_managers: + server_manager.manager.timestamps["start"] = start_time + server_manager.detect_engine_start() + + # Verify the MS replica set survived the erase/reformat cycle intact - this is the + # critical guarantee relied upon by the raft database erase-then-restart logic. + post_erase_replicas = self.get_replicas(dmg) + self.log.info("MS replica ranks after erase/reformat: %s", post_erase_replicas) + self.assertEqual( + pre_erase_replicas, post_erase_replicas, + "MS replica ranks changed as a result of system erase/reformat") # Check that we have cleared storage by checking pool list if dmg.get_pool_list_uuids(): diff --git a/src/tests/ftest/control/dmg_system_reformat.yaml b/src/tests/ftest/control/dmg_system_reformat.yaml index ca86e2aa8d8..fe3a50a9471 100644 --- a/src/tests/ftest/control/dmg_system_reformat.yaml +++ b/src/tests/ftest/control/dmg_system_reformat.yaml @@ -1,13 +1,27 @@ hosts: - test_servers: 1 + test_servers: 3 test_clients: 1 timeout: 300 +setup: !mux + single_replica: + mgmt_svc_replicas_qty: 1 + multi_replica: + mgmt_svc_replicas_qty: 3 server_config: - name: daos_server engines_per_host: 1 engines: 0: storage: auto system_ram_reserved: 32 -pool: - size: 90% + variant: !mux + standard: + name: daos_server + non_standard: + name: daos_nonstd + port: 10515 +pool: !mux + default: + size: 90% + md_on_ssd_p2: + size: 90% + mem_ratio: 25 diff --git a/src/tests/ftest/util/test_utils_pool.py b/src/tests/ftest/util/test_utils_pool.py index b45d10b4848..a69df7007a5 100644 --- a/src/tests/ftest/util/test_utils_pool.py +++ b/src/tests/ftest/util/test_utils_pool.py @@ -39,6 +39,7 @@ def get_size_params(pool): """ return {"size": None, "tier_ratio": None, + "mem_ratio": None, "scm_size": pool.scm_per_rank, "nvme_size": pool.nvme_per_rank}