fix(kuma-cp): remove Dataplane for Pod without IP #4964
Merged
Conversation
This file contains 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
Signed-off-by: Jakub Dyszkiewicz <[email protected]>
Signed-off-by: Jakub Dyszkiewicz <[email protected]>
Signed-off-by: Jakub Dyszkiewicz <[email protected]>
Signed-off-by: Jakub Dyszkiewicz <[email protected]>
Signed-off-by: Jakub Dyszkiewicz <[email protected]>
Signed-off-by: Jakub Dyszkiewicz <[email protected]>
Signed-off-by: Jakub Dyszkiewicz <[email protected]>
|
It's kind of nice that today Pod and DPP are always 1 to 1. We don't show Pods in Kuma GUI, so if there is no DPP users may think there is no Pod either (but in reality it's evicted). Can we change the DPP model to allow an empty address only if |
I looked around at the code. This is an interesting idea, but it would be hard to pull this off. There is a lot of code that has the assumption that we have an address. I feel that going in this direction we will generate many bugs. |
michaelbeaumont
approved these changes
Sep 8, 2022
lobkovilya
approved these changes
Sep 8, 2022
|
@Mergifyio backport release-1.8 |
✅ Backports have been created
|
mergify bot
pushed a commit
that referenced
this pull request
Sep 8, 2022
Signed-off-by: Jakub Dyszkiewicz <[email protected]> (cherry picked from commit f341cff)
jakubdyszkiewicz
added a commit
that referenced
this pull request
Sep 8, 2022
…4980) fix(kuma-cp): remove Dataplane for Pod without IP (#4964) Signed-off-by: Jakub Dyszkiewicz <[email protected]> (cherry picked from commit f341cff) Co-authored-by: Jakub Dyszkiewicz <[email protected]>
12 tasks
|
@Mergifyio backport release-1.7 release-1.6 |
mergify bot
pushed a commit
that referenced
this pull request
Oct 5, 2022
Signed-off-by: Jakub Dyszkiewicz <[email protected]> (cherry picked from commit f341cff)
mergify bot
pushed a commit
that referenced
this pull request
Oct 5, 2022
Signed-off-by: Jakub Dyszkiewicz <[email protected]> (cherry picked from commit f341cff) # Conflicts: # pkg/plugins/runtime/k8s/controllers/pod_status_controller.go # test/e2e/graceful/eviction.go # test/e2e_env/kubernetes/jobs/jobs.go # test/e2e_env/kubernetes/kubernetes_suite_test.go
✅ Backports have been created
|
lobkovilya
pushed a commit
that referenced
this pull request
Oct 5, 2022
Signed-off-by: Jakub Dyszkiewicz <[email protected]> Signed-off-by: Ilya Lobkov <[email protected]> # Conflicts: # pkg/plugins/runtime/k8s/controllers/pod_status_controller.go # test/e2e/graceful/eviction.go # test/e2e_env/kubernetes/jobs/jobs.go # test/e2e_env/kubernetes/kubernetes_suite_test.go
lobkovilya
pushed a commit
that referenced
this pull request
Oct 5, 2022
…5096) fix(kuma-cp): remove Dataplane for Pod without IP (#4964) Signed-off-by: Jakub Dyszkiewicz <[email protected]> (cherry picked from commit f341cff) Co-authored-by: Jakub Dyszkiewicz <[email protected]>
lobkovilya
pushed a commit
that referenced
this pull request
Oct 5, 2022
…5097) fix(kuma-cp): remove Dataplane for Pod without IP (#4964) Signed-off-by: Jakub Dyszkiewicz <[email protected]> Signed-off-by: Ilya Lobkov <[email protected]> # Conflicts: # pkg/plugins/runtime/k8s/controllers/pod_status_controller.go # test/e2e/graceful/eviction.go # test/e2e_env/kubernetes/jobs/jobs.go # test/e2e_env/kubernetes/kubernetes_suite_test.go Co-authored-by: Jakub Dyszkiewicz <[email protected]>
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Our pod -> dp reconciler was ignoring reconciliation in two cases
Pod IP is empty
Our assumption was that if the pod is empty = pod is being created.
The problem is that the Pod can lose its IP, for example - when it's evicted.
In this scenario, we wouldn't reconcile the pod to mark it as unhealthy.
Currently, evicted pods are sometimes marked as unhealthy - when at least one container is down and IP is still there.
Our Dataplane model requires the IP address
Dataplane#networking.address.When reconciling pod with empty IP I considered
I picked option 3. because other options sound like a hack even though technically we won't use unhealthy Dataplane.
Pod is completed (all containers aside from kuma-sidecar are terminated)
This was added so we remove Dataplane for a finished job in pod status reconciler and not recreate them in pod reconciler.
I was tempted to remove this functionality, but I don't want to introduce a breaking change while fixing other issue.
Instead, I changed the logic to remove Dataplane when Pod Succeeded.
I also added some additional logging around the pod reconciler.
Checklist prior to review
syscall.Mkfifohave equivalent implementation on the other OS --UPGRADE.md? --> Changelog:entry here?