POC: Scheduler Config & Logs Dashboard Exploration#262
POC: Scheduler Config & Logs Dashboard Exploration#262aryunewaskar77-art wants to merge 5 commits into
Conversation
Signed-off-by: aryunewaskar77-art <aaryaanewaskar@gmail.com>
…Dashboard - Initialize react-i18next and configure EN/ZH translation infrastructure - Localize Dashboard, Jobs, Pods, Queues, and PodGroups views - Add persistent language switcher with robust language detection - Implement interpolation-based dynamic resource count translations - Refactor error handling for reactive language updates - Standardize Kubernetes terminology and dynamic labels - Localize resource creation dialogs and dynamic placeholders - Clean up duplicate keys and improve translation consistency Signed-off-by: aryunewaskar77-art <aaryaanewaskar@gmail.com>
Signed-off-by: aryunewaskar77-art <aaryaanewaskar@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new backend endpoints for configuration management and log streaming, along with a comprehensive frontend overhaul that includes internationalization, new configuration editors, and various UI components. The review identified several critical issues, including a bug in log line splitting, missing YAML validation for configuration updates, potential runtime errors in the log stream's error handling, and flaws in the type inference logic for configuration arguments. It is also recommended to remove temporary test scripts from the repository to maintain a clean codebase.
| const k8sLog = new Log(kc); | ||
| const logStream = new stream.PassThrough(); | ||
|
|
||
| logStream.on("data", (chunk) => { | ||
| const dataStr = chunk.toString(); | ||
| const lines = dataStr.split("\n"); | ||
| lines.forEach((line) => { | ||
| if (line.trim()) { | ||
| res.write(`data: ${line}\n\n`); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| const logRequest = await k8sLog.log( | ||
| "volcano-system", | ||
| podName, | ||
| undefined, // containerName | ||
| logStream, | ||
| { | ||
| follow: true, | ||
| tailLines, | ||
| pretty: false, | ||
| } | ||
| ); | ||
|
|
||
| // Handle Disconnection | ||
| req.on("close", () => { | ||
| console.log(`Client disconnected from log stream for ${podName}`); | ||
| try { | ||
| if (logRequest) logRequest.destroy(); | ||
| } catch (err) { | ||
| console.error("Error destroying log request:", err); | ||
| } | ||
| try { | ||
| logStream.destroy(); | ||
| } catch (err) { | ||
| console.error("Error destroying log stream:", err); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The current log streaming implementation has several issues:
- Line Splitting Bug: It splits chunks by
\nand writes them immediately. If a log line is split across two chunks (e.g.,part1andpart2\n), it will be sent as two separate SSE messages, breaking the log output. - Empty Line Filtering:
if (line.trim())incorrectly skips legitimate empty log lines or lines containing only whitespace. - Cleanup Timing: The
closehandler is registered after theawait k8sLog.logcall. If the client disconnects while the request is being initiated, the cleanup logic won't run, leading to potential resource leaks.
Using the readline module solves the splitting issue and provides a cleaner interface.
// Initialize Log Stream
const k8sLog = new Log(kc);
const logStream = new stream.PassThrough();
const rl = readline.createInterface({ input: logStream, terminal: false });
rl.on("line", (line) => {
if (!res.destroyed) {
res.write(`data: ${line}\n\n`);
}
});
let logRequest;
req.on("close", () => {
console.log(`Client disconnected from log stream for ${podName}`);
try {
if (logRequest) logRequest.destroy();
} catch (err) {
console.error("Error destroying log request:", err);
}
try {
logStream.destroy();
rl.close();
} catch (err) {
console.error("Error destroying log stream:", err);
}
});
logRequest = await k8sLog.log(
"volcano-system",
podName,
undefined, // containerName
logStream,
{
follow: true,
tailLines,
pretty: false,
}
);| Log, | ||
| } from "@kubernetes/client-node"; | ||
| import yaml from "js-yaml"; | ||
| import stream from "stream"; |
There was a problem hiding this comment.
The readline module is required to correctly handle line-by-line streaming of logs from the Kubernetes API, especially when dealing with chunked data that might split a single log line across multiple chunks.
| import stream from "stream"; | |
| import stream from "stream"; | |
| import readline from "readline"; |
| name: "volcano-scheduler-configmap", | ||
| namespace: "volcano-system", | ||
| }); | ||
| const conf = cm.data ? cm.data["volcano-scheduler.conf"] : ""; |
There was a problem hiding this comment.
If the ConfigMap exists but the volcano-scheduler.conf key is missing from the data object, conf will be undefined. This can cause issues when sending the JSON response or during frontend parsing. It's safer to provide a default empty string.
| const conf = cm.data ? cm.data["volcano-scheduler.conf"] : ""; | |
| const conf = cm.data?.["volcano-scheduler.conf"] || ""; |
| const { conf } = req.body; | ||
| if (typeof conf !== "string") { | ||
| return res.status(400).json({ error: "Invalid body, conf must be string" }); | ||
| } |
There was a problem hiding this comment.
The PATCH endpoint should validate that the provided configuration is at least valid YAML before attempting to update the ConfigMap. This prevents malformed data from being saved, which could cause the scheduler to fail upon reload.
const { conf } = req.body;
if (typeof conf !== "string") {
return res.status(400).json({ error: "Invalid body, conf must be string" });
}
try {
yaml.load(conf);
} catch (e) {
return res.status(400).json({ error: "Invalid YAML format in configuration" });
}| res.status(500).json({ | ||
| error: "Failed to initiate log stream", | ||
| details: err.body?.message || err.message, | ||
| }); |
There was a problem hiding this comment.
If an error occurs after res.writeHead has been called (e.g., during k8sLog.log), attempting to call res.status(500) will throw an 'ERR_HTTP_HEADERS_SENT' error, crashing the request handler. You should check res.headersSent before attempting to set the status code.
} catch (err) {
console.error("Error initiating log stream:", err);
if (!res.headersSent) {
res.status(500).json({
error: "Failed to initiate log stream",
details: err.body?.message || err.message,
});
} else {
res.end();
}
}| const k8s = require('@kubernetes/client-node'); | ||
| const kc = new k8s.KubeConfig(); | ||
| kc.loadFromDefault(); | ||
| const api = kc.makeApiClient(k8s.CoreV1Api); | ||
|
|
||
| async function test() { | ||
| try { | ||
| const options = { headers: { 'Content-Type': 'application/merge-patch+json' } }; | ||
| const res = await api.patchNamespacedConfigMap( | ||
| { | ||
| name: 'volcano-scheduler-configmap', | ||
| namespace: 'volcano-system', | ||
| body: { data: { "volcano-scheduler.conf": "test" } } | ||
| }, | ||
| options | ||
| ); | ||
| console.log("Success!"); | ||
| } catch (err) { | ||
| console.log("Error:", err.body); | ||
| } | ||
| } | ||
| test(); |
There was a problem hiding this comment.
This test script appears to be a temporary development artifact and should be removed from the repository to maintain a clean codebase.
| let typedVal: string | number | boolean = String(val); | ||
| if (val === "true" || val === true) typedVal = true; | ||
| else if (val === "false" || val === false) | ||
| typedVal = false; | ||
| else if (!isNaN(Number(val)) && typeof val !== "boolean") | ||
| typedVal = Number(val); | ||
| else typedVal = val as string | number | boolean; |
There was a problem hiding this comment.
The logic for inferring types from plugin arguments is flawed. Specifically, Number("") returns 0, which means an empty string argument will be incorrectly converted to the number 0. Additionally, String(val) followed by checks against boolean literals is redundant if the value is already a boolean. A more robust check for numeric strings is needed.
| let typedVal: string | number | boolean = String(val); | |
| if (val === "true" || val === true) typedVal = true; | |
| else if (val === "false" || val === false) | |
| typedVal = false; | |
| else if (!isNaN(Number(val)) && typeof val !== "boolean") | |
| typedVal = Number(val); | |
| else typedVal = val as string | number | boolean; | |
| let typedVal: string | number | boolean = val; | |
| if (val === "true") typedVal = true; | |
| else if (val === "false") typedVal = false; | |
| else if (typeof val === "string" && val.trim() !== "" && !isNaN(Number(val))) { | |
| typedVal = Number(val); | |
| } |
Signed-off-by: aryunewaskar77-art <aaryaanewaskar@gmail.com>
|
Adding label DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
2 participants
This proof-of-concept explores the proposed
/schedulersection for Volcano Dashboard and focuses on two core areas from issue #197:The goal of this work is not to implement the entire mentorship project, but to validate assumptions around dashboard architecture, Kubernetes integration patterns, and implementation complexity.
Implemented exploration areas:
Config Tab
volcano-scheduler-configmap)Logs Tab
tailLinessupportThis POC was built to better understand the interaction between Dashboard UI components, tRPC procedures, Kubernetes APIs, and scheduler-specific workflows before proposing a phased implementation plan.
Future exploration: