Skip to content

Conversation

imroc
Copy link
Contributor

@imroc imroc commented Aug 13, 2025

Remove the error log in Wrote CNI configuration cuz it only could be "open /host/etc/cni/net.d/05-cilium.conflist: no such file or directory" when cilium-agent startup at the first time.

See the comments below for detailed explanation:

		if err != nil && !os.IsNotExist(err) { // If it's other errors, it should be logged at here (debug level).
			c.logger.Debug(
				"Failed to read existing CNI configuration file",
				logfields.Error, err,
				logfields.Destination, dest,
			)
		}
		if err := renameio.WriteFile(dest, contents, 0600); err != nil { // If write failed, will return error, will not go to the following code; If no error, the err variable is shadowed, the following err variable will use the previous err variable.
			return fmt.Errorf("failed to write CNI configuration file at %s: %w", dest, err)
		}
		c.logger.Info(
			"Wrote CNI configuration file",
			logfields.Error, err, // Error here must be the NotExist error (usually startup first time), it's meaningless, may even be misleading, should be removed
			logfields.Destination, dest,
		)

Logging example:

time=2025-08-13T01:36:48.119502555Z level=info source=/go/src/github.com/cilium/cilium/daemon/cmd/cni/config.go:355 msg="Wrote CNI configuration file" module=agent.infra.cni-config error="open /host/etc/cni/net.d/05-cilium.conflist: no such file or directory" destination=/host/etc/cni/net.d/05-cilium.conflist

@imroc imroc requested a review from a team as a code owner August 13, 2025 02:05
@imroc imroc requested a review from gandro August 13, 2025 02:05
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 13, 2025
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Aug 13, 2025
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I agree that the existing output is confusing.

However, Debug logs are not shown to users by default, whil Info logs are. Would it make sense to promote the Failed to read existing CNI configuration file to Info level as well?

@gandro gandro added area/cni Impacts the Container Networking Interface between Cilium and the orchestrator. release-note/misc This PR makes changes that have no direct user impact. labels Aug 13, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 13, 2025
@imroc imroc force-pushed the remove-useless-error-log branch from 579b80d to 1a26099 Compare August 13, 2025 07:25
remove the error log in Wrote CNI configuration cuz it only could be
"open /host/etc/cni/net.d/05-cilium.conflist: no such file or directory"
when cilium-agent startup at the first time

Signed-off-by: roc <roc@imroc.cc>
@imroc imroc force-pushed the remove-useless-error-log branch from 1a26099 to e9233a8 Compare August 13, 2025 07:26
@imroc
Copy link
Contributor Author

imroc commented Aug 13, 2025

Thanks for the PR. I agree that the existing output is confusing.

However, Debug logs are not shown to users by default, whil Info logs are. Would it make sense to promote the Failed to read existing CNI configuration file to Info level as well?

Yes, I've promote the Failed to read existing CNI configuration file to Info level.

@gandro gandro enabled auto-merge August 13, 2025 07:28
@gandro
Copy link
Member

gandro commented Aug 13, 2025

/test

@gandro gandro added this pull request to the merge queue Aug 13, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 13, 2025
Merged via the queue into cilium:main with commit 6c337a6 Aug 13, 2025
67 of 71 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cni Impacts the Container Networking Interface between Cilium and the orchestrator. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants