diff --git a/contract/openapi.yaml b/contract/openapi.yaml index 548e5ac..be2d96e 100644 --- a/contract/openapi.yaml +++ b/contract/openapi.yaml @@ -1814,6 +1814,25 @@ components: - "id" - "name" type: "object" + LiveCoverage: + properties: + memoryMeasuredNodeCount: + description: "메모리 측정값이 있는 노드 수 — nodeCount보다 작으면 메모리 합계는 부분 측정" + format: "int32" + type: "integer" + nodeCount: + description: "nodesLive 행 수 (플랫폼 전체 노드 수)" + format: "int32" + type: "integer" + storageMeasuredNodeCount: + description: "스토리지 측정값이 있는 노드 수 — nodeCount보다 작으면 스토리지 합계는 부분 측정" + format: "int32" + type: "integer" + required: + - "memoryMeasuredNodeCount" + - "nodeCount" + - "storageMeasuredNodeCount" + type: "object" LoginRequest: properties: email: @@ -1934,7 +1953,8 @@ components: format: "int64" type: "integer" reachable: - description: "false = 이 노드의 Proxmox API가 응답하지 않음 — 나머지 필드는 null" + description: "true = 이 노드가 상태 조회에 응답함. false = 응답하지 않음이며 나머지 필드는 모두 null.\ + \ true여도 스토리지 조회는 별도 권한이라 storage* 필드는 null일 수 있음" type: "boolean" storageTotalBytes: format: "int64" @@ -3618,6 +3638,8 @@ components: items: $ref: "#/components/schemas/IpPoolUsage" type: "array" + liveCoverage: + $ref: "#/components/schemas/LiveCoverage" nodes: items: $ref: "#/components/schemas/NodeRatio" @@ -3645,6 +3667,7 @@ components: required: - "certExpiring30dCount" - "ipPools" + - "liveCoverage" - "nodes" - "nodesLive" - "notificationFailureCount" @@ -4955,7 +4978,7 @@ info: description: "부산대학교 클라우드 플랫폼 Pickle의 REST API. 인증은 JWT Bearer, 오류 응답은 RFC 9457 problem+json(Problem\ \ 스키마)을 따릅니다." title: "Pickle API" - version: "0.36.0" + version: "0.37.0" openapi: "3.1.0" paths: /admin/announcements: diff --git a/src/main/java/kr/ac/pusan/pickle/admin/AdminNodeQueryService.java b/src/main/java/kr/ac/pusan/pickle/admin/AdminNodeQueryService.java index d95ca5c..78663a0 100644 --- a/src/main/java/kr/ac/pusan/pickle/admin/AdminNodeQueryService.java +++ b/src/main/java/kr/ac/pusan/pickle/admin/AdminNodeQueryService.java @@ -53,12 +53,25 @@ public NodeSummaryResponse getNode(long nodeId) { @Transactional(readOnly = true) public List listNodes() { + return listNodes(nodeRepository.findAll(Sort.by("id"))); + } + + /** + * The same summaries over node rows the caller already read. The system + * dashboard builds two halves from one node list — these ratios and the + * live hypervisor probe — and reads the rows once so a status change + * landing mid-request cannot leave the two halves describing different + * rows. Only basic columns are touched, so rows read outside this + * transaction are fine. + */ + @Transactional(readOnly = true) + public List listNodes(List nodes) { double cpuWarn = settingsService.decimal(SettingsService.VCPU_OVERCOMMIT_WARN, DEFAULT_VCPU_OVERCOMMIT_WARN); double memoryWarn = settingsService.decimal(SettingsService.MEMORY_USAGE_WARN, DEFAULT_MEMORY_USAGE_WARN); Map allocations = loadAllocations(); - return nodeRepository.findAll(Sort.by("id")).stream() + return nodes.stream() .map(node -> toSummary(node, allocations.getOrDefault(node.getId(), NodeAllocation.EMPTY), cpuWarn, memoryWarn)) .toList(); 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 e2a0538..609dd93 100644 --- a/src/main/java/kr/ac/pusan/pickle/admin/AdminSummaryService.java +++ b/src/main/java/kr/ac/pusan/pickle/admin/AdminSummaryService.java @@ -17,6 +17,7 @@ import kr.ac.pusan.pickle.admin.dto.OrgDashboardSummaryResponse.TopWorkspace; import kr.ac.pusan.pickle.admin.dto.SystemDashboardSummaryResponse; import kr.ac.pusan.pickle.admin.dto.SystemDashboardSummaryResponse.IpPoolUsage; +import kr.ac.pusan.pickle.admin.dto.SystemDashboardSummaryResponse.LiveCoverage; import kr.ac.pusan.pickle.admin.dto.SystemDashboardSummaryResponse.NodeRatio; import kr.ac.pusan.pickle.admin.dto.SystemDashboardSummaryResponse.Tasks; import kr.ac.pusan.pickle.common.error.ApiException; @@ -24,7 +25,6 @@ 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; @@ -168,9 +168,17 @@ and status not in ('DELETED', 'DELETING') * 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. + * + *

What one transaction did give the two node halves was one reading of + * the node table. Without it, a status change landing mid-request would let + * the ratio list and the live list describe different rows — the panel + * would show a node as ACTIVE and unreachable at the same time, which reads + * as an outage rather than as the parking the operator just did. So the + * rows are read once here and both halves are built from that one list. */ public SystemDashboardSummaryResponse systemSummary() { - List nodes = adminNodeQueryService.listNodes().stream() + List nodeRows = nodeRepository.findAll(org.springframework.data.domain.Sort.by("id")); + List nodes = adminNodeQueryService.listNodes(nodeRows).stream() .map(AdminSummaryService::toNodeRatio) .toList(); Tasks tasks = jdbcTemplate.queryForObject(""" @@ -204,10 +212,30 @@ select count(*) from vms v where s.value = 'true'::jsonb and v.status <> 'DELETED' """); + List live = nodesLive(nodeRows); return new SystemDashboardSummaryResponse(nodes, vmCountsByStatus(null), tasks, notificationFailureCount(), certExpiring30d, driftFindingRepository.countByStatus(DriftFindingStatus.OPEN), - sshPasswordEnabledVms, pools, nodesLive()); + sshPasswordEnabledVms, pools, live, liveCoverage(live)); + } + + /** + * How much of the platform the live numbers actually cover. Each live + * measurement is a per-node sum, and a sum over the nodes that answered is + * not the platform total: one node whose storage read is refused leaves a + * figure smaller than the truth, which an operator reads as free capacity + * that is not there. The counts say how many nodes are behind each sum so a + * client cannot present a subset as the whole — the same reason the org + * headroom figures null a disk number no node measured. + */ + private static LiveCoverage liveCoverage(List live) { + long memory = live.stream() + .filter(node -> node.memTotalBytes() != null && node.memUsedBytes() != null) + .count(); + long storage = live.stream() + .filter(node -> node.storageTotalBytes() != null && node.storageUsedBytes() != null) + .count(); + return new LiveCoverage(live.size(), (int) memory, (int) storage); } /** @@ -228,21 +256,17 @@ select count(*) from vms v * 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. + *

Every node is asked, an OFFLINE one included. OFFLINE excludes a node + * from new placements and leaves its existing guests running, so its RAM is + * still spoken for; skipping the probe would drop that usage out of the + * platform memory tile and show headroom the platform does not have. The + * cost of asking a host that really is dead — one read timeout per + * dashboard load — is accepted deliberately here and belongs to the + * scale-out round, which will probe nodes in parallel under a time budget. */ - private List nodesLive() { + private List nodesLive(List nodes) { 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; - } + for (Node node : nodes) { NodeStatusInfo status; try { status = proxmoxClient.nodeStatus(node.getApiHost(), node.getName()); @@ -285,7 +309,8 @@ private NodeStorageStatus guestStorage(Node node) { List storages = proxmoxClient.nodeStorage(node.getApiHost(), node.getName()); return storages.stream() - .filter(storage -> storage.storage().equals(node.getStorage())) + // node.storage is NOT NULL; the PVE-supplied name may be absent. + .filter(storage -> node.getStorage().equals(storage.storage())) .findFirst() .orElseGet(() -> storages.stream() .filter(storage -> "lvmthin".equals(storage.type()) && storage.isActive()) 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 142783d..c32e4e8 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 @@ -10,19 +10,21 @@ * 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. + *

Every node is asked, whatever status the operator gave it: an OFFLINE + * node still runs the guests it had, so its live numbers are still part of the + * platform's. {@code reachable} is therefore exactly "this node answered its + * status probe just now". * *

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. + * the status half does not. How many nodes are behind each platform sum is on + * the summary itself, in {@code liveCoverage}. */ public record NodeLiveResponse( long nodeId, String name, - @Schema(description = "false = 이 노드의 Proxmox API가 응답하지 않음 — 나머지 필드는 null") + @Schema(description = "true = 이 노드가 상태 조회에 응답함. false = 응답하지 않음이며 나머지 필드는 모두 null." + + " true여도 스토리지 조회는 별도 권한이라 storage* 필드는 null일 수 있음") boolean reachable, @Nullable Long memTotalBytes, @Nullable Long memUsedBytes, diff --git a/src/main/java/kr/ac/pusan/pickle/admin/dto/SystemDashboardSummaryResponse.java b/src/main/java/kr/ac/pusan/pickle/admin/dto/SystemDashboardSummaryResponse.java index ebdf7f0..d7e004c 100644 --- a/src/main/java/kr/ac/pusan/pickle/admin/dto/SystemDashboardSummaryResponse.java +++ b/src/main/java/kr/ac/pusan/pickle/admin/dto/SystemDashboardSummaryResponse.java @@ -1,5 +1,6 @@ package kr.ac.pusan.pickle.admin.dto; +import io.swagger.v3.oas.annotations.media.Schema; import java.util.List; import java.util.Map; import kr.ac.pusan.pickle.inventory.NodeStatus; @@ -14,7 +15,26 @@ public record SystemDashboardSummaryResponse( long openDriftFindingCount, long sshPasswordEnabledVmCount, List ipPools, - List nodesLive) { + List nodesLive, + LiveCoverage liveCoverage) { + + /** + * How many nodes each live measurement in {@code nodesLive} actually + * covers. A platform total summed over {@code nodesLive} is only the + * platform total when every node answered: a node whose storage read was + * refused, or that did not answer at all, leaves a sum smaller than the + * truth, which reads as free capacity that is not there. A client that + * sums must compare these counts with {@code nodeCount} and say so when + * they differ, instead of presenting a subset as the whole. + */ + public record LiveCoverage( + @Schema(description = "nodesLive 행 수 (플랫폼 전체 노드 수)") + int nodeCount, + @Schema(description = "메모리 측정값이 있는 노드 수 — nodeCount보다 작으면 메모리 합계는 부분 측정") + int memoryMeasuredNodeCount, + @Schema(description = "스토리지 측정값이 있는 노드 수 — nodeCount보다 작으면 스토리지 합계는 부분 측정") + int storageMeasuredNodeCount) { + } /** Per-node allocation ratios (same aggregates as {@code GET /admin/nodes}). */ public record NodeRatio( diff --git a/src/main/java/kr/ac/pusan/pickle/common/openapi/OpenApiConfig.java b/src/main/java/kr/ac/pusan/pickle/common/openapi/OpenApiConfig.java index 204e791..e1d2ab4 100644 --- a/src/main/java/kr/ac/pusan/pickle/common/openapi/OpenApiConfig.java +++ b/src/main/java/kr/ac/pusan/pickle/common/openapi/OpenApiConfig.java @@ -41,7 +41,7 @@ public class OpenApiConfig { /** Contract version served in {@code info.version}; bump on any contract change. */ - public static final String CONTRACT_VERSION = "0.36.0"; + public static final String CONTRACT_VERSION = "0.37.0"; /** Name of the bearer-JWT security scheme in the published spec. */ private static final String BEARER_SCHEME = "bearerAuth"; diff --git a/src/main/java/kr/ac/pusan/pickle/proxmox/ProxmoxClient.java b/src/main/java/kr/ac/pusan/pickle/proxmox/ProxmoxClient.java index f2b156b..8933528 100644 --- a/src/main/java/kr/ac/pusan/pickle/proxmox/ProxmoxClient.java +++ b/src/main/java/kr/ac/pusan/pickle/proxmox/ProxmoxClient.java @@ -372,8 +372,13 @@ private String power(String apiHost, String node, int vmid, String action, Map T call(HttpMethod method, URI uri, Map form, TypeReference> responseType) { @@ -396,7 +401,12 @@ private T call(HttpMethod method, URI uri, Map form, throw new ProxmoxApiException(response.getStatusCode().value(), extractErrorMessage(body), description); } - return JSON.readValue(body, responseType).data(); + try { + return JSON.readValue(body, responseType).data(); + } catch (RuntimeException e) { + throw new ProxmoxApiException("Proxmox API returned an unreadable body on " + + description + ": " + e.getMessage(), e); + } }); } catch (ResourceAccessException e) { // connect/read timeout, connection refused/reset … — no HTTP response. 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 3cfb48c..3f569ce 100644 --- a/src/test/java/kr/ac/pusan/pickle/admin/AdminSystemSummaryNodeLiveTest.java +++ b/src/test/java/kr/ac/pusan/pickle/admin/AdminSystemSummaryNodeLiveTest.java @@ -2,9 +2,9 @@ import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.nullValue; +import static org.mockito.ArgumentMatchers.any; 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; @@ -14,6 +14,7 @@ import java.util.ArrayList; import java.util.List; import java.util.UUID; +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.dto.NodeStatusInfo; @@ -29,9 +30,11 @@ import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; import org.springframework.context.annotation.Import; +import org.springframework.data.domain.Sort; 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.context.bean.override.mockito.MockitoSpyBean; import org.springframework.test.web.servlet.MockMvc; /** @@ -42,10 +45,11 @@ * 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. + * status but not its storage is reachable with the storage half missing and + * says so in the coverage counts, a status reply carrying no payload is not an + * answer at all, a storage entry PVE sent without a name is not a crash, and a + * node the operator marked OFFLINE is asked like any other because its guests + * are still running on it. */ @SpringBootTest @AutoConfigureMockMvc @@ -74,6 +78,9 @@ class AdminSystemSummaryNodeLiveTest { @MockitoBean private ProxmoxClient proxmoxClient; + @MockitoSpyBean + private NodeRepository nodeRepository; + private String sysAdminToken; private final List createdNodeIds = new ArrayList<>(); @@ -126,6 +133,33 @@ void aStorageOnlyFailureKeepsTheNodeReachable() throws Exception { .andExpect(jsonPath("$.nodesLive[?(@.name == 'pve1')].storageTotalBytes", contains(nullValue()))) .andExpect(jsonPath("$.nodesLive[?(@.name == 'pve1')].storageUsedBytes", + contains(nullValue()))) + // The node stays on the panel, but the platform storage sum is + // now over fewer nodes than the platform has, and a client that + // sums has to be able to see that. + .andExpect(jsonPath("$.liveCoverage.nodeCount").value(1)) + .andExpect(jsonPath("$.liveCoverage.memoryMeasuredNodeCount").value(1)) + .andExpect(jsonPath("$.liveCoverage.storageMeasuredNodeCount").value(0)); + } + + /** + * The storage list comes back as whatever the PVE envelope held, so an + * entry can arrive without the name the match is made on. The node column + * is NOT NULL and the PVE side is not, so the comparison runs from the + * column — otherwise one nameless entry takes the whole dashboard down. + */ + @Test + void aStorageEntryWithoutANameDoesNotFailThePanel() throws Exception { + when(proxmoxClient.nodeStatus(anyString(), anyString())).thenReturn(STATUS); + when(proxmoxClient.nodeStorage(anyString(), anyString())).thenReturn(List.of( + new NodeStorageStatus(null, "dir", 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 == 'pve1')].reachable").value(true)) + .andExpect(jsonPath("$.nodesLive[?(@.name == 'pve1')].storageTotalBytes", contains(nullValue()))); } @@ -146,12 +180,13 @@ void aStatusReplyWithNoPayloadIsNotAnAnswer() throws Exception { } /** - * 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. + * OFFLINE keeps a node out of new placements and leaves the guests it + * already has running, so its memory is still spoken for. Asking it is the + * only way the platform memory tile can count that usage, and a node that + * really is down still degrades on its own without taking the panel. */ @Test - void anOfflineNodeIsNotProbedButStillHoldsItsRow() throws Exception { + void anOfflineNodeIsStillProbedBecauseItsGuestsKeepRunning() throws Exception { String name = "assnl-offline-" + UUID.randomUUID().toString().substring(0, 8); createOfflineNode(name); when(proxmoxClient.nodeStatus(anyString(), anyString())).thenReturn(STATUS); @@ -162,15 +197,34 @@ void anOfflineNodeIsNotProbedButStillHoldsItsRow() throws Exception { 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)); + .value(true)) + .andExpect(jsonPath("$.nodesLive[?(@.name == '" + name + "')].memUsedBytes") + .value(24_000_000_000L)) + .andExpect(jsonPath("$.nodesLive[?(@.name == 'pve1')].reachable").value(true)) + .andExpect(jsonPath("$.liveCoverage.nodeCount").value(2)) + .andExpect(jsonPath("$.liveCoverage.memoryMeasuredNodeCount").value(2)); + + verify(proxmoxClient).nodeStatus(anyString(), eq(name)); + verify(proxmoxClient).nodeStorage(anyString(), eq(name)); + } + + /** + * The ratio list and the live list are two halves of one panel. Read from + * two separate queries they can straddle a status change and describe + * different rows, and the console reads a node that is ACTIVE in one half + * and unreachable in the other as an outage nobody caused. One read of the + * node table is what keeps them consistent. + */ + @Test + void bothHalvesOfThePanelAreBuiltFromOneReadOfTheNodeTable() throws Exception { + when(proxmoxClient.nodeStatus(anyString(), anyString())).thenReturn(STATUS); + + mockMvc.perform(get("/api/v1/admin/system-summary") + .header("Authorization", "Bearer " + sysAdminToken)) + .andExpect(status().isOk()); - verify(proxmoxClient, never()).nodeStatus(anyString(), eq(name)); - verify(proxmoxClient, never()).nodeStorage(anyString(), eq(name)); + verify(nodeRepository).findAll(any(Sort.class)); } private void createOfflineNode(String name) { diff --git a/src/test/java/kr/ac/pusan/pickle/proxmox/ProxmoxClientTest.java b/src/test/java/kr/ac/pusan/pickle/proxmox/ProxmoxClientTest.java index 2487277..4e0cec7 100644 --- a/src/test/java/kr/ac/pusan/pickle/proxmox/ProxmoxClientTest.java +++ b/src/test/java/kr/ac/pusan/pickle/proxmox/ProxmoxClientTest.java @@ -428,6 +428,28 @@ void missingVmSurfacesPveMessage() { assertThat(e.apiMessage()).contains("does not exist"); } + /** + * A 200 whose body is not the envelope — an HTML error page from something + * standing in front of pveproxy, or a changed envelope mid-upgrade — must + * leave the client as the one exception callers already handle, or every + * caller has to enumerate the parser's types on top of it and the dashboard + * 500s the first time one is missed. + */ + @Test + void anUnparseableOkBodyIsATransientApiFailure() { + wm.server().stubFor(get(urlPathEqualTo("/api2/json/nodes/pve1/status")) + .willReturn(aResponse().withStatus(200) + .withHeader("Content-Type", "text/html") + .withBody("502 Bad Gateway"))); + + ProxmoxApiException e = catchThrowableOfType(ProxmoxApiException.class, + () -> client.nodeStatus(wm.apiHost(), NODE)); + + assertThat(e.statusCode()).isZero(); + assertThat(e.isTransient()).isTrue(); + assertThat(e.getMessage()).contains("/api2/json/nodes/pve1/status"); + } + @Test void transportFailureIsTransientWithoutStatusCode() { wm.server().stubFor(get(urlPathEqualTo("/api2/json/cluster/resources"))