Ignore error when adding a bridge already in the ipset#49295
Ignore error when adding a bridge already in the ipset#49295robmry merged 1 commit intomoby:masterfrom
Conversation
robmry
added
area/networking
Signed-off-by: Rob Murray <[email protected]>
robmry
force-pushed
the
ignore_subnet_already_in_ipset
branch
from
860754a to
feb2dab
Compare
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
overall LGTM
left some questions, but after seeing it only logs, probably not a blocker
| "subnet": maskedAddr, | ||
| }).Warnf("Subnet was already in the ipset") | ||
| } | ||
| n.registerIptCleanFunc(func() error { |
There was a problem hiding this comment.
If we didn't add it again because it was already there, does that mean we can also skip the "cleanup" function? Could that fail if it's ran multiple times (and therefore failing the second time)?
There was a problem hiding this comment.
Ah, looks like "failing" is in good'Ol libnetwork fashion; fail, log, and continue, so I guess it won't do harm currently, but let me know what you think (also considering the reverse; is it worse if we would skip a cleanup?)
moby/libnetwork/drivers/bridge/bridge_linux.go
Lines 1040 to 1045 in 7e78f78
There was a problem hiding this comment.
I guess we could make the cleanup func also detect a "does not exist" check; does it have such an error defined to check for?
There was a problem hiding this comment.
I don't think it's an issue, it was only getting left behind when the daemon stopped. So the cleanup function won't be set up twice. This code isn't called twice for the same subnet.
I can reproduce it now, I must have used the wrong build when I tried before. But it happens when driver init fails, without the fix to make network deletion work in that case. (Perhaps there's some missing cleanup somewhere though, I'll look in to that.)
There was a problem hiding this comment.
OK feel free to merge this one is nothing more is needed in this PR (I kept it open in case you wanted to add changes)
2 participants
- What I did
Ignore error "exists" when adding a network to an
ipset.I hit this error somehow, possibly after stopping the daemon in the debugger at an inconvenient point ... then wasn't able to repro.
So, not very satisfactory, but I think this change is the right thing anyway.
- How I did it
- How to verify it
Not sure how to provoke the error naturally. So, to test the
errors.Isand the log line, modified the code to deliberately add to the set twice. Got this on startup ...WARN[2025-01-17T19:26:27.856651959Z] Subnet was already in the ipset bridge=docker0 ipset=docker-ext-bridges-v4 subnet=172.18.0.0/16- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)