Add PodContainerSource Extension Point
#1693
Conversation
| * this implementation. | ||
| * @see PodSpec#getContainers() | ||
| */ | ||
| @Extension(ordinal = Double.MAX_VALUE) // this extension goes always first |
There was a problem hiding this comment.
Is it really needed? I think it's easier to leave the default and let implementations pick whether they should appear before or later.
There was a problem hiding this comment.
Since containers are unique I suppose it doesn't really matter which goes first. Maybe a minor optimization 1 way or the other. I'll remove it.
| * @param containerName name of container to lookup | ||
| * @return working directory path if container found and working dir specified, otherwise empty | ||
| */ | ||
| public abstract Optional<String> getContainerWorkingDir(@NonNull Pod pod, @NonNull String containerName); |
There was a problem hiding this comment.
Wondering whether we have any guarantee that a given name can't be used by both containers and ephemeral containers, as it would create an ambiguity...
There was a problem hiding this comment.
Also, would public abstract Optional<Container> getContainerWorkingDir(Pod, String) be more flexible?
There was a problem hiding this comment.
Wondering whether we have any guarantee that a given name can't be used by both containers and ephemeral containers, as it would create an ambiguity...
https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#Container
Name of the container specified as a DNS_LABEL. Each container in a pod must have a unique name (DNS_LABEL). Cannot be updated.
This is confirmed if you attempt to apply a spec with duplicate names. It doesn't matter if they are defined in containers, initContainers, or ephemeralContainers.
❯ kubectl apply -f deployment-with-dup-container-names.yaml
The Deployment "foo" is invalid: spec.template.spec.initContainers[0].name: Duplicate value: "a"
Also, would
public abstract Optional<Container> getContainerWorkingDir(Pod, String)be more flexible?
Yes, I will refactor to return the Container instance.
There was a problem hiding this comment.
Also, would
public abstract Optional<Container> getContainerWorkingDir(Pod, String)be more flexible?
I take back what I said 😄 . While it makes sense at first, fabric8 models Container and EphemeralContainer as different types. From the previous PR it sounded like the whole purpose of this extension is to isolate ephemeral container references to extension plugins, so this extension is to abstract over those differences.
In a future PR I will be adding 1 more method to PodContainerSource to get the container status. Statuses are not properties of Container or EphemeralContainer, they are accesses via pod.getStatus().getContainerStatuses().
public abstract Optional<ContainerStatus> getContainerStatus(@NonNull Pod pod, @NonNull String containerName);These 2 methods will allow my extension to reuse ContainerExecDecorator, as shown in this PR, and display ephemeral container logs (future PR that updates KubernetesComputer#goContainerLog).
ContainerExecDecorator does A LOT of complicated stuff which would be a shame to replicate and maintain separately given this small abstraction makes it reusable. Hopefully we can figure out an acceptable solution.
There was a problem hiding this comment.
fabric8 models Container and EphemeralContainer as different types
😞
Indeed, I don't see anything better.
cronik
force-pushed
the
feature/ephemeral-containers-working-dir
branch
from
90b029c to
f9e5ac9
Compare
src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodContainerSource.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public Optional<String> getContainerWorkingDir(@NotNull Pod pod, @NotNull String containerName) { | ||
| return pod.getSpec().getEphemeralContainers().stream() |
There was a problem hiding this comment.
In a real pod, pod.getSpec().getEphemeralContainers() could be null (just mentioning for your production extension)
src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodContainerSourceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodContainerSourceTest.java
Outdated
Show resolved
Hide resolved
Add PodContainerSource extension point to lookup container working directory. This allows plugins to use ContainerExecDecorator with alternate container sources, like ephemeral containers.
cronik
force-pushed
the
feature/ephemeral-containers-working-dir
branch
from
f9e5ac9 to
17c5a69
Compare
Vlatombe
changed the title
PodContainerSource Extension Point
2 participants
Add
PodContainerSourceextension point to lookup container working directory. This allows plugins to useContainerExecDecoratorwith alternate container sources, like ephemeral containers.Testing done
Submitter checklist