Skip to content
This repository has been archived by the owner on Sep 12, 2023. It is now read-only.

Import volcano api #62

Merged
merged 1 commit into from
Apr 13, 2020
Merged

Import volcano api #62

merged 1 commit into from
Apr 13, 2020

Conversation

hzxuzhonghu
Copy link
Contributor

Volcano is now being in the process of becoming a CNCF sandbox project. And many operators like tf-operator has already migrated to volcano scheduler

cncf/toc#318

@kubeflow-bot
Copy link

This change is Reviewable

@k82cn
Copy link
Contributor

k82cn commented Apr 9, 2020

/cc @gaocegege @richardsliu

@Jeffwan
Copy link
Member

Jeffwan commented Apr 9, 2020

Vendor has been deprecated. I will file another PR to delete it. In this case, I don't think you need to vendor dependencies. This project has been migrate to go mod already.

k8s.io/code-generator v0.15.10
k8s.io/kube-openapi v0.0.0-20190228160746-b3a7cee44a30
k8s.io/kubernetes v1.15.10
k8s.io/kubernetes v1.18.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we try to minimize the dependency changes? Another thing is k8s.io/Kubernetes version doesn't match with apimachinery and client-go

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to keep current version :) Is there any reason for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we import 1.15.10, you can see the replace below. This is autogenrerated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Em. It must be some libraries requires high version? I notice volcano go.mod requires 1.16.9 https://github.com/volcano-sh/volcano/blob/9c314bd9e262413be6a902ae1dc6dbe133732ca5/go.mod#L49-L54

which matches apimachinery and client-go version here. k8s.io/kubernetes must be from other libraries.

@@ -66,8 +66,8 @@ type JobController struct {
// KubeClientSet is a standard kubernetes clientset.
KubeClientSet kubeclientset.Interface

// KubeBatchClientSet is a standard kube-batch clientset.
KubeBatchClientSet kubebatchclient.Interface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my question is, do we want to have some backward compatibility or do we want to support different backends in case some one want to use kube-batch instead of valcano?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stand on my own point, as volcano is more actively maintained, it is better to migrate to it in the next release

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much efforts would there be to support multiple backends? Would it be possible to make it pluggable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way is to update NewJobController, and add a new argument to indicate whether to use volcano or kube-batch. Then it will require all the operators to change. I think it is not worth doing that.

@hzxuzhonghu
Copy link
Contributor Author

Vendor has been deprecated. I will file another PR to delete it. In this case, I don't think you need to vendor dependencies. This project has been migrate to go mod already.

Agree, i just follow the previous way

@@ -51,6 +41,7 @@ replace (
k8s.io/kube-scheduler => k8s.io/kube-scheduler v0.15.10
k8s.io/kubectl => k8s.io/kubectl v0.15.11-beta.0
k8s.io/kubelet => k8s.io/kubelet v0.15.10
k8s.io/kubernetes => k8s.io/kubernetes v1.15.10
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jeffwan please see here

@terrytangyuan
Copy link
Member

Vendor has been deprecated. I will file another PR to delete it. In this case, I don't think you need to vendor dependencies. This project has been migrate to go mod already.

Yes let's delete it.

Copy link
Member

@Jeffwan Jeffwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hzxuzhonghu Please help rebase the changes, I remove vendor dependencies in a separate PR. This will makes PR clean.

k8s.io/code-generator v0.15.10
k8s.io/kube-openapi v0.0.0-20190228160746-b3a7cee44a30
k8s.io/kubernetes v1.15.10
k8s.io/kubernetes v1.18.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Em. It must be some libraries requires high version? I notice volcano go.mod requires 1.16.9 https://github.com/volcano-sh/volcano/blob/9c314bd9e262413be6a902ae1dc6dbe133732ca5/go.mod#L49-L54

which matches apimachinery and client-go version here. k8s.io/kubernetes must be from other libraries.

@hzxuzhonghu
Copy link
Contributor Author

ok, will do

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
PTAL @Jeffwan

@Jeffwan
Copy link
Member

Jeffwan commented Apr 13, 2020

looks good to me

@terrytangyuan
Copy link
Member

/approve

@k8s-ci-robot
Copy link

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit fda69c7 into kubeflow:master Apr 13, 2020
@hzxuzhonghu
Copy link
Contributor Author

Thank you all

georgkaleido pushed a commit to georgkaleido/common that referenced this pull request Jun 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants