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

Support passing custom predicates options to KubeadmConfigReconciler #7949

Closed
hzxuzhonghu opened this issue Jan 19, 2023 · 12 comments
Closed
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/backlog Higher priority than priority/awaiting-more-evidence.

Comments

@hzxuzhonghu
Copy link
Member

User Story

We are importing cluster-api controllers in kurator, basically for installing k8s on any platform, especially on running VMs.

In our case, on developing installation on VMs, we depend on kubeadmControlplane API to provide k8s controlplane configurations as well. But we has the kubeadmControlplane has reference to kurator customMachine API. So there are lots of errors from KubeadmConfigReconciler.

The basic idea is can we add custom filter function in controller.Options, so we can pass the filter function as below.

	c, err := ctrl.NewControllerManagedBy(mgr).
		For(&controlplanev1.KubeadmControlPlane{}, builder.WithPredicates(...) ).
		Owns(&clusterv1.Machine{}).
		WithOptions(options).
		WithEventFilter(predicates.ResourceNotPausedAndHasFilterLabel(ctrl.LoggerFrom(ctx), r.WatchFilterValue)).
		Build(r)

Detailed Description

[A clear and concise description of what you want to happen.]

Anything else you would like to add:

[Miscellaneous information that will assist in solving the issue.]

/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 19, 2023
@hzxuzhonghu
Copy link
Member Author

I am willing to contribute, thanks

@fabriziopandini
Copy link
Member

controller.Options is a type we import for controller runtime, so we can't express an opinion on the proposed change.

However, if this change gets accepted in controller runtime, then IMO controller runtime should also take care of applying the filter inside WithOptions(options) instead of requiring consumers to add a builder.WithPredicates(...) into For(&controlplanev1.KubeadmControlPlane{} as proposed above (not sure I understood this correctly, might be representing your intent as diff and replacing ... with actual code could clarify it).

/triage needs-information

@k8s-ci-robot k8s-ci-robot added the triage/needs-information Indicates an issue needs more information in order to work on it. label Jan 19, 2023
@hzxuzhonghu
Copy link
Member Author

controller.Options looks a good choice to add that. Do you think I should propose this request to controller-runtime ?

@sbueringer
Copy link
Member

I think making this configurable in our controller doesn't require adding it to controller.Options. Honestly no idea if it would make sense to add it there (cc @vincepri).

Independent of that, I wonder if you basically want the same as: #7775

@hzxuzhonghu
Copy link
Member Author

Not exactly same, here in kurator it want to filter out CR by reference kind.

@fabriziopandini
Copy link
Member

fabriziopandini commented Jan 30, 2023

commented on the PR. and echoing here as weel

My personal opinion is that we should not add a new knob to filter what we reconcile which is orthogonal to WatchFilterValue and its possible improvements being discussed in #7775.
IMO those efforts should converge and come up with a common definition of what a filter is, that could translate into e.g. a watch filter struct or other solutions.

Also, worth to notice that such type of change in order to actually work should be accepted by provider implementers as well, and this requires a discussion during office hours or a small proposal to rally on async.

@fabriziopandini
Copy link
Member

/triage accepted
/assign @hzxuzhonghu

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 21, 2023
@k8s-triage-robot
Copy link

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

  • Confirm that this issue is still relevant with /triage accepted (org members only)
  • Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

@k8s-ci-robot k8s-ci-robot removed the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Mar 20, 2024
@fabriziopandini
Copy link
Member

/priority backlog

@k8s-ci-robot k8s-ci-robot added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Apr 11, 2024
@fabriziopandini fabriziopandini removed the triage/needs-information Indicates an issue needs more information in order to work on it. label Apr 12, 2024
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Apr 12, 2024
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

CAPI contributors will take a look as soon as possible, apply one of the triage/* labels and provide further guidance.

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.

@fabriziopandini
Copy link
Member

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

The issue is not active since some time now and no other user expressed interest in the use case
/close

@k8s-ci-robot
Copy link
Contributor

@fabriziopandini: Closing this issue.

In response to this:

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

The issue is not active since some time now and no other user expressed interest in the use case
/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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants