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/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); 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);