From 4f85d9434985fedeba08030c5d95e959056234cf Mon Sep 17 00:00:00 2001 From: yessjun Date: Mon, 10 Aug 2026 20:01:14 +0900 Subject: [PATCH 1/2] fix: correct the failure paths of the live hypervisor reads The usage series and the system panel's live node block each turned a recoverable refusal into the wrong answer. An unconfigured API token makes the client refuse before the request leaves, and only the transport exception was caught, so a state the configuration explicitly permits produced a 500 instead of the designed 503. On the node block, the storage probe shared a try with the status probe, so losing the storage read right alone discarded a successful status read and reported the node as not answering; the two probes are now separate answers and a null status envelope reads as unreachable rather than dereferencing past the catch. A node the operator marked OFFLINE is no longer probed at all, matching the status poller, and keeps its row so the node count stays whole. RRD rows without a timestamp are dropped where the series is mapped, since the contract makes that field required and a point with no place on the axis cannot be charted. The three reads also no longer run inside a read-only transaction. Each held a pooled database connection for the whole Proxmox call, up to the client read timeout, so a stalled pveproxy plus a refreshing dashboard could drain the pool and take unrelated endpoints down with it. The boundary is safe because nothing here needs one transaction: every access lookup, repository call and collaborator carries its own, the summary's tiles are independent counters, and the entities read afterwards are touched on basic columns only, so no lazy association is left outside a transaction. --- .../pickle/admin/AdminNodeMetricsService.java | 14 +- .../pickle/admin/AdminSummaryService.java | 62 +++++++-- .../pickle/admin/dto/NodeLiveResponse.java | 9 ++ .../admin/dto/NodeMetricPointResponse.java | 24 ++-- .../kr/ac/pusan/pickle/inventory/Node.java | 5 +- .../kr/ac/pusan/pickle/proxmox/RrdValues.java | 18 +++ .../ac/pusan/pickle/vm/VmMetricsService.java | 15 ++- .../pickle/vm/dto/VmMetricPointResponse.java | 20 +-- .../admin/AdminNodeMetricsServiceTest.java | 122 ++++++++++++++++++ .../admin/AdminSystemSummaryNodeLiveTest.java | 113 ++++++++++++++++ .../pusan/pickle/vm/VmMetricsServiceTest.java | 48 ++++++- 11 files changed, 409 insertions(+), 41 deletions(-) create mode 100644 src/main/java/kr/ac/pusan/pickle/proxmox/RrdValues.java create mode 100644 src/test/java/kr/ac/pusan/pickle/admin/AdminNodeMetricsServiceTest.java diff --git a/src/main/java/kr/ac/pusan/pickle/admin/AdminNodeMetricsService.java b/src/main/java/kr/ac/pusan/pickle/admin/AdminNodeMetricsService.java index 1350ea4..130f541 100644 --- a/src/main/java/kr/ac/pusan/pickle/admin/AdminNodeMetricsService.java +++ b/src/main/java/kr/ac/pusan/pickle/admin/AdminNodeMetricsService.java @@ -15,13 +15,18 @@ import org.slf4j.LoggerFactory; import org.springframework.http.HttpStatus; import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; /** * Usage time series of one node (contract op {@code getAdminNodeMetrics}), * read live from the hypervisor's RRD store like the per-VM series. Unlike a * VM, a node has no "not provisioned yet" state: a node row exists because the * host exists, so a host that cannot be asked is an outage and answers 503. + * + *

Deliberately not {@code @Transactional}, for the same reason as the per-VM + * series: the hypervisor read can run to the client read timeout and must not + * hold a pooled database connection while it does. The one database read here + * carries its own transaction and only basic columns of the node are read + * afterwards. */ @Service public class AdminNodeMetricsService { @@ -39,7 +44,6 @@ public AdminNodeMetricsService(NodeRepository nodeRepository, ProxmoxClient prox this.clock = clock; } - @Transactional(readOnly = true) public NodeMetricsResponse metrics(long nodeId, RrdTimeframe timeframe) { Node node = nodeRepository.findById(nodeId).orElseThrow(() -> new ApiException( HttpStatus.NOT_FOUND, ErrorCodes.RESOURCE_NOT_FOUND, "리소스를 찾을 수 없습니다", @@ -48,9 +52,13 @@ public NodeMetricsResponse metrics(long nodeId, RrdTimeframe timeframe) { try { points = proxmoxClient.nodeRrdData(node.getApiHost(), node.getName(), timeframe) .stream() + // A row with no timestamp has no place on the axis. + .filter(sample -> sample.time() != null) .map(NodeMetricPointResponse::from) .toList(); - } catch (ProxmoxApiException e) { + } catch (ProxmoxApiException | IllegalStateException e) { + // An unconfigured API token refuses before the request leaves, which + // is the same "PVE cannot be asked" as a transport failure. log.warn("Node {} usage read failed: {}", node.getName(), e.getMessage()); throw new ApiException(HttpStatus.SERVICE_UNAVAILABLE, ErrorCodes.METRICS_UNAVAILABLE, "사용량 데이터를 불러오지 못했습니다", "잠시 후 다시 시도해 주세요."); diff --git a/src/main/java/kr/ac/pusan/pickle/admin/AdminSummaryService.java b/src/main/java/kr/ac/pusan/pickle/admin/AdminSummaryService.java index 8098ab6..e2a0538 100644 --- a/src/main/java/kr/ac/pusan/pickle/admin/AdminSummaryService.java +++ b/src/main/java/kr/ac/pusan/pickle/admin/AdminSummaryService.java @@ -24,6 +24,7 @@ import kr.ac.pusan.pickle.config.ClockConfig; import kr.ac.pusan.pickle.inventory.Node; import kr.ac.pusan.pickle.inventory.NodeRepository; +import kr.ac.pusan.pickle.inventory.NodeStatus; import kr.ac.pusan.pickle.ipam.IpPoolRepository; import kr.ac.pusan.pickle.ipam.IpamService; import kr.ac.pusan.pickle.orgs.OrgRepository; @@ -158,7 +159,16 @@ and status not in ('DELETED', 'DELETING') published, expiring30d, attention); } - @Transactional(readOnly = true) + /** + * Platform panel. Deliberately not {@code @Transactional}: it ends in a + * live hypervisor probe per node, and a shared transaction would pin a + * pooled database connection for the whole of that — long enough, with a + * stalled pveproxy, for a refreshing dashboard to drain the pool and take + * unrelated endpoints down with it. Nothing here needs one transaction: + * every tile is an independent counter, each query and each collaborator + * carries its own read transaction, and the entities read afterwards are + * touched on basic columns only. + */ public SystemDashboardSummaryResponse systemSummary() { List nodes = adminNodeQueryService.listNodes().stream() .map(AdminSummaryService::toNodeRatio) @@ -212,24 +222,56 @@ select count(*) from vms v * show. Both refusals the client can raise before a reply are caught for * that reason — the HTTP/transport failure and the unconfigured-token * refusal, which is the same "cannot ask PVE" from the operator's side. + * + *

The two probes are separate answers. Status decides reachability; + * storage is a second call behind a second right ({@code Datastore.Audit}), + * so losing it leaves the two storage fields null on a node that is still + * reachable rather than blanking a tile that did answer. + * + *

An OFFLINE node is not probed at all — the operator has already said + * the host is down, and asking it only buys a timeout per dashboard load + * (the status poller skips OFFLINE for the same reason). It still gets a + * row, so the node count stays whole, and that row is + * {@code reachable: false}: no live numbers are known either way, and the + * node's own status is already on the panel beside it in the ratio list, + * which is where "the operator parked it" is said. */ private List nodesLive() { List live = new ArrayList<>(); for (Node node : nodeRepository.findAll(org.springframework.data.domain.Sort.by("id"))) { + if (node.getStatus() == NodeStatus.OFFLINE) { + live.add(NodeLiveResponse.unreachable(node.getId(), node.getName())); + continue; + } + NodeStatusInfo status; try { - NodeStatusInfo status = proxmoxClient.nodeStatus(node.getApiHost(), node.getName()); - NodeStorageStatus storage = guestStorage(node); - live.add(new NodeLiveResponse(node.getId(), node.getName(), true, - status.memory() == null ? null : status.memory().total(), - status.memory() == null ? null : status.memory().used(), - status.cpu(), - storage == null ? null : storage.total(), - storage == null ? null : storage.used(), - clock.instant())); + status = proxmoxClient.nodeStatus(node.getApiHost(), node.getName()); } catch (ProxmoxApiException | IllegalStateException e) { log.warn("Node {} live probe failed: {}", node.getName(), e.getMessage()); live.add(NodeLiveResponse.unreachable(node.getId(), node.getName())); + continue; + } + // The client hands back whatever the PVE envelope held, so a 200 + // with no data is possible and is not a reachable answer. + if (status == null) { + log.warn("Node {} live probe returned an empty status payload", node.getName()); + live.add(NodeLiveResponse.unreachable(node.getId(), node.getName())); + continue; + } + NodeStorageStatus storage = null; + try { + storage = guestStorage(node); + } catch (ProxmoxApiException | IllegalStateException e) { + log.warn("Node {} guest-storage read failed, tile keeps the status half: {}", + node.getName(), e.getMessage()); } + live.add(new NodeLiveResponse(node.getId(), node.getName(), true, + status.memory() == null ? null : status.memory().total(), + status.memory() == null ? null : status.memory().used(), + status.cpu(), + storage == null ? null : storage.total(), + storage == null ? null : storage.used(), + clock.instant())); } return live; } diff --git a/src/main/java/kr/ac/pusan/pickle/admin/dto/NodeLiveResponse.java b/src/main/java/kr/ac/pusan/pickle/admin/dto/NodeLiveResponse.java index 8b25bec..142783d 100644 --- a/src/main/java/kr/ac/pusan/pickle/admin/dto/NodeLiveResponse.java +++ b/src/main/java/kr/ac/pusan/pickle/admin/dto/NodeLiveResponse.java @@ -9,6 +9,15 @@ * right now, beside the allocation ratios the database already knows. Every * measurement is nullable because the answer to "the host is not answering" is * this row with {@code reachable} false — not a missing row and not an error. + * + *

{@code reachable} false also covers a node the platform chose not to ask: + * an OFFLINE node is skipped rather than waited on, and the answer is the same + * one either way — no live measurement is known. Why it is unknown is on the + * panel already, in the node's own status beside these numbers. + * + *

The measurements come from two independent hypervisor calls, so a + * reachable node can still carry null storage: the storage half needs a right + * the status half does not. */ public record NodeLiveResponse( long nodeId, diff --git a/src/main/java/kr/ac/pusan/pickle/admin/dto/NodeMetricPointResponse.java b/src/main/java/kr/ac/pusan/pickle/admin/dto/NodeMetricPointResponse.java index 370026f..d43218f 100644 --- a/src/main/java/kr/ac/pusan/pickle/admin/dto/NodeMetricPointResponse.java +++ b/src/main/java/kr/ac/pusan/pickle/admin/dto/NodeMetricPointResponse.java @@ -2,6 +2,7 @@ import io.swagger.v3.oas.annotations.media.Schema; import java.time.Instant; +import kr.ac.pusan.pickle.proxmox.RrdValues; import kr.ac.pusan.pickle.proxmox.dto.NodeRrdSample; import org.jspecify.annotations.Nullable; @@ -9,6 +10,10 @@ * Contract schema {@code NodeMetricPoint}. Nullable throughout for the same * reason as the VM series: an RRD row omits the keys it has no data for, and a * gap must stay a gap. + * + *

{@code time} is the exception for the same reason it is on the VM series: + * it is required by the contract and a point that cannot be placed on the axis + * cannot be charted, so a timeless row is dropped where the series is mapped. */ public record NodeMetricPointResponse( Instant time, @@ -27,23 +32,20 @@ public record NodeMetricPointResponse( @Nullable Double netinBps, @Nullable Double netoutBps) { + /** Callers drop timeless rows first (see the class note); everything else may be null. */ public static NodeMetricPointResponse from(NodeRrdSample sample) { return new NodeMetricPointResponse( - sample.time() == null ? null : Instant.ofEpochSecond(sample.time()), + Instant.ofEpochSecond(sample.time()), sample.cpu(), sample.iowait(), sample.loadavg(), - bytes(sample.memtotal()), - bytes(sample.memused()), - bytes(sample.swaptotal()), - bytes(sample.swapused()), - bytes(sample.roottotal()), - bytes(sample.rootused()), + RrdValues.bytes(sample.memtotal()), + RrdValues.bytes(sample.memused()), + RrdValues.bytes(sample.swaptotal()), + RrdValues.bytes(sample.swapused()), + RrdValues.bytes(sample.roottotal()), + RrdValues.bytes(sample.rootused()), sample.netin(), sample.netout()); } - - private static Long bytes(Double value) { - return value == null ? null : Math.round(value); - } } diff --git a/src/main/java/kr/ac/pusan/pickle/inventory/Node.java b/src/main/java/kr/ac/pusan/pickle/inventory/Node.java index c4e1fdf..5823890 100644 --- a/src/main/java/kr/ac/pusan/pickle/inventory/Node.java +++ b/src/main/java/kr/ac/pusan/pickle/inventory/Node.java @@ -56,7 +56,10 @@ public class Node { @Column(name = "ip_pool_id") private Long ipPoolId; - /** Thin-pool size in GB, measured by the infra inventory script; null until measured (V76). */ + /** + * Thin-pool size in GB, measured on the host by an operations tool; null + * until measured (V76). + */ @Column(name = "disk_capacity_gb") private Long diskCapacityGb; diff --git a/src/main/java/kr/ac/pusan/pickle/proxmox/RrdValues.java b/src/main/java/kr/ac/pusan/pickle/proxmox/RrdValues.java new file mode 100644 index 0000000..2d22840 --- /dev/null +++ b/src/main/java/kr/ac/pusan/pickle/proxmox/RrdValues.java @@ -0,0 +1,18 @@ +package kr.ac.pusan.pickle.proxmox; + +/** How RRD encodes the values both metric series read, in one place. */ +public final class RrdValues { + + private RrdValues() { + } + + /** + * RRD carries byte counters as doubles (consolidated averages), so every + * byte field of a metric point is rounded back to a whole count here. A + * gap stays a gap: an absent counter maps to null and never to zero, which + * a chart would draw as a measured floor. + */ + public static Long bytes(Double value) { + return value == null ? null : Math.round(value); + } +} diff --git a/src/main/java/kr/ac/pusan/pickle/vm/VmMetricsService.java b/src/main/java/kr/ac/pusan/pickle/vm/VmMetricsService.java index c41595a..f1c17c1 100644 --- a/src/main/java/kr/ac/pusan/pickle/vm/VmMetricsService.java +++ b/src/main/java/kr/ac/pusan/pickle/vm/VmMetricsService.java @@ -17,7 +17,6 @@ import org.slf4j.LoggerFactory; import org.springframework.http.HttpStatus; import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; /** * Usage time series of one VM (contract op {@code getVmMetrics}), read live @@ -31,6 +30,13 @@ * The one case that is genuinely not an error is a VM with no guest behind it * yet (or a deleted one): there is nothing to ask about, so it answers 200 with * {@code available: false} and never touches Proxmox. + * + *

Deliberately not {@code @Transactional}: the hypervisor read can sit for + * the whole client read timeout, and holding a pooled connection across it lets + * a stalled PVE plus a polling chart drain the pool out from under unrelated + * endpoints. Both database reads below carry their own transaction (the access + * lookup and the repository call), and only basic columns of the entities they + * return are touched afterwards, so nothing lazy is left outside one. */ @Service public class VmMetricsService { @@ -53,7 +59,6 @@ public VmMetricsService(VmAccessService vmAccessService, NodeRepository nodeRepo this.clock = clock; } - @Transactional(readOnly = true) public VmMetricsResponse metrics(AuthenticatedUser actor, long vmId, RrdTimeframe timeframe) { Vm vm = vmAccessService.of(actor, vmId).requireVisible(); if (vm.getProxmoxVmid() == null || vm.getStatus() == VmStatus.DELETED) { @@ -67,10 +72,14 @@ public VmMetricsResponse metrics(AuthenticatedUser actor, long vmId, RrdTimefram points = proxmoxClient .vmRrdData(node.getApiHost(), node.getName(), vm.getProxmoxVmid(), timeframe) .stream() + // A row with no timestamp has no place on the axis. + .filter(sample -> sample.time() != null) .map(VmMetricPointResponse::from) .toList(); - } catch (ProxmoxApiException e) { + } catch (ProxmoxApiException | IllegalStateException e) { // The message carries the PVE reason and never the token (client layer). + // An unconfigured token refuses the same way from the caller's side: + // PVE cannot be asked, so the answer is the outage, not a 500. log.warn("VM {} usage read failed on node {}: {}", vmId, node.getName(), e.getMessage()); throw metricsUnavailable(); diff --git a/src/main/java/kr/ac/pusan/pickle/vm/dto/VmMetricPointResponse.java b/src/main/java/kr/ac/pusan/pickle/vm/dto/VmMetricPointResponse.java index 338200e..dcea9b1 100644 --- a/src/main/java/kr/ac/pusan/pickle/vm/dto/VmMetricPointResponse.java +++ b/src/main/java/kr/ac/pusan/pickle/vm/dto/VmMetricPointResponse.java @@ -2,6 +2,7 @@ import io.swagger.v3.oas.annotations.media.Schema; import java.time.Instant; +import kr.ac.pusan.pickle.proxmox.RrdValues; import kr.ac.pusan.pickle.proxmox.dto.VmRrdSample; import org.jspecify.annotations.Nullable; @@ -9,6 +10,11 @@ * Contract schema {@code VmMetricPoint}. Every metric is nullable because an * RRD row omits the keys for intervals the VM was not running in, and that * absence is the honest gap a chart must draw rather than a zero. + * + *

{@code time} is the exception: it is the point's position on the axis and + * the contract's only required field here, so a row that arrives without one + * is dropped where the series is mapped rather than shipped as a null the + * console would have to guess about. */ public record VmMetricPointResponse( Instant time, @@ -25,21 +31,17 @@ public record VmMetricPointResponse( @Nullable Double diskReadBps, @Nullable Double diskWriteBps) { + /** Callers drop timeless rows first (see the class note); everything else may be null. */ public static VmMetricPointResponse from(VmRrdSample sample) { return new VmMetricPointResponse( - sample.time() == null ? null : Instant.ofEpochSecond(sample.time()), + Instant.ofEpochSecond(sample.time()), sample.cpu(), - bytes(sample.mem()), - bytes(sample.memhost()), - bytes(sample.maxmem()), + RrdValues.bytes(sample.mem()), + RrdValues.bytes(sample.memhost()), + RrdValues.bytes(sample.maxmem()), sample.netin(), sample.netout(), sample.diskread(), sample.diskwrite()); } - - /** RRD carries byte counters as doubles (consolidated averages); a gap stays a gap. */ - private static Long bytes(Double value) { - return value == null ? null : Math.round(value); - } } diff --git a/src/test/java/kr/ac/pusan/pickle/admin/AdminNodeMetricsServiceTest.java b/src/test/java/kr/ac/pusan/pickle/admin/AdminNodeMetricsServiceTest.java new file mode 100644 index 0000000..8440093 --- /dev/null +++ b/src/test/java/kr/ac/pusan/pickle/admin/AdminNodeMetricsServiceTest.java @@ -0,0 +1,122 @@ +package kr.ac.pusan.pickle.admin; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.time.Clock; +import java.time.Instant; +import java.time.ZoneOffset; +import java.util.List; +import java.util.Optional; +import kr.ac.pusan.pickle.admin.dto.NodeMetricsResponse; +import kr.ac.pusan.pickle.common.error.ApiException; +import kr.ac.pusan.pickle.common.error.ErrorCodes; +import kr.ac.pusan.pickle.inventory.Node; +import kr.ac.pusan.pickle.inventory.NodeRepository; +import kr.ac.pusan.pickle.proxmox.ProxmoxApiException; +import kr.ac.pusan.pickle.proxmox.ProxmoxClient; +import kr.ac.pusan.pickle.proxmox.RrdTimeframe; +import kr.ac.pusan.pickle.proxmox.dto.NodeRrdSample; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.http.HttpStatus; + +/** + * The node series answers the same way the per-VM one does: RRD rows map gaps + * and all, a row with no timestamp is dropped because it cannot be placed on + * the axis, and every refusal the client can raise — over the wire or before + * the request leaves, when no API token is configured — becomes the 503 the + * contract describes rather than a 500. + */ +@ExtendWith(MockitoExtension.class) +class AdminNodeMetricsServiceTest { + + @Mock + private NodeRepository nodeRepository; + @Mock + private ProxmoxClient proxmoxClient; + + @Test + void rrdRowsBecomePointsAndTheTimelessOnesAreDropped() { + Node node = node(); + when(nodeRepository.findById(3L)).thenReturn(Optional.of(node)); + when(proxmoxClient.nodeRrdData(anyString(), anyString(), any())) + .thenReturn(List.of( + new NodeRrdSample(null, 0.5, 32.0, 0.01, 1.0, 1.0e10, 5.0e9, 4.0e9, + 0.0, 0.0, 1.0e11, 2.0e10, 10.0, 20.0), + new NodeRrdSample(1_786_335_600L, 0.25, 32.0, 0.02, 2.0, 1.0e10, 6.0e9, + 4.0e9, 0.0, 0.0, 1.0e11, 2.0e10, 10.0, null))); + + NodeMetricsResponse response = new AdminNodeMetricsService(nodeRepository, proxmoxClient, + clock()).metrics(3L, RrdTimeframe.HOUR); + + assertThat(response.timeframe()).isEqualTo("HOUR"); + assertThat(response.points()).hasSize(1); + assertThat(response.points().getFirst().time()) + .isEqualTo(Instant.ofEpochSecond(1_786_335_600L)); + assertThat(response.points().getFirst().memUsedBytes()).isEqualTo(6_000_000_000L); + assertThat(response.points().getFirst().netoutBps()).isNull(); + } + + @Test + void aHypervisorThatRefusesIsA503() { + Node node = node(); + when(nodeRepository.findById(3L)).thenReturn(Optional.of(node)); + when(proxmoxClient.nodeRrdData(anyString(), anyString(), any())) + .thenThrow(new ProxmoxApiException(596, "no such resource", "GET rrddata")); + + assertMetricsUnavailable(); + } + + @Test + void anUnconfiguredApiTokenIsTheSameOutageAsARefusal() { + Node node = node(); + when(nodeRepository.findById(3L)).thenReturn(Optional.of(node)); + // The client refuses before the request leaves when the deployment + // carries no PVE token — a state the configuration explicitly allows. + when(proxmoxClient.nodeRrdData(anyString(), anyString(), any())) + .thenThrow(new IllegalStateException("Proxmox API token is not configured")); + + assertMetricsUnavailable(); + } + + @Test + void anUnknownNodeIsA404AndIsNeverAskedAbout() { + when(nodeRepository.findById(404L)).thenReturn(Optional.empty()); + + assertThatThrownBy(() -> service().metrics(404L, RrdTimeframe.HOUR)) + .isInstanceOfSatisfying(ApiException.class, ex -> { + assertThat(ex.getStatus()).isEqualTo(HttpStatus.NOT_FOUND); + assertThat(ex.getCode()).isEqualTo(ErrorCodes.RESOURCE_NOT_FOUND); + }); + } + + private void assertMetricsUnavailable() { + assertThatThrownBy(() -> service().metrics(3L, RrdTimeframe.HOUR)) + .isInstanceOfSatisfying(ApiException.class, ex -> { + assertThat(ex.getStatus()).isEqualTo(HttpStatus.SERVICE_UNAVAILABLE); + assertThat(ex.getCode()).isEqualTo(ErrorCodes.METRICS_UNAVAILABLE); + }); + } + + private static Node node() { + Node node = mock(Node.class); + when(node.getApiHost()).thenReturn("https://pve-test:8006"); + when(node.getName()).thenReturn("pve-test"); + return node; + } + + private static Clock clock() { + return Clock.fixed(Instant.parse("2026-08-10T00:00:00Z"), ZoneOffset.UTC); + } + + private AdminNodeMetricsService service() { + return new AdminNodeMetricsService(nodeRepository, proxmoxClient, clock()); + } +} diff --git a/src/test/java/kr/ac/pusan/pickle/admin/AdminSystemSummaryNodeLiveTest.java b/src/test/java/kr/ac/pusan/pickle/admin/AdminSystemSummaryNodeLiveTest.java index 2a7b634..3cfb48c 100644 --- a/src/test/java/kr/ac/pusan/pickle/admin/AdminSystemSummaryNodeLiveTest.java +++ b/src/test/java/kr/ac/pusan/pickle/admin/AdminSystemSummaryNodeLiveTest.java @@ -1,23 +1,35 @@ package kr.ac.pusan.pickle.admin; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.nullValue; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; +import java.util.ArrayList; +import java.util.List; +import java.util.UUID; import kr.ac.pusan.pickle.proxmox.ProxmoxApiException; import kr.ac.pusan.pickle.proxmox.ProxmoxClient; +import kr.ac.pusan.pickle.proxmox.dto.NodeStatusInfo; +import kr.ac.pusan.pickle.proxmox.dto.NodeStorageStatus; import kr.ac.pusan.pickle.security.JwtService; import kr.ac.pusan.pickle.support.EmbeddedPostgresConfig; import kr.ac.pusan.pickle.support.SeedFixtures; import kr.ac.pusan.pickle.user.UserRepository; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; import org.springframework.context.annotation.Import; +import org.springframework.jdbc.core.JdbcTemplate; import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.bean.override.mockito.MockitoBean; import org.springframework.test.web.servlet.MockMvc; @@ -28,6 +40,12 @@ * The client is stubbed to refuse here rather than left to fail on its own: * "the host happens to be unreachable from the test machine" would assert the * same thing by accident and stop asserting it the day that changes. + * + *

The other cases the live block has to keep apart: a node that answered its + * status but not its storage is reachable with the storage half missing, a + * status reply carrying no payload is not an answer at all, and a node the + * operator marked OFFLINE is not asked in the first place while still holding + * its row on the panel. */ @SpringBootTest @AutoConfigureMockMvc @@ -35,6 +53,12 @@ @Import(EmbeddedPostgresConfig.class) class AdminSystemSummaryNodeLiveTest { + private static final NodeStatusInfo STATUS = new NodeStatusInfo( + new NodeStatusInfo.CpuInfo(32, 16, 2, "test"), + new NodeStatusInfo.MemoryInfo(64_000_000_000L, 24_000_000_000L, + 40_000_000_000L, 40_000_000_000L), + 0.125); + @Autowired private MockMvc mockMvc; @@ -44,17 +68,28 @@ class AdminSystemSummaryNodeLiveTest { @Autowired private JwtService jwtService; + @Autowired + private JdbcTemplate jdbcTemplate; + @MockitoBean private ProxmoxClient proxmoxClient; private String sysAdminToken; + private final List createdNodeIds = new ArrayList<>(); + @BeforeEach void setUp() { sysAdminToken = jwtService.createAccessToken( userRepository.findByEmail(SeedFixtures.SYSADMIN_EMAIL).orElseThrow()); } + @AfterEach + void tearDown() { + createdNodeIds.forEach(id -> jdbcTemplate.update("delete from nodes where id = ?", id)); + createdNodeIds.clear(); + } + @Test void anUnreachableHypervisorDegradesTheTileInsteadOfFailingThePanel() throws Exception { when(proxmoxClient.nodeStatus(anyString(), anyString())) @@ -69,4 +104,82 @@ void anUnreachableHypervisorDegradesTheTileInsteadOfFailingThePanel() throws Exc .andExpect(jsonPath("$.nodesLive[?(@.name == 'pve1')].reachable").value(false)) .andExpect(jsonPath("$.nodesLive[?(@.reachable == true)]").isEmpty()); } + + /** + * The storage read needs {@code Datastore.Audit}, which the status read does + * not: losing that one right must cost the two storage numbers and nothing + * else, or the panel claims a node is down while it is answering. + */ + @Test + void aStorageOnlyFailureKeepsTheNodeReachable() throws Exception { + when(proxmoxClient.nodeStatus(anyString(), anyString())).thenReturn(STATUS); + when(proxmoxClient.nodeStorage(anyString(), anyString())) + .thenThrow(new ProxmoxApiException(403, "Permission check failed", + "GET /nodes/pve1/storage")); + + mockMvc.perform(get("/api/v1/admin/system-summary") + .header("Authorization", "Bearer " + sysAdminToken)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.nodesLive[?(@.name == 'pve1')].reachable").value(true)) + .andExpect(jsonPath("$.nodesLive[?(@.name == 'pve1')].memUsedBytes") + .value(24_000_000_000L)) + .andExpect(jsonPath("$.nodesLive[?(@.name == 'pve1')].storageTotalBytes", + contains(nullValue()))) + .andExpect(jsonPath("$.nodesLive[?(@.name == 'pve1')].storageUsedBytes", + contains(nullValue()))); + } + + /** + * The client hands back whatever {@code {"data": …}} carried, so a 200 with + * an empty envelope reaches the caller as null and must read as "not + * answering" instead of taking the whole dashboard down with it. + */ + @Test + void aStatusReplyWithNoPayloadIsNotAnAnswer() throws Exception { + when(proxmoxClient.nodeStatus(anyString(), anyString())).thenReturn(null); + + mockMvc.perform(get("/api/v1/admin/system-summary") + .header("Authorization", "Bearer " + sysAdminToken)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.nodesLive[?(@.name == 'pve1')].reachable").value(false)) + .andExpect(jsonPath("$.nodesLive[?(@.reachable == true)]").isEmpty()); + } + + /** + * A node the operator marked OFFLINE is known to be down, so probing it only + * buys a timeout per dashboard load. It keeps its row so the node count + * stays whole. + */ + @Test + void anOfflineNodeIsNotProbedButStillHoldsItsRow() throws Exception { + String name = "assnl-offline-" + UUID.randomUUID().toString().substring(0, 8); + createOfflineNode(name); + when(proxmoxClient.nodeStatus(anyString(), anyString())).thenReturn(STATUS); + when(proxmoxClient.nodeStorage(anyString(), anyString())).thenReturn(List.of( + new NodeStorageStatus("local-lvm", "lvmthin", 1, 1, 2_000_000_000_000L, + 500_000_000_000L, 1_500_000_000_000L, 0.25))); + + mockMvc.perform(get("/api/v1/admin/system-summary") + .header("Authorization", "Bearer " + sysAdminToken)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.nodesLive[?(@.name == '" + name + "')].nodeId").exists()) + .andExpect(jsonPath("$.nodesLive[?(@.name == '" + name + "')].reachable") + .value(false)) + // The nodes that are not OFFLINE still answer, so the skip is a + // filter on that one node and not the block giving up. + .andExpect(jsonPath("$.nodesLive[?(@.name == 'pve1')].reachable").value(true)); + + verify(proxmoxClient, never()).nodeStatus(anyString(), eq(name)); + verify(proxmoxClient, never()).nodeStorage(anyString(), eq(name)); + } + + private void createOfflineNode(String name) { + Long id = jdbcTemplate.queryForObject(""" + insert into nodes (name, api_host, status, cpu_threads, memory_mb, + vm_bridge, storage) + values (?, 'https://pve-offline:8006', 'OFFLINE', 16, 32768, 'vmbr2', 'local-lvm') + returning id + """, Long.class, name); + createdNodeIds.add(id); + } } diff --git a/src/test/java/kr/ac/pusan/pickle/vm/VmMetricsServiceTest.java b/src/test/java/kr/ac/pusan/pickle/vm/VmMetricsServiceTest.java index 76cbdf9..af7a772 100644 --- a/src/test/java/kr/ac/pusan/pickle/vm/VmMetricsServiceTest.java +++ b/src/test/java/kr/ac/pusan/pickle/vm/VmMetricsServiceTest.java @@ -36,10 +36,12 @@ import org.springframework.http.HttpStatus; /** - * The three answers the usage read can give, none of which is reachable - * against the test database: a VM with no guest behind it must not produce a - * Proxmox call at all, a live one must map the RRD rows gaps and all, and a - * hypervisor that refuses must become a 503 rather than an empty chart. + * The answers the usage read can give, none of which is reachable against the + * test database: a VM with no guest behind it must not produce a Proxmox call + * at all, a live one must map the RRD rows gaps and all while dropping any row + * that carries no timestamp, and a hypervisor that refuses must become a 503 + * rather than an empty chart — whether it refused over the wire or refused + * before the request left, because no API token is configured. */ @ExtendWith(MockitoExtension.class) class VmMetricsServiceTest { @@ -134,6 +136,44 @@ void aHypervisorThatRefusesIsA503RatherThanAnEmptyChart() { }); } + @Test + void anUnconfiguredApiTokenIsTheSameOutageAsARefusal() { + Vm vm = runningVm(); + grantVisible(vm); + Node node = node(); + when(nodeRepository.findById(3L)).thenReturn(Optional.of(node)); + // What the client raises before the request leaves when the deployment + // carries no PVE token — a state the configuration explicitly allows. + when(proxmoxClient.vmRrdData(anyString(), anyString(), anyInt(), any())) + .thenThrow(new IllegalStateException("Proxmox API token is not configured")); + + assertThatThrownBy(() -> service().metrics(ACTOR, 1L, RrdTimeframe.HOUR)) + .isInstanceOfSatisfying(ApiException.class, ex -> { + assertThat(ex.getStatus()).isEqualTo(HttpStatus.SERVICE_UNAVAILABLE); + assertThat(ex.getCode()).isEqualTo(ErrorCodes.METRICS_UNAVAILABLE); + }); + } + + @Test + void aRowWithoutATimestampIsDroppedRatherThanCharted() { + Vm vm = runningVm(); + grantVisible(vm); + Node node = node(); + when(nodeRepository.findById(3L)).thenReturn(Optional.of(node)); + when(proxmoxClient.vmRrdData(anyString(), anyString(), anyInt(), any())) + .thenReturn(List.of( + new VmRrdSample(null, 0.5, 2.0, 1.0e9, 1.0e9, 4.0e9, + 1.0, 2.0, 3.0, 4.0), + new VmRrdSample(1_786_335_600L, 0.25, 2.0, 2.0e9, 2.0e9, 4.0e9, + 1.0, 2.0, 3.0, 4.0))); + + VmMetricsResponse response = service().metrics(ACTOR, 1L, RrdTimeframe.HOUR); + + assertThat(response.points()).hasSize(1); + assertThat(response.points().getFirst().time()) + .isEqualTo(Instant.ofEpochSecond(1_786_335_600L)); + } + private Vm runningVm() { Vm vm = mock(Vm.class); when(vm.getProxmoxVmid()).thenReturn(100_001); From 33df97fb289541da40da27f8bf01f4b2a7265e47 Mon Sep 17 00:00:00 2001 From: yessjun Date: Mon, 10 Aug 2026 20:01:14 +0900 Subject: [PATCH 2/2] docs: explain why an added node empties the disk capacity figure --- .../java/kr/ac/pusan/pickle/admin/OrgHeadroomService.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/main/java/kr/ac/pusan/pickle/admin/OrgHeadroomService.java b/src/main/java/kr/ac/pusan/pickle/admin/OrgHeadroomService.java index 77cde70..8d1bff1 100644 --- a/src/main/java/kr/ac/pusan/pickle/admin/OrgHeadroomService.java +++ b/src/main/java/kr/ac/pusan/pickle/admin/OrgHeadroomService.java @@ -87,6 +87,12 @@ public HeadroomResult headroom(Long orgId) { * with the node, so one unmeasured ACTIVE node makes the sum a number * smaller than the truth — which would read as less headroom than there is. * The whole figure goes null in that case instead. + * + *

Which is what an added node looks like: a new node arrives with no + * measurement, so the disk capacity of the whole platform reads null until + * that node is measured, and every surface hanging off it (the dashboard + * disk bar, the capacity trend's disk line) empties for as long. That is + * this rule working, not a regression introduced by adding the node. */ public PlatformCapacity capacity() { List nodes = nodeRepository.findByStatusOrderByIdAsc(NodeStatus.ACTIVE);