From 35e276bf2960484fb9db1921c9a7c2898c136af4 Mon Sep 17 00:00:00 2001 From: dannygeng Date: Tue, 28 Jul 2026 11:48:57 +0800 Subject: [PATCH 1/6] fix: add dependency-aware startup checks --- .../io/pixelsdb/pixels/daemon/DaemonMain.java | 14 +- .../pixels/daemon/ServerContainer.java | 178 ++++--- .../pixelsdb/pixels/daemon/StartupCheck.java | 41 ++ .../daemon/metadata/MetadataReadyCheck.java | 114 +++++ .../daemon/metadata/MetadataServer.java | 2 +- .../daemon/retina/RetinaReadyCheck.java | 197 ++++++++ .../pixels/daemon/retina/RetinaServer.java | 89 +--- .../daemon/retina/RetinaServerImpl.java | 33 +- .../daemon/transaction/TransServer.java | 146 +----- .../pixels/daemon/TestServerContainer.java | 478 ++++++++++++++++++ 10 files changed, 1012 insertions(+), 280 deletions(-) create mode 100644 pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/StartupCheck.java create mode 100644 pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/metadata/MetadataReadyCheck.java create mode 100644 pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/retina/RetinaReadyCheck.java create mode 100644 pixels-daemon/src/test/java/io/pixelsdb/pixels/daemon/TestServerContainer.java diff --git a/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/DaemonMain.java b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/DaemonMain.java index c2ca3b9e34..7f9e653454 100644 --- a/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/DaemonMain.java +++ b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/DaemonMain.java @@ -6,10 +6,12 @@ import io.pixelsdb.pixels.daemon.cache.CacheWorker; import io.pixelsdb.pixels.daemon.index.IndexServer; import io.pixelsdb.pixels.daemon.node.NodeServer; +import io.pixelsdb.pixels.daemon.retina.RetinaReadyCheck; import io.pixelsdb.pixels.daemon.retina.RetinaServer; import io.pixelsdb.pixels.daemon.exception.NoSuchServerException; import io.pixelsdb.pixels.daemon.heartbeat.HeartbeatCoordinator; import io.pixelsdb.pixels.daemon.heartbeat.HeartbeatWorker; +import io.pixelsdb.pixels.daemon.metadata.MetadataReadyCheck; import io.pixelsdb.pixels.daemon.metadata.MetadataServer; import io.pixelsdb.pixels.daemon.metrics.MetricsServer; import io.pixelsdb.pixels.daemon.scaling.ScalingMetricsServer; @@ -108,7 +110,7 @@ public static void main(String[] args) container.addServer("metadata", metadataServer); // start transaction server TransServer transServer = new TransServer(transServerPort); - container.addServer("transaction", transServer); + container.addServer("transaction", transServer, new RetinaReadyCheck()); // start query schedule server QueryScheduleServer queryScheduleServer = new QueryScheduleServer(queryScheduleServerPort); container.addServer("query_schedule", queryScheduleServer); @@ -174,7 +176,8 @@ else if(role.equals(NodeProto.NodeRole.RETINA)) { // start retina server on worker node RetinaServer retinaServer = new RetinaServer(retinaServerPort); - container.addServer("retina", retinaServer); + container.addServer("retina", retinaServer, + new MetadataReadyCheck()); if (indexServerEnabled) { @@ -220,7 +223,7 @@ else if(role.equals(NodeProto.NodeRole.RETINA)) boolean done = true; for (String serverName : container.getServerNames()) { - if (container.checkServer(serverName, 0)) + if (container.checkServer(serverName)) { done = false; break; @@ -253,10 +256,7 @@ else if(role.equals(NodeProto.NodeRole.RETINA)) { for (String serverName : container.getServerNames()) { - if (!container.checkServer(serverName)) - { - container.startServer(serverName); - } + container.startServer(serverName); } TimeUnit.SECONDS.sleep(1); } diff --git a/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/ServerContainer.java b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/ServerContainer.java index e0d11be1a9..d34f32b2e8 100644 --- a/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/ServerContainer.java +++ b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/ServerContainer.java @@ -25,10 +25,10 @@ import org.apache.logging.log4j.Logger; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.concurrent.TimeUnit; /** * @author hank @@ -37,95 +37,155 @@ public class ServerContainer { private static Logger log = LogManager.getLogger(ServerContainer.class); - private Map serverMap = null; + private static final class ServerHandle + { + private final Server server; + private final List startupChecks; + private Thread thread; + + private ServerHandle(Server server, List startupChecks) + { + this.server = server; + this.startupChecks = startupChecks; + } + } + private final Map serverHandles; public ServerContainer () { - this.serverMap = new HashMap<>(); + this.serverHandles = new HashMap<>(); } - public void addServer (String name, Server server) + public synchronized void addServer ( + String name, Server server, StartupCheck... startupChecks) { - Thread thread = new Thread(server); - thread.start(); - this.serverMap.put(name, server); + if (this.serverHandles.containsKey(name)) + { + throw new IllegalArgumentException("server is already registered: " + name); + } + this.serverHandles.put(name, new ServerHandle(server, Arrays.asList(startupChecks))); + startServerThread(name); } - public List getServerNames() + public synchronized List getServerNames() { - return new ArrayList<>(this.serverMap.keySet()); + return new ArrayList<>(this.serverHandles.keySet()); } /** - * retry 3 times by default. sleep one second after each retry. - * @param name - * @return - * @throws NoSuchServerException + * Ensure that a server has one thread responsible for its lifecycle. + * A server may still be waiting for startup checks while its + * {@link Server#isRunning()} method returns false. */ - public boolean checkServer(String name) throws NoSuchServerException + public synchronized void startServer(String name) throws NoSuchServerException { - return this.checkServer(name, 3); + ServerHandle handle = this.serverHandles.get(name); + if (handle == null) + { + throw new NoSuchServerException(); + } + Thread serverThread = handle.thread; + if ((serverThread != null && serverThread.isAlive()) + || handle.server.isRunning()) + { + log.debug("Server {} is already starting or running, skip duplicate start", name); + return; + } + startServerThread(name); } /** - * - * @param name - * @param retry times to retry, sleep one second after each retry. - * @return true if server is running. - * @throws NoSuchServerException + * Check whether the thread responsible for a server's lifecycle is alive. */ - public boolean checkServer(String name, int retry) throws NoSuchServerException + public synchronized boolean checkServer(String name) + throws NoSuchServerException { - Server server = this.serverMap.get(name); - if (server == null) + ServerHandle handle = this.serverHandles.get(name); + if (handle == null) { throw new NoSuchServerException(); } - boolean serverIsRunning = false; - try - { - if (!server.isRunning()) - { - for (int i = 0; i < retry; ++i) - { - // try 3 times - TimeUnit.SECONDS.sleep(1); - if (server.isRunning()) - { - serverIsRunning = true; - break; - } - } - } - else - { - serverIsRunning = true; - } - } catch (InterruptedException e) - { - log.error( - "interrupted while checking server.", e); - } - return serverIsRunning; + Thread serverThread = handle.thread; + return serverThread != null && serverThread.isAlive(); } - public void startServer(String name) throws NoSuchServerException + public synchronized void shutdownServer(String name) throws NoSuchServerException { - this.shutdownServer(name); - Thread serverThread = new Thread(this.serverMap.get(name)); - serverThread.start(); + ServerHandle handle = this.serverHandles.get(name); + if (handle == null) + { + throw new NoSuchServerException(); + } + if (handle.server.isRunning()) + { + handle.server.shutdown(); + } + Thread serverThread = handle.thread; + if (serverThread != null && serverThread.isAlive() + && serverThread != Thread.currentThread()) + { + serverThread.interrupt(); + } } - public void shutdownServer(String name) throws NoSuchServerException + private void startServerThread(String name) { - Server server = this.serverMap.get(name); - if (server == null) + ServerHandle handle = this.serverHandles.get(name); + if (handle == null) { - throw new NoSuchServerException(); + throw new IllegalStateException("server is not registered: " + name); } - if (checkServer(name, 0) == true) + Thread existingThread = handle.thread; + if (existingThread != null && existingThread.isAlive()) { - server.shutdown(); + return; } + + handle.thread = new Thread(() -> + { + try + { + long startupDeadline = System.nanoTime() + 60_000_000_000L; + for (StartupCheck startupCheck : handle.startupChecks) + { + long remainingNanos = startupDeadline - System.nanoTime(); + if (remainingNanos <= 0) + { + throw new IllegalStateException( + "Timed out waiting for startup checks of " + name + + " after 60 seconds"); + } + log.debug("Server {} is waiting for {}", name, startupCheck.getDescription()); + startupCheck.awaitReady(startupDeadline); + if (System.nanoTime() >= startupDeadline) + { + throw new IllegalStateException( + "Timed out waiting for startup checks of " + name + + " after 60 seconds"); + } + } + handle.server.run(); + } + catch (InterruptedException e) + { + Thread.currentThread().interrupt(); + log.info("Server {} startup was interrupted", name); + } + catch (Throwable e) + { + log.error("Server {} failed during startup or execution", name, e); + } + finally + { + synchronized (ServerContainer.this) + { + if (handle.thread == Thread.currentThread()) + { + handle.thread = null; + } + } + } + }, name); + handle.thread.start(); } } diff --git a/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/StartupCheck.java b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/StartupCheck.java new file mode 100644 index 0000000000..44dd465c30 --- /dev/null +++ b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/StartupCheck.java @@ -0,0 +1,41 @@ +/* + * Copyright 2026 PixelsDB. + * + * This file is part of Pixels. + * + * Pixels is free software: you can redistribute it and/or modify + * it under the terms of the Affero GNU General Public License as + * published by the Free Software Foundation, either version 3 of + * the License, or (at your option) any later version. + * + * Pixels is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Affero GNU General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public + * License along with Pixels. If not, see + * . + */ +package io.pixelsdb.pixels.daemon; + +/** + * A check that must complete before a server starts serving requests. + * + *

The wait is interruptible so that a daemon shutdown can cancel a server + * that is still waiting for one of its startup checks.

+ * + * @author PixelsDB + */ +public interface StartupCheck +{ + String getDescription(); + + /** + * Wait until this startup check is satisfied or the supplied deadline expires. + * + * @param deadlineNanos an absolute deadline from {@link System#nanoTime()} + * @throws InterruptedException if the wait is interrupted + */ + void awaitReady(long deadlineNanos) throws InterruptedException; +} diff --git a/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/metadata/MetadataReadyCheck.java b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/metadata/MetadataReadyCheck.java new file mode 100644 index 0000000000..d3cd04fb2c --- /dev/null +++ b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/metadata/MetadataReadyCheck.java @@ -0,0 +1,114 @@ +/* + * Copyright 2026 PixelsDB. + * + * This file is part of Pixels. + * + * Pixels is free software: you can redistribute it and/or modify + * it under the terms of the Affero GNU General Public License as + * published by the Free Software Foundation, either version 3 of + * the License, or (at your option) any later version. + * + * Pixels is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Affero GNU General Public License for more details. + * + * You should have received a copy of the Affero GNU General Public + * License along with Pixels. If not, see + * . + */ +package io.pixelsdb.pixels.daemon.metadata; + +import io.grpc.ManagedChannel; +import io.grpc.ManagedChannelBuilder; +import io.grpc.StatusRuntimeException; +import io.grpc.health.v1.HealthCheckRequest; +import io.grpc.health.v1.HealthCheckResponse; +import io.grpc.health.v1.HealthGrpc; +import io.pixelsdb.pixels.common.utils.ConfigFactory; +import io.pixelsdb.pixels.daemon.StartupCheck; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import java.util.concurrent.TimeUnit; + +/** + * Waits until the Metadata gRPC health service reports SERVING. + * + * @author PixelsDB + */ +public class MetadataReadyCheck implements StartupCheck +{ + private static final Logger log = LogManager.getLogger(MetadataReadyCheck.class); + private static final long HEALTH_CHECK_TIMEOUT_MS = 1_000L; + private static final long RETRY_INTERVAL_MS = 1_000L; + + @Override + public String getDescription() + { + return "Metadata gRPC health service to report SERVING"; + } + + @Override + public void awaitReady(long deadlineNanos) throws InterruptedException + { + ConfigFactory config = ConfigFactory.Instance(); + String host = config.getProperty("metadata.server.host"); + int port = Integer.parseInt(config.getProperty("metadata.server.port")); + + ManagedChannel channel = ManagedChannelBuilder.forAddress(host, port) + .usePlaintext() + .build(); + try + { + HealthGrpc.HealthBlockingStub stub = HealthGrpc.newBlockingStub(channel); + log.info("Waiting for Metadata server {}:{} to report SERVING", host, port); + while (true) + { + long remainingNanos = deadlineNanos - System.nanoTime(); + if (remainingNanos <= 0) + { + throw new IllegalStateException( + "Timed out waiting for Metadata server " + host + ":" + port + + " to report SERVING"); + } + try + { + HealthCheckResponse response = stub + .withDeadlineAfter( + Math.min( + TimeUnit.MILLISECONDS.toNanos(HEALTH_CHECK_TIMEOUT_MS), + remainingNanos), + TimeUnit.NANOSECONDS) + .check(HealthCheckRequest.newBuilder().setService("metadata").build()); + if (response.getStatus() == HealthCheckResponse.ServingStatus.SERVING) + { + log.info("Metadata server {}:{} is ready", host, port); + return; + } + } + catch (StatusRuntimeException e) + { + if (Thread.currentThread().isInterrupted()) + { + throw new InterruptedException("Interrupted while checking Metadata readiness"); + } + log.debug("Metadata health check failed: {}", e.getStatus()); + } + remainingNanos = deadlineNanos - System.nanoTime(); + if (remainingNanos <= 0) + { + throw new IllegalStateException( + "Timed out waiting for Metadata server " + host + ":" + port + + " to report SERVING"); + } + TimeUnit.NANOSECONDS.sleep(Math.min( + TimeUnit.MILLISECONDS.toNanos(RETRY_INTERVAL_MS), remainingNanos)); + } + } + finally + { + channel.shutdownNow(); + } + } +} diff --git a/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/metadata/MetadataServer.java b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/metadata/MetadataServer.java index d9a2a4d043..2ccf04a9ff 100644 --- a/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/metadata/MetadataServer.java +++ b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/metadata/MetadataServer.java @@ -40,7 +40,7 @@ public class MetadataServer implements Server { private static final Logger log = LogManager.getLogger(MetadataServer.class); - private boolean running = false; + private volatile boolean running = false; private final io.grpc.Server rpcServer; private final HealthStatusManager health = new HealthStatusManager(); diff --git a/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/retina/RetinaReadyCheck.java b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/retina/RetinaReadyCheck.java new file mode 100644 index 0000000000..6413672399 --- /dev/null +++ b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/retina/RetinaReadyCheck.java @@ -0,0 +1,197 @@ +/* + * Copyright 2026 PixelsDB. + * + * This file is part of Pixels. + * + * Pixels is free software: you can redistribute it and/or modify + * it under the terms of the Affero GNU General Public License as + * published by the Free Software Foundation, either version 3 of + * the License, or (at your option) any later version. + * + * Pixels is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Affero GNU General Public License for more details. + * + * You should have received a copy of the Affero GNU General Public + * License along with Pixels. If not, see + * . + */ +package io.pixelsdb.pixels.daemon.retina; + +import io.etcd.jetcd.ByteSequence; +import io.etcd.jetcd.KeyValue; +import io.etcd.jetcd.options.GetOption; +import io.pixelsdb.pixels.common.utils.ConfigFactory; +import io.pixelsdb.pixels.common.utils.Constants; +import io.pixelsdb.pixels.common.utils.EtcdUtil; +import io.pixelsdb.pixels.daemon.StartupCheck; +import io.pixelsdb.pixels.daemon.heartbeat.NodeStatus; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.TimeUnit; + +/** + * Waits until every configured Retina node has a leased READY heartbeat. + * + * @author PixelsDB + */ +public class RetinaReadyCheck implements StartupCheck +{ + private static final Logger log = LogManager.getLogger(RetinaReadyCheck.class); + private static final long ETCD_READ_TIMEOUT_MS = 1_000L; + private static final long RETRY_INTERVAL_MS = 1_000L; + + @Override + public String getDescription() + { + return "all configured Retina nodes to report READY"; + } + + @Override + public void awaitReady(long deadlineNanos) throws InterruptedException + { + ConfigFactory config = ConfigFactory.Instance(); + if (!Boolean.parseBoolean(config.getProperty("retina.enable"))) + { + return; + } + + Set expected = loadExpectedNodes(config); + if (expected.isEmpty()) + { + throw new IllegalStateException( + "retina.enable=true but $PIXELS_HOME/etc/retina has no nodes"); + } + + String lastReason = null; + log.info("Waiting for {} Retina node(s) to report READY", expected.size()); + + while (true) + { + long remainingNanos = deadlineNanos - System.nanoTime(); + if (remainingNanos <= 0) + { + throw new IllegalStateException("Timed out waiting for Retina readiness"); + } + String reason = getReadinessReason(expected, remainingNanos); + if (reason == null) + { + log.info("All Retina nodes are READY"); + return; + } + if (!reason.equals(lastReason)) + { + log.info("Retina readiness is pending: {}", reason); + lastReason = reason; + } + remainingNanos = deadlineNanos - System.nanoTime(); + if (remainingNanos <= 0) + { + throw new IllegalStateException( + "Timed out waiting for Retina readiness; last reason: " + reason); + } + TimeUnit.NANOSECONDS.sleep(Math.min( + TimeUnit.MILLISECONDS.toNanos(RETRY_INTERVAL_MS), remainingNanos)); + } + } + + private Set loadExpectedNodes(ConfigFactory config) + { + String pixelsHome = config.getProperty("pixels.home"); + if (pixelsHome == null || pixelsHome.isEmpty()) + { + throw new IllegalStateException("pixels.home is not configured"); + } + Path retinaFile = Paths.get(pixelsHome, "etc", "retina"); + if (!Files.isRegularFile(retinaFile)) + { + throw new IllegalStateException(retinaFile + " is missing"); + } + + Set expected = new LinkedHashSet<>(); + try + { + for (String raw : Files.readAllLines(retinaFile, StandardCharsets.UTF_8)) + { + String line = raw.trim(); + if (line.isEmpty() || line.startsWith("#")) + { + continue; + } + expected.add(line.split("\\s+", 2)[0]); + } + } + catch (Exception e) + { + throw new IllegalStateException( + "Failed to load expected Retina nodes from " + retinaFile, e); + } + return expected; + } + + private String getReadinessReason(Set expected, long timeoutNanos) + throws InterruptedException + { + String prefix = Constants.HEARTBEAT_RETINA_LITERAL; + Map observed; + try + { + ByteSequence prefixBytes = ByteSequence.from(prefix, StandardCharsets.UTF_8); + GetOption getOption = GetOption.builder().isPrefix(true).build(); + List all = EtcdUtil.Instance().getClient().getKVClient() + .get(prefixBytes, getOption) + .get(Math.min( + TimeUnit.MILLISECONDS.toNanos(ETCD_READ_TIMEOUT_MS), + timeoutNanos), + TimeUnit.NANOSECONDS) + .getKvs(); + observed = new HashMap<>(all.size() * 2); + for (KeyValue kv : all) + { + String key = kv.getKey().toString(StandardCharsets.UTF_8); + if (key.length() > prefix.length()) + { + observed.put(key.substring(prefix.length()), kv); + } + } + } + catch (InterruptedException e) + { + throw e; + } + catch (Exception e) + { + return "Etcd heartbeat read failed: " + e.getMessage(); + } + + for (String host : expected) + { + KeyValue kv = observed.get(host); + if (kv == null) + { + return "Retina node " + host + " has no heartbeat status"; + } + if (kv.getLease() <= 0) + { + return "Retina node " + host + " has heartbeat status without lease"; + } + String status = kv.getValue().toString(StandardCharsets.UTF_8).trim(); + if (!String.valueOf(NodeStatus.READY.StatusCode).equals(status)) + { + return "Retina node " + host + " heartbeat status is " + status; + } + } + return null; + } +} diff --git a/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/retina/RetinaServer.java b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/retina/RetinaServer.java index ab1697d4da..5ce26563bc 100644 --- a/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/retina/RetinaServer.java +++ b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/retina/RetinaServer.java @@ -19,15 +19,10 @@ */ package io.pixelsdb.pixels.daemon.retina; -import io.grpc.ManagedChannel; -import io.grpc.ManagedChannelBuilder; import io.grpc.ServerBuilder; -import io.grpc.StatusRuntimeException; -import io.grpc.health.v1.HealthCheckRequest; -import io.grpc.health.v1.HealthCheckResponse; -import io.grpc.health.v1.HealthGrpc; import io.pixelsdb.pixels.common.server.Server; -import io.pixelsdb.pixels.common.utils.ConfigFactory; +import io.pixelsdb.pixels.daemon.heartbeat.HeartbeatWorker; +import io.pixelsdb.pixels.daemon.heartbeat.NodeStatus; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -44,16 +39,14 @@ public class RetinaServer implements Server { private static final Logger log = LogManager.getLogger(RetinaServer.class); - private boolean running = false; - private final io.grpc.Server rpcServer; + private volatile boolean running = false; + private final int port; + private volatile io.grpc.Server rpcServer; public RetinaServer(int port) { checkArgument(port > 0 && port <= 65535, "illegal rpc port"); - // Issue #935: ensure metadata server has been already started - waitForMetadataServer(); - this.rpcServer = ServerBuilder.forPort(port) - .addService(new RetinaServerImpl()).build(); + this.port = port; } @Override @@ -66,12 +59,16 @@ public boolean isRunning() public void shutdown() { this.running = false; - try - { - this.rpcServer.shutdown().awaitTermination(5, TimeUnit.SECONDS); - } catch (InterruptedException e) + io.grpc.Server server = this.rpcServer; + if (server != null) { - log.error("Interrupted when shutdown rpc server", e); + try + { + server.shutdown().awaitTermination(5, TimeUnit.SECONDS); + } catch (InterruptedException e) + { + log.error("Interrupted when shutdown rpc server", e); + } } } @@ -80,9 +77,16 @@ public void run() { try { - this.rpcServer.start(); + HeartbeatWorker.setCurrentStatus(NodeStatus.INIT); + RetinaServerImpl service = new RetinaServerImpl(); + service.setReadyListener(() -> publishReady(service)); + io.grpc.Server server = ServerBuilder.forPort(port) + .addService(service).build(); + this.rpcServer = server; + server.start(); this.running = true; - this.rpcServer.awaitTermination(); + publishReady(service); + server.awaitTermination(); } catch (IOException e) { log.error("I/O error when running", e); @@ -95,49 +99,12 @@ public void run() } } - private void waitForMetadataServer() + private void publishReady(RetinaServerImpl service) { - ConfigFactory config = ConfigFactory.Instance(); - String metadataHost = config.getProperty("metadata.server.host"); - int metadataPort = Integer.parseInt(config.getProperty("metadata.server.port")); - ManagedChannel channel = ManagedChannelBuilder.forAddress(metadataHost, metadataPort) - .usePlaintext().build(); - - HealthGrpc.HealthBlockingStub stub = HealthGrpc.newBlockingStub(channel); - int retry = 0; - int maxRetry = 30; - - while (retry < maxRetry) - { - try - { - HealthCheckResponse response = stub.check(HealthCheckRequest.newBuilder().setService("metadata").build()); - if (response.getStatus() == HealthCheckResponse.ServingStatus.SERVING) - { - log.info("Metadata server is ready"); - channel.shutdown(); - return; - } - } catch (StatusRuntimeException e) - { - log.info("Metadata health check failed, sleep one second and retry"); - } - - retry++; - try - { - Thread.sleep(1000); - } catch (InterruptedException e) - { - log.error("Failed to sleep for retry to check metadata server status", e); - Thread.currentThread().interrupt(); - break; - } - } - channel.shutdown(); - if (retry >= maxRetry) + if (this.running && service.isReady()) { - log.error("Timeout waiting for metadata server to be ready"); + HeartbeatWorker.setCurrentStatus(NodeStatus.READY); + log.info("Retina service is ready"); } } } diff --git a/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/retina/RetinaServerImpl.java b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/retina/RetinaServerImpl.java index 1bc27d61e3..66fbbfa3cc 100644 --- a/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/retina/RetinaServerImpl.java +++ b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/retina/RetinaServerImpl.java @@ -36,8 +36,6 @@ import io.pixelsdb.pixels.common.metadata.domain.*; import io.pixelsdb.pixels.common.utils.ConfigFactory; import io.pixelsdb.pixels.common.utils.IndexUtils; -import io.pixelsdb.pixels.daemon.heartbeat.HeartbeatWorker; -import io.pixelsdb.pixels.daemon.heartbeat.NodeStatus; import io.pixelsdb.pixels.index.IndexProto; import io.pixelsdb.pixels.retina.RGVisibility; import io.pixelsdb.pixels.retina.RecoveryCheckpoint; @@ -75,6 +73,7 @@ public class RetinaServerImpl extends RetinaWorkerServiceGrpc.RetinaWorkerServic private final RetinaResourceManager retinaResourceManager; private final Striped updateLocks = Striped.lock(1024); private volatile RetinaStatus status; + private volatile Runnable readyListener; private IndexOption[] indexOptionPool; /** @@ -110,7 +109,7 @@ public RetinaServerImpl() publishStartupLifecycle(recoveryContext, recoveryResult); startRetinaMetricsLogThread(); boolean ready = this.status.getState() == RetinaState.READY; - logger.info(ready ? "Retina service is ready" : "Retina service is recovering"); + logger.info(ready ? "Retina initialization is ready" : "Retina service is recovering"); } catch (Exception e) { @@ -122,12 +121,31 @@ public RetinaServerImpl() .setState(RetinaState.FAILED) .build(); this.retinaResourceManager.setRecovering(false); - HeartbeatWorker.setCurrentStatus(NodeStatus.INIT); logger.error("Error while initializing RetinaServerImpl", e); throw new IllegalStateException("Failed to initialize RetinaServerImpl", e); } } + public void setReadyListener(Runnable readyListener) + { + this.readyListener = readyListener; + } + + boolean isReady() + { + RetinaStatus current = this.status; + return current != null && current.getState() == RetinaState.READY; + } + + private void notifyReady() + { + Runnable listener = this.readyListener; + if (listener != null && isReady()) + { + listener.run(); + } + } + private RecoveryContext prepareRecoveryContext() throws RetinaException { String recoveryEpoch = UUID.randomUUID().toString(); @@ -357,7 +375,6 @@ private void initializeRecoveredResources() throws Exception private void publishStartupLifecycle(RecoveryContext context, RecoveryResult result) throws RetinaException { this.retinaResourceManager.setRecovering(true); - HeartbeatWorker.setCurrentStatus(NodeStatus.INIT); this.status = RetinaStatus.newBuilder() .setState(RetinaState.RECOVERING) .setRecoveryEpoch(context.recoveryEpoch) @@ -371,7 +388,9 @@ private void publishStartupLifecycle(RecoveryContext context, RecoveryResult res .setState(RetinaState.READY) .build(); this.retinaResourceManager.setRecovering(false); - HeartbeatWorker.setCurrentStatus(NodeStatus.READY); + // Do not notify the ready listener here. RetinaServerImpl finishes + // initialization before RetinaServer starts the gRPC server, so + // RetinaServer publishes READY only after the RPC server is available. } } @@ -1065,7 +1084,7 @@ public void markReady(RetinaProto.MarkReadyRequest request, .setState(RetinaState.READY) .build(); this.retinaResourceManager.setRecovering(false); - HeartbeatWorker.setCurrentStatus(NodeStatus.READY); + notifyReady(); } catch (RetinaException e) { diff --git a/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/transaction/TransServer.java b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/transaction/TransServer.java index 05dd64192a..e51e33f9b8 100644 --- a/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/transaction/TransServer.java +++ b/pixels-daemon/src/main/java/io/pixelsdb/pixels/daemon/transaction/TransServer.java @@ -20,27 +20,13 @@ package io.pixelsdb.pixels.daemon.transaction; import java.io.IOException; -import java.nio.charset.StandardCharsets; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.Paths; -import java.util.HashMap; -import java.util.LinkedHashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; import java.util.concurrent.TimeUnit; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import io.etcd.jetcd.KeyValue; import io.grpc.ServerBuilder; import io.pixelsdb.pixels.common.server.Server; -import io.pixelsdb.pixels.common.utils.ConfigFactory; -import io.pixelsdb.pixels.common.utils.Constants; -import io.pixelsdb.pixels.common.utils.EtcdUtil; -import io.pixelsdb.pixels.daemon.heartbeat.NodeStatus; /** * @author hank @@ -50,14 +36,7 @@ public class TransServer implements Server { private static final Logger log = LogManager.getLogger(TransServer.class); - /** - * Default time to wait for all expected Retina nodes to reach READY before giving up - * and aborting the trans server boot. Overridable by {@code trans.server.retina.readiness.timeout.ms}. - */ - private static final long DEFAULT_RETINA_READINESS_TIMEOUT_MS = 10 * 60 * 1000L; - private static final long RETINA_READINESS_POLL_INTERVAL_MS = 1_000L; - - private boolean running = false; + private volatile boolean running = false; private final io.grpc.Server rpcServer; public TransServer(int port) @@ -91,7 +70,6 @@ public void run() { try { - awaitRetinaReady(); this.rpcServer.start(); this.running = true; this.rpcServer.awaitTermination(); @@ -106,126 +84,4 @@ public void run() this.shutdown(); } } - - /** - * Boot-time gate. When {@code retina.enable=true}, blocks until every node listed in - * {@code $PIXELS_HOME/etc/retina} reports {@code NodeStatus.READY} via heartbeat. When - * {@code retina.enable=false}, returns immediately. On timeout, throws so that - * {@link #run()} aborts and the supervisor can restart the process. - * - *

This is intentionally a one-shot check executed before the gRPC server starts. - * Once the trans server is serving, it does not re-check Retina lifecycle state. - */ - private void awaitRetinaReady() - { - ConfigFactory config = ConfigFactory.Instance(); - if (!Boolean.parseBoolean(config.getProperty("retina.enable"))) - { - return; - } - - // Load expected Retina nodes from $PIXELS_HOME/etc/retina. - Path retinaFile = Paths.get(config.getProperty("pixels.home"), "etc", "retina"); - if (!Files.isRegularFile(retinaFile)) - { - throw new IllegalStateException(retinaFile + " is missing"); - } - Set expected = new LinkedHashSet<>(); - try - { - for (String raw : Files.readAllLines(retinaFile, StandardCharsets.UTF_8)) - { - String line = raw.trim(); - if (line.isEmpty() || line.startsWith("#")) - { - continue; - } - String host = line.split("\\s+", 2)[0]; - expected.add(host); - } - } catch (IOException e) - { - throw new IllegalStateException("Failed to load expected Retina nodes from " - + "$PIXELS_HOME/etc/retina", e); - } - if (expected.isEmpty()) - { - throw new IllegalStateException( - "retina.enable=true but $PIXELS_HOME/etc/retina has no nodes"); - } - - long deadline = System.currentTimeMillis() + DEFAULT_RETINA_READINESS_TIMEOUT_MS; - EtcdUtil etcd = EtcdUtil.Instance(); - String prefix = Constants.HEARTBEAT_RETINA_LITERAL; - int prefixLen = prefix.length(); - log.info("Waiting for {} Retina node(s) to report READY (timeout {} ms)", - expected.size(), DEFAULT_RETINA_READINESS_TIMEOUT_MS); - while (true) - { - String reason = null; - // Poll all Retina heartbeat keys once and check whether every expected node is READY. - Map observed; - try - { - List all = etcd.getKeyValuesByPrefix(prefix); - observed = new HashMap<>(all.size() * 2); - for (KeyValue kv : all) - { - String key = kv.getKey().toString(StandardCharsets.UTF_8); - if (key.length() > prefixLen) - { - observed.put(key.substring(prefixLen), kv); - } - } - } catch (RuntimeException e) - { - observed = null; - reason = "etcd heartbeat read failed: " + e.getMessage(); - } - if (reason == null) - { - for (String host : expected) - { - KeyValue kv = observed.get(host); - if (kv == null) - { - reason = "Retina node " + host + " has no heartbeat status"; - break; - } - if (kv.getLease() <= 0) - { - reason = "Retina node " + host + " has heartbeat status without lease"; - break; - } - String status = kv.getValue().toString(StandardCharsets.UTF_8).trim(); - if (!String.valueOf(NodeStatus.READY.StatusCode).equals(status)) - { - reason = "Retina node " + host + " heartbeat status is " + status; - break; - } - } - } - if (reason == null) - { - log.info("All Retina nodes are READY, starting trans server"); - return; - } - if (System.currentTimeMillis() >= deadline) - { - throw new IllegalStateException( - "Timed out waiting for Retina readiness after " - + DEFAULT_RETINA_READINESS_TIMEOUT_MS - + " ms; last reason: " + reason); - } - try - { - Thread.sleep(RETINA_READINESS_POLL_INTERVAL_MS); - } catch (InterruptedException e) - { - Thread.currentThread().interrupt(); - throw new IllegalStateException( - "Interrupted while waiting for Retina readiness", e); - } - } - } } diff --git a/pixels-daemon/src/test/java/io/pixelsdb/pixels/daemon/TestServerContainer.java b/pixels-daemon/src/test/java/io/pixelsdb/pixels/daemon/TestServerContainer.java new file mode 100644 index 0000000000..4779ca15a0 --- /dev/null +++ b/pixels-daemon/src/test/java/io/pixelsdb/pixels/daemon/TestServerContainer.java @@ -0,0 +1,478 @@ +/* + * Copyright 2026 PixelsDB. + * + * This file is part of Pixels. + * + * Pixels is free software: you can redistribute it and/or modify + * it under the terms of the Affero GNU General Public License as + * published by the Free Software Foundation, either version 3 of + * the License, or (at your option) any later version. + * + * Pixels is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Affero GNU General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public + * License along with Pixels. If not, see + * . + */ +package io.pixelsdb.pixels.daemon; + +import io.pixelsdb.pixels.common.server.Server; +import io.pixelsdb.pixels.daemon.exception.NoSuchServerException; +import org.junit.Test; + +import java.util.Collections; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +public class TestServerContainer +{ + private static final long WAIT_TIMEOUT_SECONDS = 2; + private static final long NO_EVENT_TIMEOUT_MILLIS = 100; + + @Test + public void testAddServerRegistersStartsAndShutsDownServer() throws Exception + { + BlockingServer server = new BlockingServer(true); + ServerContainer container = new ServerContainer(); + try + { + container.addServer("managed", server); + + assertTrue(server.started.await(WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)); + assertEquals(Collections.singletonList("managed"), container.getServerNames()); + assertTrue(container.checkServer("managed")); + } + finally + { + stopBlockingServer(container, "managed", server); + } + assertEquals(1, server.shutdownCount.get()); + assertFalse(container.checkServer("managed")); + } + + @Test + public void testStartServerDoesNotDuplicateActiveLifecycleThread() throws Exception + { + BlockingServer server = new BlockingServer(false); + ServerContainer container = new ServerContainer(); + try + { + container.addServer("slow", server); + assertTrue(server.started.await(WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)); + + container.startServer("slow"); + + assertFalse(server.secondRun.await( + NO_EVENT_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS)); + assertEquals(1, server.runCount.get()); + } + finally + { + stopBlockingServer(container, "slow", server); + } + } + + @Test + public void testStartupChecksRunInOrderBeforeServer() throws Exception + { + BlockingServer server = new BlockingServer(false); + BlockingStartupCheck firstCheck = new BlockingStartupCheck("first check"); + BlockingStartupCheck secondCheck = new BlockingStartupCheck("second check"); + ServerContainer container = new ServerContainer(); + try + { + container.addServer("checked", server, firstCheck, secondCheck); + + assertTrue(firstCheck.entered.await(WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)); + assertFalse(secondCheck.entered.await( + NO_EVENT_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS)); + + container.startServer("checked"); + firstCheck.allowReady(); + + assertTrue(secondCheck.entered.await(WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)); + assertFalse(server.started.await(NO_EVENT_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS)); + + container.startServer("checked"); + secondCheck.allowReady(); + + assertTrue(server.started.await(WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)); + assertEquals(1, server.runCount.get()); + assertFalse(server.secondRun.await( + NO_EVENT_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS)); + } + finally + { + firstCheck.allowReady(); + secondCheck.allowReady(); + stopBlockingServer(container, "checked", server); + } + } + + @Test + public void testStartupCheckFailurePreventsServerStartup() throws Exception + { + BlockingServer server = new BlockingServer(false); + FailingStartupCheck startupCheck = new FailingStartupCheck(); + ServerContainer container = new ServerContainer(); + container.addServer("failing", server, startupCheck); + + assertTrue(startupCheck.executed.await(WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)); + awaitLifecycleStopped(container, "failing"); + + assertEquals(0, server.runCount.get()); + assertEquals(1L, server.started.getCount()); + } + + @Test + public void testShutdownInterruptsPendingStartupCheck() throws Exception + { + BlockingServer server = new BlockingServer(false); + BlockingStartupCheck startupCheck = new BlockingStartupCheck("blocking check"); + ServerContainer container = new ServerContainer(); + try + { + container.addServer("waiting", server, startupCheck); + assertTrue(startupCheck.entered.await(WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)); + + container.shutdownServer("waiting"); + + assertTrue(startupCheck.interrupted.await(WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)); + awaitLifecycleStopped(container, "waiting"); + assertEquals(0, server.runCount.get()); + } + finally + { + container.shutdownServer("waiting"); + startupCheck.allowReady(); + server.release(); + awaitLifecycleStopped(container, "waiting"); + } + } + + @Test + public void testRunningServerIsNotRestartedAfterRunReturns() throws Exception + { + AsyncServer server = new AsyncServer(); + ServerContainer container = new ServerContainer(); + try + { + container.addServer("async", server); + assertTrue(server.started.await(WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)); + awaitLifecycleStopped(container, "async"); + + container.startServer("async"); + + assertFalse(server.restarted.await( + NO_EVENT_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS)); + assertEquals(1, server.runCount.get()); + assertTrue(server.isRunning()); + } + finally + { + container.shutdownServer("async"); + } + assertEquals(1, server.shutdownCount.get()); + assertFalse(server.isRunning()); + } + + @Test + public void testStoppedServerCanBeRestarted() throws Exception + { + RestartableServer server = new RestartableServer(); + ServerContainer container = new ServerContainer(); + + container.addServer("restartable", server); + assertTrue(server.firstRun.await(WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)); + awaitLifecycleStopped(container, "restartable"); + + container.startServer("restartable"); + + assertTrue(server.secondRun.await(WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)); + awaitLifecycleStopped(container, "restartable"); + assertEquals(2, server.runCount.get()); + } + + @Test + public void testUnknownServerOperationsFail() throws Exception + { + ServerContainer container = new ServerContainer(); + + assertNoSuchServer(() -> container.startServer("missing")); + assertNoSuchServer(() -> container.checkServer("missing")); + assertNoSuchServer(() -> container.shutdownServer("missing")); + } + + @Test + public void testDuplicateServerRegistrationIsRejected() throws Exception + { + BlockingServer first = new BlockingServer(false); + BlockingServer duplicate = new BlockingServer(false); + ServerContainer container = new ServerContainer(); + try + { + container.addServer("duplicate", first); + assertTrue(first.started.await(WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)); + + try + { + container.addServer("duplicate", duplicate); + fail("duplicate server registration should fail"); + } + catch (IllegalArgumentException expected) + { + assertEquals("server is already registered: duplicate", expected.getMessage()); + } + + assertEquals(1, first.runCount.get()); + assertEquals(0, duplicate.runCount.get()); + } + finally + { + stopBlockingServer(container, "duplicate", first); + } + } + + private static void stopBlockingServer( + ServerContainer container, String name, BlockingServer server) throws Exception + { + try + { + container.shutdownServer(name); + } + finally + { + server.release(); + } + if (server.started.getCount() == 0) + { + assertTrue(server.stopped.await(WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)); + } + awaitLifecycleStopped(container, name); + } + + private static void awaitLifecycleStopped(ServerContainer container, String name) + throws Exception + { + long deadline = System.nanoTime() + + TimeUnit.SECONDS.toNanos(WAIT_TIMEOUT_SECONDS); + while (container.checkServer(name) && System.nanoTime() < deadline) + { + TimeUnit.MILLISECONDS.sleep(10); + } + assertFalse("server lifecycle thread did not stop", container.checkServer(name)); + } + + private static void assertNoSuchServer(ServerOperation operation) throws Exception + { + try + { + operation.run(); + fail("operation on an unknown server should fail"); + } + catch (NoSuchServerException expected) + { + // Expected. + } + } + + private interface ServerOperation + { + void run() throws NoSuchServerException; + } + + private static final class BlockingStartupCheck implements StartupCheck + { + private final String description; + private final CountDownLatch entered = new CountDownLatch(1); + private final CountDownLatch ready = new CountDownLatch(1); + private final CountDownLatch interrupted = new CountDownLatch(1); + + private BlockingStartupCheck(String description) + { + this.description = description; + } + + @Override + public String getDescription() + { + return description; + } + + @Override + public void awaitReady(long deadlineNanos) throws InterruptedException + { + entered.countDown(); + try + { + long remainingNanos = deadlineNanos - System.nanoTime(); + if (remainingNanos <= 0 + || !ready.await(remainingNanos, TimeUnit.NANOSECONDS)) + { + throw new IllegalStateException(description + " timed out"); + } + } + catch (InterruptedException e) + { + interrupted.countDown(); + throw e; + } + } + + private void allowReady() + { + ready.countDown(); + } + } + + private static final class FailingStartupCheck implements StartupCheck + { + private final CountDownLatch executed = new CountDownLatch(1); + + @Override + public String getDescription() + { + return "failing startup check"; + } + + @Override + public void awaitReady(long deadlineNanos) + { + executed.countDown(); + throw new IllegalStateException("test startup check failed"); + } + } + + private static final class BlockingServer implements Server + { + private final boolean reportRunning; + private final CountDownLatch started = new CountDownLatch(1); + private final CountDownLatch stopped = new CountDownLatch(1); + private final CountDownLatch stop = new CountDownLatch(1); + private final CountDownLatch secondRun = new CountDownLatch(1); + private final AtomicInteger runCount = new AtomicInteger(); + private final AtomicInteger shutdownCount = new AtomicInteger(); + private volatile boolean running; + + private BlockingServer(boolean reportRunning) + { + this.reportRunning = reportRunning; + } + + @Override + public boolean isRunning() + { + return reportRunning && running; + } + + @Override + public void run() + { + if (runCount.incrementAndGet() > 1) + { + secondRun.countDown(); + } + running = true; + started.countDown(); + try + { + stop.await(); + } + catch (InterruptedException e) + { + Thread.currentThread().interrupt(); + } + finally + { + running = false; + stopped.countDown(); + } + } + + @Override + public void shutdown() + { + shutdownCount.incrementAndGet(); + stop.countDown(); + } + + private void release() + { + stop.countDown(); + } + } + + private static final class AsyncServer implements Server + { + private final CountDownLatch started = new CountDownLatch(1); + private final CountDownLatch restarted = new CountDownLatch(1); + private final AtomicInteger runCount = new AtomicInteger(); + private final AtomicInteger shutdownCount = new AtomicInteger(); + private volatile boolean running; + + @Override + public boolean isRunning() + { + return running; + } + + @Override + public void run() + { + if (runCount.incrementAndGet() > 1) + { + restarted.countDown(); + } + running = true; + started.countDown(); + } + + @Override + public void shutdown() + { + shutdownCount.incrementAndGet(); + running = false; + } + } + + private static final class RestartableServer implements Server + { + private final CountDownLatch firstRun = new CountDownLatch(1); + private final CountDownLatch secondRun = new CountDownLatch(1); + private final AtomicInteger runCount = new AtomicInteger(); + + @Override + public boolean isRunning() + { + return false; + } + + @Override + public void run() + { + int currentRun = runCount.incrementAndGet(); + if (currentRun == 1) + { + firstRun.countDown(); + } + else if (currentRun == 2) + { + secondRun.countDown(); + } + } + + @Override + public void shutdown() + { + } + } +} From f9e149c04d454104bdc72b9cf9fc3d6c78b3518c Mon Sep 17 00:00:00 2001 From: dannygeng Date: Tue, 28 Jul 2026 11:50:14 +0800 Subject: [PATCH 2/6] fix(pixels-install): harden Bash execution and shell compatibility --- skills/pixels-install/claude/agent.md | 11 +- skills/pixels-install/codex/SKILL.md | 11 +- .../scripts/build_install_pixels.sh | 8 + .../scripts/build_pixels_trino_artifacts.sh | 8 + .../scripts/check_prerequisites.sh | 61 ++++++- .../scripts/configure_pixels.sh | 8 + skills/pixels-install/scripts/install_etcd.sh | 10 ++ skills/pixels-install/scripts/install_jdk.sh | 8 + .../pixels-install/scripts/install_maven.sh | 8 + .../pixels-install/scripts/install_mysql.sh | 10 ++ .../scripts/install_shell_helpers.sh | 22 ++- .../pixels-install/scripts/install_trino.sh | 8 + .../scripts/install_trino_cluster.sh | 30 ++-- .../scripts/install_trino_shell_helpers.sh | 68 +++++--- .../pixels-install/scripts/lib/shell_env.sh | 136 +++++++++++++++- .../scripts/prepare_deployment.sh | 10 ++ .../scripts/prepare_trino_cluster.sh | 10 ++ skills/pixels-install/scripts/progress.sh | 10 ++ skills/pixels-install/scripts/smoke_test.sh | 13 +- .../shared-scripts/setup_cluster.sh | 77 ++++++++- .../tests/shell_compatibility.sh | 150 ++++++++++++++++++ 21 files changed, 628 insertions(+), 49 deletions(-) create mode 100644 skills/pixels-install/tests/shell_compatibility.sh diff --git a/skills/pixels-install/claude/agent.md b/skills/pixels-install/claude/agent.md index 57bde4a468..6633f9423b 100644 --- a/skills/pixels-install/claude/agent.md +++ b/skills/pixels-install/claude/agent.md @@ -30,8 +30,17 @@ Use these helper scripts when they directly fit the current environment and the - `install_trino.sh`: installs Trino (per the [466 deployment docs](https://trino.io/docs/466/installation/deployment.html)) on the **current node only** — run it once per node in the cluster, it does not SSH anywhere itself (use `install_trino_cluster.sh` to fan it out across the cluster instead of running it by hand on each node). It reads `trino-deployment.env`, validates this node's role (`coordinator` or `worker`) and the coordinator host, and confirms the role plus install/data paths before writing files; in non-interactive mode, set `CONFIRM_TRINO_INSTALL=true` only after the user confirms the target. Downloads `trino-server-.tar.gz` (default 466) from Maven Central into the confirmed install parent with the confirmed home symlink for version-switch-friendly installs. Writes `etc/node.properties` (`node.data-dir` under the confirmed data dir, outside the versioned install dir so it survives version switches; `node.id` generated once and preserved across re-runs), `etc/jvm.config` (official defaults plus the two `--add-opens` flags `pixels-trino` requires), `etc/log.properties`, and `etc/config.properties` — `discovery.uri` always points at the coordinator's real IP from `trino-deployment.env`, never `localhost`; this node's coordinator/worker role is read from `trino-deployment.env` or inferred from this host's own IPs, overridable with `TRINO_ROLE`. For `pixelsdb/pixels-trino`, either clones/builds locally (requires Pixels already `mvn install`ed locally and JDK 23) or consumes prebuilt `PIXELS_TRINO_CONNECTOR_ZIP` and `PIXELS_TRINO_LISTENER_ZIP` so remote Trino nodes do not need a Pixels checkout or Maven build. Installs both documented plugins by directly unzipping their zips under `plugin/` and preserving the zip's versioned top-level directory names, e.g. `pixels-trino-connector-0.2.0-SNAPSHOT` and `pixels-trino-listener-0.2.0-SNAPSHOT`; do not rename them to unversioned stable directories. Writes Trino's catalog file `etc/catalog/pixels.properties` with only catalog properties (`connector.name=pixels`, `cloud.function.switch=off`, `clean.intermediate.result=true` by default). This catalog file is **not** the same thing as Pixels' client/runtime config file `$PIXELS_HOME/etc/pixels.properties`; never copy Pixels runtime properties such as `metadata.server.host` or `etcd.hosts` into the Trino catalog. Also prepares the Trino-side lightweight Pixels client home (`TRINO_PIXELS_HOME`, default `$HOME/opt/pixels`, not `$HOME/opt/pixels/etc`) with `etc/pixels.properties` and `logs/`, then writes `export PIXELS_HOME=...` and `export PIXELS_CONFIG=$PIXELS_HOME/etc/pixels.properties` directly into the current user's bash/zsh profile (and mirrors them into `STATE_DIR/toolchain.env` for later helper-script processes). It does **not** create `~/.pixels-trino-env.sh`. Use `TRINO_PIXELS_CONFIG_SOURCE` to copy a known-good Pixels config into that client home, and use `TRINO_PIXELS_METADATA_SERVER_HOST`, `TRINO_PIXELS_TRANS_SERVER_HOST`, `TRINO_PIXELS_QUERY_SCHEDULE_SERVER_HOST`, and `TRINO_PIXELS_ETCD_HOSTS` to rewrite endpoints so Trino nodes do not fall back to `localhost`. Writes `etc/event-listener.properties` for the listener and defaults its log directory to a Trino-side path under the install parent unless `PIXELS_TRINO_LISTENER_LOG_DIR` is set. Best-effort installs `trino-cli` into `bin/trino`. - `install_trino_shell_helpers.sh`: optional, and **always asks first**, because it bakes a specific list of remote hostnames (from `trino-deployment.env`) into a shell profile and assumes passwordless SSH between them is already set up. By default it installs only `start_trino_cluster`/`stop_trino_cluster`/`restart_trino_cluster`/`trino_cli` on the Trino coordinator recorded in `trino-deployment.env`; when invoked from another host, it copies a minimal installer over SSH and writes the coordinator user's `~/.trino-shell-helpers.sh` plus shell-profile source line there. In the generated functions, the coordinator is always operated locally and every worker over SSH. `stop_trino_cluster` stops workers before the coordinator; `restart_trino_cluster` runs stop then start. The generated start/stop/restart/client helpers explicitly export `PIXELS_HOME` and `PIXELS_CONFIG` before invoking Trino locally or over SSH, without wrapping or replacing Trino's `bin/launcher`. Set `TRINO_SHELL_HELPERS_TARGET=local` only when the user explicitly wants the functions on the current host. Leave `INSTALL_TRINO_SHELL_HELPERS` unset to be prompted `[y/N]`; set `true`/`false` to answer non-interactively once the user has actually agreed, or `ASSUME_YES=true`. +All executable entrypoints in this skill are Bash programs. Invoke them directly +so their `#!/usr/bin/env bash` shebang is honored; do not run them with `zsh` +or `source` them into the current shell. Bash must be installed on every +deployment node even when that account's login shell is Zsh. Each entrypoint +validates the deployment account's configured login shell from passwd/NSS and +accepts only Bash or Zsh. Profile exports go to `.bashrc` or `.zshrc` +accordingly, while the generated Pixels and Trino helper files are checked in +both Bash and Zsh syntax before they are sourced. + All of these scripts anchor paths on `$HOME` (e.g. `~/opt/...`) and detect the -current login shell (`$SHELL`) to pick `~/.bashrc` or `~/.zshrc` before +current account's login shell from passwd/NSS to pick `~/.bashrc` or `~/.zshrc` before persisting any `export`. Never assume the deployment user is named `pixels` or `ubuntu`, and never write environment variables into a global file such as `/etc/environment` — `skills/pixels-install/scripts/lib/shell_env.sh` diff --git a/skills/pixels-install/codex/SKILL.md b/skills/pixels-install/codex/SKILL.md index 4fc84de608..602913e028 100644 --- a/skills/pixels-install/codex/SKILL.md +++ b/skills/pixels-install/codex/SKILL.md @@ -38,8 +38,17 @@ Use these helpers when they directly fit the current environment and the user-ap - `install_trino.sh`: installs Trino (per the [466 deployment docs](https://trino.io/docs/466/installation/deployment.html)) on the **current node only** — run it once per node in the cluster, it does not SSH anywhere itself (use `install_trino_cluster.sh` to fan it out across the cluster instead of running it by hand on each node). It reads `trino-deployment.env`, validates this node's role (`coordinator` or `worker`) and the coordinator host, and confirms the role plus install/data paths before writing files; in non-interactive mode, set `CONFIRM_TRINO_INSTALL=true` only after the user confirms the target. Downloads `trino-server-.tar.gz` (default 466) from Maven Central into the confirmed install parent with the confirmed home symlink for version-switch-friendly installs. Writes `etc/node.properties` (`node.data-dir` under the confirmed data dir, outside the versioned install dir so it survives version switches; `node.id` generated once and preserved across re-runs), `etc/jvm.config` (official defaults plus the two `--add-opens` flags `pixels-trino` requires), `etc/log.properties`, and `etc/config.properties` — `discovery.uri` always points at the coordinator's real IP from `trino-deployment.env`, never `localhost`; this node's coordinator/worker role is read from `trino-deployment.env` or inferred from this host's own IPs, overridable with `TRINO_ROLE`. For `pixelsdb/pixels-trino`, either clones/builds locally (requires Pixels already `mvn install`ed locally and JDK 23) or consumes prebuilt `PIXELS_TRINO_CONNECTOR_ZIP` and `PIXELS_TRINO_LISTENER_ZIP` so remote Trino nodes do not need a Pixels checkout or Maven build. Installs both documented plugins by directly unzipping their zips under `plugin/` and preserving the zip's versioned top-level directory names, e.g. `pixels-trino-connector-0.2.0-SNAPSHOT` and `pixels-trino-listener-0.2.0-SNAPSHOT`; do not rename them to unversioned stable directories. Writes Trino's catalog file `etc/catalog/pixels.properties` with only catalog properties (`connector.name=pixels`, `cloud.function.switch=off`, `clean.intermediate.result=true` by default). This catalog file is **not** the same thing as Pixels' client/runtime config file `$PIXELS_HOME/etc/pixels.properties`; never copy Pixels runtime properties such as `metadata.server.host` or `etcd.hosts` into the Trino catalog. Also prepares the Trino-side lightweight Pixels client home (`TRINO_PIXELS_HOME`, default `$HOME/opt/pixels`, not `$HOME/opt/pixels/etc`) with `etc/pixels.properties` and `logs/`, then writes `export PIXELS_HOME=...` and `export PIXELS_CONFIG=$PIXELS_HOME/etc/pixels.properties` directly into the current user's bash/zsh profile (and mirrors them into `STATE_DIR/toolchain.env` for later helper-script processes). It does **not** create `~/.pixels-trino-env.sh`. Use `TRINO_PIXELS_CONFIG_SOURCE` to copy a known-good Pixels config into that client home, and use `TRINO_PIXELS_METADATA_SERVER_HOST`, `TRINO_PIXELS_TRANS_SERVER_HOST`, `TRINO_PIXELS_QUERY_SCHEDULE_SERVER_HOST`, and `TRINO_PIXELS_ETCD_HOSTS` to rewrite endpoints so Trino nodes do not fall back to `localhost`. Writes `etc/event-listener.properties` for the listener and defaults its log directory to a Trino-side path under the install parent unless `PIXELS_TRINO_LISTENER_LOG_DIR` is set. Best-effort installs `trino-cli` into `bin/trino`. - `install_trino_shell_helpers.sh`: optional, and **always asks first**, because it bakes a specific list of remote hostnames (from `trino-deployment.env`) into a shell profile and assumes passwordless SSH between them is already set up. By default it installs only `start_trino_cluster`/`stop_trino_cluster`/`restart_trino_cluster`/`trino_cli` on the Trino coordinator recorded in `trino-deployment.env`; when invoked from another host, it copies a minimal installer over SSH and writes the coordinator user's `~/.trino-shell-helpers.sh` plus shell-profile source line there. In the generated functions, the coordinator is always operated locally and every worker over SSH. `stop_trino_cluster` stops workers before the coordinator; `restart_trino_cluster` runs stop then start. The generated start/stop/restart/client helpers explicitly export `PIXELS_HOME` and `PIXELS_CONFIG` before invoking Trino locally or over SSH, without wrapping or replacing Trino's `bin/launcher`. Set `TRINO_SHELL_HELPERS_TARGET=local` only when the user explicitly wants the functions on the current host. Leave `INSTALL_TRINO_SHELL_HELPERS` unset to be prompted `[y/N]`; set `true`/`false` to answer non-interactively once the user has actually agreed, or `ASSUME_YES=true`. +All executable entrypoints in this skill are Bash programs. Invoke them directly +so their `#!/usr/bin/env bash` shebang is honored; do not run them with `zsh` +or `source` them into the current shell. Bash must be installed on every +deployment node even when that account's login shell is Zsh. Each entrypoint +validates the deployment account's configured login shell from passwd/NSS and +accepts only Bash or Zsh. Profile exports go to `.bashrc` or `.zshrc` +accordingly, while the generated Pixels and Trino helper files are checked in +both Bash and Zsh syntax before they are sourced. + All of these scripts anchor paths on `$HOME` (e.g. `~/opt/...`) and detect the -current login shell (`$SHELL`) to pick `~/.bashrc` or `~/.zshrc` before +current account's login shell from passwd/NSS to pick `~/.bashrc` or `~/.zshrc` before persisting any `export`. Never assume the deployment user is named `pixels` or `ubuntu`, and never write environment variables into a global file such as `/etc/environment` — `scripts/lib/shell_env.sh` diff --git a/skills/pixels-install/scripts/build_install_pixels.sh b/skills/pixels-install/scripts/build_install_pixels.sh index 1df95ab46d..4c83a6c3e1 100755 --- a/skills/pixels-install/scripts/build_install_pixels.sh +++ b/skills/pixels-install/scripts/build_install_pixels.sh @@ -1,4 +1,12 @@ #!/usr/bin/env bash +if [ -z "${BASH_VERSION:-}" ]; then + printf 'ERROR: pixels-install scripts must be executed by Bash; do not run this script with zsh.\n' >&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" diff --git a/skills/pixels-install/scripts/build_pixels_trino_artifacts.sh b/skills/pixels-install/scripts/build_pixels_trino_artifacts.sh index 49249c2160..2f083004ce 100755 --- a/skills/pixels-install/scripts/build_pixels_trino_artifacts.sh +++ b/skills/pixels-install/scripts/build_pixels_trino_artifacts.sh @@ -1,4 +1,12 @@ #!/usr/bin/env bash +if [ -z "${BASH_VERSION:-}" ]; then + printf 'ERROR: pixels-install scripts must be executed by Bash; do not run this script with zsh.\n' >&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -euo pipefail # Builds pixelsdb/pixels-trino once on the local machine and records the diff --git a/skills/pixels-install/scripts/check_prerequisites.sh b/skills/pixels-install/scripts/check_prerequisites.sh index 2ea1d2d0ce..8fa1277b4c 100755 --- a/skills/pixels-install/scripts/check_prerequisites.sh +++ b/skills/pixels-install/scripts/check_prerequisites.sh @@ -1,4 +1,12 @@ #!/usr/bin/env bash +if [ -z "${BASH_VERSION:-}" ]; then + printf 'ERROR: pixels-install scripts must be executed by Bash; do not run this script with zsh.\n' >&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -uo pipefail # Validates OS, architecture, memory, disk, ports, host resolution, @@ -29,6 +37,29 @@ fail() { exit 1 } +check_shell() { + local shell_path shell_name + + shell_path="$(login_shell_path 2>/dev/null || true)" + if [[ -z "$shell_path" ]]; then + result_record shell fail "could not determine the current account's login shell" + return + fi + + shell_name="$(login_shell_name 2>/dev/null || true)" + if [[ -z "$shell_name" ]]; then + result_record shell fail "unsupported login shell: $shell_path (only bash and zsh are supported)" + return + fi + + if ! command -v bash >/dev/null 2>&1; then + result_record shell fail "Bash is required to run pixels-install scripts but was not found on PATH" + return + fi + + result_record shell ok "$shell_name login shell detected at $shell_path; Bash runtime is available" +} + require_command() { command -v "$1" >/dev/null 2>&1 || fail "required command not found: $1" } @@ -173,10 +204,33 @@ check_ssh_hosts() { fi for host in $CHECK_SSH_HOSTS; do - if ssh "${ssh_args[@]}" "$(ssh_target "$host")" true >/dev/null 2>&1; then - result_record "ssh:$host" ok "reachable" + if ssh "${ssh_args[@]}" "$(ssh_target "$host")" 'bash -s' >/dev/null 2>&1 <<'REMOTE' +set -euo pipefail + +user="$(id -un)" +shell_path="" +if command -v getent >/dev/null 2>&1; then + shell_path="$(getent passwd "$user" 2>/dev/null | awk -F: 'NR == 1 { print $7; exit }')" +fi +if [[ -z "$shell_path" && -r /etc/passwd ]]; then + shell_path="$(awk -F: -v account="$user" '$1 == account { print $7; exit }' /etc/passwd)" +fi + +[[ -n "$shell_path" ]] || exit 1 +case "$(basename "$shell_path")" in + bash|zsh) + ;; + *) + exit 1 + ;; +esac +command -v "$(basename "$shell_path")" >/dev/null 2>&1 +command -v bash >/dev/null 2>&1 +REMOTE + then + result_record "ssh:$host" ok "reachable; remote login shell is bash or zsh and Bash is available" else - result_record "ssh:$host" fail "SSH check failed" + result_record "ssh:$host" fail "SSH or remote shell validation failed" fi done } @@ -189,6 +243,7 @@ main() { result_reset log "checking prerequisites" + check_shell check_os check_memory check_disk diff --git a/skills/pixels-install/scripts/configure_pixels.sh b/skills/pixels-install/scripts/configure_pixels.sh index 4fec18b7e1..0556e4ca3d 100755 --- a/skills/pixels-install/scripts/configure_pixels.sh +++ b/skills/pixels-install/scripts/configure_pixels.sh @@ -1,4 +1,12 @@ #!/usr/bin/env bash +if [ -z "${BASH_VERSION:-}" ]; then + printf 'ERROR: pixels-install scripts must be executed by Bash; do not run this script with zsh.\n' >&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" diff --git a/skills/pixels-install/scripts/install_etcd.sh b/skills/pixels-install/scripts/install_etcd.sh index ae26efac28..72a4b839e0 100755 --- a/skills/pixels-install/scripts/install_etcd.sh +++ b/skills/pixels-install/scripts/install_etcd.sh @@ -1,9 +1,19 @@ #!/usr/bin/env bash +if [ -z "${BASH_VERSION:-}" ]; then + printf 'ERROR: pixels-install scripts must be executed by Bash; do not run this script with zsh.\n' >&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # shellcheck source=lib/shell_env.sh source "$SCRIPT_DIR/lib/shell_env.sh" +require_bash_runtime || exit 1 +require_supported_login_shell >/dev/null || exit 1 SKILL_DIR="${SKILL_DIR:-$(skill_dir)}" STATE_DIR="${STATE_DIR:-$(state_dir)}" diff --git a/skills/pixels-install/scripts/install_jdk.sh b/skills/pixels-install/scripts/install_jdk.sh index 28b41358f6..eee59a018f 100755 --- a/skills/pixels-install/scripts/install_jdk.sh +++ b/skills/pixels-install/scripts/install_jdk.sh @@ -1,4 +1,12 @@ #!/usr/bin/env bash +if [ -z "${BASH_VERSION:-}" ]; then + printf 'ERROR: pixels-install scripts must be executed by Bash; do not run this script with zsh.\n' >&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -euo pipefail # Installs a Zulu OpenJDK build for the current server's CPU architecture diff --git a/skills/pixels-install/scripts/install_maven.sh b/skills/pixels-install/scripts/install_maven.sh index c5a0e2d6c7..206aba1bf9 100755 --- a/skills/pixels-install/scripts/install_maven.sh +++ b/skills/pixels-install/scripts/install_maven.sh @@ -1,4 +1,12 @@ #!/usr/bin/env bash +if [ -z "${BASH_VERSION:-}" ]; then + printf 'ERROR: pixels-install scripts must be executed by Bash; do not run this script with zsh.\n' >&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" diff --git a/skills/pixels-install/scripts/install_mysql.sh b/skills/pixels-install/scripts/install_mysql.sh index 98cf51bcfc..3a55e494eb 100755 --- a/skills/pixels-install/scripts/install_mysql.sh +++ b/skills/pixels-install/scripts/install_mysql.sh @@ -1,4 +1,12 @@ #!/usr/bin/env bash +if [ -z "${BASH_VERSION:-}" ]; then + printf 'ERROR: pixels-install scripts must be executed by Bash; do not run this script with zsh.\n' >&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -euo pipefail # Installs MySQL, sets the root password, and creates the Pixels metadata @@ -18,6 +26,8 @@ set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # shellcheck source=lib/shell_env.sh source "$SCRIPT_DIR/lib/shell_env.sh" +require_bash_runtime || exit 1 +require_supported_login_shell >/dev/null || exit 1 SKILL_DIR="${SKILL_DIR:-$(skill_dir)}" STATE_DIR="${STATE_DIR:-$(state_dir)}" REPO_ROOT="${REPO_ROOT:-$(require_repo_root)}" diff --git a/skills/pixels-install/scripts/install_shell_helpers.sh b/skills/pixels-install/scripts/install_shell_helpers.sh index d63b4c70ab..745c87b162 100755 --- a/skills/pixels-install/scripts/install_shell_helpers.sh +++ b/skills/pixels-install/scripts/install_shell_helpers.sh @@ -1,4 +1,12 @@ #!/usr/bin/env bash +if [ -z "${BASH_VERSION:-}" ]; then + printf 'ERROR: pixels-install scripts must be executed by Bash; do not run this script with zsh.\n' >&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -euo pipefail # Installs Pixels shell functions for day-to-day operation. By default the @@ -146,16 +154,20 @@ local_functions_file() { write_functions_file() { local functions_file="$1" + local default_pixels_home log "writing start_pixels/stop_pixels/restart_pixels to $functions_file" mkdir -p "$(dirname "$functions_file")" + default_pixels_home="$(shell_quote "$PIXELS_HOME")" cat > "$functions_file" </dev/null 2>&1 && env $ssh_target bash $(shell_quote "$REMOTE_SCRIPT_DIR/install_shell_helpers.sh")" } main() { diff --git a/skills/pixels-install/scripts/install_trino.sh b/skills/pixels-install/scripts/install_trino.sh index 77f1e48504..e17b4265c4 100755 --- a/skills/pixels-install/scripts/install_trino.sh +++ b/skills/pixels-install/scripts/install_trino.sh @@ -1,4 +1,12 @@ #!/usr/bin/env bash +if [ -z "${BASH_VERSION:-}" ]; then + printf 'ERROR: pixels-install scripts must be executed by Bash; do not run this script with zsh.\n' >&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -euo pipefail # Installs Trino (per https://trino.io/docs/466/installation/deployment.html) diff --git a/skills/pixels-install/scripts/install_trino_cluster.sh b/skills/pixels-install/scripts/install_trino_cluster.sh index bbbf99d10e..ccfc5b770c 100755 --- a/skills/pixels-install/scripts/install_trino_cluster.sh +++ b/skills/pixels-install/scripts/install_trino_cluster.sh @@ -1,4 +1,12 @@ #!/usr/bin/env bash +if [ -z "${BASH_VERSION:-}" ]; then + printf 'ERROR: pixels-install scripts must be executed by Bash; do not run this script with zsh.\n' >&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -uo pipefail # Fans out install_trino.sh across the whole cluster described by @@ -21,6 +29,8 @@ set -uo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # shellcheck source=lib/shell_env.sh source "$SCRIPT_DIR/lib/shell_env.sh" +require_bash_runtime || exit 1 +require_supported_login_shell >/dev/null || exit 1 SKILL_DIR="${SKILL_DIR:-$(skill_dir)}" STATE_DIR="${STATE_DIR:-$(state_dir)}" @@ -185,7 +195,7 @@ run_local_coordinator() { [[ -n "$PIXELS_TRINO_CONNECTOR_ZIP" ]] && env_args+=(PIXELS_TRINO_CONNECTOR_ZIP="$PIXELS_TRINO_CONNECTOR_ZIP") [[ -n "$PIXELS_TRINO_LISTENER_ZIP" ]] && env_args+=(PIXELS_TRINO_LISTENER_ZIP="$PIXELS_TRINO_LISTENER_ZIP") - if env "${env_args[@]}" "$SCRIPT_DIR/install_trino.sh" >"$log_file" 2>&1; then + if env "${env_args[@]}" bash "$SCRIPT_DIR/install_trino.sh" >"$log_file" 2>&1; then result_record "node:$name" ok "coordinator install succeeded (log: $log_file)" else result_record "node:$name" fail "coordinator install failed, see $log_file (tail: $(tail -n 1 "$log_file" 2>/dev/null))" @@ -213,38 +223,38 @@ launch_worker() { scp_opts+=(-o BatchMode=yes -o ConnectTimeout=10 -o StrictHostKeyChecking=accept-new) remote_deployment_file="$REMOTE_STATE_DIR/trino-deployment.env" - remote_env="TRINO_PIXELS_HOME='$TRINO_PIXELS_HOME' TRINO_PIXELS_CONFIG='$TRINO_PIXELS_CONFIG' TRINO_PIXELS_METADATA_SERVER_HOST='$TRINO_PIXELS_METADATA_SERVER_HOST' TRINO_PIXELS_TRANS_SERVER_HOST='$TRINO_PIXELS_TRANS_SERVER_HOST' TRINO_PIXELS_QUERY_SCHEDULE_SERVER_HOST='$TRINO_PIXELS_QUERY_SCHEDULE_SERVER_HOST' TRINO_PIXELS_ETCD_HOSTS='$TRINO_PIXELS_ETCD_HOSTS' TRINO_PIXELS_METADATA_SERVER_PORT='$TRINO_PIXELS_METADATA_SERVER_PORT' TRINO_PIXELS_TRANS_SERVER_PORT='$TRINO_PIXELS_TRANS_SERVER_PORT' TRINO_PIXELS_QUERY_SCHEDULE_SERVER_PORT='$TRINO_PIXELS_QUERY_SCHEDULE_SERVER_PORT' TRINO_PIXELS_ETCD_PORT='$TRINO_PIXELS_ETCD_PORT'" + remote_env="TRINO_PIXELS_HOME=$(shell_quote "$TRINO_PIXELS_HOME") TRINO_PIXELS_CONFIG=$(shell_quote "$TRINO_PIXELS_CONFIG") TRINO_PIXELS_METADATA_SERVER_HOST=$(shell_quote "$TRINO_PIXELS_METADATA_SERVER_HOST") TRINO_PIXELS_TRANS_SERVER_HOST=$(shell_quote "$TRINO_PIXELS_TRANS_SERVER_HOST") TRINO_PIXELS_QUERY_SCHEDULE_SERVER_HOST=$(shell_quote "$TRINO_PIXELS_QUERY_SCHEDULE_SERVER_HOST") TRINO_PIXELS_ETCD_HOSTS=$(shell_quote "$TRINO_PIXELS_ETCD_HOSTS") TRINO_PIXELS_METADATA_SERVER_PORT=$(shell_quote "$TRINO_PIXELS_METADATA_SERVER_PORT") TRINO_PIXELS_TRANS_SERVER_PORT=$(shell_quote "$TRINO_PIXELS_TRANS_SERVER_PORT") TRINO_PIXELS_QUERY_SCHEDULE_SERVER_PORT=$(shell_quote "$TRINO_PIXELS_QUERY_SCHEDULE_SERVER_PORT") TRINO_PIXELS_ETCD_PORT=$(shell_quote "$TRINO_PIXELS_ETCD_PORT")" if [[ -f "$TRINO_PIXELS_CONFIG_SOURCE" ]]; then remote_pixels_config="$REMOTE_ARTIFACT_DIR/$(basename "$TRINO_PIXELS_CONFIG_SOURCE")" - remote_env="$remote_env TRINO_PIXELS_CONFIG_SOURCE='$remote_pixels_config'" + remote_env="$remote_env TRINO_PIXELS_CONFIG_SOURCE=$(shell_quote "$remote_pixels_config")" fi if [[ -n "$PIXELS_TRINO_CONNECTOR_ZIP" ]]; then remote_connector_zip="$REMOTE_ARTIFACT_DIR/$(basename "$PIXELS_TRINO_CONNECTOR_ZIP")" [[ -n "$PIXELS_TRINO_LISTENER_ZIP" ]] && remote_listener_zip="$REMOTE_ARTIFACT_DIR/$(basename "$PIXELS_TRINO_LISTENER_ZIP")" remote_script="$REMOTE_SCRIPT_DIR/install_trino.sh" - remote_command="mkdir -p '$REMOTE_STATE_DIR' '$REMOTE_SCRIPT_DIR/lib' '$REMOTE_ARTIFACT_DIR' && STATE_DIR='$REMOTE_STATE_DIR' REPO_ROOT='$REMOTE_SCRIPT_DIR' TRINO_ROLE=worker CONFIRM_TRINO_INSTALL=true PIXELS_TRINO_CONNECTOR_ZIP='$remote_connector_zip' PIXELS_TRINO_LISTENER_ZIP='$remote_listener_zip' INSTALL_PIXELS_TRINO_LISTENER='$INSTALL_PIXELS_TRINO_LISTENER' $remote_env '$remote_script'" + remote_command="mkdir -p $(shell_quote "$REMOTE_STATE_DIR") $(shell_quote "$REMOTE_SCRIPT_DIR/lib") $(shell_quote "$REMOTE_ARTIFACT_DIR") && STATE_DIR=$(shell_quote "$REMOTE_STATE_DIR") REPO_ROOT=$(shell_quote "$REMOTE_SCRIPT_DIR") TRINO_ROLE=worker CONFIRM_TRINO_INSTALL=true PIXELS_TRINO_CONNECTOR_ZIP=$(shell_quote "$remote_connector_zip") PIXELS_TRINO_LISTENER_ZIP=$(shell_quote "$remote_listener_zip") INSTALL_PIXELS_TRINO_LISTENER=$(shell_quote "$INSTALL_PIXELS_TRINO_LISTENER") $remote_env bash $(shell_quote "$remote_script")" else - remote_command="cd '$REMOTE_REPO_ROOT' && mkdir -p '$REMOTE_STATE_DIR' '$REMOTE_ARTIFACT_DIR' && if [ -x '$REMOTE_SKILL_SCRIPT' ]; then script='$REMOTE_SKILL_SCRIPT'; elif [ -x '$REMOTE_DEV_SCRIPT' ]; then script='$REMOTE_DEV_SCRIPT'; else echo 'install_trino.sh not found; install the pixels-install skill or set REMOTE_SKILL_SCRIPT' >&2; exit 1; fi; STATE_DIR='$REMOTE_STATE_DIR' REPO_ROOT='$REMOTE_REPO_ROOT' TRINO_ROLE=worker CONFIRM_TRINO_INSTALL=true INSTALL_PIXELS_TRINO_LISTENER='$INSTALL_PIXELS_TRINO_LISTENER' $remote_env \"\$script\"" + remote_command="cd $(shell_quote "$REMOTE_REPO_ROOT") && mkdir -p $(shell_quote "$REMOTE_STATE_DIR") $(shell_quote "$REMOTE_ARTIFACT_DIR") && if [ -x $(shell_quote "$REMOTE_SKILL_SCRIPT") ]; then script=$(shell_quote "$REMOTE_SKILL_SCRIPT"); elif [ -x $(shell_quote "$REMOTE_DEV_SCRIPT") ]; then script=$(shell_quote "$REMOTE_DEV_SCRIPT"); else echo 'install_trino.sh not found; install the pixels-install skill or set REMOTE_SKILL_SCRIPT' >&2; exit 1; fi; STATE_DIR=$(shell_quote "$REMOTE_STATE_DIR") REPO_ROOT=$(shell_quote "$REMOTE_REPO_ROOT") TRINO_ROLE=worker CONFIRM_TRINO_INSTALL=true INSTALL_PIXELS_TRINO_LISTENER=$(shell_quote "$INSTALL_PIXELS_TRINO_LISTENER") $remote_env bash \"\$script\"" fi ( { echo "--- preparing remote state dir on $name ($ssh_target) ---" - ssh "${ssh_args[@]}" "$(remote_spec "$ssh_target")" "mkdir -p '$REMOTE_STATE_DIR'" && + ssh "${ssh_args[@]}" "$(remote_spec "$ssh_target")" "mkdir -p $(shell_quote "$REMOTE_STATE_DIR")" && echo "--- copying trino-deployment.env to $name ($ssh_target) ---" scp "${scp_opts[@]}" "$TRINO_DEPLOYMENT_FILE" "$(remote_spec "$ssh_target"):$remote_deployment_file" && if [[ -f "$TRINO_PIXELS_CONFIG_SOURCE" ]]; then echo "--- copying Trino-side Pixels client config to $name ($ssh_target) ---" - ssh "${ssh_args[@]}" "$(remote_spec "$ssh_target")" "mkdir -p '$REMOTE_ARTIFACT_DIR'" && + ssh "${ssh_args[@]}" "$(remote_spec "$ssh_target")" "mkdir -p $(shell_quote "$REMOTE_ARTIFACT_DIR")" && scp "${scp_opts[@]}" "$TRINO_PIXELS_CONFIG_SOURCE" "$(remote_spec "$ssh_target"):$remote_pixels_config" fi && if [[ -n "$PIXELS_TRINO_CONNECTOR_ZIP" ]]; then echo "--- copying minimal installer to $name ($ssh_target) ---" - ssh "${ssh_args[@]}" "$(remote_spec "$ssh_target")" "mkdir -p '$REMOTE_SCRIPT_DIR/lib' '$REMOTE_ARTIFACT_DIR'" && + ssh "${ssh_args[@]}" "$(remote_spec "$ssh_target")" "mkdir -p $(shell_quote "$REMOTE_SCRIPT_DIR/lib") $(shell_quote "$REMOTE_ARTIFACT_DIR")" && scp "${scp_opts[@]}" "$SCRIPT_DIR/install_trino.sh" "$(remote_spec "$ssh_target"):$REMOTE_SCRIPT_DIR/install_trino.sh" && scp "${scp_opts[@]}" "$SCRIPT_DIR/lib/shell_env.sh" "$(remote_spec "$ssh_target"):$REMOTE_SCRIPT_DIR/lib/shell_env.sh" && - ssh "${ssh_args[@]}" "$(remote_spec "$ssh_target")" "chmod +x '$REMOTE_SCRIPT_DIR/install_trino.sh'" && + ssh "${ssh_args[@]}" "$(remote_spec "$ssh_target")" "chmod +x $(shell_quote "$REMOTE_SCRIPT_DIR/install_trino.sh")" && echo "--- copying prebuilt pixels-trino artifacts to $name ($ssh_target) ---" && scp "${scp_opts[@]}" "$PIXELS_TRINO_CONNECTOR_ZIP" "$(remote_spec "$ssh_target"):$remote_connector_zip" && if [[ -n "$PIXELS_TRINO_LISTENER_ZIP" ]]; then @@ -252,7 +262,7 @@ launch_worker() { fi fi && echo "--- running install_trino.sh on $name ($ssh_target) ---" && - ssh "${ssh_args[@]}" "$(remote_spec "$ssh_target")" "$remote_command" + ssh "${ssh_args[@]}" "$(remote_spec "$ssh_target")" "bash -lc $(shell_quote "$remote_command")" } >"$log_file" 2>&1 echo "$?" > "$marker_file" ) & diff --git a/skills/pixels-install/scripts/install_trino_shell_helpers.sh b/skills/pixels-install/scripts/install_trino_shell_helpers.sh index 643f6888db..8575eba8ac 100755 --- a/skills/pixels-install/scripts/install_trino_shell_helpers.sh +++ b/skills/pixels-install/scripts/install_trino_shell_helpers.sh @@ -1,4 +1,12 @@ #!/usr/bin/env bash +if [ -z "${BASH_VERSION:-}" ]; then + printf 'ERROR: pixels-install scripts must be executed by Bash; do not run this script with zsh.\n' >&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -euo pipefail # Optional convenience step (asks before doing anything): installs @@ -41,6 +49,7 @@ TRINO_HTTP_PORT="${TRINO_HTTP_PORT:-8080}" TRINO_FUNCTIONS_FILE="${TRINO_FUNCTIONS_FILE:-}" TRINO_PIXELS_HOME="${TRINO_PIXELS_HOME:-${PIXELS_HOME:-$HOME/opt/pixels}}" TRINO_PIXELS_CONFIG="${TRINO_PIXELS_CONFIG:-${PIXELS_CONFIG:-$TRINO_PIXELS_HOME/etc/pixels.properties}}" +TRINO_DEFAULT_HOME_LINK="${TRINO_HOME_LINK:-$HOME/opt/trino-server}" TRINO_SHELL_HELPERS_TARGET="${TRINO_SHELL_HELPERS_TARGET:-coordinator}" REMOTE_STATE_DIR="${REMOTE_STATE_DIR:-.agents/state/pixels-install}" REMOTE_SCRIPT_DIR="${REMOTE_SCRIPT_DIR:-.agents/skills/pixels-install/scripts}" @@ -148,11 +157,13 @@ target_description() { # worker (operated over SSH from the coordinator). build_node_list() { local node + local -a worker_targets=() coordinator_target >/dev/null TRINO_NODE_LIST=("$(remote_spec "$TRINO_COORDINATOR_SSH_TARGET")") - for node in ${TRINO_WORKER_SSH_TARGETS:-}; do + read -r -a worker_targets <<< "${TRINO_WORKER_SSH_TARGETS:-}" + for node in "${worker_targets[@]}"; do TRINO_NODE_LIST+=("$(remote_spec "$node")") done } @@ -163,13 +174,13 @@ local_functions_file() { write_functions_file() { local functions_file="$1" - local nodes_literal node default_pixels_home default_pixels_config + local nodes_literal node default_home_link default_pixels_home default_pixels_config nodes_literal="" for node in "${TRINO_NODE_LIST[@]}"; do - nodes_literal+=" \"$node\" -" + nodes_literal+=" $(shell_quote "$node")"$'\n' done + default_home_link="$(shell_quote "$TRINO_DEFAULT_HOME_LINK")" default_pixels_home="$(shell_quote "$TRINO_PIXELS_HOME")" default_pixels_config="$(shell_quote "$TRINO_PIXELS_CONFIG")" @@ -186,12 +197,13 @@ write_functions_file() { TRINO_NODES=( $nodes_literal) -TRINO_REMOTE_HOME_LINK="\${TRINO_REMOTE_HOME_LINK:-${TRINO_HOME_LINK:-$HOME/opt/trino-server}}" +TRINO_DEFAULT_HOME_LINK=$default_home_link +TRINO_REMOTE_HOME_LINK="\${TRINO_REMOTE_HOME_LINK:-\$TRINO_DEFAULT_HOME_LINK}" TRINO_DEFAULT_PIXELS_HOME=$default_pixels_home TRINO_DEFAULT_PIXELS_CONFIG=$default_pixels_config _trino_home() { - printf '%s\n' "\${TRINO_HOME_LINK:-\$HOME/opt/trino-server}" + printf '%s\n' "\${TRINO_HOME_LINK:-\$TRINO_DEFAULT_HOME_LINK}" } _trino_pixels_home() { @@ -219,13 +231,20 @@ _trino_remote_launcher() { printf '%s/bin/launcher' "\$TRINO_REMOTE_HOME_LINK" } +_trino_shell_quote() { + printf '%q' "\$1" +} + _trino_remote_run() { local node="\$1" local action="\$2" - local pixels_home pixels_config + local pixels_home pixels_config remote_launcher remote_command remote_bash_command pixels_home="\$(_trino_pixels_home)" pixels_config="\$(_trino_pixels_config)" - ssh -n "\$node" "export PIXELS_HOME='\$pixels_home'; export PIXELS_CONFIG='\$pixels_config'; \\"\$(_trino_remote_launcher)\\" \$action" + remote_launcher="\$(_trino_remote_launcher)" + remote_command="export PIXELS_HOME=\$(_trino_shell_quote "\$pixels_home"); export PIXELS_CONFIG=\$(_trino_shell_quote "\$pixels_config"); \$(_trino_shell_quote "\$remote_launcher") \$(_trino_shell_quote "\$action")" + remote_bash_command="bash -lc \$(_trino_shell_quote "\$remote_command")" + ssh -n -o BatchMode=yes -o ConnectTimeout=10 "\$node" "\$remote_bash_command" } start_trino_cluster() { @@ -246,21 +265,26 @@ start_trino_cluster() { echo "trino cluster started" } +# This file is sourced into the user's login shell, which may be zsh, where +# arrays are 1-indexed. Iterate over elements instead of numeric indices so +# the coordinator/worker split is identical under bash and zsh. stop_trino_cluster() { - local idx node + local node coordinator="" first=true - for ((idx = \${#TRINO_NODES[@]} - 1; idx >= 0; idx--)); do - node="\${TRINO_NODES[\$idx]}" - if (( idx > 0 )); then - echo "stopping trino on \$node (worker, remote)" - _trino_remote_run "\$node" stop || { echo "failed to stop worker \$node" >&2; return 1; } - else - echo "stopping trino on \$node (coordinator, local)" - _export_trino_pixels_env - "\$(_trino_home)/bin/launcher" stop || { echo "failed to stop coordinator \$node" >&2; return 1; } + for node in "\${TRINO_NODES[@]}"; do + if [[ "\$first" == "true" ]]; then + coordinator="\$node" + first=false + continue fi + echo "stopping trino on \$node (worker, remote)" + _trino_remote_run "\$node" stop || { echo "failed to stop worker \$node" >&2; return 1; } done + echo "stopping trino on \$coordinator (coordinator, local)" + _export_trino_pixels_env + "\$(_trino_home)/bin/launcher" stop || { echo "failed to stop coordinator \$coordinator" >&2; return 1; } + echo "trino cluster stopped" } @@ -297,7 +321,8 @@ install_local() { build_node_list functions_file="$(local_functions_file)" write_functions_file "$functions_file" - bash -n "$functions_file" || fail "generated functions file has a syntax error: $functions_file" + validate_shell_compatibility "$functions_file" || + fail "generated functions file is not compatible with Bash and Zsh: $functions_file" persist_source_line "$functions_file" log "trino shell helpers installed on current host: start_trino_cluster, stop_trino_cluster, restart_trino_cluster, trino_cli" @@ -338,9 +363,12 @@ install_remote_coordinator() { if [[ -n "$TRINO_FUNCTIONS_FILE" ]]; then remote_env+=("TRINO_FUNCTIONS_FILE=$(shell_quote "$TRINO_FUNCTIONS_FILE")") fi + if [[ -n "${PROFILE_FILE:-}" ]]; then + remote_env+=("PROFILE_FILE=$(shell_quote "$PROFILE_FILE")") + fi remote_env_string="$(printf '%s ' "${remote_env[@]}")" - ssh "${ssh_args[@]}" "$remote" "chmod +x $(shell_quote "$REMOTE_SCRIPT_DIR/install_trino_shell_helpers.sh") && env $remote_env_string $(shell_quote "$REMOTE_SCRIPT_DIR/install_trino_shell_helpers.sh")" + ssh "${ssh_args[@]}" "$remote" "chmod +x $(shell_quote "$REMOTE_SCRIPT_DIR/install_trino_shell_helpers.sh") && command -v bash >/dev/null 2>&1 && env $remote_env_string bash $(shell_quote "$REMOTE_SCRIPT_DIR/install_trino_shell_helpers.sh")" } main() { diff --git a/skills/pixels-install/scripts/lib/shell_env.sh b/skills/pixels-install/scripts/lib/shell_env.sh index dc98a45b6d..45df50d196 100644 --- a/skills/pixels-install/scripts/lib/shell_env.sh +++ b/skills/pixels-install/scripts/lib/shell_env.sh @@ -103,23 +103,114 @@ state_dir() { DEFAULT_STATE_DIR="$(state_dir)" DEFAULT_TOOLCHAIN_ENV_FILE="$DEFAULT_STATE_DIR/toolchain.env" +# Return the login shell configured for the account running the installer. +# `$SHELL` can describe the parent agent/SSH process instead of the target +# account, so passwd/NSS is authoritative whenever it is available. +login_shell_path() { + local user="${LOGIN_SHELL_USER:-${DEPLOYMENT_USER:-$(id -un 2>/dev/null || printf '%s' "${USER:-}")}}" + local shell_path="" + local passwd_lookup_available=false + + if command -v getent >/dev/null 2>&1 && [[ -n "$user" ]]; then + passwd_lookup_available=true + shell_path="$(getent passwd "$user" 2>/dev/null | awk -F: 'NR == 1 { print $7; exit }')" + fi + if [[ -z "$shell_path" && -r /etc/passwd && -n "$user" ]]; then + passwd_lookup_available=true + shell_path="$(awk -F: -v account="$user" '$1 == account { print $7; exit }' /etc/passwd)" + fi + if [[ -z "$shell_path" && "$passwd_lookup_available" == "false" ]]; then + shell_path="${SHELL:-}" + fi + + [[ -n "$shell_path" ]] || { + printf 'ERROR: could not determine the login shell for account %s\n' "${user:-unknown}" >&2 + return 1 + } + printf '%s\n' "$shell_path" +} + +login_shell_name() { + local shell_path + shell_path="$(login_shell_path)" || return 1 + + case "$(basename "$shell_path")" in + bash|zsh) + basename "$shell_path" + ;; + *) + printf 'ERROR: unsupported login shell %s; pixels-install supports bash or zsh only\n' "$shell_path" >&2 + return 1 + ;; + esac +} + +require_bash_runtime() { + [[ -n "${BASH_VERSION:-}" ]] || { + printf 'ERROR: pixels-install scripts must be executed by Bash; run the script directly instead of with zsh or source\n' >&2 + return 1 + } + command -v bash >/dev/null 2>&1 || { + printf 'ERROR: Bash is required on the deployment node but was not found on PATH\n' >&2 + return 1 + } +} + +require_supported_login_shell() { + local shell_name + shell_name="$(login_shell_name)" || return 1 + command -v "$shell_name" >/dev/null 2>&1 || { + printf 'ERROR: configured login shell %s is not executable on this node\n' "$shell_name" >&2 + return 1 + } + printf '%s\n' "$shell_name" +} + +# Quote a literal value for a shell assignment. This is used for toolchain.env +# and for values embedded in remote shell commands. +shell_quote() { + printf "'%s'" "$(printf '%s' "$1" | sed "s/'/'\\\\''/g")" +} + +# Quote a profile value while preserving the intentional $HOME/$PIXELS_HOME +# expressions emitted by profile_path_value() and install_trino.sh. +profile_value_quote() { + local value="$1" + local variable + local suffix + + for variable in '$HOME' '$PIXELS_HOME'; do + if [[ "$value" == "$variable" ]]; then + printf '%s\n' "$variable" + return + fi + if [[ "$value" == "$variable/"* ]]; then + suffix="${value#"$variable"}" + printf '%s%s\n' "$variable" "$(shell_quote "$suffix")" + return + fi + done + + shell_quote "$value" +} + # Print the profile file that should receive persisted exports. -# Honors an explicit PROFILE_FILE override; otherwise picks the rc file -# matching the user's current login shell ($SHELL). +# Honors an explicit PROFILE_FILE override, but still validates the actual +# account login shell before returning. detect_profile_file() { + local shell_name + shell_name="$(require_supported_login_shell)" || return 1 + if [[ -n "${PROFILE_FILE:-}" ]]; then printf '%s\n' "$PROFILE_FILE" return fi - local shell_name - shell_name="$(basename "${SHELL:-bash}")" - case "$shell_name" in zsh) printf '%s\n' "$HOME/.zshrc" ;; - *) + bash) printf '%s\n' "$HOME/.bashrc" ;; esac @@ -146,7 +237,7 @@ persist_export() { local profile_file="$1" local name="$2" local value="$3" - local desired_line="export ${name}=${value}" + local desired_line="export ${name}=$(profile_value_quote "$value")" local tmp touch "$profile_file" @@ -211,7 +302,7 @@ persist_toolchain_var() { touch "$file" tmp="$(mktemp "${file}.XXXXXX")" grep -v "^export ${name}=" "$file" > "$tmp" 2>/dev/null || true - printf 'export %s=%s\n' "$name" "$value" >> "$tmp" + printf 'export %s=%s\n' "$name" "$(shell_quote "$value")" >> "$tmp" mv "$tmp" "$file" } @@ -219,6 +310,9 @@ persist_toolchain_var() { # bin directories it set onto PATH, so a script invoked as a fresh process # can still find `java`/`mvn`/`etcdctl` installed by an earlier step. load_toolchain_env() { + require_bash_runtime || return 1 + require_supported_login_shell >/dev/null || return 1 + local file="${TOOLCHAIN_ENV_FILE:-$DEFAULT_TOOLCHAIN_ENV_FILE}" local java_home_was_set=false java_home_value="" local maven_home_was_set=false maven_home_value="" @@ -267,6 +361,32 @@ load_toolchain_env() { fi } +# Validate a generated file that will be sourced by the user's interactive +# shell. The installer itself remains Bash-only, but generated helpers must +# parse in both supported login shells. +validate_shell_compatibility() { + local file="$1" + + [[ -f "$file" ]] || { + printf 'ERROR: generated shell file not found: %s\n' "$file" >&2 + return 1 + } + bash -n "$file" || { + printf 'ERROR: generated shell file is invalid Bash: %s\n' "$file" >&2 + return 1 + } + + if command -v zsh >/dev/null 2>&1; then + zsh -n "$file" || { + printf 'ERROR: generated shell file is invalid Zsh: %s\n' "$file" >&2 + return 1 + } + elif [[ "$(login_shell_name)" == "zsh" ]]; then + printf 'ERROR: the login shell is Zsh, but zsh was not found to validate %s\n' "$file" >&2 + return 1 + fi +} + # --- Structured result reporting -------------------------------------------- # Diagnostic scripts (check_prerequisites.sh, smoke_test.sh, ...) run many # independent checks. Bailing out on the first failure (the old behavior, diff --git a/skills/pixels-install/scripts/prepare_deployment.sh b/skills/pixels-install/scripts/prepare_deployment.sh index 4bbfa3bdf9..7464530a6c 100755 --- a/skills/pixels-install/scripts/prepare_deployment.sh +++ b/skills/pixels-install/scripts/prepare_deployment.sh @@ -1,9 +1,19 @@ #!/usr/bin/env bash +if [ -z "${BASH_VERSION:-}" ]; then + printf 'ERROR: pixels-install scripts must be executed by Bash; do not run this script with zsh.\n' >&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # shellcheck source=lib/shell_env.sh source "$SCRIPT_DIR/lib/shell_env.sh" +require_bash_runtime || exit 1 +require_supported_login_shell >/dev/null || exit 1 SKILL_DIR="${SKILL_DIR:-$(skill_dir)}" STATE_DIR="${STATE_DIR:-$(state_dir)}" SHARED_SETUP_CLUSTER="${SHARED_SETUP_CLUSTER:-$SKILL_DIR/shared-scripts/setup_cluster.sh}" diff --git a/skills/pixels-install/scripts/prepare_trino_cluster.sh b/skills/pixels-install/scripts/prepare_trino_cluster.sh index 471449e49f..ad12ab4a4e 100755 --- a/skills/pixels-install/scripts/prepare_trino_cluster.sh +++ b/skills/pixels-install/scripts/prepare_trino_cluster.sh @@ -1,4 +1,12 @@ #!/usr/bin/env bash +if [ -z "${BASH_VERSION:-}" ]; then + printf 'ERROR: pixels-install scripts must be executed by Bash; do not run this script with zsh.\n' >&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -euo pipefail # Collects the Trino cluster topology (one coordinator, zero or more @@ -13,6 +21,8 @@ set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # shellcheck source=lib/shell_env.sh source "$SCRIPT_DIR/lib/shell_env.sh" +require_bash_runtime || exit 1 +require_supported_login_shell >/dev/null || exit 1 SKILL_DIR="${SKILL_DIR:-$(skill_dir)}" STATE_DIR="${STATE_DIR:-$(state_dir)}" SHARED_SETUP_CLUSTER="${SHARED_SETUP_CLUSTER:-$SKILL_DIR/shared-scripts/setup_cluster.sh}" diff --git a/skills/pixels-install/scripts/progress.sh b/skills/pixels-install/scripts/progress.sh index bd6893ae9b..33e2aaebb2 100755 --- a/skills/pixels-install/scripts/progress.sh +++ b/skills/pixels-install/scripts/progress.sh @@ -1,4 +1,12 @@ #!/usr/bin/env bash +if [ -z "${BASH_VERSION:-}" ]; then + printf 'ERROR: pixels-install scripts must be executed by Bash; do not run this script with zsh.\n' >&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -euo pipefail # Thin CLI over the phase-checkpoint helpers in lib/shell_env.sh. A full @@ -17,6 +25,8 @@ set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # shellcheck source=lib/shell_env.sh source "$SCRIPT_DIR/lib/shell_env.sh" +require_bash_runtime || exit 1 +require_supported_login_shell >/dev/null || exit 1 usage() { cat <&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -uo pipefail # Verifies the installed layout, configuration, metadata access, etcd @@ -624,7 +632,7 @@ verify_trino_cluster_status() { local -a targets=() local -a names=() local -a ssh_args=() - local i target name output + local i target name output remote_status_command read -r -a targets <<< "${TRINO_WORKER_SSH_TARGETS:-}" read -r -a names <<< "${TRINO_WORKER_NAMES:-}" @@ -637,7 +645,8 @@ verify_trino_cluster_status() { for ((i = 0; i < ${#targets[@]}; i++)); do target="${targets[$i]}" name="${names[$i]:-worker-$i}" - if output="$(ssh "${ssh_args[@]}" "$(remote_spec "$target")" "export PIXELS_HOME='$TRINO_PIXELS_HOME'; export PIXELS_CONFIG='$TRINO_PIXELS_CONFIG'; '$TRINO_HOME_LINK/bin/launcher' status" 2>&1)"; then + remote_status_command="export PIXELS_HOME=$(shell_quote "$TRINO_PIXELS_HOME"); export PIXELS_CONFIG=$(shell_quote "$TRINO_PIXELS_CONFIG"); $(shell_quote "$TRINO_HOME_LINK/bin/launcher") status" + if output="$(ssh "${ssh_args[@]}" "$(remote_spec "$target")" "bash -lc $(shell_quote "$remote_status_command")" 2>&1)"; then result_record "trino_status:$name" ok "remote launcher reports running" else result_record "trino_status:$name" fail "remote launcher status failed on $target: $(printf '%s' "$output" | tail -n 1)" diff --git a/skills/pixels-install/shared-scripts/setup_cluster.sh b/skills/pixels-install/shared-scripts/setup_cluster.sh index 79ad8418bb..9ae60e4bbc 100755 --- a/skills/pixels-install/shared-scripts/setup_cluster.sh +++ b/skills/pixels-install/shared-scripts/setup_cluster.sh @@ -1,9 +1,23 @@ #!/usr/bin/env bash +if [ -z "${BASH_VERSION:-}" ]; then + printf 'ERROR: pixels-install scripts must be executed by Bash; do not run this script with zsh.\n' >&2 + exit 1 +fi +if [ "${BASH_SOURCE[0]}" != "$0" ]; then + printf 'ERROR: do not source pixels-install installer scripts; execute this script directly with Bash.\n' >&2 + return 1 2>/dev/null || exit 1 +fi set -euo pipefail # Run this script on your local machine. It logs in to each remote server, # ensures an SSH key exists, updates /etc/hosts, and distributes public keys. +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=../scripts/lib/shell_env.sh +source "$SCRIPT_DIR/../scripts/lib/shell_env.sh" +require_bash_runtime || exit 1 +require_supported_login_shell >/dev/null || exit 1 + SSH_USER="${SSH_USER:-root}" # Set to empty if SSH_TARGETS already contains user@host or uses ssh config User. SSH_PORT="${SSH_PORT:-}" # Leave empty to use ssh config / default port 22. VERIFY_REMOTE_LOGIN="${VERIFY_REMOTE_LOGIN:-true}" @@ -328,6 +342,48 @@ run_scp_to_remote() { scp "${args[@]}" "$source_file" "$(remote_spec "$target"):$remote_file" } +check_remote_shell() { + local target="$1" + local remote_user="$2" + + run_ssh "$target" 'bash -s' -- "$remote_user" <<'REMOTE' +set -euo pipefail + +remote_user="$1" +shell_path="" +if command -v getent >/dev/null 2>&1; then + shell_path="$(getent passwd "$remote_user" 2>/dev/null | awk -F: 'NR == 1 { print $7; exit }')" +fi +if [[ -z "$shell_path" && -r /etc/passwd ]]; then + shell_path="$(awk -F: -v account="$remote_user" '$1 == account { print $7; exit }' /etc/passwd)" +fi + +[[ -n "$shell_path" ]] || { + echo "could not determine login shell for user '$remote_user' on $(hostname)" >&2 + exit 1 +} + +case "$(basename "$shell_path")" in + bash|zsh) + ;; + *) + echo "unsupported login shell '$shell_path' for user '$remote_user' on $(hostname); expected bash or zsh" >&2 + exit 1 + ;; +esac + +command -v "$(basename "$shell_path")" >/dev/null 2>&1 || { + echo "configured login shell '$shell_path' is not executable on $(hostname)" >&2 + exit 1 +} +command -v bash >/dev/null 2>&1 || { + echo "Bash is required on $(hostname) to run pixels-install scripts" >&2 + exit 1 +} +echo "remote account '$remote_user' uses $(basename "$shell_path"); Bash runtime is available" >&2 +REMOTE +} + validate_config() { local count="${#SSH_TARGETS[@]}" local i @@ -642,6 +698,25 @@ main() { check_local_connectivity local server_count="${#SSH_TARGETS[@]}" + local -a failed_shells=() + local i target hostname user + + log "checking remote login shells and Bash runtime" + for ((i = 0; i < server_count; i++)); do + target="${SSH_TARGETS[$i]}" + hostname="${HOST_NAMES[$i]}" + user="${HOST_USERS[$i]}" + if ! check_remote_shell "$target" "$user"; then + failed_shells+=("$hostname (target: $target, user: $user)") + fi + done + if [[ "${#failed_shells[@]}" -gt 0 ]]; then + printf '\nRemote shell validation failed for these servers:\n' >&2 + for item in "${failed_shells[@]}"; do + printf ' - %s\n' "$item" >&2 + done + exit 1 + fi WORKDIR="$(mktemp -d /tmp/setup_cluster_ssh.XXXXXX)" trap cleanup EXIT @@ -651,7 +726,7 @@ main() { local pubkeys_file="$WORKDIR/public_keys" local -a hostnames=() local -a remote_login_failures=() - local i target hostname user pubkey failure + local pubkey failure { printf '%s\n' "$MANAGED_BEGIN" diff --git a/skills/pixels-install/tests/shell_compatibility.sh b/skills/pixels-install/tests/shell_compatibility.sh new file mode 100644 index 0000000000..2fa46e9968 --- /dev/null +++ b/skills/pixels-install/tests/shell_compatibility.sh @@ -0,0 +1,150 @@ +#!/usr/bin/env bash +set -euo pipefail + +SKILL_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +SHELL_ENV="$SKILL_DIR/scripts/lib/shell_env.sh" + +command -v bash >/dev/null 2>&1 || { + printf 'Bash is required for this test\n' >&2 + exit 1 +} +command -v zsh >/dev/null 2>&1 || { + printf 'Zsh is required for this test\n' >&2 + exit 1 +} + +TEST_ROOT="$(mktemp -d "${TMPDIR:-/tmp}/pixels-install-shell-test.XXXXXX")" +trap 'rm -rf "$TEST_ROOT"' EXIT + +HOME="$TEST_ROOT/home" +STATE_DIR="$TEST_ROOT/state" +PROFILE_FILE="$TEST_ROOT/profile" +mkdir -p "$HOME" "$STATE_DIR" + +login_shell="$(getent passwd "$(id -un)" | awk -F: 'NR == 1 { print $7; exit }')" +case "$(basename "$login_shell")" in + bash) expected_profile="$HOME/.bashrc" ;; + zsh) expected_profile="$HOME/.zshrc" ;; + *) printf 'unsupported test account login shell: %s\n' "$login_shell" >&2; exit 1 ;; +esac +detected_profile="$(HOME="$HOME" SHELL=/bin/bash bash -c 'source "$1"; detect_profile_file' bash "$SHELL_ENV")" +[[ "$detected_profile" == "$expected_profile" ]] + +if zsh "$SKILL_DIR/scripts/progress.sh" --help >"$TEST_ROOT/zsh-entrypoint-error" 2>&1; then + printf 'a Bash installer unexpectedly ran under zsh\n' >&2 + exit 1 +fi +grep -Fq 'must be executed by Bash' "$TEST_ROOT/zsh-entrypoint-error" +if bash -c 'source "$1"' bash "$SKILL_DIR/scripts/progress.sh" >"$TEST_ROOT/source-entrypoint-error" 2>&1; then + printf 'a Bash installer unexpectedly succeeded when sourced\n' >&2 + exit 1 +fi +grep -Fq 'do not source pixels-install installer scripts' "$TEST_ROOT/source-entrypoint-error" + +quoted_value="$TEST_ROOT/path with space" +quoted_profile="$TEST_ROOT/profile with space" +PROFILE_FILE="$quoted_profile" +mkdir -p "$quoted_value" +bash -c 'source "$1"; persist_export "$2" TEST_PATH "$3"' \ + bash "$SHELL_ENV" "$PROFILE_FILE" "$quoted_value" +grep -Fxq "export TEST_PATH='$quoted_value'" "$PROFILE_FILE" + +PIXELS_FUNCTIONS_FILE="$TEST_ROOT/pixels helpers.sh" +PIXELS_HOME="$TEST_ROOT/pixels home" +env \ + HOME="$HOME" \ + STATE_DIR="$STATE_DIR" \ + PROFILE_FILE="$PROFILE_FILE" \ + PIXELS_FUNCTIONS_FILE="$PIXELS_FUNCTIONS_FILE" \ + PIXELS_HOME="$PIXELS_HOME" \ + PIXELS_SHELL_HELPERS_TARGET=local \ + INSTALL_PIXELS_SHELL_HELPERS=true \ + "$SKILL_DIR/scripts/install_shell_helpers.sh" + +bash -n "$PIXELS_FUNCTIONS_FILE" +zsh -n "$PIXELS_FUNCTIONS_FILE" +grep -Fq "[ -f \"$PIXELS_FUNCTIONS_FILE\" ] && source \"$PIXELS_FUNCTIONS_FILE\"" "$PROFILE_FILE" +bash -c 'source "$1"; [[ "$(_pixels_home)" == "$2" ]]' \ + bash "$PIXELS_FUNCTIONS_FILE" "$PIXELS_HOME" +zsh -f -c 'source "$1"; [[ "$(_pixels_home)" == "$2" ]]' \ + zsh "$PIXELS_FUNCTIONS_FILE" "$PIXELS_HOME" + +TRINO_DEPLOYMENT_FILE="$STATE_DIR/trino-deployment.env" +cat > "$TRINO_DEPLOYMENT_FILE" < "$TEST_ROOT/bash-remote-command" +zsh -f -c 'source "$1"; ssh() { printf "%s\n" "$*"; }; TRINO_REMOTE_HOME_LINK="$2"; TRINO_PIXELS_HOME="$3"; TRINO_PIXELS_CONFIG="$4"; _trino_remote_run worker-1 start' \ + zsh "$TRINO_FUNCTIONS_FILE" "$remote_home" "$remote_pixels_home" "$remote_pixels_config" > "$TEST_ROOT/zsh-remote-command" +for remote_output in "$TEST_ROOT/bash-remote-command" "$TEST_ROOT/zsh-remote-command"; do + grep -Fq 'PIXELS_HOME=' "$remote_output" + grep -Fq 'launcher' "$remote_output" + grep -Fq 'start' "$remote_output" +done + +mock_bin="$TEST_ROOT/mock-bin" +mkdir -p "$mock_bin" +cat > "$mock_bin/ssh" <<'EOF' +#!/usr/bin/env bash +set -euo pipefail + +last="${!#}" +if [[ "$last" == bash\ -lc\ * ]]; then + bash -n -c "$last" +fi +EOF +cat > "$mock_bin/scp" <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF +chmod +x "$mock_bin/ssh" "$mock_bin/scp" + +env \ + PATH="$mock_bin:$PATH" \ + STATE_DIR="$STATE_DIR" \ + TRINO_DEPLOYMENT_FILE="$TRINO_DEPLOYMENT_FILE" \ + RUN_LOCAL_COORDINATOR=false \ + CONFIRM_TRINO_CLUSTER_INSTALL=true \ + REMOTE_REPO_ROOT="$TEST_ROOT/remote repo" \ + REMOTE_SKILL_SCRIPT="$TEST_ROOT/remote repo/install trino.sh" \ + REMOTE_DEV_SCRIPT="$TEST_ROOT/remote repo/install trino.sh" \ + REMOTE_STATE_DIR="$TEST_ROOT/remote state" \ + REMOTE_SCRIPT_DIR="$TEST_ROOT/remote scripts" \ + REMOTE_ARTIFACT_DIR="$TEST_ROOT/remote artifacts" \ + "$SKILL_DIR/scripts/install_trino_cluster.sh" >/dev/null + +printf 'shell compatibility checks passed\n' From 429be80712a99f55b80a6146bba5e5b35576bac6 Mon Sep 17 00:00:00 2001 From: dannygeng Date: Tue, 28 Jul 2026 12:52:13 +0800 Subject: [PATCH 3/6] test(pixels-install): catch bash/zsh array index drift in cluster helpers --- .../tests/shell_compatibility.sh | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/skills/pixels-install/tests/shell_compatibility.sh b/skills/pixels-install/tests/shell_compatibility.sh index 2fa46e9968..2d49016f5e 100644 --- a/skills/pixels-install/tests/shell_compatibility.sh +++ b/skills/pixels-install/tests/shell_compatibility.sh @@ -116,6 +116,56 @@ for remote_output in "$TEST_ROOT/bash-remote-command" "$TEST_ROOT/zsh-remote-com grep -Fq 'start' "$remote_output" done +# Bash arrays are 0-indexed and Zsh arrays are 1-indexed, so array index +# arithmetic parses cleanly under both shells while silently skipping a node +# in one of them. `bash -n`/`zsh -n` cannot catch that, so drive the cluster +# functions with a stubbed launcher and compare which nodes they act on. +mkdir -p "$TRINO_HOME_LINK/bin" +cat > "$TRINO_HOME_LINK/bin/launcher" <<'EOF' +#!/usr/bin/env bash +printf 'local %s\n' "$1" +EOF +chmod +x "$TRINO_HOME_LINK/bin/launcher" + +cluster_order_script=' +source "$1" +TRINO_HOME_LINK="$2" +_trino_remote_run() { printf "remote %s %s\n" "$1" "$2"; } +stop_trino_cluster +start_trino_cluster +' +bash -c "$cluster_order_script" bash "$TRINO_FUNCTIONS_FILE" "$TRINO_HOME_LINK" \ + > "$TEST_ROOT/bash-cluster-order" +zsh -f -c "$cluster_order_script" zsh "$TRINO_FUNCTIONS_FILE" "$TRINO_HOME_LINK" \ + > "$TEST_ROOT/zsh-cluster-order" + +cat > "$TEST_ROOT/expected-cluster-order" <<'EOF' +stopping trino on root@worker-1 (worker, remote) +remote root@worker-1 stop +stopping trino on root@worker-2 (worker, remote) +remote root@worker-2 stop +stopping trino on root@coordinator (coordinator, local) +local stop +trino cluster stopped +starting trino on root@coordinator (coordinator, local) +local start +starting trino on root@worker-1 (worker, remote) +remote root@worker-1 start +starting trino on root@worker-2 (worker, remote) +remote root@worker-2 start +trino cluster started +EOF +if ! cmp -s "$TEST_ROOT/bash-cluster-order" "$TEST_ROOT/expected-cluster-order"; then + printf 'the cluster functions act on the wrong nodes under bash\n' >&2 + diff -u "$TEST_ROOT/expected-cluster-order" "$TEST_ROOT/bash-cluster-order" >&2 + exit 1 +fi +if ! cmp -s "$TEST_ROOT/zsh-cluster-order" "$TEST_ROOT/bash-cluster-order"; then + printf 'the cluster functions act on different nodes under zsh than under bash\n' >&2 + diff -u "$TEST_ROOT/bash-cluster-order" "$TEST_ROOT/zsh-cluster-order" >&2 + exit 1 +fi + mock_bin="$TEST_ROOT/mock-bin" mkdir -p "$mock_bin" cat > "$mock_bin/ssh" <<'EOF' From ec69a967f8bea9fcfbe8a974899406e358ba17d8 Mon Sep 17 00:00:00 2001 From: dannygeng Date: Tue, 28 Jul 2026 12:59:49 +0800 Subject: [PATCH 4/6] feat(skills): add Cursor install/uninstall support for pixels-install --- AGENTS.md | 5 +- skills/README.md | 27 ++- skills/install.sh | 49 ++++- skills/pixels-install/cursor/SKILL.md | 184 ++++++++++++++++++ .../pixels-install/scripts/lib/shell_env.sh | 6 +- skills/pixels-install/skill.yaml | 1 + .../tests/shell_compatibility.sh | 12 ++ skills/uninstall.sh | 32 ++- 8 files changed, 301 insertions(+), 15 deletions(-) create mode 100644 skills/pixels-install/cursor/SKILL.md diff --git a/AGENTS.md b/AGENTS.md index 57ff965c3e..86bd50d6bd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,4 +43,7 @@ - For serverless changes, ensure planner/invoker/worker settings remain consistent (input/intermediate/output storage schemes in `pixels-turbo/README.md`). ## Optional skills -- For guided Pixels deployment, install and use the `pixels-install` skill from `skills/pixels-install`. +- For guided Pixels deployment, install and use the `pixels-install` skill from `skills/pixels-install`: + - Cursor: `./skills/install.sh --skill pixels-install --tool cursor --scope project` + - Codex: `./skills/install.sh --skill pixels-install --tool codex --scope project` + - Claude: `./skills/install.sh --skill pixels-install --tool claude --scope project` diff --git a/skills/README.md b/skills/README.md index 6f8114566b..f9b2ff20cb 100644 --- a/skills/README.md +++ b/skills/README.md @@ -10,6 +10,8 @@ Pixels; users explicitly install a skill with `skills/install.sh`. - `skills//`: One independent skill development directory. - `skills//skill.yaml`: Skill metadata, script map, and phase list. - `skills//codex/SKILL.md`: Codex skill entrypoint. +- `skills//cursor/SKILL.md`: Cursor skill entrypoint (falls back to + `codex/SKILL.md` when absent). - `skills//claude/agent.md`: Claude Code agent entrypoint, when supported. - `skills//scripts/`: Runtime scripts bundled with the installed skill. - `skills//scripts/lib/`: Shell helpers sourced by that skill's own scripts. @@ -19,8 +21,8 @@ Pixels; users explicitly install a skill with `skills/install.sh`. ### Commands - `./skills/list.sh`: List skill directories that contain `skill.yaml`. -- `./skills/install.sh --skill --tool --scope `: Install a skill for the selected tool and scope. -- `./skills/uninstall.sh --skill --tool --scope `: Remove an installed skill for the selected tool and scope. +- `./skills/install.sh --skill --tool --scope `: Install a skill for the selected tool and scope. +- `./skills/uninstall.sh --skill --tool --scope `: Remove an installed skill for the selected tool and scope. `--agent ` is accepted by install/uninstall as a deprecated alias for `--skill ` during the migration from the old `agents/` layout. @@ -41,6 +43,23 @@ discovery path: $HOME/.agents/skills// ``` +Cursor project-scope installs copy the selected skill into Cursor's project +skill discovery path: + +```text +/.cursor/skills// +``` + +Cursor global-scope installs copy the selected skill into Cursor's personal +skill discovery path: + +```text +$HOME/.cursor/skills// +``` + +Cursor installs never write into `.cursor/skills-cursor/`, which is reserved +for Cursor's built-in skills. + Claude project/global installs copy the Claude markdown agent to the matching `.claude/agents/` directory and copy scripts/references to a companion assets directory: @@ -58,4 +77,6 @@ installed skill bundle. The pixels-install scripts default to: $HOME/.agents/state/pixels-install/ ``` -Override with `STATE_DIR=` when a different state location is required. +That state location is shared across Codex and Cursor installs of the same +skill so progress/config stay in one place. Override with `STATE_DIR=` +when a different state location is required. diff --git a/skills/install.sh b/skills/install.sh index f21dc07055..b8590a3866 100755 --- a/skills/install.sh +++ b/skills/install.sh @@ -23,12 +23,13 @@ available_skills() { usage() { cat < --tool --scope + $0 --skill --tool --scope Required arguments: --skill Skill directory name under skills/. --agent is accepted as a deprecated alias. - --tool Target AI tool. + --tool + Target AI tool. --scope project: install into this repository. global: install into the current user's tool config. @@ -37,6 +38,8 @@ Examples: $0 --skill pixels-install --tool codex --scope project $0 --skill pixels-install --tool codex --scope global $0 --skill pixels-install --tool claude --scope project + $0 --skill pixels-install --tool cursor --scope project + $0 --skill pixels-install --tool cursor --scope global Available skills: $(available_skills) @@ -107,7 +110,7 @@ parse_args() { done [ -n "$SKILL" ] || ERRORS+=("missing --skill ") - [ -n "$TOOL" ] || ERRORS+=("missing --tool ") + [ -n "$TOOL" ] || ERRORS+=("missing --tool ") [ -n "$SCOPE" ] || ERRORS+=("missing --scope ") if [ -n "$SKILL" ]; then @@ -116,8 +119,8 @@ parse_args() { if [ -n "$TOOL" ]; then case "$TOOL" in - codex|claude) ;; - *) ERRORS+=("--tool must be codex or claude") ;; + codex|claude|cursor) ;; + *) ERRORS+=("--tool must be codex, claude, or cursor") ;; esac fi @@ -180,6 +183,14 @@ codex_target_dir() { fi } +cursor_target_dir() { + if [ "$SCOPE" = "project" ]; then + printf '%s/.cursor/skills/%s\n' "$(project_root)" "$SKILL" + else + printf '%s/.cursor/skills/%s\n' "$HOME" "$SKILL" + fi +} + claude_agent_target() { if [ "$SCOPE" = "project" ]; then printf '%s/.claude/agents/%s.md\n' "$(project_root)" "$SKILL" @@ -212,6 +223,33 @@ install_codex() { echo "Installed Codex skill '$SKILL' to $target_dir" } +install_cursor() { + local source_dir target_dir source_skill + source_dir="$(skill_dir)" + source_skill="$source_dir/cursor/SKILL.md" + target_dir="$(cursor_target_dir)" + + # Prefer a Cursor-specific entrypoint; fall back to the Codex SKILL.md, + # which already uses Cursor-compatible YAML frontmatter. + if [ ! -f "$source_skill" ]; then + source_skill="$source_dir/codex/SKILL.md" + fi + [ -f "$source_skill" ] || fail_with_usage "Cursor SKILL.md not found: $source_dir/cursor/SKILL.md (or codex/SKILL.md)" + + case "$target_dir" in + */.cursor/skills-cursor/*|*/.cursor/skills-cursor) + fail_with_usage "refusing to install into .cursor/skills-cursor (reserved for Cursor built-in skills)" + ;; + esac + + rm -rf "$target_dir" + mkdir -p "$target_dir" + cp "$source_skill" "$target_dir/SKILL.md" + copy_common_assets "$source_dir" "$target_dir" + + echo "Installed Cursor skill '$SKILL' to $target_dir" +} + install_claude() { local source_dir source_agent target_agent target_assets source_dir="$(skill_dir)" @@ -272,4 +310,5 @@ ensure_skill_exists case "$TOOL" in codex) install_codex ;; claude) install_claude ;; + cursor) install_cursor ;; esac diff --git a/skills/pixels-install/cursor/SKILL.md b/skills/pixels-install/cursor/SKILL.md new file mode 100644 index 0000000000..fcb4de4c35 --- /dev/null +++ b/skills/pixels-install/cursor/SKILL.md @@ -0,0 +1,184 @@ +--- +name: pixels-install +description: Guide, execute, and verify a basic Pixels deployment from docs/INSTALL.md using bundled helper scripts. Use when the user asks to install, configure, resume, validate, or troubleshoot a basic Pixels deployment or optional Trino setup. +--- + +# pixels-install + +Help the user install a basic Pixels deployment according to `docs/INSTALL.md`. + +Treat `docs/INSTALL.md` as the source of truth for the installation flow. Use the bundled scripts only as helpers for deterministic, repeatable, or easy-to-misconfigure steps. Do not treat this skill as a one-shot unattended installer. + +## Resource Paths + +- Skill directory: use the directory containing this `SKILL.md`. +- Helper scripts: `scripts/`. +- Shared helper scripts: `shared-scripts/`. +- Runtime state: `STATE_DIR` when set; otherwise the scripts resolve it to `/.agents/state/pixels-install` for project installs (from either `.cursor/skills/` or `.agents/skills/`) or `$HOME/.agents/state/pixels-install` for global installs. +- Pixels source tree: `REPO_ROOT` when set; otherwise scripts try to discover the current Git repository root and fail with a clear message if they cannot. + +### Helper scripts + +Use these helpers when they directly fit the current environment and the user-approved goal: + +- `prepare_deployment.sh`: collect and confirm the Pixels deployment topology before any install writes to disk. With no node arguments it prompts for single-node vs. cluster, `PIXELS_HOME`, the coordinator node, whether the coordinator also runs a worker, and worker nodes; with arguments, use `--coordinator ` plus repeated `--worker <...>` (legacy `--node` still works). Writes `STATE_DIR/deployment.env` and optionally delegates SSH setup to `shared-scripts/setup_cluster.sh`. In non-interactive mode, set `CONFIRM_PIXELS_DEPLOYMENT=true` only after the user has reviewed the chosen topology and paths. +- `check_prerequisites.sh`: validate OS, architecture, memory, disk, ports, host resolution, privilege, and optional SSH reachability. Runs every check and exits with a structured `=== check_prerequisites result ===` summary (one `ok|warn|fail|skip : ` line per check, plus a final `summary: ok=N warn=N fail=N skip=N status=pass|fail` line) instead of stopping at the first failure, so a single run shows every problem at once. +- `install_jdk.sh`: install a Zulu OpenJDK build (default JDK 23) matching the server's CPU architecture (x86_64 or aarch64) under `~/opt`, and persist `JAVA_HOME` only into the current user's shell profile. Looks for a JDK that already satisfies `JDK_VERSION` first — checking `JAVA_HOME` if set, otherwise resolving whatever `java` is on `PATH` (e.g. an `apt`-installed JDK that never exported `JAVA_HOME`) — and if one is found, skips the download entirely and only fixes up the environment variable to point at it. Only downloads/installs a fresh Zulu build when nothing on the system satisfies the version requirement. Falls back to pointing at the manual `.deb` method in `docs/INSTALL.md` if the Azul metadata API lookup fails. +- `install_maven.sh`: install or validate Maven 3.8+ (default pinned version `3.9.8`; override with `MAVEN_VERSION` only if the user explicitly asks for a different one) when the current Maven is missing or incompatible with the selected JDK. Same "skip if already satisfied" logic as `install_jdk.sh`: if an existing `mvn` (from `apt`, a prior manual install, etc.) already meets the minimum version, it reuses that installation's home directory instead of downloading anything. Fresh installs go into `~/opt/apache-maven-` with a `~/opt/maven` symlink, never into `/opt`. +- `install_mysql.sh`: install MySQL, then interactively confirm the root password and the `pixels` metadata-DB-user password (default `password` for both, never applied silently), create the `pixels_metadata` database/user, and load `scripts/sql/metadata_schema.sql` from the Pixels source tree. It validates DB/user identifiers, quotes SQL values, avoids putting the root password in command-line arguments, and writes shell-quoted `STATE_DIR/deployment.secrets.env` (mode 600) so the confirmed credentials flow into `configure_pixels.sh` automatically. +- `install_etcd.sh`: install and optionally start the bundled etcd 3.3.4 package under `~/opt`. Always fixes the shipped `conf.yml`'s hardcoded `/home/ubuntu/...` `data-dir` to the real install path. By default (`ETCD_ALLOW_REMOTE=false`) it keeps etcd localhost-only. For a cluster, set `ETCD_ALLOW_REMOTE=true` only after confirming private networking/security-group rules; the script then requires `CONFIRM_ETCD_REMOTE_ACCESS=true`, `ASSUME_YES=true`, or an interactive confirmation before binding the client/peer listeners for remote access. Can also install and enable a `systemd` unit so etcd survives reboots and restarts on failure, but only after asking — it never installs this silently. Leave `INSTALL_ETCD_SYSTEMD_SERVICE` unset to be prompted interactively (`[y/N]`, defaults to no); set it to `true`/`false` to answer non-interactively once the user has actually agreed, or `ASSUME_YES=true` to answer yes to this and other yes/no prompts. Declining (or running non-interactively with no explicit answer) just skips the unit — etcd still starts, as a manually backgrounded process instead. Falls back the same way when `systemctl` isn't available at all. Restarts/re-enables the installed service automatically when the config actually changed. +- `build_install_pixels.sh`: build and install Pixels into the confirmed `PIXELS_HOME` from `deployment.env`, then add the MySQL JDBC connector. It prints the repo, deployment file, and install target before running; in non-interactive mode, set `CONFIRM_PIXELS_INSTALL=true` only after the user confirms the target. If you must bypass the prepared deployment file, pass `USE_DEPLOYMENT_FILE=false PIXELS_HOME=` explicitly. `AUTO_CONFIRM_INSTALL` (default `true`) auto-answers `install.sh`'s prompts only when that is safe; if `PIXELS_HOME/etc/pixels.properties` or `pixels-cpp.properties` already exist, it forces the interactive path instead, because `install.sh` would otherwise prompt to add/remove config options or overwrite `pixels-cpp.properties` outright — exactly what `Claude agent`/`SKILL.md` say not to do without the user's say-so. +- `configure_pixels.sh`: update `PIXELS_HOME/etc/pixels.properties` after database, etcd, host, port, topology, and path values are known. Automatically sources `deployment.env` and `deployment.secrets.env` (if present), so `PIXELS_HOME`, coordinator service hosts, worker names, and MySQL credentials stay in sync with what the earlier scripts confirmed. It writes `$PIXELS_HOME/etc/workers` from `PIXELS_WORKERS` by default. It fails instead of silently writing the documented default DB password unless `METADATA_DB_PASSWORD` is explicit or `ALLOW_DEFAULT_METADATA_PASSWORD=true` is set. +- `install_shell_helpers.sh`: optional convenience step that **asks first** before writing shell functions. By default it installs only `start_pixels`/`stop_pixels`/`restart_pixels` on the Pixels coordinator recorded in `deployment.env`; when invoked from another host, it copies a minimal installer over SSH and writes the coordinator user's `~/.pixels-shell-helpers.sh` plus shell-profile source line there. These wrap `$PIXELS_HOME/sbin/start-pixels.sh` and `$PIXELS_HOME/sbin/stop-pixels.sh`. Set `PIXELS_SHELL_HELPERS_TARGET=local` only when the user explicitly wants the functions on the current host. Leave `INSTALL_PIXELS_SHELL_HELPERS` unset to be prompted `[y/N]`; set `true`/`false` to answer non-interactively once the user has actually agreed, or `ASSUME_YES=true`. +- `smoke_test.sh`: verify the installed layout, configuration, Java (`CHECK_JAVA=true`), metadata access, etcd health, Pixels topology (`deployment.env` plus `$PIXELS_HOME/etc/workers`), core service ports (`CHECK_CORE_SERVICES=true`), `$PIXELS_HOME/logs` error patterns (`CHECK_PIXELS_LOGS=true`), and basic CLI behavior when applicable. It also checks Trino layout/catalog files when `CHECK_TRINO=true` and `trino-deployment.env` is present, can separately validate the lightweight Trino-side Pixels client config plus shell-profile `PIXELS_HOME`/`PIXELS_CONFIG` exports (`CHECK_TRINO_PIXELS_CLIENT=true`) without requiring a full Pixels runtime layout (`CHECK_PIXELS_LAYOUT=false`), can check Trino launcher status across the recorded cluster (`CHECK_TRINO_CLUSTER_STATUS=true`), checks that Trino logs did not fall back to classpath `pixels.properties` (`CHECK_TRINO_LOGS=true`), waits for `/v1/info` to report `starting=false` (`CHECK_TRINO_READY=true`), and can run `trino --catalog pixels --execute "SHOW SCHEMAS"` (`CHECK_TRINO_CLI=true`). `SHOW SCHEMAS` only needs to succeed; an empty Pixels catalog is expected before data is loaded. Same structured-summary behavior as `check_prerequisites.sh` above. +- `progress.sh`: records which of `skill.yaml`'s `phases` have actually completed, in `STATE_DIR/progress.env`. `progress.sh mark [note]` after a phase succeeds, `progress.sh show` to see what's already done, `progress.sh is-done ` to check one phase, `progress.sh reset` to start over. Exists so a session interrupted partway through a multi-phase install can resume from the actual state instead of guessing or re-running completed steps. +- `build_pixels_trino_artifacts.sh`: optional Trino-prep step. Builds `pixelsdb/pixels-trino` once on the local machine where Pixels modules have already been `mvn install`ed, copies `pixels-trino-connector-*.zip` and `pixels-trino-listener-*.zip` into `STATE_DIR/pixels-trino-artifacts/`, and writes `STATE_DIR/pixels-trino-artifacts.env` for `install_trino_cluster.sh`. In non-interactive mode, set `CONFIRM_PIXELS_TRINO_ARTIFACTS=true` only after the user confirms the checkout/artifact paths. +- `install_trino_cluster.sh`: run **on the coordinator** after `prepare_trino_cluster.sh`. It prints and confirms the Trino deployment file, version, install parent, home link, data dir, coordinator, coordinator scheduling mode, workers, Trino-side Pixels client home/config source, Pixels service endpoints, and any prebuilt pixels-trino plugin zips before installing; in non-interactive mode, set `CONFIRM_TRINO_CLUSTER_INSTALL=true` only after the user confirms the fan-out. Installs Trino locally for the coordinator's own role, then dispatches `install_trino.sh` on every worker **in parallel** over the coordinator -> worker passwordless SSH that `prepare_trino_cluster.sh` already set up, instead of requiring the user or skill to copy the same command and run it by hand once per node. Copies the current `STATE_DIR/trino-deployment.env` to each worker first (workers don't need a shared filesystem). If `PIXELS_TRINO_CONNECTOR_ZIP` and `PIXELS_TRINO_LISTENER_ZIP` are provided, or `STATE_DIR/pixels-trino-artifacts.env` exists from `build_pixels_trino_artifacts.sh`, it also copies a minimal installer plus those prebuilt plugin zips to each worker, so workers do not need a Pixels checkout or Maven build. It also copies the selected Trino-side Pixels client config (`TRINO_PIXELS_CONFIG_SOURCE`, defaulting to `$PIXELS_HOME/etc/pixels.properties` when available) and lets `install_trino.sh` rewrite service endpoints through `TRINO_PIXELS_*` variables. Remote Trino nodes receive Trino, the two pixels-trino plugin zips, Trino catalog/listener configs, and a lightweight `~/opt/pixels/etc/pixels.properties` client config — not a full Pixels installation. Emits the same structured per-node summary as `smoke_test.sh`/`check_prerequisites.sh`, with a log file per node under `STATE_DIR/logs/` for diagnosing any failure. +- `prepare_trino_cluster.sh`: collects and confirms the Trino topology and install locations: one coordinator, zero or more workers, whether the coordinator also accepts scheduled work, Trino version, HTTP port, install parent, home symlink/current path, and data directory. With no node arguments it prompts interactively; with arguments, use `--coordinator ` plus repeated `--worker <...>`. Writes `STATE_DIR/trino-deployment.env`. Optionally delegates to `shared-scripts/setup_cluster.sh` (`--setup-ssh true`) so the coordinator and every worker can passwordlessly SSH into each other, which the cluster shell helpers below depend on. In non-interactive mode, set `CONFIRM_TRINO_DEPLOYMENT=true` only after the user has reviewed the topology and paths. +- `install_trino.sh`: installs Trino (per the [466 deployment docs](https://trino.io/docs/466/installation/deployment.html)) on the **current node only** — run it once per node in the cluster, it does not SSH anywhere itself (use `install_trino_cluster.sh` to fan it out across the cluster instead of running it by hand on each node). It reads `trino-deployment.env`, validates this node's role (`coordinator` or `worker`) and the coordinator host, and confirms the role plus install/data paths before writing files; in non-interactive mode, set `CONFIRM_TRINO_INSTALL=true` only after the user confirms the target. Downloads `trino-server-.tar.gz` (default 466) from Maven Central into the confirmed install parent with the confirmed home symlink for version-switch-friendly installs. Writes `etc/node.properties` (`node.data-dir` under the confirmed data dir, outside the versioned install dir so it survives version switches; `node.id` generated once and preserved across re-runs), `etc/jvm.config` (official defaults plus the two `--add-opens` flags `pixels-trino` requires), `etc/log.properties`, and `etc/config.properties` — `discovery.uri` always points at the coordinator's real IP from `trino-deployment.env`, never `localhost`; this node's coordinator/worker role is read from `trino-deployment.env` or inferred from this host's own IPs, overridable with `TRINO_ROLE`. For `pixelsdb/pixels-trino`, either clones/builds locally (requires Pixels already `mvn install`ed locally and JDK 23) or consumes prebuilt `PIXELS_TRINO_CONNECTOR_ZIP` and `PIXELS_TRINO_LISTENER_ZIP` so remote Trino nodes do not need a Pixels checkout or Maven build. Installs both documented plugins by directly unzipping their zips under `plugin/` and preserving the zip's versioned top-level directory names, e.g. `pixels-trino-connector-0.2.0-SNAPSHOT` and `pixels-trino-listener-0.2.0-SNAPSHOT`; do not rename them to unversioned stable directories. Writes Trino's catalog file `etc/catalog/pixels.properties` with only catalog properties (`connector.name=pixels`, `cloud.function.switch=off`, `clean.intermediate.result=true` by default). This catalog file is **not** the same thing as Pixels' client/runtime config file `$PIXELS_HOME/etc/pixels.properties`; never copy Pixels runtime properties such as `metadata.server.host` or `etcd.hosts` into the Trino catalog. Also prepares the Trino-side lightweight Pixels client home (`TRINO_PIXELS_HOME`, default `$HOME/opt/pixels`, not `$HOME/opt/pixels/etc`) with `etc/pixels.properties` and `logs/`, then writes `export PIXELS_HOME=...` and `export PIXELS_CONFIG=$PIXELS_HOME/etc/pixels.properties` directly into the current user's bash/zsh profile (and mirrors them into `STATE_DIR/toolchain.env` for later helper-script processes). It does **not** create `~/.pixels-trino-env.sh`. Use `TRINO_PIXELS_CONFIG_SOURCE` to copy a known-good Pixels config into that client home, and use `TRINO_PIXELS_METADATA_SERVER_HOST`, `TRINO_PIXELS_TRANS_SERVER_HOST`, `TRINO_PIXELS_QUERY_SCHEDULE_SERVER_HOST`, and `TRINO_PIXELS_ETCD_HOSTS` to rewrite endpoints so Trino nodes do not fall back to `localhost`. Writes `etc/event-listener.properties` for the listener and defaults its log directory to a Trino-side path under the install parent unless `PIXELS_TRINO_LISTENER_LOG_DIR` is set. Best-effort installs `trino-cli` into `bin/trino`. +- `install_trino_shell_helpers.sh`: optional, and **always asks first**, because it bakes a specific list of remote hostnames (from `trino-deployment.env`) into a shell profile and assumes passwordless SSH between them is already set up. By default it installs only `start_trino_cluster`/`stop_trino_cluster`/`restart_trino_cluster`/`trino_cli` on the Trino coordinator recorded in `trino-deployment.env`; when invoked from another host, it copies a minimal installer over SSH and writes the coordinator user's `~/.trino-shell-helpers.sh` plus shell-profile source line there. In the generated functions, the coordinator is always operated locally and every worker over SSH. `stop_trino_cluster` stops workers before the coordinator; `restart_trino_cluster` runs stop then start. The generated start/stop/restart/client helpers explicitly export `PIXELS_HOME` and `PIXELS_CONFIG` before invoking Trino locally or over SSH, without wrapping or replacing Trino's `bin/launcher`. Set `TRINO_SHELL_HELPERS_TARGET=local` only when the user explicitly wants the functions on the current host. Leave `INSTALL_TRINO_SHELL_HELPERS` unset to be prompted `[y/N]`; set `true`/`false` to answer non-interactively once the user has actually agreed, or `ASSUME_YES=true`. + +All executable entrypoints in this skill are Bash programs. Invoke them directly +so their `#!/usr/bin/env bash` shebang is honored; do not run them with `zsh` +or `source` them into the current shell. Bash must be installed on every +deployment node even when that account's login shell is Zsh. Each entrypoint +validates the deployment account's configured login shell from passwd/NSS and +accepts only Bash or Zsh. Profile exports go to `.bashrc` or `.zshrc` +accordingly, while the generated Pixels and Trino helper files are checked in +both Bash and Zsh syntax before they are sourced. + +All of these scripts anchor paths on `$HOME` (e.g. `~/opt/...`) and detect the +current account's login shell from passwd/NSS to pick `~/.bashrc` or `~/.zshrc` before +persisting any `export`. Never assume the deployment user is named `pixels` +or `ubuntu`, and never write environment variables into a global file such +as `/etc/environment` — `scripts/lib/shell_env.sh` +centralizes this logic and is shared by `install_jdk.sh`, `install_maven.sh`, +`install_etcd.sh`, `build_install_pixels.sh`, `configure_pixels.sh`, +`check_prerequisites.sh`, `smoke_test.sh`, `install_trino.sh`, +`install_trino_cluster.sh`, `prepare_trino_cluster.sh`, +`build_pixels_trino_artifacts.sh`, +`install_trino_shell_helpers.sh`, and `progress.sh`. It also defines the +structured-result (`result_record`/`result_emit_summary`) and phase-checkpoint +(`mark_phase_done`/`phase_is_done`/`print_progress`) helpers described above. + +Each `install_*.sh` script runs as its own process, so exporting a variable +in one script does not make it visible to the next one. `docs/INSTALL.md`'s +own walkthrough handles this by running `source ~/.bashrc` right after +appending to it, but sourcing the user's real rc file from a *non*-interactive +script is unreliable (Ubuntu's default `~/.bashrc` returns immediately for +non-interactive shells, before reaching anything appended at the bottom). So +in addition to persisting into the user's real shell profile, +`shell_env.sh` also mirrors `JAVA_HOME`/`MAVEN_HOME`/`ETCD`/`PIXELS_HOME`/`PIXELS_CONFIG` +into a small `STATE_DIR/toolchain.env` file with no such guard, +and every script sources it (`load_toolchain_env`) at startup. This means +running `install_jdk.sh` then `install_maven.sh` as two separate tool calls +still works without the skill needing to manually re-source anything in +between — but if you set any of these variables yourself before running a +step, your explicit environment variable still wins. + +The skill may run simple commands from `docs/INSTALL.md` directly when that is clearer than adding another wrapper script. + +### Workflow + +The phase sequence is fixed in `skill.yaml`'s `phases` list — this section is decision guidance for each phase, not a second copy of that ordering. Before doing anything else, run `scripts/progress.sh show` to see which phases (if any) already completed in a prior session, and skip straight to the first phase that is not yet marked done instead of re-deriving state from scratch or blindly redoing finished work. After each phase actually succeeds, run `scripts/progress.sh mark ` (phase names match `skill.yaml`) so a later session — or this one, after a long gap — can pick up correctly. + +- **discover_install_target**: clarify single node vs. cluster, `PIXELS_HOME`, coordinator vs. worker roles, storage/query engine needs, whether to start services now, and whether optional components are in scope. Determine the actual login user (`whoami`) and home directory (`$HOME`) instead of assuming `pixels` or `ubuntu`; every path this skill writes is anchored on that `$HOME`. Do not proceed with defaults until the user has confirmed the topology and install locations. +- **check_prerequisites**: run `check_prerequisites.sh` and read its full structured summary before deciding what to install — see "Failure Handling" below for what to do with anything it reports as `fail`. +- **prepare_deployment_config**: run for both single-node and cluster deployments so later scripts read the same confirmed `PIXELS_HOME`, coordinator service hosts, and worker list from `deployment.env`. Prefer `prepare_deployment.sh` with no node arguments for an interactive prompt, or explicit `--coordinator ... --worker ...` arguments when the user has already provided the topology. +- **install_or_verify_jdk**: `install_jdk.sh` already skips the download when anything on the system (apt-installed JDK, a previous run, etc.) satisfies `JDK_VERSION`; it only selects and installs a Zulu build under `~/opt` when nothing qualifies. JDK 23+ is required for the documented Pixels + Trino 466 path. Ask before installing a downloaded JDK package. +- **install_or_verify_maven**: `install_maven.sh` has the same "skip if already satisfied" behavior for Maven 3.8+ (default pinned `3.9.8`, only overridden if the user asks). Ensure `mvn -v` ends up using the selected JDK. +- **install_or_verify_mysql** / **install_or_verify_etcd**: only after the user confirms credentials/ports (see Guardrails — never apply the default MySQL password silently); `install_etcd.sh` keeps localhost-only by default, asks before remote etcd access, and asks before installing its `systemd` auto-start unit. +- **build_install_pixels**: install Pixels from the repository into the confirmed `PIXELS_HOME` from `deployment.env` and place the MySQL JDBC connector under `PIXELS_HOME/lib`. In a Pixels cluster, worker nodes need this full installed `PIXELS_HOME` runtime layout to run `$PIXELS_HOME/bin/start-daemon.sh worker`, but they do **not** need to rebuild Pixels locally; build/install once, then distribute the installed `PIXELS_HOME` to worker nodes (for example with `$PIXELS_HOME/sbin/rsync-cluster.sh` after `etc/workers` is correct). +- **configure_pixels**: update `pixels.properties` only once paths, hosts, ports, database, etcd, topology, and cache settings are confirmed; it derives `pixels.var.dir` from the resolved `PIXELS_HOME`, writes `$PIXELS_HOME/etc/workers` from `PIXELS_WORKERS`, and reuses the MySQL credentials `install_mysql.sh` already confirmed. If MySQL was configured elsewhere, pass `METADATA_DB_PASSWORD` explicitly. +- **start_pixels_optional**: only when the user asks to start services or startup is part of the requested task. +- **install_shell_helpers_optional**: only if the user wants convenience functions. Install Pixels helpers on the Pixels coordinator and Trino helpers on the Trino coordinator; do not install them on the agent's current host unless that host is the corresponding coordinator or the user explicitly asks for a local helper install. Both helper scripts ask by default before writing to the target user's shell profile. +- **smoke_test**: run after configuration or startup changes; read the full structured summary (see "Failure Handling" below), don't just check the exit code. For a final deployment check, enable the relevant checks from the actual topology, e.g. `CHECK_PIXELS_LOGS=true CHECK_TRINO=true CHECK_TRINO_CLUSTER_STATUS=true CHECK_TRINO_CLI=true`. `SHOW SCHEMAS` success is enough before data is loaded; do not require returned schemas to be non-empty. +- **optional_trino_install**: only when the user explicitly asks. Clarify the topology first: coordinator vs. worker(s), how many workers, whether the coordinator also accepts scheduled work (`node-scheduler.include-coordinator`; official docs recommend `false` for a dedicated coordinator, `true` for a single-node test setup), `TRINO_INSTALL_PARENT`, `TRINO_HOME_LINK`, and `TRINO_DATA_DIR`. Never infer a fixed Trino cluster shape; use the user's topology or `prepare_trino_cluster.sh`'s confirmed output. For a Trino cluster whose remote nodes should not build Pixels, run `build_pixels_trino_artifacts.sh` once where Pixels has already been `mvn install`ed, or pass `PIXELS_TRINO_CONNECTOR_ZIP` and `PIXELS_TRINO_LISTENER_ZIP` explicitly to `install_trino_cluster.sh`; the remote nodes should only receive Trino plus those two plugins, Trino catalog/listener configs, and the lightweight Trino-side Pixels client config under `TRINO_PIXELS_HOME` (default `~/opt/pixels`). They must not be required to receive the full Pixels install, Maven, or source checkout. Run `prepare_trino_cluster.sh --setup-ssh true`, then run `install_trino_cluster.sh` **on the coordinator** to install Trino on the coordinator and fan out to every worker in parallel over SSH, instead of repeating `install_trino.sh` by hand once per node. Configure Trino's `etc/catalog/pixels.properties` as a Trino catalog file only; it is not `$PIXELS_HOME/etc/pixels.properties`. For the Trino-side Pixels client config, set `PIXELS_HOME`/`TRINO_PIXELS_HOME` to the parent directory (e.g. `~/opt/pixels`, never `~/opt/pixels/etc`) and set/export `PIXELS_CONFIG=$PIXELS_HOME/etc/pixels.properties`; all Trino nodes need this because the pixels-trino connector initializes on workers as well as the coordinator. `install_trino.sh` writes those two exports directly to the user's bash/zsh profile. When the skill itself starts, stops, checks, or runs CLI against Trino from a non-interactive SSH command, inline `PIXELS_HOME` and `PIXELS_CONFIG` in that command because remote non-interactive shells may not read `.bashrc`/`.zshrc`. Ensure `metadata.server.host`, `trans.server.host`, `query.schedule.server.host`, and `etcd.hosts` are reachable from every Trino node and are not accidental `localhost` values in a separated topology. Configure `presto.pixels.jdbc.url` only once the coordinator's endpoint and catalog/schema values are known. Offer `install_trino_shell_helpers.sh` only after the user agrees (it asks by default anyway). +- **optional_hadoop_or_monitoring**: out of scope unless the user asks — see Optional Components below. + +### Failure handling + +`check_prerequisites.sh` and `smoke_test.sh` never stop at the first failed check — they run every check and print a structured summary block (`===