Add PodControlInterface and PodControllerRefManager #73
Conversation
Kubernetes pkg/controller/controller_utils.go doesn’t have this logic. This is from kubeflow/training-operator/pull/998. I think it’s safe to keep the logic here.
Use `KeyFunc` instead of k8s.io/kubernetes/pkg/controller.KeyFunc
Current implementation use PodControllerRefManager from `k8s.io/kubernetes/pkg/controller` and self-implemented `ServiceControllerRefManager` which brings some problems. 1. Pod impl may change along with k8s upgrade, service impl doesn’t 2. It’s not helping us remove Kubernetes direct dependency. 3. It’s not that straighforward for maintainers and contributors. This change make sure we folk everything we need and remove ref_manager dependency.
Since this file contains both BaseControllerRefManager, PodControllerRefManager and ServiceControllerRefManager. It makes sense to rename it to controller_ref_manager.go
|
This change is |
| @@ -0,0 +1,377 @@ | |||
| // Copyright 2019 The Kubeflow Authors | |||
There was a problem hiding this comment.
If we think it's fine to sync pod_ref_manager, it makes sense to rename to controller_ref_manager.go.
|
@gaocegege Address the feedbacks, please have another look |
+1 for this suggestion. |
|
/lgtm |
|
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. |
|
OK, then I think we can merge this one. |
|
/approve |
|
[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:
Approvers can indicate their approval by writing |
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
PodControlInterfaceandPodControllerRefManagerfromk8s.io/kubernetes/pkg/controllerand self-implementedServiceControlInterfaceandServiceControllerRefManagerwhich bring some problems.service_ref_managerwhich is kind of unclear until I find Remove the podControl and serviceControl interfaces #36Solution
Consider the situation that Kubernetes upstream doesn't have implementation of
ServiceControlInterfaceandServiceControllerRefManager, I would suggest we just folkPodControlInterfaceandPodControllerRefManagerlocally.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. kubernetes/client-go#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.