Skip to content
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.

Sign up for GitHub

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

Jump to bottom

idempotent portmap teardown #420

Closed
wants to merge 1 commit into from
Closed

idempotent portmap teardown #420

wants to merge 1 commit into from

Conversation

aojea
Copy link
Contributor

@aojea aojea commented Dec 9, 2019 •
edited
Loading

It turns out that the portmap plugin is not idempotent if its executed in parallel.
The errors are caused due to a race of different iptables executions deleting the chains.
This patch adds a mutex based in a file lock that solves the concurrency problems.

xref: #418

@aojea aojea force-pushed the portmapErrors branch 3 times, most recently from d8e28fc to 4fb7b7f Compare December 9, 2019 18:16
@aojea
Copy link
Contributor Author

aojea commented Dec 9, 2019

/assign @dcbw @danwinship

@aojea aojea force-pushed the portmapErrors branch 2 times, most recently from 3cfa6c3 to b07022f Compare December 9, 2019 19:53
@aojea aojea changed the title Portmap doesn't fail if chain doesn't exist Portmap should not fail if chain doesn't exist Dec 9, 2019
@aojea aojea mentioned this pull request Dec 9, 2019
Closed
@aojea aojea force-pushed the portmapErrors branch 5 times, most recently from e3b6765 to 599d4fc Compare December 10, 2019 10:58
@aojea aojea changed the title Portmap should not fail if chain doesn't exist idempotent portmap teardown Dec 10, 2019
@aojea
portmap: add a mutex
185988b 
Use a cross-proces mutex based in a lock file to make
the teardown portmap function idempotent.

The teardown portmap function is not idempotent if it
is executed in parallel because it has concurrency
issues modifying the iptables rules.

Signed-off-by: Antonio Ojea <[email protected]>
aojea
aojea commented Dec 10, 2019
View reviewed changes
plugins/meta/portmap/chain.go
"github.com/mattn/go-shellwords"
)

const lockfile = "/tmp/portmap.lock"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this the right approach?

@aojea aojea force-pushed the portmapErrors branch 6 times, most recently from 615f3a7 to 185988b Compare December 11, 2019 09:55
@mars1024
Copy link
Member

Related PR #408

@aojea
Copy link
Contributor Author

aojea commented Dec 11, 2019

Related PR #408

I've added a test to the PR that verifies the idempotency if there are concurrent executions.
I've tried the approach of the mentioned PR but was still flaky and failed constantly without adding a sleep to the test between goroutines.
The external lock guarantees the atomicity of the operation, as you can see with 10 goroutines there are no errors tearing down the portmap rules

@mars1024
Copy link
Member

Related PR #408

I've added a test to the PR that verifies the idempotency if there are concurrent executions.
I've tried the approach of the mentioned PR but was still flaky and failed constantly without adding a sleep to the test between goroutines.
The external lock guarantees the atomicity of the operation, as you can see with 10 goroutines there are no errors tearing down the portmap rules

Do you have the old PR branch which I can take a look at?
If we want to make a function idempotent, we must make sure every step of this function is idempotent, just like #408, maybe more error handlers are needed.

IMO, global locker should be our last choice, but I believe it will work.

@mars1024
Copy link
Member

/cc @squeed

@aojea
Copy link
Contributor Author

aojea commented Dec 11, 2019

@mars1024 I've create a new PR so you can compare #421

@aojea
Copy link
Contributor Author

aojea commented Dec 11, 2019

/close in favor of #421

@aojea aojea closed this Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aojea @mars1024