fix: Inspector panel editing, PVC auto-binding, headless service support, and resource heal/reschedule events#32
Conversation
…ces, and rendering updates
|
@laxmanfhaneendra is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe pull request enhances resource editing with conditional UI fields (memory limits, probes, secret types, TLS configuration, cluster IP), implements PVC-to-PV binding simulation logic, adds event handlers for resource healing and rescheduling, removes legacy auto-bind behavior from StorageResources, and extends CLI argument parsing to support cluster IP specification for Services. Changes
Sequence DiagramsequenceDiagram
actor Tick as Simulation Tick
participant PVCs as PersistentVolumeClaims<br/>(Pending)
participant PVs as PersistentVolumes<br/>(Available)
participant Binding as Binding Logic
participant EventBus as Event Bus
Tick->>Tick: _tickPVCBinding(dt)
Tick->>PVCs: Collect all Pending claims
Tick->>PVs: Collect Available/Unbound volumes
Binding->>Binding: Filter by storage class match
Binding->>Binding: Find tightest-fit PV<br/>(min capacity ≥ requested)
Binding->>PVCs: Set volumeName & mark Bound
Binding->>PVs: Set claimRef & mark Bound
PVCs->>EventBus: Emit 'Bound' event
PVs->>EventBus: Emit 'Bound' event
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
js/engine/SimulationTick.js (1)
981-984: Unuseddtparameter.
_tickPVCBinding(dt)never usesdt. Other_tick*methods takedtfor consistency, so this is fine as-is, but you can drop it or prefix with_if you prefer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/engine/SimulationTick.js` around lines 981 - 984, The _tickPVCBinding(dt) function has an unused dt parameter; to avoid linter warnings without changing callers, rename the parameter to _dt (i.e., _tickPVCBinding(_dt)) or prefix it with an underscore so it’s clear/ignored by linters, and update any nearby JSDoc or references to the parameter name if present; do not remove the parameter unless you also update all call sites that pass dt.js/ui/CommandBar.js (1)
646-660: Deduplicate--clusteripparsing into a small flag helper.The space-separated (
--clusterip <v>) and inline (--clusterip=<v>) branches duplicate thenone → Nonenormalization and thekind === 'Service'guard. Since_cmdGetalready uses a similar pattern for other flags and this shape is likely to recur (e.g., for future--type,--portflags oncreate), a tiny extractor keeps things tidy and makes it harder to miss one branch when updating later.♻️ Proposed refactor
- const ipArgIdx = args.indexOf('--clusterip'); - if (ipArgIdx !== -1 && args[ipArgIdx + 1] && kind === 'Service') { - let ipVal = args[ipArgIdx + 1]; - if (ipVal.toLowerCase() === 'none') ipVal = 'None'; - def.spec.clusterIP = ipVal; - } else { - const inlineIp = args.find(a => a.startsWith('--clusterip=')); - if (inlineIp && kind === 'Service') { - let ipVal = inlineIp.split('=')[1]; - if (ipVal.toLowerCase() === 'none') ipVal = 'None'; - def.spec.clusterIP = ipVal; - } - } + if (kind === 'Service') { + const rawIp = this._getFlagValue(args, '--clusterip'); + if (rawIp) { + def.spec.clusterIP = rawIp.toLowerCase() === 'none' ? 'None' : rawIp; + } + }And add a helper somewhere near the other private methods:
_getFlagValue(args, flag) { const idx = args.indexOf(flag); if (idx !== -1 && args[idx + 1] && !args[idx + 1].startsWith('--')) { return args[idx + 1]; } const prefix = `${flag}=`; const inline = args.find(a => a.startsWith(prefix)); return inline ? inline.slice(prefix.length) : null; }As a side effect the helper also guards against
--clusterip --other-flagsilently consuming the next flag as the IP value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/ui/CommandBar.js` around lines 646 - 660, Introduce a small helper method named _getFlagValue(args, flag) near the other private methods that returns the flag value for either space-separated (--flag value) or inline (--flag=value) forms and returns null if the next token starts with '--'; then replace the duplicated parsing in the clusterIP block by calling _getFlagValue(args, '--clusterip'), keep the existing kind === 'Service' guard, and apply the same normalization (if value.toLowerCase() === 'none' then set to 'None') before assigning to def.spec.clusterIP. Ensure the helper name _getFlagValue and the clusterip assignment logic are used so future flags (e.g., --type, --port) can reuse it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.html`:
- Around line 1208-1232: The memory input values are incorrectly gated on
cpuReq/cpuLim so saved memory doesn't repopulate; update the inputs with id
"edit-mem-req" and "edit-mem-lim" to use the actual memory variables (memReq and
memLim) as their value defaults (e.g. value="${memReq || ''}" and
value="${memLim || ''}") instead of value="${cpuReq ? memReq : ''}" /
value="${cpuLim ? memLim : ''}"; ensure the save-path guard that checks
memReq/memLim element values still operates on the input elements' .value so
existing memory values are preserved when reopening the editor.
- Around line 1386-1395: The TLS hosts entry is being hardcoded to
resource.metadata.name + '.example.com' which can drift from the actual ingress
rule host; update the logic in the block that reads ingressTls (and any code
that sets updates.spec.tls) to derive the host from the current rule if present
(e.g. prefer updates.spec.rules[0].host or resource.spec.rules[0].host when
available) and only fall back to resource.metadata.name + '.example.com' when no
rule host exists, then set updates.spec.tls = [{ hosts: [derivedHost],
secretName: sn }] (or [] when sn is empty) so the TLS hosts match the ingress
rule added by _showIngressPathPicker.
- Around line 1371-1379: The save logic ignores an empty Cluster IP input so
clearing the field is a no-op; update the handler that reads
document.getElementById('edit-cluster-ip') (clusterIpInput) to explicitly set
updates.spec (or ensure updates.spec exists) and assign updates.spec.clusterIP =
'' or delete updates.spec.clusterIP when clusterIpInput.value.trim() is empty
(to represent clearing/auto-assigned intent), preserving the existing behavior
that sets a capitalized 'None' when the user types 'none'; ensure tests or
downstream code expect an empty string or absence of clusterIP and handle
accordingly.
- Around line 1333-1350: The save path currently replaces updates.spec.resources
with a hardcoded CPU and requires both memReq and memLim to be non-empty; change
this so the handler reads the existing resources object (if any) and only
updates/sets requests.memory and limits.memory based on memReq.value.trim() and
memLim.value.trim() individually (preserve any existing cpu, ephemeral-storage,
gpu, hugepages, etc.), and if a field is empty treat that as a deletion (remove
the corresponding requests.memory or limits.memory key or set it undefined so
the downstream merger will drop it) rather than skipping the entire save; also
for probesCheck, instead of assigning null to
updates.spec.livenessProbe/readinessProbe when unchecked, remove those keys from
updates.spec (or use delete) so we don't leave literal nulls unless the
downstream update API explicitly requires nulls.
- Around line 1234-1244: The editor currently mutates resource.type directly
when editing Secrets which is not picked up by ClusterState.update; instead,
when handling the secret-type change (the code that reads secretType and the
block that later calls cluster.update(uid, updates)), add the type into the
updates object under spec (e.g. ensure updates.spec exists and set
updates.spec.type = secretType.value) so cluster.update(uid, updates) will
persist the change; also audit Secret creation in js/ui/CommandBar.js (the
template that creates { spec: { type: 'Opaque', data: {} } }) and either
normalize the template to match the Secret constructor (which expects
metadata.secretType) or update the constructor to read spec.type so both
creation and edits use the same field.
In `@js/engine/GameEngine.js`:
- Around line 224-233: The pod "heal" branch currently sets Ready=True with
reason 'ContainersReady' and marks containerStatuses as running but doesn't
follow the project's condition conventions nor clear prior failure state; update
the block handling data.kind === 'Pod' to: 1) set conditions exactly as
elsewhere (setCondition('ContainersReady','True','ContainersReady') and
setCondition('Ready','True','PodReady')), 2) for each entry in
res.status.containerStatuses ensure cs.ready = true, cs.started = true (or
equivalent flag used elsewhere), clear cs.lastState (remove terminated/waiting
details) and reset cs.restartCount to 0, and 3) also clear any probe/failure
counters (e.g., _probeFailCount or similar) associated with the pod/container so
the next _tickPodLifecycle/_evaluateProbe won't immediately re-crash it; locate
usages via setCondition and res.status.containerStatuses to implement these
updates.
In `@js/engine/SimulationTick.js`:
- Around line 1019-1039: Remove the redundant direct assignment bestPV.phase =
'Bound' in SimulationTick.js: the code already calls bestPV.bind(...) (which
sets this.phase = 'Bound') or the fallback bestPV.setPhase('Bound'), so delete
the extra bestPV.phase = 'Bound' line to avoid duplication; locate the block
handling PVC/PV binding (references: pvc.bind, pvc.setPhase, bestPV.bind,
bestPV.setPhase) and remove only that final explicit phase assignment.
---
Nitpick comments:
In `@js/engine/SimulationTick.js`:
- Around line 981-984: The _tickPVCBinding(dt) function has an unused dt
parameter; to avoid linter warnings without changing callers, rename the
parameter to _dt (i.e., _tickPVCBinding(_dt)) or prefix it with an underscore so
it’s clear/ignored by linters, and update any nearby JSDoc or references to the
parameter name if present; do not remove the parameter unless you also update
all call sites that pass dt.
In `@js/ui/CommandBar.js`:
- Around line 646-660: Introduce a small helper method named _getFlagValue(args,
flag) near the other private methods that returns the flag value for either
space-separated (--flag value) or inline (--flag=value) forms and returns null
if the next token starts with '--'; then replace the duplicated parsing in the
clusterIP block by calling _getFlagValue(args, '--clusterip'), keep the existing
kind === 'Service' guard, and apply the same normalization (if
value.toLowerCase() === 'none' then set to 'None') before assigning to
def.spec.clusterIP. Ensure the helper name _getFlagValue and the clusterip
assignment logic are used so future flags (e.g., --type, --port) can reuse it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a4cbf35-10a6-4a0b-8282-b24460dc7349
📒 Files selected for processing (5)
index.htmljs/engine/GameEngine.jsjs/engine/SimulationTick.jsjs/resources/StorageResources.jsjs/ui/CommandBar.js
💤 Files with no reviewable changes (1)
- js/resources/StorageResources.js
| const memReq = resource.spec?.resources?.requests?.memory || '128Mi'; | ||
| const memLim = resource.spec?.resources?.limits?.memory || '256Mi'; | ||
| const cpuReq = resource.spec?.resources?.requests?.cpu || ''; | ||
| const cpuLim = resource.spec?.resources?.limits?.cpu || ''; | ||
| const hasProbes = !!resource.spec?.livenessProbe; | ||
|
|
||
| extraFields += ` | ||
| <div class="mt-3"> | ||
| <div class="text-[10px] text-white/40 uppercase tracking-wider mb-1">Replicas</div> | ||
| <input id="edit-replicas" type="number" min="0" max="20" value="${resource.spec?.replicas ?? 1}" class="w-full px-3 py-1.5 text-sm text-white bg-black/40 border border-white/15 rounded-lg outline-none focus:border-sky-500/50 font-mono" /> | ||
| </div> | ||
| <div class="mt-3"> | ||
| <div class="text-[10px] text-white/40 uppercase tracking-wider mb-1">Memory Request & Limit</div> | ||
| <div class="flex gap-2"> | ||
| <input id="edit-mem-req" type="text" value="${cpuReq ? memReq : ''}" placeholder="Req e.g. 128Mi" class="w-1/2 px-3 py-1.5 text-sm text-white bg-black/40 border border-white/15 rounded-lg outline-none focus:border-sky-500/50 font-mono" /> | ||
| <input id="edit-mem-lim" type="text" value="${cpuLim ? memLim : ''}" placeholder="Lim e.g. 256Mi" class="w-1/2 px-3 py-1.5 text-sm text-white bg-black/40 border border-white/15 rounded-lg outline-none focus:border-sky-500/50 font-mono" /> | ||
| </div> | ||
| </div> | ||
| <div class="mt-3"> | ||
| <div class="text-[10px] text-white/40 uppercase tracking-wider mb-1">Production Readiness</div> | ||
| <label class="flex items-center gap-2 mt-1"> | ||
| <input type="checkbox" id="edit-probes" ${hasProbes ? 'checked' : ''} class="rounded border-white/15 bg-black/40 text-sky-500"> | ||
| <span class="text-xs text-white/80">Enable Liveness & Readiness Probes</span> | ||
| </label> | ||
| </div>`; |
There was a problem hiding this comment.
Bug: memory fields gated on CPU values, so saved memory settings won't repopulate.
Lines 1222–1223 gate the memory input values on cpuReq/cpuLim rather than on the memory values themselves:
<input id="edit-mem-req" ... value="${cpuReq ? memReq : ''}" ... />
<input id="edit-mem-lim" ... value="${cpuLim ? memLim : ''}" ... />Because this dialog's save path never persists a user-facing CPU request/limit (only the hardcoded 100m/500m at Line 1338‑1339), cpuReq/cpuLim will typically be empty strings and the memory fields will render blank even when the resource already has memory set. It also means if the user previously set only memory via another path, re-opening the editor shows empty inputs, and clicking Save without re-entering a value causes the if (memReq && memLim && memReq.value.trim() && memLim.value.trim()) guard at Line 1336 to fail and silently drop the existing values.
🛠️ Proposed fix
- const memReq = resource.spec?.resources?.requests?.memory || '128Mi';
- const memLim = resource.spec?.resources?.limits?.memory || '256Mi';
- const cpuReq = resource.spec?.resources?.requests?.cpu || '';
- const cpuLim = resource.spec?.resources?.limits?.cpu || '';
+ const memReq = resource.spec?.resources?.requests?.memory || '';
+ const memLim = resource.spec?.resources?.limits?.memory || '';
const hasProbes = !!resource.spec?.livenessProbe;
@@
- <input id="edit-mem-req" type="text" value="${cpuReq ? memReq : ''}" placeholder="Req e.g. 128Mi" ... />
- <input id="edit-mem-lim" type="text" value="${cpuLim ? memLim : ''}" placeholder="Lim e.g. 256Mi" ... />
+ <input id="edit-mem-req" type="text" value="${memReq}" placeholder="Req e.g. 128Mi" ... />
+ <input id="edit-mem-lim" type="text" value="${memLim}" placeholder="Lim e.g. 256Mi" ... />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 1208 - 1232, The memory input values are incorrectly
gated on cpuReq/cpuLim so saved memory doesn't repopulate; update the inputs
with id "edit-mem-req" and "edit-mem-lim" to use the actual memory variables
(memReq and memLim) as their value defaults (e.g. value="${memReq || ''}" and
value="${memLim || ''}") instead of value="${cpuReq ? memReq : ''}" /
value="${cpuLim ? memLim : ''}"; ensure the save-path guard that checks
memReq/memLim element values still operates on the input elements' .value so
existing memory values are preserved when reopening the editor.
| if (kind === 'Secret') { | ||
| extraFields += ` | ||
| <div class="mt-3"> | ||
| <div class="text-[10px] text-white/40 uppercase tracking-wider mb-1">Secret Type</div> | ||
| <select id="edit-secret-type" class="w-full px-3 py-1.5 text-sm text-white bg-black/40 border border-white/15 rounded-lg outline-none focus:border-sky-500/50"> | ||
| <option value="Opaque" ${resource.type === 'Opaque' ? 'selected' : ''}>Opaque</option> | ||
| <option value="kubernetes.io/tls" ${resource.type === 'kubernetes.io/tls' ? 'selected' : ''}>kubernetes.io/tls</option> | ||
| <option value="kubernetes.io/dockerconfigjson" ${resource.type === 'kubernetes.io/dockerconfigjson' ? 'selected' : ''}>kubernetes.io/dockerconfigjson</option> | ||
| </select> | ||
| </div>`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the Secret resource class and inspect where `type` is declared/read.
fd -t f 'ConfigResources' -x rg -nP -C2 '\b(class\s+Secret\b|this\.type|this\.spec\.type|\.type\s*=|secret.*type)' {}
rg -nP -C2 '\bSecret\b' --type=js -g '!node_modules/**' | rg -nP '(type|spec\.type)' -C1Repository: rohitg00/k8sgames
Length of output: 5762
🏁 Script executed:
# Get the specific lines from index.html where the editor reads/writes Secret type
sed -n '1235,1245p' index.html | cat -n
# Also check line 1383 area
sed -n '1380,1390p' index.html | cat -nRepository: rohitg00/k8sgames
Length of output: 1475
🏁 Script executed:
# Search for how Secret resources are created/initialized when using CommandBar template
rg -n 'spec.*type|secretType' --type=js -C3
# Look for the cluster.update method to understand the proper update mechanism
rg -n 'cluster\.update|this\.engine\.cluster\.update' --type=js -C3Repository: rohitg00/k8sgames
Length of output: 22732
🏁 Script executed:
# Check how new Secret instances are created in the codebase
rg -n 'new Secret|Secret\(' --type=js -C3
# Look for the resource instantiation logic - how template objects become class instances
rg -n 'getResourcesByKind|instantiate|createResource' --type=js -A5 -B2Repository: rohitg00/k8sgames
Length of output: 49108
🏁 Script executed:
# Check how cluster.update works and what it does compared to direct mutation
sed -n '520,540p' js/engine/GameEngine.js | cat -nRepository: rohitg00/k8sgames
Length of output: 963
🏁 Script executed:
# Check more context around line 1383 - where is the update call?
sed -n '1370,1410p' index.html | cat -n
# Also look for save/update functionality in the editor
rg -n 'this\.engine\.cluster\.update|cluster\.update|editor.*save|dialog.*save' index.html -A2 -B2Repository: rohitg00/k8sgames
Length of output: 2836
🏁 Script executed:
# Check how cluster.update method works with the updates parameter
rg -n 'cluster\.update\s*\(' js/engine/ClusterState.js -A10
# Also verify the merge/update logic
rg -n 'update\s*\(' js/engine/ClusterState.js -B2 -A8Repository: rohitg00/k8sgames
Length of output: 840
🏁 Script executed:
# Get the full update method
sed -n '127,180p' js/engine/ClusterState.js | cat -nRepository: rohitg00/k8sgames
Length of output: 2475
Line 1383: Secret type mutation should use the update mechanism, not direct assignment.
The editor mutates resource.type directly (line 1383), then calls cluster.update(uid, updates) with an updates object that doesn't include the type change. The ClusterState.update() method only merges spec, status, and metadata fields—not custom top-level properties like type. This is inconsistent with how other editor fields are handled (e.g., clusterIP, Ingress TLS), which use updates.spec or updates.metadata.
Fix: Change line 1383 to include the type in the updates object:
if (secretType && kind === 'Secret') {
updates.spec = updates.spec || {};
updates.spec.type = secretType.value;
}This aligns with the cluster update pattern and ensures the type change is properly tracked in the update payload.
Additionally: The CommandBar template (line 631, js/ui/CommandBar.js) creates Secrets with { spec: { type: 'Opaque', data: {} } }, but the Secret constructor expects metadata.secretType to initialize the instance property. While this currently works due to the default 'Opaque' value matching the template, consider normalizing the Secret creation path so template structure aligns with constructor expectations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 1234 - 1244, The editor currently mutates
resource.type directly when editing Secrets which is not picked up by
ClusterState.update; instead, when handling the secret-type change (the code
that reads secretType and the block that later calls cluster.update(uid,
updates)), add the type into the updates object under spec (e.g. ensure
updates.spec exists and set updates.spec.type = secretType.value) so
cluster.update(uid, updates) will persist the change; also audit Secret creation
in js/ui/CommandBar.js (the template that creates { spec: { type: 'Opaque',
data: {} } }) and either normalize the template to match the Secret constructor
(which expects metadata.secretType) or update the constructor to read spec.type
so both creation and edits use the same field.
|
|
||
| const memReq = document.getElementById('edit-mem-req'); | ||
| const memLim = document.getElementById('edit-mem-lim'); | ||
| if (memReq && memLim && memReq.value.trim() && memLim.value.trim()) { | ||
| updates.spec.resources = { | ||
| requests: { cpu: '100m', memory: memReq.value.trim() }, | ||
| limits: { cpu: '500m', memory: memLim.value.trim() } | ||
| }; | ||
| } | ||
|
|
||
| const probesCheck = document.getElementById('edit-probes'); | ||
| if (probesCheck && probesCheck.checked) { | ||
| updates.spec.livenessProbe = { httpGet: { path: '/health', port: 80 } }; | ||
| updates.spec.readinessProbe = { httpGet: { path: '/ready', port: 80 } }; | ||
| } else if (probesCheck) { | ||
| updates.spec.livenessProbe = null; | ||
| updates.spec.readinessProbe = null; | ||
| } |
There was a problem hiding this comment.
Save path overwrites existing spec.resources (including CPU) with hardcoded values and drops the save unless both memory fields are filled.
Two issues in the same block:
-
Line 1337‑1340 wholesale replaces
updates.spec.resourceswith a new object whose CPU is hardcoded to100m/500m. If the resource (or a prior template) had CPU requests/limits, ephemeral-storage, GPU, hugepages, etc., they are silently lost on save. Since the UI only exposes memory, the save path should only update memory and preserve any other keys already present. -
Line 1336 requires both
memReqandmemLimto be non-empty. If the user clears one of them intending to unset it, the save is silently dropped (no error, no delete). Combined with the display bug at Lines 1222‑1223, this makes it easy to unintentionally wipe previous memory config by hitting Save.
Separately, Lines 1348‑1349 set livenessProbe/readinessProbe to null when the checkbox is unchecked. Depending on how cluster.update merges updates, this may leave literal null values in the spec (vs. removing the keys); worth verifying that downstream consumers (e.g., renderers, metrics, YAML export) tolerate null.
🛠️ Proposed fix for (1) and (2)
- const memReq = document.getElementById('edit-mem-req');
- const memLim = document.getElementById('edit-mem-lim');
- if (memReq && memLim && memReq.value.trim() && memLim.value.trim()) {
- updates.spec.resources = {
- requests: { cpu: '100m', memory: memReq.value.trim() },
- limits: { cpu: '500m', memory: memLim.value.trim() }
- };
- }
+ const memReqEl = document.getElementById('edit-mem-req');
+ const memLimEl = document.getElementById('edit-mem-lim');
+ if (memReqEl || memLimEl) {
+ const prev = resource.spec?.resources || {};
+ const reqVal = memReqEl?.value.trim();
+ const limVal = memLimEl?.value.trim();
+ updates.spec.resources = {
+ requests: { ...(prev.requests || {}) },
+ limits: { ...(prev.limits || {}) },
+ };
+ if (reqVal) updates.spec.resources.requests.memory = reqVal;
+ else delete updates.spec.resources.requests.memory;
+ if (limVal) updates.spec.resources.limits.memory = limVal;
+ else delete updates.spec.resources.limits.memory;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 1333 - 1350, The save path currently replaces
updates.spec.resources with a hardcoded CPU and requires both memReq and memLim
to be non-empty; change this so the handler reads the existing resources object
(if any) and only updates/sets requests.memory and limits.memory based on
memReq.value.trim() and memLim.value.trim() individually (preserve any existing
cpu, ephemeral-storage, gpu, hugepages, etc.), and if a field is empty treat
that as a deletion (remove the corresponding requests.memory or limits.memory
key or set it undefined so the downstream merger will drop it) rather than
skipping the entire save; also for probesCheck, instead of assigning null to
updates.spec.livenessProbe/readinessProbe when unchecked, remove those keys from
updates.spec (or use delete) so we don't leave literal nulls unless the
downstream update API explicitly requires nulls.
| const clusterIpInput = document.getElementById('edit-cluster-ip'); | ||
| if (clusterIpInput) { | ||
| let val = clusterIpInput.value.trim(); | ||
| if (val) { | ||
| if (val.toLowerCase() === 'none') val = 'None'; | ||
| updates.spec = updates.spec || {}; | ||
| updates.spec.clusterIP = val; | ||
| } | ||
| } |
There was a problem hiding this comment.
Cluster IP field can't be cleared once set.
The save only updates spec.clusterIP when the input is non-empty (Line 1374). If a user wants to remove a previously assigned IP (e.g., to go back to auto-assigned), clearing the field and hitting Save is a no-op. Consider explicitly clearing spec.clusterIP on empty input, or reflect that intent in the helper text.
🛠️ Proposed fix
- const clusterIpInput = document.getElementById('edit-cluster-ip');
- if (clusterIpInput) {
- let val = clusterIpInput.value.trim();
- if (val) {
- if (val.toLowerCase() === 'none') val = 'None';
- updates.spec = updates.spec || {};
- updates.spec.clusterIP = val;
- }
- }
+ const clusterIpInput = document.getElementById('edit-cluster-ip');
+ if (clusterIpInput) {
+ const val = clusterIpInput.value.trim();
+ updates.spec = updates.spec || {};
+ updates.spec.clusterIP = val
+ ? (val.toLowerCase() === 'none' ? 'None' : val)
+ : '';
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 1371 - 1379, The save logic ignores an empty Cluster
IP input so clearing the field is a no-op; update the handler that reads
document.getElementById('edit-cluster-ip') (clusterIpInput) to explicitly set
updates.spec (or ensure updates.spec exists) and assign updates.spec.clusterIP =
'' or delete updates.spec.clusterIP when clusterIpInput.value.trim() is empty
(to represent clearing/auto-assigned intent), preserving the existing behavior
that sets a capitalized 'None' when the user types 'none'; ensure tests or
downstream code expect an empty string or absence of clusterIP and handle
accordingly.
| const ingressTls = document.getElementById('edit-ingress-tls'); | ||
| if (ingressTls && kind === 'Ingress') { | ||
| updates.spec = updates.spec || {}; | ||
| const sn = ingressTls.value.trim(); | ||
| if (sn) { | ||
| updates.spec.tls = [{ hosts: [resource.metadata.name + '.example.com'], secretName: sn }]; | ||
| } else { | ||
| updates.spec.tls = []; | ||
| } | ||
| } |
There was a problem hiding this comment.
Ingress TLS hosts is hardcoded and can drift from the real rule host.
Line 1391 always sets hosts: [resource.metadata.name + '.example.com'], but _showIngressPathPicker (and any later edits) may have set spec.rules[0].host to something different. When that host doesn't match the TLS hosts entry, the TLS config is effectively non-functional for the actual routed host.
Prefer deriving from the existing rule, falling back to the <name>.example.com convention only when no rule host is present:
🛠️ Proposed fix
- if (sn) {
- updates.spec.tls = [{ hosts: [resource.metadata.name + '.example.com'], secretName: sn }];
- } else {
+ if (sn) {
+ const host = resource.spec?.rules?.[0]?.host || `${resource.metadata.name}.example.com`;
+ updates.spec.tls = [{ hosts: [host], secretName: sn }];
+ } else {
updates.spec.tls = [];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const ingressTls = document.getElementById('edit-ingress-tls'); | |
| if (ingressTls && kind === 'Ingress') { | |
| updates.spec = updates.spec || {}; | |
| const sn = ingressTls.value.trim(); | |
| if (sn) { | |
| updates.spec.tls = [{ hosts: [resource.metadata.name + '.example.com'], secretName: sn }]; | |
| } else { | |
| updates.spec.tls = []; | |
| } | |
| } | |
| const ingressTls = document.getElementById('edit-ingress-tls'); | |
| if (ingressTls && kind === 'Ingress') { | |
| updates.spec = updates.spec || {}; | |
| const sn = ingressTls.value.trim(); | |
| if (sn) { | |
| const host = resource.spec?.rules?.[0]?.host || `${resource.metadata.name}.example.com`; | |
| updates.spec.tls = [{ hosts: [host], secretName: sn }]; | |
| } else { | |
| updates.spec.tls = []; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 1386 - 1395, The TLS hosts entry is being hardcoded
to resource.metadata.name + '.example.com' which can drift from the actual
ingress rule host; update the logic in the block that reads ingressTls (and any
code that sets updates.spec.tls) to derive the host from the current rule if
present (e.g. prefer updates.spec.rules[0].host or resource.spec.rules[0].host
when available) and only fall back to resource.metadata.name + '.example.com'
when no rule host exists, then set updates.spec.tls = [{ hosts: [derivedHost],
secretName: sn }] (or [] when sn is empty) so the TLS hosts match the ingress
rule added by _showIngressPathPicker.
| } else if (data.kind === 'Pod') { | ||
| res.status.phase = 'Running'; | ||
| res.setCondition('Ready', 'True', 'ContainersReady'); | ||
| if (res.status.containerStatuses) { | ||
| for (const cs of res.status.containerStatuses) { | ||
| cs.ready = true; | ||
| cs.state = { running: { startedAt: new Date().toISOString() } }; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Heal path for pods: consider also setting ContainersReady and clearing restart/terminated state.
Setting Ready=True with reason 'ContainersReady' is slightly off — elsewhere (SimulationTick.js:300-301) the convention is setCondition('ContainersReady','True','ContainersReady') plus setCondition('Ready','True','PodReady'). Additionally, the container statuses are marked ready = true and state.running is set, but lastState/restartCount from a prior CrashLoopBackOff/OOMKilled aren't cleared, and cs.started isn't set back to true. On the next _tickPodLifecycle/_evaluateProbe iteration, cpu/mem pressure or leftover _probeFailCount could immediately re-crash the container, making "heal" feel flaky.
♻️ Suggested adjustment
} else if (data.kind === 'Pod') {
res.status.phase = 'Running';
- res.setCondition('Ready', 'True', 'ContainersReady');
+ res.setCondition('ContainersReady', 'True', 'ContainersReady');
+ res.setCondition('Ready', 'True', 'PodReady');
if (res.status.containerStatuses) {
for (const cs of res.status.containerStatuses) {
cs.ready = true;
+ cs.started = true;
+ cs._probeFailCount = 0;
cs.state = { running: { startedAt: new Date().toISOString() } };
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (data.kind === 'Pod') { | |
| res.status.phase = 'Running'; | |
| res.setCondition('Ready', 'True', 'ContainersReady'); | |
| if (res.status.containerStatuses) { | |
| for (const cs of res.status.containerStatuses) { | |
| cs.ready = true; | |
| cs.state = { running: { startedAt: new Date().toISOString() } }; | |
| } | |
| } | |
| } | |
| } else if (data.kind === 'Pod') { | |
| res.status.phase = 'Running'; | |
| res.setCondition('ContainersReady', 'True', 'ContainersReady'); | |
| res.setCondition('Ready', 'True', 'PodReady'); | |
| if (res.status.containerStatuses) { | |
| for (const cs of res.status.containerStatuses) { | |
| cs.ready = true; | |
| cs.started = true; | |
| cs._probeFailCount = 0; | |
| cs.state = { running: { startedAt: new Date().toISOString() } }; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/engine/GameEngine.js` around lines 224 - 233, The pod "heal" branch
currently sets Ready=True with reason 'ContainersReady' and marks
containerStatuses as running but doesn't follow the project's condition
conventions nor clear prior failure state; update the block handling data.kind
=== 'Pod' to: 1) set conditions exactly as elsewhere
(setCondition('ContainersReady','True','ContainersReady') and
setCondition('Ready','True','PodReady')), 2) for each entry in
res.status.containerStatuses ensure cs.ready = true, cs.started = true (or
equivalent flag used elsewhere), clear cs.lastState (remove terminated/waiting
details) and reset cs.restartCount to 0, and 3) also clear any probe/failure
counters (e.g., _probeFailCount or similar) associated with the pod/container so
the next _tickPodLifecycle/_evaluateProbe won't immediately re-crash it; locate
usages via setCondition and res.status.containerStatuses to implement these
updates.
| this.eventBus.on('resource:rescheduled', (data) => { | ||
| if (!data.target || !this.cluster) return; | ||
| const pods = this.cluster.getResourcesByKind('Pod') || []; | ||
| const isNode = (this.cluster.getResourcesByKind('Node') || []).some(n => n.metadata?.name === data.target); | ||
|
|
||
| if (isNode) { | ||
| const podsOnNode = pods.filter(p => p.spec?.nodeName === data.target); | ||
| for (const p of podsOnNode) { | ||
| this.cluster.remove(p.uid); | ||
| } | ||
| } else { | ||
| const podToDelete = pods.find(p => p.metadata?.name === data.target); | ||
| if (podToDelete) { | ||
| this.cluster.remove(podToDelete.uid); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm all emitters of resource:rescheduled and whether any pass a kind discriminator.
rg -nP -C2 "emit\(\s*['\"]resource:rescheduled['\"]"Repository: rohitg00/k8sgames
Length of output: 395
🏁 Script executed:
#!/bin/bash
# 1. Get context around the resource:rescheduled emit in IncidentEngine
echo "=== IncidentEngine.js context around emit ==="
sed -n '430,450p' js/engine/IncidentEngine.js | cat -nRepository: rohitg00/k8sgames
Length of output: 985
🏁 Script executed:
#!/bin/bash
# 2. Get the full handler in GameEngine.js lines 237-253
echo "=== GameEngine.js handler (lines 237-253) ==="
sed -n '237,253p' js/engine/GameEngine.js | cat -nRepository: rohitg00/k8sgames
Length of output: 897
🏁 Script executed:
#!/bin/bash
# 3. Find cluster.remove() implementation
echo "=== Searching for cluster.remove definition ==="
rg -nP "remove\s*\(" js/engine/ | grep -i "cluster\|class" | head -20
</parameter>Repository: rohitg00/k8sgames
Length of output: 857
🏁 Script executed:
#!/bin/bash
# 4. Search for _evictedOwnerCounts in SimulationTick
echo "=== _evictedOwnerCounts usage ==="
rg -nP "_evictedOwnerCounts" --context 3Repository: rohitg00/k8sgames
Length of output: 1755
🏁 Script executed:
#!/bin/bash
# Read cluster.remove() implementation in ClusterState.js
echo "=== ClusterState.js remove() method ==="
sed -n '171,200p' js/engine/ClusterState.js | cat -nRepository: rohitg00/k8sgames
Length of output: 1194
🏁 Script executed:
#!/bin/bash
# Search for cluster:deleted:Pod emissions
echo "=== cluster:deleted:Pod emissions ==="
rg -nP "cluster:deleted" --context 2Repository: rohitg00/k8sgames
Length of output: 1203
🏁 Script executed:
#!/bin/bash
# Check what remove() method does - full context
echo "=== Full remove() method ==="
sed -n '171,195p' js/engine/ClusterState.js | cat -nRepository: rohitg00/k8sgames
Length of output: 1061
🏁 Script executed:
#!/bin/bash
# Verify PodEviction incident structure - find where incidents are created/defined
echo "=== PodEviction incident definitions ==="
rg -nP "PodEviction" --context 3 | head -40Repository: rohitg00/k8sgames
Length of output: 2545
🏁 Script executed:
#!/bin/bash
# Search for _emitEvent implementation to understand how cluster:deleted:Pod is emitted
echo "=== _emitEvent implementation ==="
rg -nP "_emitEvent\(" js/engine/ClusterState.js -A 5 | head -50Repository: rohitg00/k8sgames
Length of output: 1066
🏁 Script executed:
#!/bin/bash
# Search for where cluster:deleted events are emitted
echo "=== Searching for emit cluster:deleted ==="
rg -nP "emit.*cluster:deleted" --context 2Repository: rohitg00/k8sgames
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Check if cluster.remove() is followed by emit event
echo "=== Full remove() method end section ==="
sed -n '171,210p' js/engine/ClusterState.js | cat -nRepository: rohitg00/k8sgames
Length of output: 1553
🏁 Script executed:
#!/bin/bash
# Find where the event bus transforms or constructs event names with cluster: prefix
echo "=== Event bus implementation search ==="
fd -e js | xargs grep -l "eventBus" | head -5Repository: rohitg00/k8sgames
Length of output: 179
🏁 Script executed:
#!/bin/bash
# Check how _emitEvent calls are made and if they're batched/transformed
echo "=== _emitEvent calls in ClusterState ==="
rg -nP "_emitEvent\(" js/engine/ClusterState.js | head -20Repository: rohitg00/k8sgames
Length of output: 422
🏁 Script executed:
#!/bin/bash
# Look at eventBus implementation - likely in index.js or GameEngine
echo "=== EventBus or event handling pattern ==="
rg -nP "class.*EventBus|new.*EventBus|emit\(" js/engine/GameEngine.js | head -30Repository: rohitg00/k8sgames
Length of output: 1872
🏁 Script executed:
#!/bin/bash
# Get full context of how cluster:deleted:Pod listener is attached in SimulationTick
echo "=== SimulationTick eventBus listener setup ==="
sed -n '1,30p' js/engine/SimulationTick.js | cat -nRepository: rohitg00/k8sgames
Length of output: 1346
🏁 Script executed:
#!/bin/bash
# Check full EventBus implementation - how does it handle event names?
echo "=== EventBus class full implementation ==="
sed -n '5,60p' js/engine/GameEngine.js | cat -nRepository: rohitg00/k8sgames
Length of output: 1920
🏁 Script executed:
#!/bin/bash
# Check if ClusterState connects its _emitEvent to the eventBus
echo "=== ClusterState constructor and event binding ==="
sed -n '1,100p' js/engine/ClusterState.js | cat -nRepository: rohitg00/k8sgames
Length of output: 3652
🏁 Script executed:
#!/bin/bash
# Search for how cluster events are emitted - check the batchedEvents mechanism
echo "=== ClusterState batchedEvents and event flow ==="
sed -n '55,85p' js/engine/ClusterState.js | cat -nRepository: rohitg00/k8sgames
Length of output: 1022
Add kind discriminator to PodEviction event payload to resolve node-vs-pod name collision.
When a pod and node share a name (rare but possible in a simulated cluster), the handler at lines 237-253 checks for isNode first and evicts only pods on that node — the target pod is never removed. The fix is straightforward: include kind: 'Pod' in the emit at js/engine/IncidentEngine.js:441, matching the pattern used by resource:healed events for other incident types.
Additionally, cluster.remove(p.uid) emits cluster:deleted:Pod, which is only processed by _evictedOwnerCounts in SimulationTick if the pod has ownerReferences (line 13). Bare pods will be silently evicted with no controller replacement — confirm this is the intended UX for PodEviction on standalone pods.
| if (bestPV) { | ||
| if (typeof pvc.bind === 'function') { | ||
| pvc.bind(bestPV.metadata.name); | ||
| } else { | ||
| pvc.setPhase('Bound'); | ||
| pvc.spec = pvc.spec || {}; | ||
| pvc.spec.volumeName = bestPV.metadata.name; | ||
| pvc.recordEvent('Normal', 'Bound', `Bound to pv ${bestPV.metadata.name}`); | ||
| } | ||
|
|
||
| if (typeof bestPV.bind === 'function') { | ||
| bestPV.bind(pvc.metadata.name, pvc.metadata.namespace, pvc.metadata.uid); | ||
| } else { | ||
| bestPV.setPhase('Bound'); | ||
| bestPV.spec = bestPV.spec || {}; | ||
| bestPV.spec.claimRef = { name: pvc.metadata.name, namespace: pvc.metadata.namespace, uid: pvc.metadata.uid }; | ||
| bestPV.recordEvent('Normal', 'Bound', `Bound to pvc ${pvc.metadata.name}`); | ||
| } | ||
|
|
||
| bestPV.phase = 'Bound'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether PVC instances expose uid on metadata or only as a direct property.
rg -nP -C2 '\b(uid|metadata\.uid)\s*=' js/resources/ResourceBase.js js/resources/StorageResources.js
echo "---"
# Also see how PV.bind is called elsewhere to compare uid source.
rg -nP -C2 '\.bind\(' js/engine/ js/resources/Repository: rohitg00/k8sgames
Length of output: 2486
🏁 Script executed:
# Look at PersistentVolumeClaim constructor and uid assignment
rg -nP -A 15 'class PersistentVolumeClaim' js/resources/StorageResources.js
# Also check how uid is assigned in constructor
rg -nP -A 20 'constructor\s*\(' js/resources/StorageResources.js | head -80Repository: rohitg00/k8sgames
Length of output: 3505
🏁 Script executed:
# Find the bind() method implementation in PersistentVolume or its parent
ast-grep --pattern 'bind($_, $_, $_) { $$$ }' js/resources/
# Also search for bind with fewer parameters
rg -nP -B 2 -A 8 'bind\s*\(' js/resources/StorageResources.jsRepository: rohitg00/k8sgames
Length of output: 750
🏁 Script executed:
# Find setPhase implementation to confirm it sets phase
rg -nP -B 2 -A 5 'setPhase\s*\(' js/resources/ | head -60Repository: rohitg00/k8sgames
Length of output: 955
🏁 Script executed:
# Find ResourceBase constructor
rg -nP -B 5 -A 30 'class ResourceBase' js/resources/ResourceBase.js | head -80Repository: rohitg00/k8sgames
Length of output: 1542
🏁 Script executed:
# Search for where metadata.uid is set in ResourceBase constructor
rg -nP -B 3 -A 10 'this\.metadata\s*=' js/resources/ResourceBase.js | head -60Repository: rohitg00/k8sgames
Length of output: 1182
Remove the redundant phase assignment at line 1038.
The concern about pvc.metadata.uid is incorrect—ResourceBase's constructor initializes this.metadata.uid = generateUID() for all resource instances, including PVC.
However, line 1038 (bestPV.phase = 'Bound') is genuinely redundant: bestPV.bind() already sets this.phase = 'Bound' (line 35 of StorageResources.js), and the fallback bestPV.setPhase('Bound') is also called. The extra assignment is safe to remove.
♻️ Suggested cleanup
- bestPV.phase = 'Bound';
- }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/engine/SimulationTick.js` around lines 1019 - 1039, Remove the redundant
direct assignment bestPV.phase = 'Bound' in SimulationTick.js: the code already
calls bestPV.bind(...) (which sets this.phase = 'Bound') or the fallback
bestPV.setPhase('Bound'), so delete the extra bestPV.phase = 'Bound' line to
avoid duplication; locate the block handling PVC/PV binding (references:
pvc.bind, pvc.setPhase, bestPV.bind, bestPV.setPhase) and remove only that final
explicit phase assignment.
There was a problem hiding this comment.
Pull request overview
This PR improves in-game Kubernetes resource configuration and simulation fidelity by expanding the inspector/editor and command bar, and by centralizing PVC↔PV binding plus adding new engine event handlers.
Changes:
- Add
--clusterip/--clusterip=support forkubectl create servicein the command bar. - Replace PVC self-binding timers with a per-tick, best-fit PVC→PV auto-binding routine.
- Add new inspector editing fields (memory, probes, clusterIP) and implement
resource:healed/resource:rescheduledevent handlers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
js/ui/CommandBar.js |
Parses --clusterip flags to set Service spec.clusterIP at creation time. |
js/resources/StorageResources.js |
Removes the PVC timer-based self-binding behavior. |
js/engine/SimulationTick.js |
Adds _tickPVCBinding() to bind Pending PVCs to Available PVs using best-fit selection. |
js/engine/GameEngine.js |
Adds handlers for healed/rescheduled events to restore or evict resources. |
index.html |
Extends the edit dialog UI to edit workload memory/probes and Service clusterIP. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <input id="edit-mem-req" type="text" value="${cpuReq ? memReq : ''}" placeholder="Req e.g. 128Mi" class="w-1/2 px-3 py-1.5 text-sm text-white bg-black/40 border border-white/15 rounded-lg outline-none focus:border-sky-500/50 font-mono" /> | ||
| <input id="edit-mem-lim" type="text" value="${cpuLim ? memLim : ''}" placeholder="Lim e.g. 256Mi" class="w-1/2 px-3 py-1.5 text-sm text-white bg-black/40 border border-white/15 rounded-lg outline-none focus:border-sky-500/50 font-mono" /> |
There was a problem hiding this comment.
The memory inputs are pre-filled only when cpuReq / cpuLim are present (value="${cpuReq ? memReq : ''}"), which will hide existing memory values when CPU requests/limits aren’t set (and also prevents showing the defaults you compute). Use the memory fields themselves to decide the default (memReq/memLim) rather than gating on CPU.
| <input id="edit-mem-req" type="text" value="${cpuReq ? memReq : ''}" placeholder="Req e.g. 128Mi" class="w-1/2 px-3 py-1.5 text-sm text-white bg-black/40 border border-white/15 rounded-lg outline-none focus:border-sky-500/50 font-mono" /> | |
| <input id="edit-mem-lim" type="text" value="${cpuLim ? memLim : ''}" placeholder="Lim e.g. 256Mi" class="w-1/2 px-3 py-1.5 text-sm text-white bg-black/40 border border-white/15 rounded-lg outline-none focus:border-sky-500/50 font-mono" /> | |
| <input id="edit-mem-req" type="text" value="${memReq}" placeholder="Req e.g. 128Mi" class="w-1/2 px-3 py-1.5 text-sm text-white bg-black/40 border border-white/15 rounded-lg outline-none focus:border-sky-500/50 font-mono" /> | |
| <input id="edit-mem-lim" type="text" value="${memLim}" placeholder="Lim e.g. 256Mi" class="w-1/2 px-3 py-1.5 text-sm text-white bg-black/40 border border-white/15 rounded-lg outline-none focus:border-sky-500/50 font-mono" /> |
| if (val) { | ||
| if (val.toLowerCase() === 'none') val = 'None'; | ||
| updates.spec = updates.spec || {}; | ||
| updates.spec.clusterIP = val; |
There was a problem hiding this comment.
The Cluster IP field only updates spec.clusterIP when a non-empty value is provided. If a Service already has a clusterIP set, clearing the input won’t remove/reset it back to auto-assign behavior. Consider explicitly clearing the field when the input is empty (e.g., delete spec.clusterIP or set it to a sentinel that cluster.update interprets as removal).
| if (val) { | |
| if (val.toLowerCase() === 'none') val = 'None'; | |
| updates.spec = updates.spec || {}; | |
| updates.spec.clusterIP = val; | |
| updates.spec = updates.spec || {}; | |
| if (val) { | |
| if (val.toLowerCase() === 'none') val = 'None'; | |
| updates.spec.clusterIP = val; | |
| } else { | |
| updates.spec.clusterIP = null; |
| const pvcClass = pvc.spec?.storageClassName || 'standard'; | ||
| const pvcStorage = pvc.spec?.resources?.requests?.storage || '10Gi'; | ||
| const pvcCap = pvc.capacityBytes !== undefined ? pvc.capacityBytes : this.cluster._parseMemory(pvcStorage); | ||
|
|
||
| for (const pv of pvs) { | ||
| const pvPhase = pv.phase || pv.status?.phase; | ||
| if (pvPhase !== 'Available' && pvPhase !== 'Pending') continue; | ||
| if (pv.spec?.claimRef) continue; | ||
|
|
||
| const pvClass = pv.spec?.storageClassName || 'standard'; | ||
| if (pvClass !== pvcClass) continue; | ||
|
|
||
| const pvStorage = pv.spec?.capacity?.storage || '10Gi'; | ||
| const pvCap = pv.capacityBytes !== undefined ? pv.capacityBytes : this.cluster._parseMemory(pvStorage); | ||
|
|
||
| if (pvCap >= pvcCap) { | ||
| if (!bestPV || pvCap < bestPVCap) { |
There was a problem hiding this comment.
PVC/PV capacity matching mixes units: capacityBytes from StorageResources is in bytes, but the fallback uses this.cluster._parseMemory(...) which returns Mi units. If one side is a class instance (has capacityBytes) and the other is a plain ResourceBase (no capacityBytes), comparisons like pvCap >= pvcCap can be wrong (e.g., a 1Gi PV in bytes will appear to satisfy a 2Gi PVC in Mi). Use a single storage parser that returns the same unit (prefer bytes, like parseStorage) for both PV and PVC requests/capacity.
| if (bestPV) { | ||
| if (typeof pvc.bind === 'function') { | ||
| pvc.bind(bestPV.metadata.name); | ||
| } else { | ||
| pvc.setPhase('Bound'); | ||
| pvc.spec = pvc.spec || {}; | ||
| pvc.spec.volumeName = bestPV.metadata.name; | ||
| pvc.recordEvent('Normal', 'Bound', `Bound to pv ${bestPV.metadata.name}`); |
There was a problem hiding this comment.
When a PVC has a bind() method, this path calls pvc.bind(...) but doesn’t set pvc.spec.volumeName, while the fallback path does. That leads to inconsistent state/YAML output depending on whether the PVC is a StorageResources instance vs a generic ResourceBase. Consider setting pvc.spec.volumeName (and/or updating the PVC class bind() implementation) so both paths produce the same binding fields.
| if (!data.target || !this.cluster) return; | ||
| const pods = this.cluster.getResourcesByKind('Pod') || []; | ||
| const isNode = (this.cluster.getResourcesByKind('Node') || []).some(n => n.metadata?.name === data.target); | ||
|
|
||
| if (isNode) { | ||
| const podsOnNode = pods.filter(p => p.spec?.nodeName === data.target); | ||
| for (const p of podsOnNode) { | ||
| this.cluster.remove(p.uid); | ||
| } | ||
| } else { | ||
| const podToDelete = pods.find(p => p.metadata?.name === data.target); |
There was a problem hiding this comment.
resource:rescheduled infers whether data.target is a Node by checking for a Node with the same name. That’s ambiguous if pod and node names can collide, and it ignores any data.kind that may be emitted with the event. Prefer using an explicit data.kind (or separate events) and include namespace for Pod targets so the correct resource is evicted.
| if (!data.target || !this.cluster) return; | |
| const pods = this.cluster.getResourcesByKind('Pod') || []; | |
| const isNode = (this.cluster.getResourcesByKind('Node') || []).some(n => n.metadata?.name === data.target); | |
| if (isNode) { | |
| const podsOnNode = pods.filter(p => p.spec?.nodeName === data.target); | |
| for (const p of podsOnNode) { | |
| this.cluster.remove(p.uid); | |
| } | |
| } else { | |
| const podToDelete = pods.find(p => p.metadata?.name === data.target); | |
| if (!data.target || !data.kind || !this.cluster) return; | |
| const pods = this.cluster.getResourcesByKind('Pod') || []; | |
| if (data.kind === 'Node') { | |
| const podsOnNode = pods.filter(p => p.spec?.nodeName === data.target); | |
| for (const p of podsOnNode) { | |
| this.cluster.remove(p.uid); | |
| } | |
| } else if (data.kind === 'Pod' && data.namespace) { | |
| const podToDelete = pods.find( | |
| p => p.metadata?.name === data.target && p.metadata?.namespace === data.namespace | |
| ); |
| const ipArgIdx = args.indexOf('--clusterip'); | ||
| if (ipArgIdx !== -1 && args[ipArgIdx + 1] && kind === 'Service') { | ||
| let ipVal = args[ipArgIdx + 1]; | ||
| if (ipVal.toLowerCase() === 'none') ipVal = 'None'; | ||
| def.spec.clusterIP = ipVal; | ||
| } else { | ||
| const inlineIp = args.find(a => a.startsWith('--clusterip=')); | ||
| if (inlineIp && kind === 'Service') { | ||
| let ipVal = inlineIp.split('=')[1]; |
There was a problem hiding this comment.
For the inline form --clusterip=..., this will set def.spec.clusterIP even when the value is empty (e.g. --clusterip=), producing a Service with clusterIP: "". Consider validating the parsed value and only setting clusterIP when it’s non-empty (or treating an empty value as “auto-assign”).
| const ipArgIdx = args.indexOf('--clusterip'); | |
| if (ipArgIdx !== -1 && args[ipArgIdx + 1] && kind === 'Service') { | |
| let ipVal = args[ipArgIdx + 1]; | |
| if (ipVal.toLowerCase() === 'none') ipVal = 'None'; | |
| def.spec.clusterIP = ipVal; | |
| } else { | |
| const inlineIp = args.find(a => a.startsWith('--clusterip=')); | |
| if (inlineIp && kind === 'Service') { | |
| let ipVal = inlineIp.split('=')[1]; | |
| if (kind === 'Service') { | |
| let ipVal; | |
| const ipArgIdx = args.indexOf('--clusterip'); | |
| if (ipArgIdx !== -1 && args[ipArgIdx + 1]) { | |
| ipVal = args[ipArgIdx + 1]; | |
| } else { | |
| const inlineIp = args.find(a => a.startsWith('--clusterip=')); | |
| if (inlineIp) { | |
| const parsedIp = inlineIp.slice('--clusterip='.length); | |
| if (parsedIp) ipVal = parsedIp; | |
| } | |
| } | |
| if (ipVal) { |
| updates.spec.resources = { | ||
| requests: { cpu: '100m', memory: memReq.value.trim() }, | ||
| limits: { cpu: '500m', memory: memLim.value.trim() } | ||
| }; |
There was a problem hiding this comment.
This writes the new memory values to updates.spec.resources and also hard-codes CPU values. In this codebase, workload resource requests/limits live on the pod template containers (spec.template.spec.containers[].resources) and are what scheduling/quota/health logic reads, so these updates won’t affect the pods created from the workload. Update the pod template container resources instead (and preserve any existing CPU values rather than overwriting them with constants).
| updates.spec.resources = { | |
| requests: { cpu: '100m', memory: memReq.value.trim() }, | |
| limits: { cpu: '500m', memory: memLim.value.trim() } | |
| }; | |
| const existingContainers = (((resource || {}).spec || {}).template || {}).spec && Array.isArray(resource.spec.template.spec.containers) | |
| ? resource.spec.template.spec.containers | |
| : []; | |
| if (existingContainers.length > 0) { | |
| updates.spec.template = updates.spec.template || {}; | |
| updates.spec.template.spec = updates.spec.template.spec || {}; | |
| updates.spec.template.spec.containers = existingContainers.map((container) => { | |
| const existingResources = container.resources || {}; | |
| const existingRequests = existingResources.requests || {}; | |
| const existingLimits = existingResources.limits || {}; | |
| return { | |
| ...container, | |
| resources: { | |
| ...existingResources, | |
| requests: { | |
| ...existingRequests, | |
| memory: memReq.value.trim() | |
| }, | |
| limits: { | |
| ...existingLimits, | |
| memory: memLim.value.trim() | |
| } | |
| } | |
| }; | |
| }); | |
| } |
| if (probesCheck && probesCheck.checked) { | ||
| updates.spec.livenessProbe = { httpGet: { path: '/health', port: 80 } }; | ||
| updates.spec.readinessProbe = { httpGet: { path: '/ready', port: 80 } }; | ||
| } else if (probesCheck) { | ||
| updates.spec.livenessProbe = null; | ||
| updates.spec.readinessProbe = null; |
There was a problem hiding this comment.
Probe configuration is being applied to the workload object (updates.spec.livenessProbe/readinessProbe), but the simulation evaluates probes on pod containers (pod.spec.containers[i].livenessProbe/readinessProbe). As-is, enabling this toggle won’t actually affect health checks for pods created from Deployments/StatefulSets/ReplicaSets. Apply/remove these probes on the pod template containers instead (e.g., spec.template.spec.containers[].*Probe).
| if (probesCheck && probesCheck.checked) { | |
| updates.spec.livenessProbe = { httpGet: { path: '/health', port: 80 } }; | |
| updates.spec.readinessProbe = { httpGet: { path: '/ready', port: 80 } }; | |
| } else if (probesCheck) { | |
| updates.spec.livenessProbe = null; | |
| updates.spec.readinessProbe = null; | |
| if (probesCheck) { | |
| updates.spec = updates.spec || {}; | |
| updates.spec.template = updates.spec.template || {}; | |
| updates.spec.template.spec = updates.spec.template.spec || {}; | |
| updates.spec.template.spec.containers = updates.spec.template.spec.containers || [{}]; | |
| updates.spec.template.spec.containers[0] = updates.spec.template.spec.containers[0] || {}; | |
| if (probesCheck.checked) { | |
| updates.spec.template.spec.containers[0].livenessProbe = { httpGet: { path: '/health', port: 80 } }; | |
| updates.spec.template.spec.containers[0].readinessProbe = { httpGet: { path: '/ready', port: 80 } }; | |
| } else { | |
| updates.spec.template.spec.containers[0].livenessProbe = null; | |
| updates.spec.template.spec.containers[0].readinessProbe = null; | |
| } |
| res.status.phase = 'Running'; | ||
| res.setCondition('Ready', 'True', 'KubeletReady'); | ||
| } else if (data.kind === 'Pod') { | ||
| res.status.phase = 'Running'; |
There was a problem hiding this comment.
This handler mutates res.status.phase directly. Elsewhere the codebase uses setPhase()/setStatus() to keep resourceVersion/events consistent; bypassing that can cause missed phase-change events and stale indexes/UI. Use res.setPhase('Running') (or setStatus) for both Node and Pod healing.
| res.status.phase = 'Running'; | |
| res.setCondition('Ready', 'True', 'KubeletReady'); | |
| } else if (data.kind === 'Pod') { | |
| res.status.phase = 'Running'; | |
| res.setPhase('Running'); | |
| res.setCondition('Ready', 'True', 'KubeletReady'); | |
| } else if (data.kind === 'Pod') { | |
| res.setPhase('Running'); |
Summary
This PR addresses several gaps in the inspector panel, simulation engine, and command bar that were preventing correct resource configuration and level validation.
Changes
Inspector Panel - index.html
Simulation Engine - js/engine/SimulationTick.js
Storage Resources - js/resources/StorageResources.js
Game Engine - js/engine/GameEngine.js
Command Bar - js/ui/CommandBar.js
Summary by CodeRabbit