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

Refactor/kubernetes configs split #534

Merged
merged 5 commits into from Jul 7, 2021
Merged

Refactor/kubernetes configs split #534

merged 5 commits into from Jul 7, 2021

Conversation

GerkinDev
Copy link
Contributor

@GerkinDev GerkinDev commented Feb 6, 2021 •
edited

This PR starts with a refactor of the makefile to mutualize redundant parts, to reduce the overall makefile size and increase its maintainability.

Then, each kubernetes source is splitted in its own erb.

Closes #531
Facilitate resolution of #519

Feedbacks welcome. More work related to #519 coming next. (I already have the fluentd conf somewhere)

@GerkinDev GerkinDev force-pushed the refactor/kubernetes-configs-split branch from 4e0b0b2 to a8db885 Compare February 6, 2021 23:20
@repeatedly
Copy link
Member

Splitting configurations seems good.

One suggest is using cp is better for conf/kubernetes/*.conf because these configurations don't need erb execution.

@GerkinDev GerkinDev mentioned this pull request May 6, 2021
Open
@github-actions
Copy link

This PR has been automatically marked as stale because it has been open 90 days with no activity. Remove stale label or comment or this PR will be closed in 30 days

@github-actions github-actions bot added the stale label May 13, 2021
@github-actions
Copy link

This PR was automatically closed because of stale in 30 days

@github-actions github-actions bot closed this Jun 13, 2021
@GerkinDev
Copy link
Contributor Author

GerkinDev commented Jun 13, 2021 •
edited

This is sad that this PR has been stalled and closed because of a suggestion. Templating is being used for many files. Static ones may be then templated without makefile change.

@repeatedly is it really a blocking suggestion ? This PR, even without replacing some erb with static config still brings huge QOL improvements to configuration for end users.

@cosmo0920
Copy link
Contributor

cosmo0920 commented Jun 15, 2021 •
edited

Hi, these suggested changes are lack of COPY steps on Dockerfile like as:
https://github.com/fluent/fluentd-kubernetes-daemonset/blob/master/docker-image/v1.12/debian-elasticsearch7/Dockerfile#L34-L38

We should process copying files into each of images.
Without these COPY steps, users are struggled why configuration files are missing in the built containers.

@GerkinDev
Copy link
Contributor Author

@cosmo0920 you're right.

So I'm willing to take back that work for a couple of hours in the current week to polish it. Will you be able to merge it before it goes stale ? Will I have to open another PR ?

@ashie ashie reopened this Jun 16, 2021
@ashie
Copy link
Member

ashie commented Jun 16, 2021 •
edited

I've reopened this pull request, please continue your work here.
@GerkinDev Could you resolve conflict and resolve the thing pointed out by @cosmo0920 ?

@ashie ashie added pending Can't merge by some reason and removed stale labels Jun 16, 2021
@GerkinDev GerkinDev force-pushed the refactor/kubernetes-configs-split branch 2 times, most recently from fb32d5c to c63b930 Compare June 16, 2021 18:49
@GerkinDev
Copy link
Contributor Author

@ashie @cosmo0920 here you are people ! I took the freedom to rebuild al src to see what outputs looks like.

Waiting for your reviews

@ashie ashie added enhancement and removed pending Can't merge by some reason labels Jun 17, 2021
@ashie ashie requested review from cosmo0920 and ashie June 17, 2021 05:27
@GerkinDev
Copy link
Contributor Author

Hmm, seeing the pollution in diffs inducted by the make src-all, would you like me to force push just 1 commit before to remove all the chore of rebuilt conf files ?

@ashie
Copy link
Member

ashie commented Jun 18, 2021 •
edited

Hmm, seeing the pollution in diffs inducted by the make src-all, would you like me to force push just 1 commit before to remove all the chore of rebuilt conf files ?

Don't worry, keep as is.
Although it seems no problem, we'll check regressions. Please wait for a while (probably a few days).

ashie
ashie reviewed Jun 22, 2021
View reviewed changes
Makefile Show resolved Hide resolved
ashie
ashie requested changes Jun 22, 2021
View reviewed changes
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
templates/conf/kubernetes.conf.erb Show resolved Hide resolved
GerkinDev added a commit to KnodesCommunity/fluentd-kubernetes-daemonset that referenced this pull request Jun 28, 2021
@GerkinDev
chore: change order in src
c71294c 
Application of feedback fluent#534 (comment)
ashie
ashie requested changes Jul 2, 2021
View reviewed changes
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
ashie
ashie requested changes Jul 2, 2021
View reviewed changes
Copy link
Member

@ashie ashie left a comment

Choose a reason for hiding this comment

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

Could you add container-image-template and container-image-template-all to .PHONY placed in the tail of Makefile?

@ashie
Copy link
Member

ashie commented Jul 2, 2021

DCO check is failed: https://github.com/fluent/fluentd-kubernetes-daemonset/pull/534/checks?check_run_id=2929861869
Could you add Signed-off-by: to the commit?

@GerkinDev
Copy link
Contributor Author

As a reply to #534 (comment), I didn't test each target, since https://github.com/fluent/fluentd-kubernetes-daemonset/blob/master/MAINTAINING.md mention only src-all, so I'm not aware of all the workflows or cases you expect to work, and what they should do. I've tested make src-all though, that does what I expect.

Moreover, make dockerfile-all is never mentioned anywhere, so I'm wondering if this target is really needed, since dockerfiles are rendered anyway with make src, ran on each container target in make src-all. Thus my comment earlier about redundant rules, that might even not be used at all.

So, am I supposed to keep all the rules previously existing ? Are they all necessary ? Weren't those legacy of older build workflows ?

Thank you

@ashie
Copy link
Member

ashie commented Jul 2, 2021

I'm wondering if this target is really needed

The benefit of make dockerfile-all (or variants) is it can reduce build time significantly, when we modify only Dockerfile:

$ time make src-all
...
real	6m30.801s
$ time make dockerfile-all
...
real	0m24.827s

@GerkinDev
Copy link
Contributor Author

GerkinDev commented Jul 3, 2021 •
edited

Sorry for the kind of WIP and broken state of the PR lately. I'm cleaning it up, and hope that this one will be the good one.

Force push of all those PR commits was due to rebase onto the latest master, and addition of signoffs on each commits.

GerkinDev added a commit to KnodesCommunity/fluentd-kubernetes-daemonset that referenced this pull request Jul 3, 2021
@GerkinDev
build: add missing all targets, add missing targets to .PHONY
e50af31 
Application of feeedback fluent#534 (comment)

Signed-off-by: GerkinDev <alexandre.j.p.germain@gmail.com>
GerkinDev added a commit to KnodesCommunity/fluentd-kubernetes-daemonset that referenced this pull request Jul 3, 2021
@GerkinDev
build: add missing all targets, add missing targets to .PHONY
26b127f 
Application of feeedback fluent#534 (comment)

Signed-off-by: GerkinDev <alexandre.j.p.germain@gmail.com>
@GerkinDev GerkinDev force-pushed the refactor/kubernetes-configs-split branch from e50af31 to 26b127f Compare July 3, 2021 10:51
GerkinDev added a commit to KnodesCommunity/fluentd-kubernetes-daemonset that referenced this pull request Jul 3, 2021
@GerkinDev
chore: change order in src
3c04841 
Application of feedback fluent#534 (comment)
GerkinDev added a commit to KnodesCommunity/fluentd-kubernetes-daemonset that referenced this pull request Jul 3, 2021
@GerkinDev
build: add missing all targets, add missing targets to .PHONY
902d19e 
Application of feeedback fluent#534 (comment)

Signed-off-by: GerkinDev <alexandre.j.p.germain@gmail.com>
@GerkinDev GerkinDev force-pushed the refactor/kubernetes-configs-split branch from 26b127f to 902d19e Compare July 3, 2021 10:53
@GerkinDev
Mutualize templates rendering for container image content
59a08a1 
Signed-off-by: GerkinDev <alexandre.j.p.germain@gmail.com>
@GerkinDev
Mutualized image generators
a09d5a9 
Signed-off-by: GerkinDev <alexandre.j.p.germain@gmail.com>
@GerkinDev
Split kubernetes sources
67ef6e6 
Signed-off-by: GerkinDev <alexandre.j.p.germain@gmail.com>
@GerkinDev
chore: change order in src
f551efa 
Application of feedback fluent#534 (comment)

Signed-off-by: GerkinDev <alexandre.j.p.germain@gmail.com>
@GerkinDev
build: add missing all targets, add missing targets to .PHONY
83878ad 
Application of feeedback fluent#534 (comment)

Signed-off-by: GerkinDev <alexandre.j.p.germain@gmail.com>
@GerkinDev GerkinDev force-pushed the refactor/kubernetes-configs-split branch from 902d19e to 83878ad Compare July 3, 2021 10:55
ashie
ashie reviewed Jul 5, 2021
View reviewed changes
Makefile
post-checkout-hook:
if [ -n "$(findstring /arm64/,$(DOCKERFILE))" ]; then \
mkdir -p $(DOCKERFILE)/hooks; \
mkdir -p docker-image/$(DOCKERFILE)/hooks; \
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!
I wondered why broken directories are created after make src-all for a while ago. This is the answer.

@ashie
Copy link
Member

ashie commented Jul 5, 2021

LGTM, thanks for you work!
I think a remaining task is adding a commit for generated files, especially for conf/kubernetes/*.conf for each images.
Otherwise we might forget to include them into images when we publish them.

@cosmo0920 Could you double-check?
Note that this PR includes a trivial bug fix: https://github.com/fluent/fluentd-kubernetes-daemonset/pull/534/files#r663615221

cosmo0920
cosmo0920 approved these changes Jul 5, 2021
View reviewed changes
Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

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

LGTM.
Split configurations and generating them work for me.

@cosmo0920 cosmo0920 requested a review from ashie July 5, 2021 09:33
@GerkinDev
Copy link
Contributor Author

Okay I'll make src-all a bit later once I'm back on my Linux env. Please wait a couple of hours

@cosmo0920
Copy link
Contributor

Okay I'll make src-all a bit later once I'm back on my Linux env. Please wait a couple of hours

Ah, you don't have to generate files with make src-all.
It will make severe conflicts if we do more time for reviewing. Merging this PR and generating files with make src-all is executed by us is more safe for conflicts.

@GerkinDev
Copy link
Contributor Author

So do you need anything else?

@cosmo0920
Copy link
Contributor

@ashie @kenhys Can we merge this?

@cosmo0920 cosmo0920 merged commit 0fab7ee into fluent:master Jul 7, 2021
@ashie
Copy link
Member

ashie commented Jul 7, 2021

👍

@cosmo0920
Copy link
Contributor

I'd applied this patch at master: 87ec024
And I kicked Fluentd kubernetes daemonset in DockerHub.

@ashie
Copy link
Member

ashie commented Jul 7, 2021 •
edited

Hmm, in_tail_container_logs is duplicated: #583
I'll fix it.

ashie added a commit that referenced this pull request Jul 7, 2021
@ashie
Remove duplicate config in conf/kubernetes.conf
3d82ca4 
Follow up for #534
Fix #583

Signed-off-by: Takuro Ashie <ashie@clear-code.com>
@ashie ashie mentioned this pull request Jul 7, 2021
Merged
ashie added a commit that referenced this pull request Jul 7, 2021
@ashie
Remove duplicate config in conf/kubernetes.conf
d0dc00d 
Follow up for #534
Fix #583

Signed-off-by: Takuro Ashie <ashie@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashie ashie ashie approved these changes

@cosmo0920 cosmo0920 cosmo0920 approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Split out kubernetes config file
4 participants
@GerkinDev @repeatedly @cosmo0920 @ashie