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

Add Hono 1.0.2 #13

Closed
wants to merge 5 commits into from
Closed

Add Hono 1.0.2 #13

wants to merge 5 commits into from

Conversation

ctron
Copy link
Contributor

@ctron ctron commented Dec 19, 2019

No description provided.

@ctron ctron requested a review from calohmn as a code owner December 19, 2019 11:36
@ctron ctron force-pushed the feature/add_hono_1 branch 5 times, most recently from 1be0c8d to 1a1526e Compare December 19, 2019 16:08
@ctron
Copy link
Contributor Author

ctron commented Dec 19, 2019

It looks like Hono doesn't deploy properly. I will take a look at this next year.

@monotek
Copy link
Contributor

monotek commented Jan 9, 2020

Pipline seems to fail here because of a timeout.
I'll have to investigate...

@monotek
Copy link
Contributor

monotek commented Jan 9, 2020

If #17 is merged the timeout issue is fixed and the chart has enough time to be installed.

I've used your hono chart for testing it:
https://github.com/eclipse/packages/runs/381186823
Seems it fails anyway because prometheus-server is in a crashloop.

@thjaeckle
Copy link
Contributor

I canceled the checks for now.

@ctron ctron force-pushed the feature/add_hono_1 branch from 1a1526e to 1f2895f Compare January 13, 2020 13:09
@ctron
Copy link
Contributor Author

ctron commented Jan 13, 2020

Ok it looks like the Prometheus pod fails due to some mmap issue with the filesystem:

level=error ts=2020-01-09T12:58:43.426Z caller=query_logger.go:85 component=activeQueryTracker msg="Error opening query log file" file=/data/queries.active err="open /data/queries.active: permission denied"
panic: Unable to create mmap-ed active query log

goroutine 1 [running]:
github.com/prometheus/prometheus/promql.NewActiveQueryTracker(0x7fff5c601136, 0x5, 0x14, 0x29db1e0, 0xc000672f30, 0x29db1e0)
	/app/promql/query_logger.go:115 +0x48c
main.main()
	/app/cmd/prometheus/main.go:364 +0x5229

I saw other people having the same issue: https://www.madebymikal.com/prometheus-2-12-query-logging-and-startup-failures-on-macos/

@monotek I saw you made a few commits on the prometheus chart, do you have an idea?

@thjaeckle
Copy link
Contributor

Why exactly does Prometheus need to be started in order to test k8s deployment of the Hono chart?
Is it a required run-time dependency?

If it is optional, I would suggest adding it only to the "package zero" chart as an option which is deactivated by default.

@ctron
Copy link
Contributor Author

ctron commented Jan 13, 2020

Why exactly does Prometheus need to be started in order to test k8s deployment of the Hono chart?
Is it a required run-time dependency?

If it is optional, I would suggest adding it only to the "package zero" chart as an option which is deactivated by default.

I am not sure if it is requires. Hono can make use of the metrics data for limiting connections etc … But maybe we can disable it during the validation of the chart. Is there a way to specify chart properties for the CI pipeline?

@sophokles73
Copy link
Member

Prometheus is not (strictly) required for running Hono. As @ctron pointed out, Hono can make use of Prometheus metrics data in order to enforce connection and data volume limits but that is optional.
Prometheus and Grafana have been enabled by default since the beginning of the Hono chart. However, we can also disable it by default and, as @thjaeckle proposed, only enable it in the package zero chart ...

# port: 5671
# trustStorePath: /etc/conf/amqp-trust-store.pem
# credentialsPath: /etc/conf/amqp-credentials.properties
# amqpMessagingNetworkSpec:
Copy link
Member

Choose a reason for hiding this comment

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

IMHO adding these whitespaces is not very helpful, because

  • it makes it harder to see where the comment ends and where the property begins and
  • it requires more work to uncomment the properties if needed

Comment on lines 18 to 28
name: hono
description: Scalable IoT messaging platform
keywords:
- IoT
- messaging
home: https://www.eclipse.org/hono/
sources:
- https://github.com/eclipse/hono
maintainers:
- name: dejanb
email: [email protected]
Copy link
Member

Choose a reason for hiding this comment

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

running helm lint locally on the 1.0.2 chart succeeds on my machine, the only thing it recommends is to add an icon. Did you get a different result when running helm lint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thus the changes: However the CI pipeline doesn't call "helm lint" directly, but runs a GitHub action:

lint-chart:
runs-on: ubuntu-latest
needs: check-for-chart-changes
steps:
- name: Checkout
uses: actions/checkout@v1
- name: Run chart-testing (lint)
uses: helm/chart-testing-action@master
with:
command: lint
config: .github/ct.yaml

@monotek
Copy link
Contributor

monotek commented Jan 13, 2020

I would disable all dependency charts by default if they're optional and Hono runs without them.

I know about an issue in Kind with local provisioning which i've encountered with the official Elasticsearch chart (see: elastic/helm-charts#429). Maybe we hit the same issue with Prometheus here.

There is already a merged pr (kubernetes-sigs/kind#1157) which should fix the problem but is not available as release yet. As soon as a new release comes out i'll update the ci pipline.

@ctron
Copy link
Contributor Author

ctron commented Jan 13, 2020

Ok, so I added:

diff --git a/charts/hono/values.yaml b/charts/hono/values.yaml
index 24b93a7..f4adead 100644
--- a/charts/hono/values.yaml
+++ b/charts/hono/values.yaml
@@ -738,7 +738,7 @@
   # Set this property to false if you want to use an already existing server
   # instead. In that case, set the "name" and "port" properties to the
   # existing server's host name and port.
-  createInstance: true
+  createInstance: false
   # host contains the host name of an existing Prometheus server.
   # This property is used to configure a corresponding datasource in Grafana
   # if createInstance is set to false.
@@ -787,7 +787,7 @@
     enabled: false
 
 grafana:
-  enabled: true
+  enabled: false
   adminPassword: admin
 
   # labels to be added to the Grafana Deployment

@sophokles73
Copy link
Member

I would disable all dependency charts by default if they're optional and hono runs without them.

we can do that but then we can no longer call it the Hono 1.0.2 chart because it will have different (default) behavior. This is not necessarily bad and would only mean that we start using separate versions for the chart vs the project, right?

@monotek
Copy link
Contributor

monotek commented Jan 13, 2020

This is not necessarily bad and would only mean that we start using separate versions for the chart vs the project, right?

In my experience charts evolve a bit faster and therefore it will be hard to keep chart and project on the same version.

@sophokles73
Copy link
Member

That's what I would expect as well. @monotek any thoughts on which version to use for the chart?
I am a little concerned that if we simply use something like 1.1.0 then people might still think that it is related to the underlying project version. Using a higher number, e.g. 5.0.0 might prevent that from happening. WDYT?

@monotek
Copy link
Contributor

monotek commented Jan 13, 2020

If disabling the optional dependencys is a breaking change for users of chart version 1.x.x i would start with 2.0.0 and add an "upgrading" section to the readme, giving these infos.

I guess if there are more changes like that chart version will be raised fast enough anyway :D

@ctron
Copy link
Contributor Author

ctron commented Jan 14, 2020

Ok, it now looks as if the deployment is successful, except for a service or PVC, which doesn't get reported by the helm linter action …

So I need to dig a bit deeper which service fails.

@sophokles73
Copy link
Member

It looks strange that each installation step runs for over 2h, doesn't it. FMPOV installing Hono should not take more than 2 minutes. How does the pipeline determine if installation has succeeded/failed?

@ctron
Copy link
Contributor Author

ctron commented Jan 14, 2020

It looks strange that each installation step runs for over 2h, doesn't it. FMPOV installing Hono should not take more than 2 minutes. How does the pipeline determine if installation has succeeded/failed?

Yes, that is the problem. It uses helm install --wait:

--wait                     if set, will wait until all Pods, PVCs, Services, and minimum number of Pods of a Deployment, StatefulSet, or ReplicaSet are in a ready state before marking the release as successful. It will wait for as long as --timeout

@axdotl
Copy link

axdotl commented Jan 14, 2020

Hi folks. I'll have a look on it later this week.
As this contribution is almost equal to the existing chart (which works fine) from https://github.com/kiwigrid/helm-charts/tree/master/charts/hawkbit-update-server, I assume it the cause for failing build is either in the chart dependencies (because this is the only thing I changed) or with the ci action.

@ctron
Copy link
Contributor Author

ctron commented Jan 22, 2020

I was able to reproduce that locally. Running helm install --wait did reproduce the behavior for me as well. I also saw why there is a difference between helm lint and this CI check, it is an additional yamllint step.

I made the service type configurable, for external facing services. The default is NodePort and you can configure the node ports as well.

@monotek
Copy link
Contributor

monotek commented Jan 22, 2020

I've tested your branch locally via:

kind create cluster --config .github/kind-config.yaml 
kubectl --namespace kube-system create sa tiller
kubectl create clusterrolebinding tiller-cluster-rule --clusterrole=cluster-admin --serviceaccount=kube-system:tiller
helm init --service-account tiller --upgrade --wait
helm dependency update charts/hono
helm upgrade --install hono charts/hono --namespace hono

What i see is, that your service is using type "Loadbalancer".
Seems Loadbalancer is not available in Kind. See: kubernetes-sigs/kind#411

Do you have a reason for using Loadbalancer? I guess the services should only be accessible from inside the cluster so the default ClusterIP setting should work for you.

Edit: NodePort should not be used as default. See: https://oteemo.com/2017/12/12/think-nodeport-kubernetes/

@ctron
Copy link
Contributor Author

ctron commented Jan 22, 2020

CI is green now … I did a few cleanups, and changed the chart version to 2.0.0, and kept the app version to 1.0.2. We can take a look at upgrading to 1.0.3 after that.

If the CI is green again, I think the PR is ready to be merged.

@ctron
Copy link
Contributor Author

ctron commented Jan 22, 2020

Do you have a reason for using Loadbalancer this? I guess the services should only be accessible from inside the cluster so the default ClusterIP setting should work for you.

Most services actually should be available form the outside. The services which should not, are already ClusterIP.

The change I made now defaults to NodePort, but allows for using LoadBalancer or ClusterIP as well. On a per-service basis.

@ctron ctron added the enhancement New feature or request label Jan 22, 2020
@ctron
Copy link
Contributor Author

ctron commented Jan 22, 2020

CI is 💚

@monotek
Copy link
Contributor

monotek commented Jan 22, 2020

NodePort as default can be problematic. See my edit above.

Normally cluster ip is fine, as most services are exposed via ingress if needed in the outside world.

@ctron
Copy link
Contributor Author

ctron commented Jan 22, 2020

So, what would be the alternative? In the context of non-HTTP and non-TCP protocols like MQTT and CoAP?

@monotek
Copy link
Contributor

monotek commented Jan 22, 2020

To be honest i did not checked what Hono does.
So yes, in the case of non http its the way to go.

@ctron
Copy link
Contributor Author

ctron commented Jan 22, 2020

Ok … coo!

So I don't see any reasons not to merge. We can always improve things in the future.

@sophokles73 any objections?

@sophokles73
Copy link
Member

sophokles73 commented Jan 22, 2020

Well, unless the chart contained in this PR gets renamed to something that clearly distinguishes it frmo the real Hono chart, which is still maintained and published by the Hono project, I am strictly opposed to merging this PR.

@ctron
Copy link
Contributor Author

ctron commented Jan 22, 2020

To my understanding the goal in Hono is to move the Helm chart from the Hono repository to this repository: eclipse-hono/hono#1638

So, what would be required from your perspective to accomplish this?

@sophokles73
Copy link
Member

FMPOV we will need to have a working CI pipeline in the IoT Packages project first.
Once that is achieved, we can create a PR with exactly the code of the latest published Hono chart and merge it. We can then update the Hono website to point users to the IoT Packages Helm repository in order to install Hono. We would then no longer publish the chart on the Eclipse download servers. The more interesting question will be, how we keep developing the chart using e.g. current master of Hono ...

@ctron
Copy link
Contributor Author

ctron commented Jan 22, 2020

FMPOV we will need to have a working CI pipeline in the IoT Packages project first.

I think we have that right now.

Once that is achieved, we can create a PR with exactly the code of the latest published Hono chart and merge it.

I am not sure why we should take this effort? We do have a script now, in this PR, which works and is already based on the original Hono chart? What would be the benefit of completely dropping the effort we have done now over the last days, and re-doing that?

As this Helm chart is named hono and the one in the Hono repository is named eclipse-hono I don't see a conflict here. There will be no unexpected automated updates.

Would there be something else, which should be changed on this PR from your perspective?

@sophokles73
Copy link
Member

I am not sure why we should take this effort? We do have a script now, in this PR, which works and is already based on the original Hono chart? What would be the benefit of completely dropping the effort we have done now over the last days, and re-doing that?

Well, it is quite hard to see what the changes are that you have introduced, i.e. since there is no common baseline (the 1.0.3 chart from the Hono project for example), there is no diff view available. So it it is very hard to review. I would therefore propose to establish this common baseline first, i.e. create a PR based on the vanilla 1.0.3 code, including the properties yaml addition that I had proposed, then merge that PR and rebase this PR so that we can see what has changed. FMPOV that is how we usually work, right? Create PRs against existing code if we do not create something new.

As this Helm chart is named hono and the one in the Hono repository is named eclipse-hono I don't see a conflict here. There will be no unexpected automated updates.

Then what is the purpose of the chart in this repository? Is it the official Eclipse Hono chart or is it an alternative way of installing Hono?

My original idea was that we could move the official Hono chart to the IoT Packages repo once we have a working CI pipeline. However, after giving it a little more thought, I wonder how we are supposed to further evolve the chart with only a master branch in place. For example, once we create a chart with appVersion 1.1.0 (i.e. after we have released Hono 1.1.0), how can we create a fix for previous charts that had appVersion 1.0.3? Without a reasonable idea of how we want to develop/maintain the charts in this repository, I have a hard time migrating the official chart to IoT Packages. Alternatively, we could keep maintaining the chart in the Hono repository but publish released charts to the IoT Packages repository for publishing. We could even use the IoT Packages CI pipeline in Hono for testing the chart ...

@ctron
Copy link
Contributor Author

ctron commented Jan 23, 2020

Then what is the purpose of the chart in this repository? Is it the official Eclipse Hono chart or is it an alternative way of installing Hono?

For the time being it is an alternative way of installing Hono. Right now this will be maintained by the packages project. Maybe in the future Hono will take over the maintenance.

@monotek
Copy link
Contributor

monotek commented Jan 23, 2020

... I wonder how we are supposed to further evolve the chart with only a master branch in place. For example, once we create a chart with appVersion 1.1.0 (i.e. after we have released Hono 1.1.0), how can we create a fix for previous charts that had appVersion 1.0.3?

The chart should work with all versions of the image so if you like to use app version 1.0.3 in a version of the chart which would normally use an higher image version you would just change the values.yaml so, that the older image is used.

@sophokles73
Copy link
Member

For a minor version difference this might be true. What about major version changes?

@calohmn
Copy link
Contributor

calohmn commented Jan 23, 2020

Then what is the purpose of the chart in this repository? Is it the official Eclipse Hono chart or is it an alternative way of installing Hono?

For the time being it is an alternative way of installing Hono. Right now this will be maintained by the packages project. Maybe in the future Hono will take over the maintenance.

As the same people are involved in both projects, I think it would be best to take care that the chart development moves forward in the same direction, keeping differences minimal. I also see the main development of the chart to better take place in the Hono repository. Adding features in Hono master that have a corresponding addition/change in the chart should result in a commit that also updates the master version of the chart, IMHO.
As for the chart in the packages project, I think it's best to start out clean with a 1:1 copy of the latest release of the chart from the Hono repo, as suggested above. From that starting point on, changes/additions can then be made in subsequent commits, possibly adopting the changes in the Hono repo as well.

@ctron
Copy link
Contributor Author

ctron commented Jan 23, 2020

@monotek Ok so you think adding a section like Ditto makes sense?

## repository for the concierge docker image
repository: docker.io/eclipse/ditto-concierge
## tag for the concierge docker image
tag: 1.0.0
## pullPolicy for the concierge docker image
pullPolicy: IfNotPresent

@ctron
Copy link
Contributor Author

ctron commented Jan 23, 2020

On the other side we could also keep the current style:

imageName: index.docker.io/eclipse/hono-adapter-coap-vertx:1.0.2

So aside from a discussion on the Hono side how to continue development of the chart from Hono. Do we need anything else?

@monotek
Copy link
Contributor

monotek commented Jan 23, 2020

sion difference this might be true. What about major version changes?

It's also no problem to install an older version of the chart to use an older image.
In the older version of the chart you could also change the image version via values.yaml

@monotek Ok so you think adding a section like Ditto makes sense?

## repository for the concierge docker image
repository: docker.io/eclipse/ditto-concierge
## tag for the concierge docker image
tag: 1.0.0
## pullPolicy for the concierge docker image
pullPolicy: IfNotPresent

Yes, image & tag should always be configurable separately.
Normally, if you create your chart with "helm create hono", the placeholder for this should already be available.

That these options already exists is also what i've assumed when i answered @sophokles73 above.

@sophokles73
Copy link
Member

It's also no problem to install an older version of the chart to use an older image.
In the older version of the chart you could also change the image version via values.yaml

I agree. But how can I fix e.g. chart version 1.0.3 (because it contains a bug) after 2.0.0 has been released and is now on master in the repository?

@ctron
Copy link
Contributor Author

ctron commented Jan 23, 2020

sion difference this might be true. What about major version changes?

It's also no problem to install an older version of the chart to use an older image.
In the older version of the chart you could also change the image version via values.yaml

@monotek Ok so you think adding a section like Ditto makes sense?

## repository for the concierge docker image
repository: docker.io/eclipse/ditto-concierge
## tag for the concierge docker image
tag: 1.0.0
## pullPolicy for the concierge docker image
pullPolicy: IfNotPresent

Yes, image & tag should always be configurable separately.
Normally, if you create your chart with "helm create hono", the placeholder for this should already be available.

That these options already exists is also what i've assumed when i answered @sophokles73 above.

Ok, then I think we should align the Hono chart with the Ditto and hawkbit approach.

As @sophokles73 seems to duplicate the effort done here in PR #25, I think it makes sense to close this PR … wait until Hono is ready for addition to the Packages project.

@ctron ctron closed this Jan 23, 2020
@monotek
Copy link
Contributor

monotek commented Jan 23, 2020

But how can I fix e.g. chart version 1.0.3 (because it contains a bug) after 2.0.0 has been released and is now on master in the repository?

I've never seen updates to multiple major chart versions in any Helm chart.
I also don't think it's really needed, because:

  • A Helm chart is only config
  • Normally you can use a chart version with all docker image versions
  • If you really need to use some custom changes to the chart, you can easily create an pr and provide it as an option. In my experience this is also the reason that a chart evolves faster as the software itself
  • If you need a quick fix you can download the chart, add your changes and use helm client on your local chart files

@ctron ctron deleted the feature/add_hono_1 branch March 23, 2020 15:42
@hakanqq66
Copy link

level=info ts=2021-08-10T08:38:15.183Z caller=main.go:389 msg="No time or size retention was set so using the default time retention" duration=15d
level=info ts=2021-08-10T08:38:15.183Z caller=main.go:443 msg="Starting Prometheus" version="(version=2.28.1, branch=HEAD, revision=b0944590a1c9a6b35dc5a696869f75f422b107a1)"
level=info ts=2021-08-10T08:38:15.183Z caller=main.go:448 build_context="(go=go1.16.5, user=root@37280701f401, date=20210701-15:18:34)"
level=info ts=2021-08-10T08:38:15.183Z caller=main.go:449 host_details=(windows)
level=info ts=2021-08-10T08:38:15.184Z caller=main.go:450 fd_limits=N/A
level=info ts=2021-08-10T08:38:15.184Z caller=main.go:451 vm_limits=N/A
level=error ts=2021-08-10T08:38:15.185Z caller=query_logger.go:87 component=activeQueryTracker msg="Error opening query log file" file=data\queries.active err="open data\queries.active: The requested operation cannot be performed on a file with a user-mapped section open."
panic: Unable to create mmap-ed active query log

I am using wmi exporter. How can I fix this error?

@sophokles73
Copy link
Member

@hakanqq66 you might want to take a look at http://www.catb.org/esr/faqs/smart-questions.html and then rethink your approach ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants