kubeflow / pipelines

Machine Learning Pipelines for Kubeflow
https://www.kubeflow.org/docs/components/pipelines/
Apache License 2.0
3.63k stars 1.64k forks source link

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

Open AndersBennedsgaard opened 5 months ago

AndersBennedsgaard commented 5 months ago

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 commented 5 months ago

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.

github-actions[bot] commented 3 months ago

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.

AndersBennedsgaard commented 3 months ago

Still relevant

juliusvonkohout commented 3 months ago

/lifecycle frozen