core: do not watch resourceversion and heartbeat node updates #14186
Conversation
There was a problem hiding this comment.
Based on the output sample posted in the issue (#14070 (comment)), I think we might want to consider whether we should ignore other fields as well, or even if we should instead focus on a subset of fields that we do want to reconcile on, and ignore all other fields.
IIRC, @subhamkrai did some work on this, though it could be older. Let's chat in an upcoming huddle about what field changes we definitely want to reconcile on, which we aren't sure about, and which we definitely don't need to reconcile on.
| // do not watch node if only resourceversion got changed | ||
| resourceQtyComparer := cmpopts.IgnoreFields(v1.ObjectMeta{}, "ResourceVersion") | ||
| diff := cmp.Diff(objOld.Spec, objNew.Spec, resourceQtyComparer) | ||
|
|
||
| // do not watch node if only LastHeartbeatTime got changed | ||
| resourceQtyComparer = cmpopts.IgnoreFields(corev1.NodeCondition{}, "LastHeartbeatTime") | ||
| diff1 := cmp.Diff(objOld.Spec, objNew.Spec, resourceQtyComparer) | ||
|
|
||
| if diff == "" && diff1 == "" { | ||
| return diff | ||
| } |
There was a problem hiding this comment.
I'm not certain about this logic. It sets up 2 different comparers separately, instead of using the comparers together to ignore both at once. Even if this is working as intended, I'd like to see a unit test for the function that proves it's working as it should be.
There was a problem hiding this comment.
It seems working, wil also add the unit test
Testing:
2024-05-10 09:47:09.009607 D | ceph-cluster-controller: only changes in the ResourceVersion or LastHeartbeatTime " &v1.Node{\n \tTypeMeta: {},\n \tObjectMeta: v1.ObjectMeta{\n \t\t... // 3 identical fields\n \t\tSelfLink: \"\",\n \t\tUID: \"245242a2-381e-4f4c-ae5d-5d055f2e7f34\",\n- \t\tResourceVersion: \"75120\",\n+ \t\tResourceVersion: \"75475\",\n \t\tGeneration: 0,\n \t\tCreationTimestamp: {Time: s\"2024-05-06 11:53:46 +0000 UTC\"},\n \t\t... // 4 identical fields\n \t\tOwnerReferences: nil,\n \t\tFinalizers: nil,\n \t\tManagedFields: []v1.ManagedFieldsEntry{\n \t\t\t... // 3 identical elements\n \t\t\t{Manager: \"kube-controller-manager\", Operation: \"Update\", APIVersion: \"v1\", Time: s\"2024-05-08 11:25:59 +0000 UTC\", ...},\n \t\t\t{Manager: \"kube-controller-manager\", Operation: \"Update\", APIVersion: \"v1\", Time: s\"2024-05-08 11:26:00 +0000 UTC\", ...},\n \t\t\t{\n \t\t\t\tManager: \"kubelet\",\n \t\t\t\tOperation: \"Update\",\n \t\t\t\tAPIVersion: \"v1\",\n- \t\t\t\tTime: s\"2024-05-10 09:42:02 +0000 UTC\",\n+ \t\t\t\tTime: s\"2024-05-10 09:47:08 +0000 UTC\",\n \t\t\t\tFieldsType: \"FieldsV1\",\n \t\t\t\tFieldsV1: &{Raw: `{\"f:metadata\":{\"f:annotations\":{\"f:csi.volume.kubernetes.io/node`...},\n \t\t\t\tSubresource: \"status\",\n \t\t\t},\n \t\t},\n \t},\n \tSpec: {PodCIDR: \"10.244.0.0/24\", PodCIDRs: {\"10.244.0.0/24\"}},\n \tStatus: v1.NodeStatus{\n \t\tCapacity: {s\"cpu\": {i: {...}, s: \"2\", Format: \"DecimalSI\"}, s\"ephemeral-storage\": {i: {...}, s: \"6354932Ki\", Format: \"BinarySI\"}, s\"hugepages-2Mi\": {s: \"0\", Format: \"DecimalSI\"}, s\"memory\": {i: {...}, Format: \"BinarySI\"}, ...},\n \t\tAllocatable: {s\"cpu\": {i: {...}, s: \"2\", Format: \"DecimalSI\"}, s\"ephemeral-storage\": {i: {...}, s: \"6354932Ki\", Format: \"BinarySI\"}, s\"hugepages-2Mi\": {s: \"0\", Format: \"DecimalSI\"}, s\"memory\": {i: {...}, Format: \"BinarySI\"}, ...},\n \t\tPhase: \"\",\n \t\tConditions: []v1.NodeCondition{\n \t\t\t{\n \t\t\t\tType: \"MemoryPressure\",\n \t\t\t\tStatus: \"False\",\n- \t\t\t\tLastHeartbeatTime: v1.Time{Time: s\"2024-05-10 09:42:02 +0000 UTC\"},\n+ \t\t\t\tLastHeartbeatTime: v1.Time{Time: s\"2024-05-10 09:47:08 +0000 UTC\"},\n \t\t\t\tLastTransitionTime: {Time: s\"2024-05-08 11:26:11 +0000 UTC\"},\n \t\t\t\tReason: \"KubeletHasSufficientMemory\",\n \t\t\t\tMessage: \"kubelet has sufficient memory available\",\n \t\t\t},\n \t\t\t{\n \t\t\t\tType: \"DiskPressure\",\n \t\t\t\tStatus: \"False\",\n- \t\t\t\tLastHeartbeatTime: v1.Time{Time: s\"2024-05-10 09:42:02 +0000 UTC\"},\n+ \t\t\t\tLastHeartbeatTime: v1.Time{Time: s\"2024-05-10 09:47:08 +0000 UTC\"},\n \t\t\t\tLastTransitionTime: {Time: s\"2024-05-08 11:26:11 +0000 UTC\"},\n \t\t\t\tReason: \"KubeletHasNoDiskPressure\",\n \t\t\t\tMessage: \"kubelet has no disk pressure\",\n \t\t\t},\n \t\t\t{\n \t\t\t\tType: \"PIDPressure\",\n \t\t\t\tStatus: \"False\",\n- \t\t\t\tLastHeartbeatTime: v1.Time{Time: s\"2024-05-10 09:42:02 +0000 UTC\"},\n+ \t\t\t\tLastHeartbeatTime: v1.Time{Time: s\"2024-05-10 09:47:08 +0000 UTC\"},\n \t\t\t\tLastTransitionTime: {Time: s\"2024-05-08 11:26:11 +0000 UTC\"},\n \t\t\t\tReason: \"KubeletHasSufficientPID\",\n \t\t\t\tMessage: \"kubelet has sufficient PID available\",\n \t\t\t},\n \t\t\t{\n \t\t\t\tType: \"Ready\",\n \t\t\t\tStatus: \"True\",\n- \t\t\t\tLastHeartbeatTime: v1.Time{Time: s\"2024-05-10 09:42:02 +0000 UTC\"},\n+ \t\t\t\tLastHeartbeatTime: v1.Time{Time: s\"2024-05-10 09:47:08 +0000 UTC\"},\n \t\t\t\tLastTransitionTime: {Time: s\"2024-05-08 11:26:11 +0000 UTC\"},\n \t\t\t\tReason: \"KubeletReady\",\n \t\t\t\tMessage: \"kubelet is posting ready status\",\n \t\t\t},\n \t\t},\n \t\tAddresses: {{Type: \"InternalIP\", Address: \"192.168.39.166\"}, {Type: \"Hostname\", Address: \"minikube\"}},\n \t\tDaemonEndpoints: {KubeletEndpoint: {Port: 10250}},\n \t\t... // 5 identical fields\n \t},\n }\n"
@subhamkrai do you have any comments? |
| resourceQtyComparer := cmpopts.IgnoreFields(v1.ObjectMeta{}, "ResourceVersion") | ||
| diff := cmp.Diff(objOld.Spec, objNew.Spec, resourceQtyComparer) | ||
|
|
||
| // do not watch node if only LastHeartbeatTime got changed | ||
| resourceQtyComparer = cmpopts.IgnoreFields(corev1.NodeCondition{}, "LastHeartbeatTime") | ||
| diff1 := cmp.Diff(objOld.Spec, objNew.Spec, resourceQtyComparer) | ||
|
|
||
| if diff == "" && diff1 == "" { | ||
| return diff | ||
| } |
There was a problem hiding this comment.
| resourceQtyComparer := cmpopts.IgnoreFields(v1.ObjectMeta{}, "ResourceVersion") | |
| diff := cmp.Diff(objOld.Spec, objNew.Spec, resourceQtyComparer) | |
| // do not watch node if only LastHeartbeatTime got changed | |
| resourceQtyComparer = cmpopts.IgnoreFields(corev1.NodeCondition{}, "LastHeartbeatTime") | |
| diff1 := cmp.Diff(objOld.Spec, objNew.Spec, resourceQtyComparer) | |
| if diff == "" && diff1 == "" { | |
| return diff | |
| } | |
| // do not watch the node if only 'LastHeartbeatTime' and 'ResourceVersion' are changed | |
| resourceQtyComparer = cmpopts.IgnoreFields(corev1.NodeCondition{}, "LastHeartbeatTime", "ResourceVersion") | |
| return resourceQtyComparer |
There was a problem hiding this comment.
resourceQtyComparer is just a structure which goes with cmp.Diff so we cant get it directly
There was a problem hiding this comment.
then we can just add this line right at L43
diff := cmp.Diff(objOld.Spec, objNew.Spec, resourceQtyComparer)
return diff
?
There was a problem hiding this comment.
One is for the struct v1.ObjectMeta{} and second is for corev1.NodeCondition{}.
So we need to comparators.
There was a problem hiding this comment.
We may use NodeType,
But not sure IgnoreFields can look for sub-sub-sub fields....
There was a problem hiding this comment.
I believe that is not possible only one nesting look feasable
There was a problem hiding this comment.
I did worked on the predicates to watch for node level taints |
|
@BlaineEXE lemme know if you have anything in your mind to stop it during reconciling, Otherwise, let's keep this a base starting point of the reconciliation and we can open more issues. |
| if diff == "" && diff1 == "" { | ||
| return diff | ||
| } | ||
| return "not-equal" |
There was a problem hiding this comment.
How about returning a bool? The actual diff isn't being returned.
| @@ -40,3 +42,38 @@ func TestIsHotPlugCM(t *testing.T) { | |||
| cm.Labels["app"] = "rook-discover" | |||
| assert.True(t, isHotPlugCM(cm)) | |||
| } | |||
|
|
|||
| func TestCompareNodes(t *testing.T) { | |||
There was a problem hiding this comment.
Please test all cases. I only see one case tested, and it doesn't give me confidence that the comparators are working as we expect them to.
What happens if only the heartbeat time changes?
What happens if only the resource version changes?
What happens if nothing changes?
What happens if a field changes, but not resource version or heartbeat?
What happens if a field changes, and only resource version?
What happens if a field changes, and only heartbeat?
| objOld := e.ObjectOld.(*corev1.Node) | ||
| objNew := e.ObjectNew.(*corev1.Node) |
There was a problem hiding this comment.
What happens if either of these casting operations fail?
I think we need to check, to do our due diligence:
obj, ok := object.(*typecast)
|
A sample node |
|
Another thing I can see is This time which may be changed and doesn't require a reconcile @travisn @BlaineEXE any thoughts? |
I have lots of thoughts, and too many questions. I wrote up a long reply to this, but ended up scrapping it because I finally learned something that I feel like is the final puzzle piece: the code path that emits this log is part of the existing predicate, which is already being used already to filter out unnecessary CephCluster reconciles. rook/pkg/operator/ceph/cluster/watcher.go Lines 160 to 178 in 496afdb In our discussions in huddle, I have been under the impression (possibly my own fault) that this is a bug where Rook is reconciling too often, but I don't think that's the case, and I think that knowledge changes how we conceptualize these code changes. Because this log line is part of the predicate and not part of the reconcile logic, what this means to me is that we shouldn't immediately interpret the "bug" report to imply that the CephCluster is reconciling when it shouldn't be. We have to take it at face value, which is that the user is seeing the log line repeatedly, and nothing more. While a user might be upset about seeing this log message often, I have no reason to jump to the conclusion that the CephCluster predicate is faulty. Users can disable that particular log message by setting IMO, unless the bug reporter can show that the CephCluster is being re-reconciled wihout need, I think the easiest solution is to tell the user to set We have the option to "optimize" the predicate and exit early in case we discover that only the resource version and heartbeat have been updated. However, this is a specific case we are optimizing, and we should make sure that we continue to process the rest of the predicate in all other cases to ensure we don't miss other important events. Because this is an optimization, I also think that we can close this PR if we want. That said, I think the PR is basically complete, so we can continue with it as we like. |
|
@BlaineEXE Good catch, if this issue is really only about the log entry and not reconciles, I have two questions:
|
yes, it is just a optimization, rather than bug. Although setting |
|
The PR adds,
|
I don't think it would be genuine to tout all of these benefits. I would stop at saying that it is an optimization, and say no more in the commit message or PR description. As I mentioned in huddle. There is nothing we can do in predicate logic that changes what controller-runtime caches. Controller runtime caches all objects that match the As for API calls, we can't necessarily say this for certain. If the predicate uses the controller-runtime I would also argue that API calls aren't "expensive" for Rook when we consider the true scope. If we were part of the data plane, I agree, but the scale of our API calls when we are only configuring the data system is largely irrelevant, unless we are truly being wasteful. I think the idea that we need to eliminate all API calls because they are bad tends to be a black-and-white viewpoint that often results in developers implementing controller logic that is not properly idempotent, which introduces bugs. I would much rather prioritize correctness over the number of API calls, and I want to keep stressing this point because of how often I see API call optimization contribute to logic bugs. |
|
@BlaineEXE I do have updated the Description message, any other thoughts? or looks good? |
parth-gr
force-pushed
the
heartbeat-node
branch
3 times, most recently
from
df510a2
to
f7ec1c8
Compare
currently node predicate watches for resourceVersion and heartbeat changes, which are not critical things. So with the PR we will add a early-exit condition for the predicate as an optimization and to ignore considering for a reconcile of cephcluster if updates are not crucial closes: rook#14070 Signed-off-by: parth-gr <partharora1010@gmail.com>
currently node predicate watches for resourceVersion and heartbeat changes, which are not critical things.
So with the PR we will add a early-exit condition for the predicate as an optimization and to ignore considering for a reconcile of cephcluster if updates are not crucial
closes: #14070
Checklist: