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

[feature] Convert KFP profile controller to a DecoratorController #10903

Open
AndersBennedsgaard opened this issue Jun 14, 2024 · 4 comments
Open

Comments

@AndersBennedsgaard
Copy link

Feature Area

/area backend

What feature would you like to see?

At the moment, the KFP profile controller is implemented as a metacontroller CompositeController. This allows for the management of child resources based on a parent that is being watched, which is Namespaces for this component.
However, according to the CompositeController documentation, the Metacontroller expects to have full control of the parent resource:

CompositeController expects to have full control over this resource. That is, you shouldn't define a CompositeController with a parent resource that already has its own controller. See DecoratorController for an API that's better suited for adding behavior to existing resources.

This is obviously not the case here, since the Namespace should be owned by the Kubeflow profile controller

Instead, converting the controller into a DecoratorController, which has a slightly different API, would be more appropriate.

Converting the existing CompositeController resource to a DecoratorController would look like this:

apiVersion: metacontroller.k8s.io/v1alpha1
kind: DecoratorController
metadata:
  name: kubeflow-pipelines-profile-controller
spec:
  resources:
  - apiVersion: v1
    resource: namespaces
    labelSelector: # only select namespaces with KFP enabled
      matchLabels:
        pipelines.kubeflow.org/enabled: "true"
  attachments:
  - apiVersion: v1
    resource: secrets
  - apiVersion: v1
    resource: configmaps
  - apiVersion: apps/v1
    resource: deployments
  - apiVersion: v1
    resource: services
  - apiVersion: networking.istio.io/v1alpha3
    resource: destinationrules
  - apiVersion: security.istio.io/v1beta1
    resource: authorizationpolicies
  hooks:
    sync:
      webhook:
        url: http://kubeflow-pipelines-profile-controller/sync
        timeout: 10s

The current sync.py has to be changed to expect a request body that includes controller, object, attachments, related, and finalizing, with object being the parent Namespace and attachments being the attachments we subscribe to in the DecoratorController.

What is the use case or pain point?

I have not been able to find any writing about the consequences of using a CompositeController for resources that aren't owned by the controller, so we can just say that unexpected errors can occur by using it like we do.

Is there a workaround currently?

Continue as usual.


Love this idea? Give it a 👍.

@AndersBennedsgaard
Copy link
Author

If we want to allow for backwards compatibility, we could expose the decorator-controller webhook URL under a new path such as /decorator-sync. This would allow users to choose whether to use a CompositeController or DecoratorController until the former is fully removed.

Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the lifecycle/stale The issue / pull request is stale, any activities remove this label. label Aug 14, 2024
@AndersBennedsgaard
Copy link
Author

Still relevant

@juliusvonkohout
Copy link
Member

/lifecycle frozen

@google-oss-prow google-oss-prow bot added lifecycle/frozen and removed lifecycle/stale The issue / pull request is stale, any activities remove this label. labels Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants