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.

Sign up for GitHub

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

Jump to bottom

feat(crd): Separate CRD and controller #345

Merged
merged 2 commits into from
Jan 29, 2018
Merged

feat(crd): Separate CRD and controller #345

merged 2 commits into from
Jan 29, 2018

Conversation

gaocegege
Copy link
Member

@gaocegege gaocegege commented Jan 24, 2018 •
edited
Loading

Close #281

  • Separate CRD and controller
  • Add CRD support in helm charts

Signed-off-by: Ce Gao ce.gao@outlook.com

This change is Reviewable

@gaocegege
WIP
69116ea 
Signed-off-by: Ce Gao <ce.gao@outlook.com>
@k8s-ci-robot
Copy link

Hi @gaocegege. Thanks for your PR.

I'm waiting for a kubernetes or tensorflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@coveralls
Copy link

coveralls commented Jan 24, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.8%) to 31.746% when pulling 9c33ef5 on gaocegege:crd into 8caa0c9 on tensorflow:master.

@jlewi
Copy link
Contributor

jlewi commented Jan 25, 2018

/ok-to-test

@gaocegege gaocegege changed the title WIP: feat(crd): Separate CRD and controller feat(crd): Separate CRD and controller Jan 25, 2018
@gaocegege
Copy link
Member Author

gaocegege commented Jan 25, 2018 •
edited
Loading

PTAL, and I think the code also should be updated in google/kubeflow.

NAME:   mottled-molly
LAST DEPLOYED: Thu Jan 25 20:49:04 2018
NAMESPACE: default
STATUS: DEPLOYED

RESOURCES:
==> v1beta1/CustomResourceDefinition
NAME                   AGE
tfjobs.tensorflow.org  1s

==> v1beta1/Deployment
NAME             DESIRED  CURRENT  UP-TO-DATE  AVAILABLE  AGE
tf-job-operator  1        0        0           0          1s

==> v1/Pod(related)
NAME                              READY  STATUS             RESTARTS  AGE
tf-job-operator-7dc65c6469-nm7jd  0/1    ContainerCreating  0         1s

@gaocegege
Copy link
Member Author

/test tf-k8s-presubmit

@gaocegege
Copy link
Member Author

gaocegege commented Jan 25, 2018 •
edited
Loading

It seems that the changes fail the e2e test, and I think we should create CRD before the e2e test. Although I do not know where to add the logic.

@jlewi Could you guide me? I am not familiar with prow, airflow or kubernetes/test-infra, so I do not find where to add the code about CRD into the test.

@jlewi
Copy link
Contributor

jlewi commented Jan 26, 2018

We call the deploy script which installs the helm chart. Is installing the helm chart not sufficient? This should get called before the E2E test runs.

@jlewi
Copy link
Contributor

jlewi commented Jan 26, 2018

It looks like setup_cluster failed for you test because we ran out of quota.

[2018-01-25 13:12:31,404] {base_task_runner.py:98} INFO - Subtask: googleapiclient.errors.HttpError: <HttpError 403 when requesting https://container.googleapis.com/v1/projects/mlkube-testing/zones/us-east1-d/clusters?alt=json returned "Insufficient regional quota to satisfy request for resource: "IN_USE_ADDRESSES". The request requires '1.0' and is short '1.0'. The regional quota is '8.0' with '0.0' available.">

It looks like we were leaking K8s clusters so I clean them up so hopefully this will fix the issue.
But our test infra needs some work.

@gaocegege
Copy link
Member Author

Thanks for your comment, and I will have another try.

/test tf-k8s-presubmit

@gaocegege
Copy link
Member Author

Ready to review, ping @jlewi

@jlewi jlewi merged commit b0ea60e into kubeflow:master Jan 29, 2018
@jlewi jlewi mentioned this pull request Jan 29, 2018
Closed
@gaocegege gaocegege deleted the crd branch January 29, 2018 14:14
ScorpioCPH
ScorpioCPH reviewed Jan 30, 2018
View reviewed changes
developer_guide.md
@@ -147,10 +148,11 @@ export MY_POD_NAME=my-pod
Now we are ready to run operator locally:

```sh
kubectl create -f examples/crd/crd.yaml
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 this should be executed after launching tf_operator.

Copy link
Member Author

Choose a reason for hiding this comment

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

emmm, the operator assumes that the CRD is created so I think it should be run before the creation of operator 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, it is not a requirement.

tf-job-operator-chart/templates/crd.yaml
@@ -0,0 +1,11 @@
apiVersion: apiextensions.k8s.io/v1beta1
Copy link
Member

Choose a reason for hiding this comment

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

Why put CRD yaml in charts, I think charts is used for Apps not CRDs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then where to place the CRD creation? I think we should create it in the charts, or the charts can not be run correctly.

Copy link
Member

@ScorpioCPH ScorpioCPH Jan 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Then where to place the CRD creation?

CRD creation is in the set-up stage.

Charts is excepted to be deployed be helm automatically.
If you want to create CRD automatically along with TFJobs, maybe we need add some dependency here (not just put a yaml here):

  • create CRD first
  • deploy a TFJob later

Copy link
Member Author

Choose a reason for hiding this comment

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

I will file an issue about the question.

@gaocegege gaocegege mentioned this pull request Jan 30, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ScorpioCPH ScorpioCPH ScorpioCPH left review comments

@jlewi jlewi jlewi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gaocegege @k8s-ci-robot @coveralls @jlewi @ScorpioCPH