search

kubeflow / common

Common APIs and libraries shared by other Kubeflow operator repositories.
Apache License 2.0
51 stars 73 forks source link

Add PodControlInterface and PodControllerRefManager #73

Closed Jeffwan closed 4 years ago

Jeffwan commented 4 years ago

This PR aims to solve following issue and it also help to remove more direct dependency on Kubernetes. Resolve part of #48 #71

Background

Current implementation uses PodControlInterface and PodControllerRefManager from k8s.io/kubernetes/pkg/controller and self-implemented ServiceControlInterface and ServiceControllerRefManager which bring some problems.

  1. PodControlInterface/PodControllerRefManager may change along with k8s version upgrade, while we don't make changes to Service. It maybe out of sync.
  2. Half and half doesn't help us remove Kubernetes direct dependency.
  3. It’s not that straightforward for maintainers and contributors. The first time I look at the change, I notice we only have service_ref_manager which is kind of unclear until I find #36

Solution

Consider the situation that Kubernetes upstream doesn't have implementation of ServiceControlInterface and ServiceControllerRefManager, I would suggest we just folk PodControlInterface and PodControllerRefManager locally.

This help remove dependency a lot and we can also make sure pods/service implementation can be updated at the same time.

In addition, I will make some efforts to this issue. https://github.com/kubernetes/client-go/issues/332. I will try to submit a PR and see feedbacks. If change can be accepted, then we don't need to maintain these auxiliary codes on our side.

/cc @gaocegege @terrytangyuan

Need @gaocegege 's review on code folk since he originally worked on this.

kubeflow-bot commented 4 years ago

This change is Reviewable

Jeffwan commented 4 years ago

@gaocegege Address the feedbacks, please have another look

gaocegege commented 4 years ago

LGTM but could you add some notes in the forked code so we can always look back what we did and where the code was forked from (with version/link).

+1 for this suggestion.

gaocegege commented 4 years ago

/lgtm

Jeffwan commented 4 years ago

@gaocegege @terrytangyuan

Thanks for the review. That's great suggestion. I will file a follow up PR to add a CONTRIBUTING.md doc and include these technical details.

gaocegege commented 4 years ago

OK, then I think we can merge this one.

terrytangyuan commented 4 years ago

/approve

k8s-ci-robot commented 4 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: terrytangyuan

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

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubeflow/common/blob/master/OWNERS)~~ [terrytangyuan] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment