-
Notifications
You must be signed in to change notification settings - Fork 437
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
Separate aggregator charts for Fluent Bit & Fluentd #195
Comments
What do you think @edsiper @naseemkullah? |
LGTM, thanks for putting effort into this. @patrick-stephens, what do you think ? |
Definitely agree, KISS is much better than rigid adherence to DRY. We should also start producing releases even if only as local tarballs that can be used. |
I'll open some PRs to add the new charts and remove the aggregation capability from the existing charts. @patrick-stephens releases currently attach the chart tarball as that's what the repo references, were you thinking of something different? @edsiper would the Fluent organisation be interested in maintaining a Fluentd Aggregator image, I've created stevehipwell/fluentd-aggregator for my chart but an official image would make a lot of sense. The idea is that for common use cases there is no additional cost of maintaining a custom image. |
Send me the details for the image and I'll have a look but I'm sure we can do something to provide an official one. Just need to check the best way to integrate it. If you need an email it's pat at calyptia.com but also feel free to create an issue and just assign or invoke me. |
@patrick-stephens sorry I forgot to add the repo to the original comment and edited it to add the repo in. The repo is stevehipwell/fluentd-aggregator. It's basically just an additional layer of common aggregation plugins added to the alpine image. I have tried creating an issue for this before, but probably in the wrong repo, will this do or do you want a separate issue in this repo? Once there is an official aggregator chart and image I plan on deprecating mine and focusing on supporting the official ones. |
Ah right, it looks like just a slightly custom configuration for fluentd so probably worth a pull request on the official fluentd repo as a separate image that gets build and pushed. I'm not entirely sure what happens with the fluentd builds at the moment though. |
@patrick-stephens I assumed that this would need a new repo similar to fluentd-kubernetes-daemonset? I'd suggest |
@patrick-stephens have you had any more thoughts about this? I'm looking to open PRs for these charts later on this week. |
Apologies for the radio silence @stevehipwell, holidays and other things distracted me. I'll have a word with @edsiper about the best option here, I'm not sure what Fluentd normally does. To me it looks like you have a fluentd + plugins image specifically targeting a Kubernetes use case. To keep it aligned with Fluentd updates, i.e. rebuilt alongside without having to submit multiple PRs, I would normally use just another tagged image - even just as a multi-stage Dockerfile (existing target for fluentd, extra target for the layer on top with your proposal). Splitting into another repo has the downside of updates being missed or not considered. It does have the upside of being more granular and I think we go with whatever the current community standard is here. |
@patrick-stephens no worries about the radio silence, I'm only picking this back up again now.
I'm not sure if there is a community standard, different project use various options. The Fluentd projects has used the multi-repo solution in the past but if it's possible to create an aggregator image as part of the primary Docker repo that'd be great. A further consideration here is which image types do we need to support? I'd suggest an Alpine image with x64 and arm support would be enough; as if you're going to customize this image you'd be best off starting with the base Fluentd image anyway. |
Yeah, Fluentd community often uses multi-repo solution. Monolithic repo solution is not encouraged to use. But, fluentd-kubernetes-daemonset repo's DockerHub build rules is already saturated and we cannot add more building rules. We should create another repo for aggregator images and use them. BTW, DockerHub's image building is very slow and I recommend to use GitHub Actions for docker image building: |
@cosmo0920 I wasn't suggesting that the existing fluentd-kubernetes-daemonset repo be used as the image I'm proposing is completely different. I'm asking if someone could creat a fluentd-aggregator repo in the fluentd organisation for the new image; assuming a multi repo solution is the right way. If you take a look at the repo I linked to that already does this you'll see it already uses GitHub actions to build the image. Slightly un-related, but it'd be great if the Fluentd releases were made into actual GitHub releases, I've wanted this for a while. This would then allow the releases to trigger new release issues and PRs on the child repos. I think this might be worth further discussion? |
Based on @cosmo0920 approval I'll sort you out another repo shortly. I'm doing a load of work on the Fluent Bit side for Github automation so agree with your second proposal but realistically will be a while before can even look so I'd say raise it separately and ideally submit a PR. @cosmo0920 may be working on it already as well but let's get a specific issue for it. |
Fluentd release model depends on standard RubyGems way. Fluentd maintainers want to their releases into RubyGems not GitHub releases. This is because Standard Rubyists refer gems into rubygems.org not GitHub releases and GitHub packages. We don't want to be forcibly to use GitHub ecosystem for every Fluentd users. |
Ah makes sense so appreciate the exposition @cosmo0920 |
Could there not be an action or something else to make a GH release after the tag has been created for the gem process? |
There is no room to use GH release for now. Most of Rubyist do not confirm GH releases. The normal releasing process is |
Does the release process not result in a tag being created in GH? A GH action added to the repo that watches for tags being pushed with a fixed pattern ( |
Yeah, it does make sense. But release note is not written in tags but in CHANGELOG.md. |
@cosmo0920 a GH action watching the repo for tag pushes could make tags with a specific format a GH release. When the release is created the details can be populated from the CHANGELOG. I'd happily open a PR on the repo to show the code and continue the discussion further? |
Why do you need to add another GH Actions for the fluentd repo? |
@cosmo0920 because you said that the Ruby process was idiomatic I wasn't suggesting changing any of that. However Fluentd isn't only relevant to Ruby users and the GitHub idiomatic way to show a release is to add metadata to a tag in the form of a GitHub release. This is an extension (open/closed) of the existing workflow and if as a Ruby user you never look at GH releases you wouldn't even know it was happening. |
Hmm, I understand. But I'm afraid that GH automation suggestion seems to be missing the point. |
Using the following workflow will create a MVP that creates a "GitHub Release" when a release tag is pushed, this means that the GitHub release watch works and non-Rubyists can find out the latest version easily from the GitHub UI. This MVP can then be extended to automate other processes such as adding the release notes to the GH release (does rake control the changelog format?) when it's created, and creating a PR in the fluentd-docker-image repo to release the new version. name: Create GH Release
on:
push:
tags:
- 'v*'
jobs:
create-gh-release:
runs-on: ubuntu-latest
permissions:
contents: write
steps:
- uses: actions/checkout@v2
- uses: ncipollo/release-action@v1
with:
token: ${{ secrets.GITHUB_TOKEN }} |
It's also useful with the API to get the latest release programmatically, e.g. during deployment with Ansible or similar. Probably best to move to a new issue though. |
Yeah, seems reasonable for me. You might already be aware of this but ClearCode team who is another part of Fluentd maintainer does not know why your patch is needed. |
The issue for the GH release workflow is fluent/fluentd#3598 and I'll open a PR to discuss the implementation. |
@patrick-stephens any luck with that repo? I've also opened fluent/fluentd-docker-image#313 & fluent/fluentd-docker-image#314 as the aggregator should be multiarch and I'm a fan of Alpine. I'd be interested if the workflow from https://github.com/calyptia/calyptia-fluentd-docker-image can be used in the Fluent repo? |
Apologies, been on holiday and was waiting on some other infrastructure bits so completely lost track of this one. I'll see if I can sort one, what name did we want? On the question of workflow, we can use it (it's Apache 2 after all) or do you mean you want to use it and aren't sure? I'm happy to review any suggestion and if it makes sense then why not - better to be consistent and keep things the same. |
@patrick-stephens I was on annual leave last week so no worries. I'd suggest the new repo should be called On the workflow, I was asking of the |
OK I'll sort shortly. On the workflow question, to me it seems fine - I'd just submit an issue+PR and we discuss there. |
@stevehipwell I've set up https://github.com/fluent/fluentd-aggregator-docker-image but can you push an initial PR asap with README, etc. basic set up? |
Thanks @patrick-stephens I'll create a PR to bring my image configuration as the official offering. On the workflow I've created a number of issues without much luck, I'm happy to open a PR but it might be best done by someone who has visibility of the current system as I don't? I've opened #318 to track the workflow so I'm going to close this issue. |
Currently the charts in this repo attempt to cover all use cases by being able to be used in either collector (
Daemonset
) or aggregator (Deployment
) modes. This adds a significant amount of complexity and cognitive load to the charts while having almost zero code reuse, the cost seems to be significantly higher than the benefit.I'd like to see new
fluentd-aggregator
andfluent-bit-aggregator
charts added to this repo and the aggregator functionality removed from the current charts. I have a heavily tested fluentd-aggregator chart that I'd be willing to contribute and I'd be more than happy to do the work to split out the Fluent Bit aggregator functionality into a new chart.I think this change would make a big difference to the maintainability of the charts, we all like DRY but KISS is often a far better approach.
Supersedes #67.
The text was updated successfully, but these errors were encountered: