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

[fluentd] allow multiple deployments #305

Merged
merged 17 commits into from
May 9, 2023
Merged

[fluentd] allow multiple deployments #305

merged 17 commits into from
May 9, 2023

Conversation

eyespies
Copy link
Contributor

to the same namespace for different configurations. This should resolve #289. I am not sure if the project is ready to go to version 1.0.0, but that was the next major number given that the changes are seemingly breaking changes.

Signed-off-by: Justin Spies justin@nextup.ai

@justin-spies
allow deploying Fluentd multiple times to the same namespace for diff…
3d37e41 
…erent configurations

Signed-off-by: Justin Spies <justin@nextup.ai>
@eyespies
Copy link
Contributor Author

Should also resolve #170, however I now see two other PRs for this same problem that were never merged. Is the Helm Chart deprecated in favor of the operator (I just learned that exists)?

If this PR is desirable still, and if there are issues with it, let me know and I will fix the issues. I did have a hard time testing this because all our ElasticSearch systems require HTTPS, which I wasn't sure how to configure for testing of this chart.

@DaemonDude23 DaemonDude23 mentioned this pull request Jan 22, 2023
Closed
@tspa-aa
Copy link

tspa-aa commented Mar 2, 2023

The PR that solves this particular problem (which I've been evading for 2+ years with a local, modified copy of the chart) is still desirable to me.

@eyespies
Copy link
Contributor Author

Hello - is there anything I can do to help get this PR merged? It's been a couple months and I see other PRs getting merged. Am happy to help out if there is anything I can do.

@eyespies
Copy link
Contributor Author

@patrick-stephens / @stevehipwell I just randomly picked your names from some previously merged PRs, is there anything I can do to help get this reviewed / approved / merged? Or if I should reach out to someone else, let me know to whom.

@patrick-stephens
Copy link
Contributor

I'm afraid I have no idea about fluentd so cannot approve it

@eyespies
Copy link
Contributor Author

@dioguerra I see your name attached to some merges for Fluentd - is this one something you can help get merged? If not, can you point me to someone who can help?

@eyespies
Copy link
Contributor Author

@patrick-stephens - thank you for reviewing / responding, appreciate you letting me know.

@stevehipwell
Copy link
Collaborator

@eyespies I don't maintain this specific chart so I'm not going to review this change. However I am in the process of adding a specific fluentd-aggregator chart based on my chart of the same name which is explicitly intended for this use case. The plan is to have specific collector (DaemonSet) and aggregator (StatefulSet) charts for both Fluent Bit and Fluentd; these will be tightly constrained and focused on doing one thing as well as possible.

@dioguerra
Copy link
Collaborator

@eyespies Hello, sorry i'm busy it was kubecon this last week, and work is not helping.

I gave a glance and everything seems good. Need to test it.

Can you drop tho the 1.0 release? And make it a minor instead? There are other users commits that we might want to add after.

@jls-appfire
(tidy) use a minor, not major, version bump
256014f 
Signed-off-by: Justin Spies <justin.spies@appfire.com>
@eyespies
Copy link
Contributor Author

Hi @dioguerra - wish I could have been at Kubecon myself. Thank you for responding, there hasn't been much movement on the FluentD side lately, and I wasn't sure how to figure out who to ask for help. Appreciate you stepping in.

I've updated the version number to 0.3.10, let me know if I should use something else or if that is good. I'm happy to make any other changes necessary, just let me know.

@dioguerra dioguerra changed the title allow deploying Fluentd multiple times [fluentd] allow multiple deployments May 2, 2023
@eyespies
Copy link
Contributor Author

eyespies commented May 3, 2023

Hi @dioguerra let me know if there is anything else I can do to help move this along.

@dioguerra
Copy link
Collaborator

dioguerra commented May 3, 2023 •
edited

Hey @eyespies.

Started this week taking some time to have a look at this. So, should be done soon. Give me some time :)

@dioguerra dioguerra added the enhancement New feature or request label May 3, 2023
@dioguerra dioguerra assigned dioguerra and eyespies and unassigned dioguerra May 3, 2023
@eyespies
Copy link
Contributor Author

