Refactor/kubernetes configs split #534

GerkinDev · 2021-02-06T23:19:05Z

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)

repeatedly · 2021-02-11T23:56:40Z

Splitting configurations seems good.

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

github-actions · 2021-05-13T10:02:55Z

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 · 2021-06-13T10:03:07Z

This PR was automatically closed because of stale in 30 days

GerkinDev · 2021-06-13T13:59:52Z

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 · 2021-06-15T06:18:19Z

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 · 2021-06-15T09:31:51Z

@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 · 2021-06-16T01:27:13Z

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

GerkinDev · 2021-06-16T18:51:28Z

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

Waiting for your reviews

GerkinDev · 2021-06-17T10:33:01Z

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 · 2021-06-18T08:04:43Z

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).

Makefile

templates/conf/kubernetes.conf.erb

Application of feedback fluent#534 (comment)

Makefile

ashie

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

ashie · 2021-07-02T08:20:34Z

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 · 2021-07-02T14:21:05Z

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 · 2021-07-02T20:05:11Z

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 · 2021-07-03T10:34:05Z

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.

Application of feeedback fluent#534 (comment) Signed-off-by: GerkinDev <[email protected]>

Application of feedback fluent#534 (comment)

Application of feeedback fluent#534 (comment) Signed-off-by: GerkinDev <[email protected]>

Signed-off-by: GerkinDev <[email protected]>

Application of feedback fluent#534 (comment) Signed-off-by: GerkinDev <[email protected]>

Application of feeedback fluent#534 (comment) Signed-off-by: GerkinDev <[email protected]>

ashie · 2021-07-05T03:23:04Z

Makefile

 post-checkout-hook:
 	if [ -n "$(findstring /arm64/,$(DOCKERFILE))" ]; then \
-		mkdir -p $(DOCKERFILE)/hooks; \
+		mkdir -p docker-image/$(DOCKERFILE)/hooks; \


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

ashie · 2021-07-05T04:44:24Z

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

LGTM.
Split configurations and generating them work for me.

GerkinDev · 2021-07-05T13:57:38Z

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

cosmo0920 · 2021-07-05T15:34:14Z

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 · 2021-07-06T23:13:04Z

So do you need anything else?

cosmo0920 · 2021-07-07T00:55:30Z

@ashie @kenhys Can we merge this?

ashie · 2021-07-07T01:05:37Z

👍

cosmo0920 · 2021-07-07T01:47:30Z

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

ashie · 2021-07-07T06:02:19Z

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

Follow up for #534 Fix #583 Signed-off-by: Takuro Ashie <[email protected]>

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

GerkinDev mentioned this pull request May 6, 2021

Support for Kubernetes audit logs json format #519

Open

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

github-actions bot closed this Jun 13, 2021

ashie reopened this Jun 16, 2021

ashie added pending To be done in the future and removed stale labels Jun 16, 2021

GerkinDev force-pushed the refactor/kubernetes-configs-split branch 2 times, most recently from fb32d5c to c63b930 Compare June 16, 2021 18:49

ashie added enhancement and removed pending To be done in the future labels Jun 17, 2021

ashie requested review from cosmo0920 and ashie June 17, 2021 05:27

ashie reviewed Jun 22, 2021

View reviewed changes

Makefile Show resolved Hide resolved

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

chore: change order in src

c71294c

Application of feedback fluent#534 (comment)

ashie requested changes Jul 2, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

ashie requested changes Jul 2, 2021

View reviewed changes

GerkinDev added a commit to KnodesCommunity/fluentd-kubernetes-daemonset that referenced this pull request Jul 3, 2021

build: add missing all targets, add missing targets to .PHONY

e50af31

Application of feeedback fluent#534 (comment) Signed-off-by: GerkinDev <[email protected]>

GerkinDev added a commit to KnodesCommunity/fluentd-kubernetes-daemonset that referenced this pull request Jul 3, 2021

build: add missing all targets, add missing targets to .PHONY

26b127f

Application of feeedback fluent#534 (comment) Signed-off-by: GerkinDev <[email protected]>

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

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

build: add missing all targets, add missing targets to .PHONY

902d19e

Application of feeedback fluent#534 (comment) Signed-off-by: GerkinDev <[email protected]>

GerkinDev force-pushed the refactor/kubernetes-configs-split branch from 26b127f to 902d19e Compare July 3, 2021 10:53

GerkinDev added 5 commits July 3, 2021 12:54

Mutualize templates rendering for container image content

59a08a1

Signed-off-by: GerkinDev <[email protected]>

Mutualized image generators

a09d5a9

Signed-off-by: GerkinDev <[email protected]>

