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

Improve Dockerfile #14

Merged
merged 4 commits into from
Jun 19, 2018
Merged

Improve Dockerfile #14

merged 4 commits into from
Jun 19, 2018
+18 −14

Conversation

ynqa
Copy link
Contributor

@ynqa ynqa commented Jun 11, 2018 •
edited by jlewi
Loading

Add Makefile to build for local/container execution, controller manifest to run it on k8s, and also improving Dockerfile.

for example to run with minikube:

# Start minikube
> minikube start (--vm-driver hyperkit)
> eval $(minikube docker-env)

# Build docker image of contoller
> make build-controller-image

# Start conponents
> argo install
> kube apply -f install/manifests/

# Checking for run
> kubectl logs -n kube-system SCHEDULEDWORKFLOW_PODNAME
time="2018-06-11T17:54:48Z" level=info msg="Creating event broadcaster"
time="2018-06-11T17:54:48Z" level=info msg="Setting up event handlers"
time="2018-06-11T17:54:48Z" level=info msg="Starting ScheduledWorkflow controller"
time="2018-06-11T17:54:48Z" level=info msg="Waiting for informer caches to sync"
time="2018-06-11T17:54:48Z" level=info msg="Starting workers"
time="2018-06-11T17:54:48Z" level=info msg="Started workers"
time="2018-06-11T17:54:48Z" level=info msg="Wait for shut down"

This change is Reviewable

@k8s-ci-robot k8s-ci-robot requested review from jlewi and vicaire June 11, 2018 17:56
@ynqa
Copy link
Contributor Author

ynqa commented Jun 11, 2018

/assign @vicaire

jlewi
jlewi reviewed Jun 11, 2018
View reviewed changes
install/manifests/scheduledworkflow-controller.yaml Outdated
@@ -0,0 +1,20 @@
apiVersion: extensions/v1beta1
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for? Why don't you just deploy it with ksonnet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added to check executions, like mpi-operator. And also I think that the components to deploy with ksonnet will be prepared on kubeflow repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the path taken by other components like tf-operator is to create ksonnet components in kubeflow/kubeflow and then use them to deploy the operator.
You can then check in a test app like
https://github.com/kubeflow/tf-operator/tree/master/test/test-app

To deploy the operator as part of tests.
The downside of this approach

If you want to check in raw YAML I think that's fine but it means maintaining two sets of configs. You'll need ksonnet anyway for E2E testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do want support for E2E testing so let's follow jlewi@ approach to avoid having to maintain two sets of configs.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, that sounds good. Thanks!

Copy link
Contributor Author

@ynqa ynqa Jun 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@jlewi So I deleted this manifest, and will try to enable to install pipelines as the component of kubeflow, okay?

@jlewi
Copy link
Contributor

jlewi commented Jun 18, 2018

/ok-to-test

@jlewi
Copy link
Contributor

jlewi commented Jun 18, 2018

@vicaire PTAL

vicaire
vicaire reviewed Jun 18, 2018
View reviewed changes
install/manifests/scheduledworkflow-controller.yaml Outdated
@@ -0,0 +1,20 @@
apiVersion: extensions/v1beta1
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, that sounds good. Thanks!

Makefile Outdated

.PHONY: build-controller
build-controller:
go build ./cmd/controller
Copy link
Contributor

Choose a reason for hiding this comment

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

There does not seem to be any file in ./cmd/...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry, I'll fix them.

Makefile Outdated

.PHONY: build-controller-image
build-controller-image:
docker build -t pipelines/scheduledworkflow-controller .
Copy link
Contributor

Choose a reason for hiding this comment

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

There does not seem to be any files in ./cmd/...

@ynqa
Copy link
Contributor Author

ynqa commented Jun 19, 2018

@vicaire By the way, is it not necessary to preparing CI?

@ynqa ynqa changed the title Enhance to run controller on kubernetes Improve Dockerfile Jun 19, 2018
@vicaire
Copy link
Contributor

vicaire commented Jun 19, 2018

ynqa@

I am not sure what you mean. Do you mean that we need continuous integration testing (CI)? If this is what you mean, then yes, it is needed.

@vicaire
Copy link
Contributor

vicaire commented Jun 19, 2018

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vicaire

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

@vicaire
Copy link
Contributor

vicaire commented Jun 19, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit 2888666 into kubeflow:master Jun 19, 2018
Linchin pushed a commit to Linchin/pipelines that referenced this pull request Apr 11, 2023
@lluunn @jlewi
install docker for testing image (kubeflow#14)
b561aff
HumairAK referenced this pull request in HumairAK/data-science-pipelines Feb 8, 2024
@HumairAK
Merge pull request #14 from HumairAK/update_packages_4
410a82d 
UPSTREAM: <carry>: update outdate package
@YuriJin-Unity YuriJin-Unity mentioned this pull request May 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlewi jlewi jlewi left review comments

@vicaire vicaire vicaire left review comments

Assignees

@vicaire vicaire

Labels
approved lgtm size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ynqa @jlewi @vicaire @k8s-ci-robot