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 NiFi chart #5772

Closed
wants to merge 4 commits into from

Conversation

@sylus
Copy link

commented May 25, 2018

What this PR does / why we need it: Adds NiFi chart (https://nifi.apache.org/)

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: Went through a variety of NiFi charts and tried to pick the best elements of each. Right now am using my extended docker NiFi image which adds toolkit, but if better can just simply call the official apache/nifi:1.6.0 image.

I realize this might be a bit of back and forth but wanted to get this started!

P.S. A corresponding Kylo charts also has a P.R. #5773

Tasks

  • Leverage apache/nifi:1.6.0 directly and documented how to extend
  • Add more documentation to values.yaml
  • Anything missing?

Platform Experts

Just including some platform experts to get their take also:

@markap14

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 25, 2018

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 25, 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

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

@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 added some commits May 25, 2018

@sylus

This comment has been minimized.

Copy link
Author

commented May 28, 2018

/assign @mattfarina

@juv

This comment has been minimized.

Copy link

commented May 30, 2018

@sylus I get an error when templating your chart. Some parse error occurs. I attached the error log below. I run Helm version helm-v2.9.1-windows-amd64.

nifi_chart_error.txt

On another note, it might be useful to expose the setting for nifi.zookeeper.root.node in the values.yaml file, e.g. named as zookeeper.rootNode.

@sylus

This comment has been minimized.

Copy link
Author

commented May 31, 2018

Thanks I will take a look at this, my Helm version is 2.8.2 and am using Linux and this commands works without parsing issues.

Will try to determine if is broken in newer release or if is a windows related parsing problem (which I also would like to work to).

@juv

This comment has been minimized.

Copy link

commented Jun 28, 2018

@sylus Any update on this?

I've tried to run this within this helm docker container, using this image.
Same error for me. Note that I copied the files I have on my Windows host to the container with docker cp C:\Development\Git\charts\incubator\nifi e63c432fe50b:/nifi/ before trying to run helm template . within /nifi

edit:
Btw, I've changed two things in the files. I can't see how that breaks the templating though. So here are the changes. I basically removed the Zookeeper dependency because I already have that running. Removing the dependency in the requirements.yaml file because I am running the helm template command in offline mode, so helm can't fetch the zookeeper chart

my requirements.yaml contains this:

dependencies:
#- name: zookeeper
#  version: 0.4.2
#  repository: https://kubernetes-charts-incubator.storage.googleapis.com
#  condition: zookeeper.enabled

And I changed the zookeeper.enabled to false in Values.yaml

zookeeper:
  ## Zookeeper chart
  ## ref: https://github.com/kubernetes/charts/tree/master/incubator/zookeeper
  enabled: false
@juv

This comment has been minimized.

Copy link

commented Jun 28, 2018

@sylus I narrowed the error down:

  1. copy conf/ folder for backup
  2. remove all files except activemq.properties
  3. error still occurs
  4. when removing this line, templating works: jms.activemq.broker.url=tcp://{{ template "nifi.fullname" . }}-activemq:61616
@sylus

This comment has been minimized.

Copy link
Author

commented Jul 3, 2018

I will be taking a look at this during this week, as I also have some minor fixes to bring up as well :) Apologies for delay!

@sylus sylus referenced this pull request Jul 17, 2018
2 of 2 tasks complete
@stale

This comment has been minimized.

Copy link

commented Aug 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. Activing will cause the issue to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the wontfix label Aug 8, 2018

@sylus

This comment has been minimized.

Copy link
Author

commented Aug 8, 2018

not stale ^_^

@stale stale bot removed the wontfix label Aug 8, 2018

@mattfarina
Copy link
Contributor

left a comment

Here is some quick feedback that is similar to your other chart.

sources:
- https://github.com/apache/nifi
maintainers:
- name: William Hearn

This comment has been minimized.

Copy link
@mattfarina

mattfarina Aug 9, 2018

Contributor

Can you update this to a valid github username? We require that to be able to contact folks.

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

This comment has been minimized.

Copy link
@mattfarina

mattfarina Aug 9, 2018

Contributor

Resource limits should be commented out. We expect this to be opt-in so folks can run this in varying environments.