eyespies commented May 3, 2023

Thank you @dioguerra - really appreciate you jumping in and helping with this PR.

dioguerra
dioguerra reviewed May 5, 2023
View reviewed changes
Copy link
Collaborator

@dioguerra dioguerra left a comment

Choose a reason for hiding this comment

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

Hello, Sorry for the delay. Thanks for your contribution, looks really nice.

Can you fix this issues so I can test everything together?

Have a good weekend!

Cheers!

charts/fluentd/values.yaml Outdated Show resolved Hide resolved
charts/fluentd/values.yaml Outdated Show resolved Hide resolved
charts/fluentd/values.yaml Outdated Show resolved Hide resolved
charts/fluentd/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/fluentd/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/fluentd/Chart.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
charts/fluentd/templates/_pod.tpl Outdated Show resolved Hide resolved
charts/fluentd/templates/_pod.tpl Outdated Show resolved Hide resolved
@stevehipwell @jls-appfire
feat(fluent-bit): Updated image to v2.1.2 (#347)
23bf160 
Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>
Signed-off-by: Justin Spies <justin.spies@appfire.com>
@jls-appfire
(tidy) fix a spelling typo
f436312 
Signed-off-by: Justin Spies <justin.spies@appfire.com>
@jls-appfire
(fix) move comment on FluentD chart changes from global README to flu…
f6d348b 
…entd README

Signed-off-by: Justin Spies <justin.spies@appfire.com>
@jls-appfire
(fix) update formatting / comments based on feedback from the team
959e9fc 
Signed-off-by: Justin Spies <justin.spies@appfire.com>
@jls-appfire
(fix) more stylistic / formatting updates; could not figure out how t…
21332b0 
…o get the containers list indented with zero spaces like everything else though.

Signed-off-by: Justin Spies <justin.spies@appfire.com>
@jls-appfire
(tidy) remove incorrectly placed file
37faeb4 
Signed-off-by: Justin Spies <justin.spies@appfire.com>
jls-appfire and others added 2 commits May 8, 2023 12:39
dioguerra
dioguerra approved these changes May 9, 2023
View reviewed changes
Copy link
Collaborator

@dioguerra dioguerra left a comment

Choose a reason for hiding this comment

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

Does it also make sense to conditionally generate the first configMap? charts/fluentd/templates/fluentd-configurations-cm.yaml

Maybe open a new PR if you think so...

@dioguerra dioguerra merged commit 09607dd into fluent:main May 9, 2023
2 checks passed
@dioguerra
Copy link
Collaborator

Wooohoo!!! 🥳 🥳 🥳 🥳

This was referenced May 9, 2023
Closed
Closed
@OmarHawk OmarHawk mentioned this pull request Sep 15, 2023
Open
Matiasmct pushed a commit to giffgaff/fluent-helm that referenced this pull request Oct 24, 2023
@eyespies @Matiasmct
[fluentd] allow multiple deployments (fluent#305)
ff368f7 
* Allow deploying Fluentd multiple times to the same namespace for different configurations
* Mount default watch dirs using `mountVarLogDirectory` and `mountDockerContainersDirectory` boolean
* Allow overwrite of default configmaps for `mainConfigMapNameOverride` and `extraFilesConfigMapNameOverride`
* Always include the main FLUENTD_CONF var; only include custom values when they are defined
* Update formatting
* Remove incorrectly placed file (CHANGELOG.md)

Signed-off-by: Justin Spies <justin.spies@appfire.com>

---------

Signed-off-by: Justin Spies <justin@nextup.ai>
Signed-off-by: Justin Spies <justin.spies@appfire.com>
@rthaenert rthaenert mentioned this pull request Mar 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dioguerra dioguerra dioguerra approved these changes

@edsiper edsiper Awaiting requested review from edsiper

@naseemkullah naseemkullah Awaiting requested review from naseemkullah

@stevehipwell stevehipwell Awaiting requested review from stevehipwell

Assignees

@eyespies eyespies

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fluentd: not able to deploy twice in the same namespace
7 participants
@eyespies @tspa-aa @patrick-stephens @stevehipwell @dioguerra @justin-spies @jls-appfire