Fix unrecoverable events functionality after move to deployments#10709
Fix unrecoverable events functionality after move to deployments#10709amisevsk merged 1 commit intoeclipse-che:masterfrom
Conversation
amisevsk
added
kind/bug
|
Can one of the admins verify this patch? |
| machines.getMachines(getContext().getIdentity()).entrySet()) { | ||
| final KubernetesMachineImpl machine = entry.getValue(); | ||
| if (machine.getPodName().equals(podName)) { | ||
| if (!Strings.isNullOrEmpty(podName) && podName.startsWith(machine.getPodName())) { |
There was a problem hiding this comment.
I think it is redundant change. As far as I see machine contains the actual name of the deployed pod (with suffix) but not configured one https://github.com/eclipse/che/blob/07263f1e30089689d71b057f747a44a29283e3c4/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/KubernetesInternalRuntime.java#L524
Please check it
There was a problem hiding this comment.
You're right, I added the getName() line later and forgot to fix. I'll update the PR.
| machines.getMachines(getContext().getIdentity()).entrySet()) { | ||
| final KubernetesMachineImpl machine = entry.getValue(); | ||
| if (machine.getPodName().equals(podName)) { | ||
| if (podName.startsWith(machine.getPodName())) { |
There was a problem hiding this comment.
Here #10709 (comment)
I mean that using equals should be fine here. Correct me if I'm wrong.
Sorry, if it was not clear at the beginning.
There was a problem hiding this comment.
Ah, sorry I misunderstood. I'm not sure equals is sufficient here -- in KubernetesDeployments, podName is set to the involved object's metadata.name. For workspace pods, this will be machine.getPodName() plus a suffix added by the replica set.
There was a problem hiding this comment.
podName is set to the involved object's metadata.name
It's true
For workspace pods, this will be
machine.getPodName()plus a suffix added by the replica set.
I guess machine.getPodName() will return the value with suffix but not configured one. I think so because be create a machine with podName that is fetched from a deployed pod.
Here is a place where we fetch deployed pod
https://github.com/eclipse/che/blob/07263f1e30089689d71b057f747a44a29283e3c4/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/KubernetesInternalRuntime.java#L513
And here we use its name
https://github.com/eclipse/che/blob/07263f1e30089689d71b057f747a44a29283e3c4/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/KubernetesInternalRuntime.java#L524
Correct me If I am wrong and deployed pod has a name without a suffix added by the replica set.
There was a problem hiding this comment.
Ah I see what you mean now -- you're right. I've reverted to using equals here.
| return workspacePods | ||
| .keySet() | ||
| .stream() | ||
| .anyMatch(workspacePodName -> podName.startsWith(workspacePodName)); |
There was a problem hiding this comment.
Please add a small comment why startsWith method should be used here.
|
ci-test |
|
ci-test build report: |
|
ci-test |
|
ci-test build report: |
Since workspace pods on Openshift/Kubernetes are created by deployments now, they do not have predictable names, and code that relies on this does not work anymore. Updates pod events functionality to account for this change, allowing unrecoverable events to be handled correctly. Signed-off-by: Angel Misevski <amisevsk@redhat.com>
benoitf
removed
the
status/code-review
6 participants
What does this PR do?
Fix unrecoverable events functionality, which was broken by the Deployments change.
Since workspace pods are created by deployments, their names are no longer predictable and it is no longer possible to match them directly to internal representations. Currently this PR matches events to machines by checking for name prefixes, as pods generated by deployments share the deployment's name, plus a random suffix. It's not the most elegant solution but seems to be the best option at this time.
What issues does this PR fix or reference?
#10559
Docs PR
N/A (bugfix)