🌱 Pass Object's GVK version when reconcile.

[haosdent]

This PR tries to pass the Object's GVK info when reconciling resources in the controller. This would be helpful when the controller is watching multiple resources.

e.g. Suppose we have a CRD PodHealthySet, which status contains ReadyPods as a counter to record how many Pods are healthy under a specific selector.

In our PodHealthySet Controller, we watch PodHealthySet and Pod resources together.
When Pod healthy status changed, we would like to update PodHealthySet.Status.ReadyPods.

If we use EnqueueRequestsFromMapFunc, to count PodHealthySet.Status.ReadyPods, request chain is

1. Find all matched PodHealthySet for a specific Pod in EnqueueRequestsFromMapFunc, then we enqueue PodHealthySet reconcile request.
2. Go through all Pods in PodHealthySet.Reconcile, and calculate ReadyPods. => This would become slower as more and more Pod and PodHealthySet objects.

If we support pass object's GVK when reconcile, we could identify reconcile from which kind of object, and then don't need the slow Step 2 above.

[haosdent]

/assign @gerred

[haosdent]

@mengqiy @pwittrock @gerred Could you help to take a look if this PR makes sense? I would fix fail test cases if you are OK with this appoarch.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: haosdent
To complete the pull request process, please ask for approval from gerred after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

@haosdent: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-controller-runtime-apidiff-master	c35caf6	link	/test pull-controller-runtime-apidiff-master
pull-controller-runtime-test-master	c35caf6	link	/test pull-controller-runtime-test-master

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[coderanger]

This feature is very specifically eschewed from controller-runtime as controllers watching multiple root objects is both an anti-pattern and about as impossible as we can make it (ex. calling For() twice is disallowed). EnqueueRequestsFromMapFunc has to map things back to the root type of the controller in all cases.

[haosdent]

Hi, @coderanger thanks a lot for your comment. We understand it is an anti-pattern, but we still would like to find an approach to address performance issue.

[coderanger]

Why do you think there is a performance issue?

[coderanger]

/hold

[haosdent]

Hi, @coderanger the performance issue is because

Go through all Pods in PodHealthySet.Reconcile, and calculate ReadyPods. => This would become slower as more and more Pod and PodHealthySet objects.

[coderanger]

@haosdent While technically yes, that is O(N) operations, they are extremely fast because it's working with a local cache of the objects. Do you have any evidence of actual performance issues causing problems in real use? I don't think we're interested in extremely footgun-y options to deal with hypothetical cases :) You can also cache one direction of the association in ownership references or annotations pretty easily, or just keep a local in-memory cache of it. I think all of those would be a better option than breaking our "stateless reconcile" model.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[FillZpp]

If we support pass object's GVK when reconcile, we could identify reconcile from which kind of object, and then don't need the slow Step 2 above.

@haosdent The question is, even if you can pass Pod into queue, what do you plan to do in reconcile? You may get all related PodHealthySet for the Pod got from queue, then get all matched Pods for each of them and calculate their ReadyPods?

But there will be a race condition that a same PodHealthySet may be handled in multiple workers at once, if the MaxConcurrentReconciles is more than 1. It goes against the design of k8s controller model.

[haosdent]

Hi, @FillZpp thx a lot for your comment.
For the example I mentioned above, when I am able to get the object's GKV, I could distinguish the reconcile is from PodHealthySet or from Pod.
If the object is PodHealthySet, I could PodHealthySet.Status.ReadyPods++ or PodHealthySet.Status.ReadyPods-- according to the Pod status. Would not have race condition at here.
The current situation is I able to get the object name, while I could not get the object type. This looks not make sense.

[haosdent]

@coderanger Sry for missing your comment. Although we have the local cache, when there are 200k pods, it would become very slow.

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.