-
Notifications
You must be signed in to change notification settings - Fork 364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issue with operator OOM restart #946
Conversation
251edd7
to
361b22d
Compare
For bug fixes, we normally add some tests to compare before & after. Could you add some tests? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! I will check out this branch and test it by myself next week.
"event", event) | ||
return ctrl.Result{}, nil | ||
} else if len(pods.Items) > 1 { | ||
// This happens when we use fake client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to have two Pods with the same name and same namespace in test mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this happens during test which we do not have the server side filtering available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind explaining it more? In my understanding, it is impossible to have two Pods with the same name in an actual situation? Why do we need to test this case? Do I miss anything? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is happening during test in which we are using fakeclient which does not support these different kinds of filters applied in the ListOption. If we don't have this, the test will fail which is not because of a real failure.
testPodEventName := fmt.Sprintf("%s.15f0c0c5c5c5c5c5", testPodName) | ||
|
||
// add a pod in a different namespace | ||
newPods := []runtime.Object{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specifications of the Pods are very similar (some parts are the same.). Can we use variables instead of expanding all of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a test case. In this test case, we have common.RayNodeLabelKey: "yes",
specified. I know some parts are the same but putting them in a separate place might not be good for test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me explain it more. My meaning is that:
spec := corev1.PodSpec{
Containers: []corev1.Container{
{
Name: "ray-worker",
Image: "rayproject/ray:2.2.0",
Command: []string{"echo"},
Args: []string{"Hello Ray"},
},
},
}
&corev1.Pod {
...
Spec: spec (Maybe need to use Deep copy)
}
&corev1.Pod {
...
Spec: spec (Maybe need to use Deep copy)
}
instead of
&corev1.Pod {
...
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Name: "ray-worker",
Image: "rayproject/ray:2.2.0",
Command: []string{"echo"},
Args: []string{"Hello Ray"},
},
},
}
}
&corev1.Pod {
...
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Name: "ray-worker",
Image: "rayproject/ray:2.2.0",
Command: []string{"echo"},
Args: []string{"Hello Ray"},
},
},
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a single location that we need to make this change. I'd suggest we make another MR to change all of these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of technical debts in KubeRay now. It's good for us to reduce the system's complexity in every PR : )
be253eb
to
26c5593
Compare
testPodEventName := fmt.Sprintf("%s.15f0c0c5c5c5c5c5", testPodName) | ||
|
||
// add a pod in a different namespace | ||
newPods := []runtime.Object{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me explain it more. My meaning is that:
spec := corev1.PodSpec{
Containers: []corev1.Container{
{
Name: "ray-worker",
Image: "rayproject/ray:2.2.0",
Command: []string{"echo"},
Args: []string{"Hello Ray"},
},
},
}
&corev1.Pod {
...
Spec: spec (Maybe need to use Deep copy)
}
&corev1.Pod {
...
Spec: spec (Maybe need to use Deep copy)
}
instead of
&corev1.Pod {
...
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Name: "ray-worker",
Image: "rayproject/ray:2.2.0",
Command: []string{"echo"},
Args: []string{"Hello Ray"},
},
},
}
}
&corev1.Pod {
...
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Name: "ray-worker",
Image: "rayproject/ray:2.2.0",
Command: []string{"echo"},
Args: []string{"Hello Ray"},
},
},
}
}
"event", event) | ||
return ctrl.Result{}, nil | ||
} else if len(pods.Items) > 1 { | ||
// This happens when we use fake client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind explaining it more? In my understanding, it is impossible to have two Pods with the same name in an actual situation? Why do we need to test this case? Do I miss anything? Thanks!
32776b4
to
820a135
Compare
@wilsonwang371 Please help address @kevin85421's comment. |
Hi @wilsonwang371, would you mind rebasing with the master's branch? Because of the tight deadline for the release v0.5.0, @architkulkarni and I will merge this PR today and file a new PR to fix/refactor if we find any bugs in this PR. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me
return ctrl.Result{}, nil | ||
} | ||
|
||
_ = r.Log.WithValues("event", request.NamespacedName) | ||
r.Log.Info("reconcile RayCluster Event", "event name", request.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this log? Is it no longer useful?
Log: ctrl.Log.WithName("controllers").WithName("RayCluster"), | ||
} | ||
|
||
// add event for reconcile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to also add an event which should get filtered out and not trigger a reconcile?
CreateFunc: func(e event.CreateEvent) bool { | ||
if eventObj, ok := e.Object.(*corev1.Event); ok { | ||
if eventObj.InvolvedObject.Kind != "Pod" || eventObj.Type != "Warning" || | ||
eventObj.Reason != "Unhealthy" || !strings.Contains(eventObj.Message, "Readiness probe failed") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a more programmatic way to get the cause of the unhealthiness, or is scanning the message string standard practice? Also, for my education, why do we only care about failures in the readiness probe and not other pod unhealthy events? Maybe @kevin85421 you can also respond if you know the answer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readiness probe failure will trigger liveness probe failure. readiness probe will not automatically trigger a pod rebuild but liveness probe failure will.
@wilsonwang371 Do you mind adding a bit more detail to the PR description? |
Co-authored-by: Kai-Hsun Chen <kaihsun@apache.org> Signed-off-by: Wilson Wang <3913185+wilsonwang371@users.noreply.github.com>
b6eb222
to
61173bf
Compare
Sorry, I am a little bit busy these days. I just took some time to rebase and update the comments. Let's wait and see how's @jeevb reply on this latest patch. |
) | ||
|
||
// NewReconciler returns a new reconcile.Reconciler | ||
func NewReconciler(mgr manager.Manager) *RayClusterReconciler { | ||
if err := mgr.GetFieldIndexer().IndexField(context.Background(), &corev1.Pod{}, podUIDIndexField, func(rawObj client.Object) []string { | ||
pod := rawObj.(*corev1.Pod) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following change may fix #601 (comment). cc @jeevb
pod := rawObj.(*corev1.Pod)
owner := metav1.GetControllerOf(pod)
if owner == nil || owner.APIVersion != rayiov1alpha1.GroupVersion.String() || owner.Kind != "RayCluster" {
return nil
}
return []string{string(pod.UID)}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
Above is wrong. This is used to enable efficient lookup operations for the local cache. The informer will still fetch cluster status from Kubernetes APIServer. That is, events which are unrelated to Ray Pods will finally be fetched by the informer. Sorry for the misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do not have time to update & test this PR tomorrow, I can merge this PR and open a new patch to fix the issues.
options := []client.ListOption{ | ||
client.MatchingFields(map[string]string{podUIDIndexField: string(event.InvolvedObject.UID)}), | ||
client.InNamespace(event.InvolvedObject.Namespace), | ||
client.MatchingLabels(map[string]string{common.RayNodeLabelKey: "yes"}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove all common.RayNodeLabelKey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found out that I misunderstood some things. Initially, I thought that we should implement all filter logic in the resource event handler (i.e. CreateFunc
) and return true only when an event is issued by a Ray Pod. With this approach, we can make the function eventReconcile
only handle event processing and not worry about event filtering. Therefore, the label common.RayNodeLabelKey
is not necessary.
However, I have since realized that the owners of the unhealthy events are Pods, which may include Ray Pods and other Pods. Therefore, we cannot use GetControllerOf
to get the owner and determine whether the event belongs to a Ray Pod or not by checking whether the owner's kind is "RayCluster" or not.
Hence, the implementation will be more complex. If we want to distinguish whether an event belongs to a Ray Pod or not, we need to
- Get the UID of the event's InvolvedObject.
- Use the UID to look up Pods from the local cache.
- Use
GetControllerOf(pod)
to get the owner, and check whether thekind
isRayCluster
or not.
It will make the function eventReconcile
easier to understand, but we need to have more I/O (read local cache) which may cause performance overhead.
In summary, we should use common.RayNodeLabelKey
to distinguish Ray Pods in eventReconcile
until we understand the cost of the look-up operation.
If you have time to help with this, that will be good. |
@kevin85421 @architkulkarni If there're minor changes to be applied, I think we can check in the code and create separate PRs for improvement. If there're some minor changes you can directly suggest on the PR, feel free to use https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request. That could reduce response time. |
Merge this PR, and will file a separate PR to fix and improve some minor issues. |
Fix issue with operator OOM restart
Why are these changes needed?
This is the patch to fix #601 issue.
In our kuberay controller, we are watching all the events so that we can find out if there is any failed health probe event. In case we see this event happening, we will try to create a new pod to replace the failed one currently. Ideally, we should not receive a large amount of these events. However, we are not correctly filtering out these messages. When a lot of unrelated events have arrived, we will end up printing too many log messages and make the controller run slow.
In this patch, we use filters to block the event messages that we do not care about as early as possible.
Related issue number
#601
Checks