## ref: http://kubernetes.io/docs/user-guide/ingress/
ingress:
enabled: true
domain: nifi.govcloud.ca

This comment has been minimized.

Copy link
@mattfarina

mattfarina Aug 9, 2018

Contributor

I imagine typical users of this chart cannot use the domain nifi.govcloud.ca. Is there another way to handle this than to have your domain? Possible example.com which is a reserved name.


## Specify a Zookeeper imagePullPolicy
## ref: http://kubernetes.io/docs/user-guide/images/#pre-pulling-images
imagePullPolicy: "Always"

This comment has been minimized.

Copy link
@mattfarina

mattfarina Aug 9, 2018

Contributor

Is there a reason this is set to always instead of "IfNotPresent"?

enabled: true

image:
repository: govcloud/docker-nifi

This comment has been minimized.

Copy link
@mattfarina

mattfarina Aug 9, 2018

Contributor

Is there a reason this nifi docker image is used instead of apache/nifi?

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

This comment has been minimized.

Copy link
@mattfarina

mattfarina Aug 9, 2018

Contributor

Can you consider either apps/v1 or apps/v1beta2 as the api version?

@tlvince

This comment has been minimized.

Copy link

commented Aug 29, 2018

@sylus thanks for this. I've made a few modifications per the feedback above in my branch. However, nifi-server currently fails to start with the following. Any ideas?

 on object creation; nested exception is java.lang.RuntimeException: Cannot Initialize Local State Provider because the 'Directory' property is set to "../data/state/local", but that directory could not be created
        at org.springframework.beans.factory.support.FactoryBeanRegistrySupport.doGetObjectFromFactoryBean(FactoryBeanRegistrySupport.java:175)
        at org.springframework.beans.factory.support.FactoryBeanRegistrySupport.getObjectFromFactoryBean(FactoryBeanRegistrySupport.java:103)
        at org.springframework.beans.factory.support.AbstractBeanFactory.getObjectForBeanInstance(AbstractBeanFactory.java:1634)
        at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:317)
        at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:202)
        at org.springframework.context.support.AbstractApplicationContext.getBean(AbstractApplicationContext.java:1084)
        at org.apache.nifi.spring.StandardFlowServiceFactoryBean.getObject(StandardFlowServiceFactoryBean.java:48)
        at org.springframework.beans.factory.support.FactoryBeanRegistrySupport.doGetObjectFromFactoryBean(FactoryBeanRegistrySupport.java:168)
        ... 39 common frames omitted
@sylus

This comment has been minimized.

Copy link
Author

commented Aug 29, 2018

Thanks so much @mattfarina for the review I know your super busy and I truly appreciate you taking the time.

Also thanks @tvince for doing this! Your awesome! I'm not immediately sure but that line is from the state-management.xml file. And I guess it works with my image because I created this folder. Temporarily if you switch to the govcloud extended image does that work?

If so I can adjust the configmaps to make sure they work with just the base image :)

Will take a look at this over the long weekend ^_^

| Parameter | Description | Default |
| ------------------------------- | -------------------------------------------------------------------- | ----------------------------------------- |
| `nifi.image.repository` | NiFi image name | `apache/nifi` |
| `nifi.image.tag` | The version of the official Traefik image to use | `1.6.0` |

This comment has been minimized.

Copy link
@colinrymer

colinrymer Aug 30, 2018

Line 38 and here refer to Traefik instead of Nifi. 😀

@stale

This comment has been minimized.

Copy link

commented Sep 29, 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 29, 2018

@stale

This comment has been minimized.

Copy link

commented Oct 13, 2018

This issue is being automatically closed due to inactivity.

@stale stale bot closed this Oct 13, 2018

@sylus

This comment has been minimized.

Copy link
Author

commented Oct 19, 2018

/reopen

@sylus

This comment has been minimized.

Copy link
Author

commented Oct 19, 2018

/remove-lifecycle stale

@sylus

This comment has been minimized.

Copy link
Author

commented Oct 19, 2018

Didn't have perm to reopen so new P.R. @ #8602

@sylus sylus referenced this pull request Oct 19, 2018
0 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.