Split kubernetes sources

67ef6e6

Signed-off-by: GerkinDev <[email protected]>

chore: change order in src

f551efa

Application of feedback fluent#534 (comment) Signed-off-by: GerkinDev <[email protected]>

build: add missing all targets, add missing targets to .PHONY

83878ad

Application of feeedback fluent#534 (comment) Signed-off-by: GerkinDev <[email protected]>

GerkinDev force-pushed the refactor/kubernetes-configs-split branch from 902d19e to 83878ad Compare July 3, 2021 10:55

ashie reviewed Jul 5, 2021

View reviewed changes

cosmo0920 approved these changes Jul 5, 2021

View reviewed changes

cosmo0920 requested a review from ashie July 5, 2021 09:33

ashie approved these changes Jul 7, 2021

View reviewed changes

cosmo0920 merged commit 0fab7ee into fluent:master Jul 7, 2021

ashie added a commit that referenced this pull request Jul 7, 2021

Remove duplicate config in conf/kubernetes.conf

3d82ca4

Follow up for #534 Fix #583 Signed-off-by: Takuro Ashie <[email protected]>

ashie mentioned this pull request Jul 7, 2021

Remove duplicate config in conf/kubernetes.conf #585

Merged

ashie added a commit that referenced this pull request Jul 7, 2021

Remove duplicate config in conf/kubernetes.conf

d0dc00d

Follow up for #534 Fix #583 Signed-off-by: Takuro Ashie <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/kubernetes configs split #534

Refactor/kubernetes configs split #534

GerkinDev commented Feb 6, 2021 •

edited

Loading

repeatedly commented Feb 11, 2021

github-actions bot commented May 13, 2021

github-actions bot commented Jun 13, 2021

GerkinDev commented Jun 13, 2021 •

edited

Loading

cosmo0920 commented Jun 15, 2021 •

edited

Loading

GerkinDev commented Jun 15, 2021

ashie commented Jun 16, 2021 •

edited

Loading

GerkinDev commented Jun 16, 2021

GerkinDev commented Jun 17, 2021

ashie commented Jun 18, 2021 •

edited

Loading

ashie left a comment

ashie commented Jul 2, 2021

GerkinDev commented Jul 2, 2021

ashie commented Jul 2, 2021

GerkinDev commented Jul 3, 2021 •

edited

Loading

ashie Jul 5, 2021

ashie commented Jul 5, 2021

cosmo0920 left a comment

GerkinDev commented Jul 5, 2021

cosmo0920 commented Jul 5, 2021

GerkinDev commented Jul 6, 2021

cosmo0920 commented Jul 7, 2021

ashie commented Jul 7, 2021

cosmo0920 commented Jul 7, 2021

ashie commented Jul 7, 2021 •

edited

Loading

Refactor/kubernetes configs split #534

Refactor/kubernetes configs split #534

Conversation

GerkinDev commented Feb 6, 2021 • edited Loading

repeatedly commented Feb 11, 2021

github-actions bot commented May 13, 2021

github-actions bot commented Jun 13, 2021

GerkinDev commented Jun 13, 2021 • edited Loading

cosmo0920 commented Jun 15, 2021 • edited Loading

GerkinDev commented Jun 15, 2021

ashie commented Jun 16, 2021 • edited Loading

GerkinDev commented Jun 16, 2021

GerkinDev commented Jun 17, 2021

ashie commented Jun 18, 2021 • edited Loading

ashie left a comment

Choose a reason for hiding this comment

ashie commented Jul 2, 2021

GerkinDev commented Jul 2, 2021

ashie commented Jul 2, 2021

GerkinDev commented Jul 3, 2021 • edited Loading

ashie Jul 5, 2021

Choose a reason for hiding this comment

ashie commented Jul 5, 2021

cosmo0920 left a comment

Choose a reason for hiding this comment

GerkinDev commented Jul 5, 2021

cosmo0920 commented Jul 5, 2021

GerkinDev commented Jul 6, 2021

cosmo0920 commented Jul 7, 2021

ashie commented Jul 7, 2021

cosmo0920 commented Jul 7, 2021

ashie commented Jul 7, 2021 • edited Loading

GerkinDev commented Feb 6, 2021 •

edited

Loading

GerkinDev commented Jun 13, 2021 •

edited

Loading

cosmo0920 commented Jun 15, 2021 •

edited

Loading

ashie commented Jun 16, 2021 •

edited

Loading

ashie commented Jun 18, 2021 •

edited

Loading

GerkinDev commented Jul 3, 2021 •

edited

Loading

ashie commented Jul 7, 2021 •

edited

Loading