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
Use iptables-restore xtables lock #1903
Conversation
- Disable our lock if iptables v1.6.2 is detected. - Re-use our lock timeout and probe interval config.
f0d8aba
to
fe7c37b
Compare
fe7c37b
to
492187b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just 2 or 3 possible nits.
iptables/table.go
Outdated
cmd.SetStdin(&inputBuf) | ||
cmd.SetStdout(&outputBuf) | ||
cmd.SetStderr(&errBuf) | ||
countNumRestoreCalls.Inc() | ||
t.writeLock.Lock() | ||
t.calicoXtablesLock.Lock() // Will be a dummy lock if our xtables lock is disabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer "Will be a dummy lock if iptables-restore is acquiring the xtables lock itself."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's more than one reason for it to be disabled but probably worth expanding the comment to explain.
iptables/table.go
Outdated
cmd := t.newCmd(t.iptablesRestoreCmd, "--noflush", "--verbose") | ||
args := []string{"--noflush", "--verbose"} | ||
if features.RestoreSupportsLock { | ||
// Versions of iptables-restore that support the xtables lock also make it mandatory. Make sure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By "mandatory", do you mean that iptables-restore will give up if the lock is not available immediately? Because it obviously isn't mandatory in the sense that you still need args to tell it to retry...
Mostly just checking my understanding here; I can't think of an obvious wording improvement to the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, it won't proceed to do any iptables programming without the lock. Its default behaviour with no args is to quit immediately if it can't acquire the lock.
Will see if I can tighten up the wording.
Use iptables-restore xtables lock
Description
Based on feature-detection work added in #1901
Todos
Release Note