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.

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

feat(chart): Add baseline Kylo chart #5773

Closed
wants to merge 6 commits into from

Conversation

@sylus
Copy link

commented May 25, 2018

What this PR does / why we need it: Adds Kylo chart (http://kylo.io)

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): N/A

Special notes for your reviewer: This is a rather large P.R. I'll admit but Kylo is a rather large application with a lot of moving pieces. ^_^ I now have Kylo (Services / UI) working properly with a default example that runs NiFi, Hive and leverages Spark for Profiling / Validations. I will shortly post a video showcasing this work. Right now values.yaml is optimized to run on a single node which was my testing via docker for mac. I can post subsequent runs / performance benchmarks as needed. Right now my docker containers are owned by me but super happy to help the Kylo folks take ownership or assist them in any way. Several of my containers needed to leverage multi stage builds in order to keep sizes low. I also realize this might be a bit of back and forth but wanted to get this started!

Youtube Video: https://www.youtube.com/watch?v=mo_-IxPxTb8

P.S. Kylo itself ships with NiFi which is included in this chart but I pushed a different P.R. here that just contains NiFi itself. #5772

Steps to install (docker for mac)

  1. You will need the Hadoop w/Hive modified chart which is available in this P.R. @ #6688 :

In the patched stable/hadoop folder:

helm install --name hadoop --namespace hadoop -f values.yaml .
  1. Using the Kylo chart included in this P.R.:
helm install --name kylo \
             --namespace hadoop \
             --set serviceType=NodePort \
             --set ingress.enabled=false \
             -f values.yaml .
  1. Wait 2-5 mins or until the Kylo UI loads without connection to NiFi issues (use port for kylo-ui)

  2. Profit!

Tasks

  • Add video showcasing working install and demo run
  • Anything missing?

Platform Experts

Just including some platform experts to get their take also:

@fabiannecci
@scottreisdorf
@markap14

Relevant Links

https://github.com/govcloud/docker-kylo (v0.9.1 branch)
https://github.com/govcloud/docker-hadoop
https://hub.docker.com/r/govcloud/docker-kylo/builds/
https://hub.docker.com/r/govcloud/docker-hadoop/builds/

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 25, 2018

Hi @sylus. Thanks for your PR.

I'm waiting for a kubernetes 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.

@sylus sylus force-pushed the govcloud:chart-kylo branch to 06ee30e May 25, 2018

@sylus sylus referenced this pull request May 25, 2018
0 of 3 tasks complete
@sylus

This comment has been minimized.

Copy link
Author

commented May 28, 2018

/assign @mattfarina

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sylus
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mattfarina

If they are not already assigned, you can assign the PR to them by writing /assign @mattfarina in a comment when ready.

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

@sylus sylus referenced this pull request Jul 17, 2018
0 of 3 tasks complete
@mattfarina

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

@sylus Sorry for my slow response on the reviews. It's worth noting that we will soon be moving to a distributed repo setup. You can find more details in the accepted proposal on the topic. We'll be documenting how folks can run their own charts repo to be included.

Separately I'll do a light review of this PR. Given the way you're doing things it might be better to have your own repo. I'll explain in the review.

@mattfarina
Copy link
Contributor

left a comment

Here is some quick feedback on some obvious things.

## ActiveMQ configuration
activemq:
image:
repository: govcloud/docker-kylo

This comment has been minimized.

Copy link
@mattfarina

mattfarina Aug 9, 2018

Contributor

I see multiple calls to govcloud/docker-kylo for the image with different tags. Why are these different tags instead of different images?

activemq:
image:
repository: govcloud/docker-kylo
pullPolicy: Always

This comment has been minimized.

Copy link
@mattfarina

mattfarina Aug 9, 2018

Contributor

Can the pullPolicy be IfNotPresent by default. This applies to all places images are fetched and have an explicit version. See the guidelines for more detail on this... https://github.com/helm/charts/blob/master/REVIEW_GUIDELINES.md#configuration


## Configure resource requests and limits
## ref: http://kubernetes.io/docs/user-guide/compute-resources/
resources:

This comment has been minimized.

Copy link
@mattfarina

mattfarina Aug 9, 2018

Contributor

Memory and CPU limits we usually want to be commented out as you can never tell, by default, where this will be run. Minikube is different than a real cluster.

Is there a good reason these are here?

## MySQL configuration
mysql:
image:
repository: mariadb

This comment has been minimized.

Copy link
@mattfarina

mattfarina Aug 9, 2018

Contributor

Instead of having all the parts, like MariaDB, right in here would it be better depend on the mariadb chart? If not, I'd be curious to know why.

@@ -0,0 +1,275 @@
apiVersion: apps/v1beta1

This comment has been minimized.

Copy link
@mattfarina

mattfarina Aug 9, 2018

Contributor

Most of these can now be apps/v1 or apps/v1beta2. Both of these are in all supported versions of kubernetes. This can be said for all the apps objects.

sources:
- https://github.com/Teradata/kylo
maintainers:
- name: William Hearn

This comment has been minimized.

Copy link
@mattfarina

mattfarina Aug 9, 2018

Contributor

The maintainers names for this repo needs to be a github username. This allows us to reference the name in issues and PRs.

@stale

This comment has been minimized.

Copy link

commented Sep 8, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale label Sep 8, 2018

@stale

This comment has been minimized.

Copy link

commented Sep 22, 2018

This issue is being automatically closed due to inactivity.

@stale stale bot closed this Sep 22, 2018

@sylus sylus referenced this pull request Oct 19, 2018
0 of 3 tasks complete
@sylus

This comment has been minimized.

Copy link
Author

commented Oct 19, 2018

Issue closed and didn't have perm to re-open so new P.R. @ #8603

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.