fix(agent): watch for changes in configuration files in config directories by conorevans · Pull Request #10379 · influxdata/telegraf

conorevans · 2022-01-05T02:07:11Z

Signed-off-by: Conor Evans coevans@tcd.ie

Required for all PRs:

Updated associated README.md. [I don't think this is necessary]
Wrote appropriate unit tests. [TODO]
Pull request title or commits are in conventional commit format [when I do tests and stuff I will squash the commits into one that follows this convention.

resolves #9985

The walkfn is virtually identical to config.LoadDirectory() - I chose duplication over abstraction, as I think the scope for the latter is limited and undesirable.

telegraf-tiger · 2022-01-05T02:07:17Z

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

Signed-off-by: Conor Evans <coevans@tcd.ie>

conorevans · 2022-01-05T02:08:54Z

!signed-cla

conorevans · 2022-01-05T02:12:28Z

cmd/telegraf/telegraf.go


+// configDirFiles returns a slice made up of filepaths correspoding
+// to config files defined in any of the config directories.
+func configDirFiles() sliceFlags {


I think it would probably make more sense for this to return []string rather than sliceFlags as in this context it's not to do with the flags.

I think so too. I don't see any place where the existing sliceFlags methods (String and Set) are called. It looks like they may be left over from a previous refactor. Let's get rid of them.

sliceFlags is used because of flag.Var so I think it will break stuff when removed..

You're right @Hipska, good catch! I will fix this in the upcoming week (have got a new laptop without env fully set up)

cmd/telegraf/telegraf.go

conorevans · 2022-01-05T02:18:45Z

cmd/telegraf/telegraf.go

+			}
+
+			if info.IsDir() {
+				// skip Kubernetes mounts, preventing loading the same config twice


I didn't remove this check. I don't immediately know if it also applies in this context, so I have to look a bit more. If it is necessary, the comment should probably be updated anyway. Happy for you to weigh in.

I think in this case since we're looking only for regular files, we should skip all directories (not just "..").

If there's a directory called "xyz.conf" we wouldn't want the function to append it to configs and return it.

cmd/telegraf/telegraf.go

telegraf-tiger · 2022-01-05T02:22:29Z

👍 This pull request doesn't change the Telegraf binary size

📦 Looks like new artifacts were built from this PR.

Expand this list to get them here ! 🐯

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_i386.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz
		static_linux_amd64.tar.gz

sjwang90 · 2022-04-11T19:15:12Z

@conorevans This PR is still named draft. Is this ready for review?

conorevans · 2022-04-11T20:28:44Z

Hi @sjwang90 as an external contributor, I marked it as draft to show it needed some early feedback (in case the approach I took was terrible). I left some comments as to the things I though maintainers might like to comment on, but it's remained inactive. I can remove the draft label if that would make it more obvious that it needed feedback?

sjwang90 · 2022-04-11T23:22:59Z

Yeah - go ahead and remove the draft title and I'll mention it to the maintainers to do a review.

iarkhanhelsky · 2022-06-03T18:55:32Z

@sjwang90 @Hipska @conorevans Hi all! Any chance to make this PR merged? I'll be glad to help if any help is needed.

…onfigs-in-directories

conorevans · 2022-06-08T21:25:40Z

Hi @iarkhanhelsky thanks to @sjwang90 and @reimda pushing this forward I will look to make their requested changes and update this.

Signed-off-by: Conor Evans <coevans@tcd.ie>

conorevans

hi sorry for delay my laptop is pretty terrible atm and was not having a fun time trying to fix git state (i wanted to squash my commits, hence the lazy last commit message)

cmd/telegraf/telegraf.go

Signed-off-by: Conor Evans <coevans@tcd.ie>

shruthikudlur · 2022-08-11T17:00:22Z

Does this code also watch for new files added into the dierctory?

reimda · 2022-08-11T22:15:56Z

Does this code also watch for new files added into the dierctory?

It only watches the files that were in the directory when telegraf loads its config. You would need to restart or sighup telegraf to watch new files.

sspaink · 2022-08-15T18:59:03Z

@conorevans If you fix the failing tests we can get this merged, thank you for your hard work!

It is failing with: cmd/telegraf/telegraf.go:388:11: cannot use &fConfigs (value of type *[]string) as flag.Value value in argument to flag.Var: *[]string does not implement flag.Value (missing method Set)

Looks like we need to keep the fConfigs to be sliceFlags and not []strings

… flag.Value interface

…with sliceFlags

conorevans · 2022-08-15T19:35:17Z

Hi @reimda @sspaink @Hipska this should be good now 🤞

telegraf-tiger · 2022-08-15T19:56:00Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_i386.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz
		static_linux_amd64.tar.gz

reimda · 2022-08-17T19:15:03Z

cmd/telegraf/telegraf.go

+			return nil
+		}
+
+		if err := filepath.Walk(dir, walkfn); err != nil {


@srebhan and I reviewed this and had the thought that walking the config dir here might not get the same list of configuration files as the code in config/config.go that initially loaded the files.

Instead of walking the path, could we change Config.LoadConfig to save the list of files? Then we know exactly which files to watch and there's no chance for difference or for a race condition. (Like if a file is added to the dir right after Config.LoadDirectory completes but before this code runs)

That's a good point. I don't think I quite have the bandwidth to implement that change - would a Telegraf maintainer be able to take over?

@conorevans thank you for working on this, it seems another community member has helped take this over: #12127 if you have time could you check if this change works for you by using the artifacts posted by the bot? thanks!

sspaink · 2022-11-03T21:45:56Z

Closing as this PR has been replaced by: #12127

draft: watch configs that are defined in config directories

955fc75

Signed-off-by: Conor Evans <coevans@tcd.ie>

conorevans commented Jan 5, 2022

View reviewed changes

cmd/telegraf/telegraf.go Outdated Show resolved Hide resolved

Hipska added area/agent fix pr to fix corresponding bug labels Apr 12, 2022

Hipska changed the title ~~draft: watch configs that are defined in config directories~~ fix(agent): watch configs that are defined in config directories Apr 12, 2022

Merge branch 'influxdata:master' into conorevans/telegraf9985-watch-c…

0d8e6ba

…onfigs-in-directories

commit

c19ae30

Signed-off-by: Conor Evans <coevans@tcd.ie>

conorevans commented Jun 21, 2022

View reviewed changes

cmd/telegraf/telegraf.go Outdated Show resolved Hide resolved

cmd/telegraf/telegraf.go Outdated Show resolved Hide resolved

fix(formatting): gofmt

41e4d13

Signed-off-by: Conor Evans <coevans@tcd.ie>

reimda added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Aug 11, 2022

reimda approved these changes Aug 11, 2022

View reviewed changes

fix(telegraf): re-add sliceFlags type as it's needed to implement the…

9f9471d

… flag.Value interface

conorevans requested review from Hipska and reimda August 15, 2022 19:32

fix(telegraf): return []string from configDirFiles as it's not to do …

b518aff

…with sliceFlags

reimda changed the title ~~fix(agent): watch configs that are defined in config directories~~ fix(agent): watch for changes in configuration files in config directories Aug 17, 2022

reimda reviewed Aug 17, 2022

View reviewed changes

MyaLongmire removed the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Aug 22, 2022

srebhan added the help wanted Request for community participation, code, contribution label Sep 20, 2022

reimda added the size/s 1 day effort, great beginniner issue label Sep 22, 2022

iarkhanhelsky mentioned this pull request Oct 31, 2022

fix(agent): watch for changes in configuration files in config directories #12127

Merged

3 tasks

sspaink mentioned this pull request Nov 3, 2022

fix(agent): Watch for changes in configuration files in config directories #12173

Closed

sspaink closed this Nov 3, 2022

Conversation

conorevans commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Required for all PRs:

Uh oh!

telegraf-tiger bot commented Jan 5, 2022

Uh oh!

conorevans commented Jan 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

telegraf-tiger bot commented Jan 5, 2022

Artifact URLs

Uh oh!

sjwang90 commented Apr 11, 2022

Uh oh!

conorevans commented Apr 11, 2022

Uh oh!

sjwang90 commented Apr 11, 2022

Uh oh!

iarkhanhelsky commented Jun 3, 2022

Uh oh!

conorevans commented Jun 8, 2022

Uh oh!

conorevans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shruthikudlur commented Aug 11, 2022

Uh oh!

reimda commented Aug 11, 2022

Uh oh!

sspaink commented Aug 15, 2022

Uh oh!

conorevans commented Aug 15, 2022

Uh oh!

telegraf-tiger bot commented Aug 15, 2022

Artifact URLs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sspaink commented Nov 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

conorevans commented Jan 5, 2022 •

edited

Loading