Skip to content
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: ResourceGroup status updates missed #1574

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

karlkfi
Copy link
Contributor

@karlkfi karlkfi commented Feb 26, 2025

The ResourceGroup "root" controller has been filtering events in the following cases:

  1. When the status is disabled
  2. When the ResourceGroup Reconciling condition was True
  3. When the kstatus was out of date according to the actuation status

These are all unnecessary to filter, because:

  1. Status disablement is already checked in the reconcile method
  2. Filtering events when Reconciling=True causes a bug where a long-running reconcile method might prevent concurrent status updates to managed resources from being reflected in the ResourceGroup status.
  3. The reconcile method already computes the kstatus, so computing it in the watch event filter is redundant.

This change removes the ResourceGroupPredicate entirely to remove these redundant and erroneous event filters.

Dependencies:

Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from karlkfi. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

The ResourceGroup "root" controller has been filtering events
in the following cases:
1. When the status is disabled
2. When the ResourceGroup Reconciling condition was True
3. When the kstatus was out of date according to the actuation status

These are all unnecessary to filter, because:
1. Status disablement is already checked in the reconcile method
2. Filtering events when Reconciling=True causes a bug where a
   long-running reconcile method might prevent concurrent status
   updates to managed resources from being reflected in the
   ResourceGroup status.
3. The reconcile method already computes the kstatus, so computing
   it in the watch event filter is redundant.

This change removes the ResourceGroupPredicate entirely to remove
these redundant and erroneous event filters.
@karlkfi karlkfi force-pushed the karl-rgc-remove-optimizations branch from b4c06d3 to 09f3461 Compare February 26, 2025 01:38
Copy link

@karlkfi: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
kpt-config-sync-presubmit-e2e-multi-repo-test-group1 09f3461 link true /test kpt-config-sync-presubmit-e2e-multi-repo-test-group1

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant