-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
Reloading agent does detect CNI plugin changes (due to FingerprintManager.Reload() not being invoked) #14614
Comments
Hi @pruiz 👋 Thanks for the report. Client config reload is kind of a tricky area because there are all sorts of dangerous race conditions that can happen during this process, so for now there is a very specific list of configuration values that are reloaded: That being said, I was investigation the history of this part of the code and noticed that it was added in this PR: #7518, but |
Hi @lgfa29, From what I see, Reload() on cni code is a noop, because the way it works is the fingerprint manager, after calling Reload, re-does the actual fingerprinting by calling 'fingerprint' on each (reloadable) fingerprint implementation again, see this code from fingerprint manager: As I understood, 'Reload' exists so each 'reloadable fingerprint' implementation can free o release resources before being "reloaded", but CNI fp does not need to release anything, so Reload is a noop in this case, as everything happens when calling fingerprint method again. Or at last that I came to when digging the code. Regards |
Ah right, I missed that detail that the fingreprinter will run again thanks! So it does seem like we may have forgotten to invoke the I will try to get some tests for this, probably in our end-to-end test suite. But thanks for the report and the PR! |
@lgfa29 sure, np. Glad to help ;) |
I'm going to lock this issue because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active issues. |
Nomad version
Nomad v1.3.5 (1359c25)
Operating system and Environment details
Linux/amd64
Issue
While trying to integrate cilium with nomad (see: #12120), I've found nomad agent's is not checking changes on CNI during SIGHUP, which means it cannot detect changes applied to CNI's configuration files. However, by inspecting the code, all the needed infrastructure is in place, it just looks like the call to FingerprintManager.Reload() is missing from SIGHUP handling code.
Reproduction steps
start nomad
make changes to /etc/cni/net.d/*
kill -HUP $NOMAD_PID
Expected Result
Reloading of CNI config changes, including errors on logfile in case CNI config can't be processed. At any point, at least the message "reloading fingerprinter" from ./client/fingerprint_manager.go:114 should appear on log file.
Actual Result
CNI changes are ignored, and nothing (related to fingerprint manager, nor CNI) appears on logfile.
Job file (if appropriate)
N/A
The text was updated successfully, but these errors were encountered: