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

[WIP] Add watch filtering by applyset label #1301

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

karlkfi
Copy link
Contributor

@karlkfi karlkfi commented Jul 2, 2024

  • Add "applyset.kubernetes.io/part-of" label to managed objects
  • Add "applyset.kubernetes.io/id" label to RootSyncs/RepoSyncs
  • Add "applyset.k8s.io/tooling" annotation to RootSyncs/RepoSyncs
  • Error if the RSync already has a tooling annotation for a different tool (unlikely, but required by KEP).
  • Generate ApplySet IDs using kubectl code for compatibility.
    Note: kubectl impl is slightly different than the KEP, in that it uses applyset.kubernetes.io instead of applyset.k8s.io
  • Update TestKubectlCreatesManagedNamespaceResourceMultiRepo & TestAddUpdateDeleteLabels with new labels & annotations
  • Update TestConflictingDefinitions_NamespaceToRoot to expect a conflict error metric until resolved (RepoSync conflicts with RootSync, even though RootSync adopting is allowed/intended).

go/config-sync-watch-filter

TODO:

  • Remove "applyset.kubernetes.io/part-of" label when an object is abandoned.
  • Distinguish between resource version conflict (needing to retry an update because the RV changed between GET & UPDATE) & management conflict (object field(s) managed by two controllers). This affects TestCRDDeleteBeforeRemoveCustomResourceV1 expectations about conflict metrics.
  • Extract TestConflictingDefinitions_RootToRoot Role watch changes to standalone PR
  • Extract conflict error message changes to standalone PR
  • Extract nice-to-have remediation cleanup to standalone PR

Dependencies:

Follow Ups:

  • Catch deletion of objects with a new resource type after the CRD is established but before applier finishes. This isn't needed to make the existing conflict tests pass, but is a related bug that might be worth fixing. It should be possible by exposing select Applier events to the Updater and having the Updater add the watch to the Remediator. Alternatively, the Remediator could watch CRDs and handle adding pending watched when the CRD is established.

Unplanned:

  • Add "applyset.k8s.io/additional-namespaces" annotation to RootSyncs/RepoSyncs.
    • This annotation would need to be applied by the reconciler, not the reconciler-manager.
    • The list of namespaces may change frequently for RootSync, whenever a new source commit is seen.
  • Add "applyset.k8s.io/contains-group-kinds" annotation to RootSyncs/RepoSyncs.
    • This annotation would need to be applied by the reconciler, not the reconciler-manager.
    • The list of namespaces may change frequently for RootSync, whenever a new source commit is seen.
    • For large apply sets, especially when using may resource types (ex: KCC or Crossplane), the inventory annotation length may dramatically increase the object size, increasing the risk of reaching the max object size in etcd.
  • Add "applyset.k8s.io/inventory" annotation to RootSyncs/RepoSyncs.
    • This annotation would need to be applied by the reconciler, not the reconciler-manager.
    • The list of namespaces may change frequently for RootSync, whenever a new source commit is seen.
    • For large apply sets, the inventory annotation length may overflow the max annotation length.
    • For large apply sets, the inventory annotation length may dramatically increase the object size, increasing the risk of reaching the max object size in etcd.
  • Error when attempting to apply an object with a non-matching "applyset.kubernetes.io/part-of" label.
    • Look ups before applying were removed from Config Sync (a few years ago) to reduce the number of API calls made and speed up applying large apply sets. Adding them back would be required to perform this kind of validation/protection.

This replaces #1239 with less refactoring. The refactoring can be done separately.

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

@karlkfi
Copy link
Contributor Author

karlkfi commented Jul 2, 2024

/retest

@karlkfi karlkfi force-pushed the karl-watch-filter-2 branch 2 times, most recently from 25924d6 to 721a7e1 Compare July 8, 2024 23:48
@google-oss-prow google-oss-prow bot added size/XL and removed size/XXL labels Jul 8, 2024
@google-oss-prow google-oss-prow bot added size/L and removed size/XL labels Jul 9, 2024
@karlkfi
Copy link
Contributor Author

karlkfi commented Jul 9, 2024

/retest

@karlkfi karlkfi changed the title [WIP] Add watch filtering by package-id label [WIP] Add watch filtering by applyset label Jul 10, 2024
@karlkfi karlkfi requested review from janetkuo and removed request for tiffanny29631 July 10, 2024 00:49
@google-oss-prow google-oss-prow bot added size/XL and removed size/L labels Jul 10, 2024
- Add "applyset.kubernetes.io/part-of" label to managed objects
- Add "applyset.kubernetes.io/id" label to RootSyncs/RepoSyncs
- Generate ApplySet IDs using kubectl code for compatibility.
  Note: kubectl impl is slightly different than the KEP, in that
  it uses "applyset.kubernetes.io" instead of "applyset.k8s.io"
Copy link

@karlkfi: The following tests 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 bb9361f link true /test kpt-config-sync-presubmit
kpt-config-sync-presubmit-e2e-multi-repo-test-group3 bb9361f link true /test kpt-config-sync-presubmit-e2e-multi-repo-test-group3
kpt-config-sync-presubmit-e2e-multi-repo-test-group1 bb9361f link true /test kpt-config-sync-presubmit-e2e-multi-repo-test-group1
kpt-config-sync-presubmit-e2e-multi-repo-test-group2 bb9361f link true /test kpt-config-sync-presubmit-e2e-multi-repo-test-group2

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-sigs/prow 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant