[replay #7511] test(computing-unit): cover the pod lookup, creation and deletion paths - #14
Open
sshiv012 wants to merge 2 commits into
Open
[replay #7511] test(computing-unit): cover the pod lookup, creation and deletion paths#14sshiv012 wants to merge 2 commits into
sshiv012 wants to merge 2 commits into
Conversation
KubernetesClientSpec covered the pure fabric8-to-map transforms and the namespace-wide wrappers, and stopped there. The single-pod half of the class - getPodByName, podExists, getPodLimits, createPod, deletePod and the pod URI - was untested, which is 42 of the file's 62 lines. None of it needs a cluster. The fabric8 client is already a constructor parameter, and the existing spec's Mockito plumbing extends to the rest of the fluent chain: withName(...).get() for the lookups and resource(pod).inNamespace(...).create() for creation, with the built pod captured and inspected. Adds 8 tests. The ones worth having are the guard that refuses to overwrite a live pod, the null-to-None wrapper that fabric8 requires, and the shared-memory volume appearing only when a size is requested. No production file is touched.
📝 WalkthroughWalkthroughThe pull request adds mocked Kubernetes client tests for pod lookup, resource limits, creation configuration, optional shared-memory volumes, and deletion. ChangesKubernetes client test coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
|
Owner
Author
|
@coderabbitai review |
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@computing-unit-managing-service/src/test/scala/org/apache/texera/service/util/KubernetesClientSpec.scala`:
- Around line 257-281: Add verifications to the test “build the pod from the
requested limits and env, and create it in the namespace” after invoking
createPod: verify namespaceable.inNamespace(namespace) and resource.create() in
addition to the existing client.resource capture, preserving the current
pod-content assertions.
- Around line 236-243: Extend the “getPodLimits” test to cover a found pod whose
first container has null ResourceRequirements and one whose resources have no
limits map, both expecting an empty map. Update KubernetesClient.getPodLimits to
null-safely handle getResources and getLimits while preserving the existing
limits-map result and missing-pod fallback.
- Around line 301-309: Extend the test around the withShm result from
build(Some("1Gi")) to inspect the computing-unit-master container’s
VolumeMounts, asserting that the dshm volume is mounted at /dev/shm. Keep the
existing dshm volume and no-volume assertions unchanged.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 23089b9f-fc7e-4472-9454-f2ba4625b977
📒 Files selected for processing (1)
computing-unit-managing-service/src/test/scala/org/apache/texera/service/util/KubernetesClientSpec.scala
Comment on lines
+236
to
+243
| "getPodLimits" should "read the first container's limits and fall back to an empty map" in { | ||
| val name = KubernetesClient.generatePodName(3) | ||
| val withLimits = | ||
| clientWithNamedPod(name, podWithLimits(3, Map("cpu" -> "2", "memory" -> "4Gi")))._1 | ||
| val missing = clientWithNamedPod(name, null)._1 | ||
|
|
||
| new KubernetesClient(withLimits).getPodLimits(3) shouldBe Map("cpu" -> "2", "memory" -> "4Gi") | ||
| new KubernetesClient(missing).getPodLimits(3) shouldBe empty |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Test pods that have no resource limits.
This test covers only a missing pod. It does not cover a found pod whose first container has no ResourceRequirements or no limits map.
KubernetesClient.getPodLimits dereferences container.getResources.getLimits, so either valid configuration can cause a null-pointer failure instead of returning the documented empty map. Add these cases and make the lookup null-safe.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@computing-unit-managing-service/src/test/scala/org/apache/texera/service/util/KubernetesClientSpec.scala`
around lines 236 - 243, Extend the “getPodLimits” test to cover a found pod
whose first container has null ResourceRequirements and one whose resources have
no limits map, both expecting an empty map. Update KubernetesClient.getPodLimits
to null-safely handle getResources and getLimits while preserving the existing
limits-map result and missing-pod fallback.
Comment on lines
+257
to
+281
| it should "build the pod from the requested limits and env, and create it in the namespace" in { | ||
| val name = KubernetesClient.generatePodName(5) | ||
| val (client, _) = clientWithNamedPod(name, null) | ||
| val namespaceable = mock(classOf[NamespaceableResource[Pod]]) | ||
| val resource = mock(classOf[Resource[Pod]]) | ||
| val captor = ArgumentCaptor.forClass(classOf[Pod]) | ||
| when(client.resource(any(classOf[Pod]))).thenReturn(namespaceable) | ||
| when(namespaceable.inNamespace(namespace)).thenReturn(resource) | ||
| // create()'s return value is not asserted; the pod is inspected through the captor below. | ||
| when(resource.create()).thenReturn(null) | ||
|
|
||
| new KubernetesClient(client).createPod(5, "2", "4Gi", "1", Map("UID" -> 9, "MODE" -> "batch")) | ||
|
|
||
| verify(client).resource(captor.capture()) | ||
| val built = captor.getValue | ||
| built.getSpec.getHostname shouldBe name | ||
| built.getSpec.getSubdomain shouldBe KubernetesConfig.computeUnitServiceName | ||
| val container = built.getSpec.getContainers.asScala.head | ||
| val limits = container.getResources.getLimits.asScala.map { case (k, v) => k -> v.toString } | ||
| limits("cpu") shouldBe "2" | ||
| limits("memory") shouldBe "4Gi" | ||
| // Env values arrive as Any and reach the container as strings. | ||
| container.getEnv.asScala.map(e => e.getName -> e.getValue).toMap shouldBe | ||
| Map("UID" -> "9", "MODE" -> "batch") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify the pod creation call.
The test stubs inNamespace(namespace) and create(), but it does not verify either call. If createPod stops after client.resource(pod), this test still passes.
Verify namespaceable.inNamespace(namespace) and resource.create() after the call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@computing-unit-managing-service/src/test/scala/org/apache/texera/service/util/KubernetesClientSpec.scala`
around lines 257 - 281, Add verifications to the test “build the pod from the
requested limits and env, and create it in the namespace” after invoking
createPod: verify namespaceable.inNamespace(namespace) and resource.create() in
addition to the existing client.resource capture, preserving the current
pod-content assertions.
Comment on lines
+301
to
+309
| val withShm = build(Some("1Gi")) | ||
| withShm.getSpec.getVolumes.asScala.map(_.getName) should contain("dshm") | ||
| withShm.getSpec.getVolumes.asScala | ||
| .find(_.getName == "dshm") | ||
| .flatMap(v => Option(v.getEmptyDir)) | ||
| .map(_.getSizeLimit.toString) shouldBe Some("1Gi") | ||
|
|
||
| Option(build(None).getSpec.getVolumes).map(_.asScala.map(_.getName)).getOrElse(Nil) should | ||
| not contain "dshm" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the /dev/shm container mount.
The test checks the dshm volume only. It does not check that computing-unit-master mounts dshm at /dev/shm.
Assert the container VolumeMount name and mount path. A volume without this mount does not configure shared memory for the container.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@computing-unit-managing-service/src/test/scala/org/apache/texera/service/util/KubernetesClientSpec.scala`
around lines 301 - 309, Extend the test around the withShm result from
build(Some("1Gi")) to inspect the computing-unit-master container’s
VolumeMounts, asserting that the dshm volume is mounted at /dev/shm. Keep the
existing dshm volume and no-volume assertions unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replay of apache#7511 for CodeRabbit evaluation.
merge-base
a61e1ee4a8b0head987bad4471c9stratumscalaSynthetic evaluation PR. Do not merge.
KubernetesClientSpec.nullconversion, and shared-memory volume configuration.