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.

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

Allow plugins to have multiple handlers #24172

Closed
wants to merge 1 commit into from
Closed

Conversation

mavenugo
Copy link
Contributor

Currently the plugins pkg allows a single handler. This assumption breaks down if there are mutiple listeners to a plugin of a certain Manifest such as NetworkDriver or IpamDriver when swarm-mode is enabled.

Fixes #23990

Signed-off-by: Madhu Venugopal madhu@docker.com

@mavenugo
Copy link
Contributor Author

ping @mrjana @aboch @cpuguy83 @bboreham

@mavenugo
Copy link
Contributor Author

Though not directly related to the new plugin infra... it would be good if @tiborvass & @anusha-ragunathan can review it as well.

@bboreham
Copy link
Contributor

As described in weaveworks/weave#2403 (comment) I have tried this and it seems to resolve the issue originally reported. Thanks!

extpointHandlers[iface] = handlers
pa, _ := GetAll(iface)
for _, p := range pa {
p.activated = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? (bool defaults to false)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is required because we want to make sure p.activated is reset to false if a 2nd handler gets registered later than the first handler gets the activation call.

Once activated, plugin never gives the callback. Hence whenever a new handler is added, we have to reset the flag so that they can receive the callback.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.

Currently the plugins pkg allows a single handler. This assumption
breaks down if there are mutiple listeners to a plugin of a certain
Manifest such as NetworkDriver or IpamDriver when swarm-mode is enabled.

Signed-off-by: Madhu Venugopal <madhu@docker.com>
@cpuguy83
Copy link
Member

cpuguy83 commented Jul 1, 2016

panic: DockerNetworkSuite.TestDockerNetworkConnectDisconnectToStoppedContainer test timed out after 5m0s

This seems to be happening reliably.

@thaJeztah thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jul 2, 2016
@icecrime
Copy link
Contributor

icecrime commented Jul 5, 2016

Ping @mavenugo!

@mavenugo
Copy link
Contributor Author

mavenugo commented Jul 5, 2016

@anusha-ragunathan @cpuguy83 @icecrime I was thinking about this issue in more detail and I think I will rather fix it in swarmkit to not use the remote-ipam driver. That way it will be consistent with network-driver and hence this issue will not be exposed on the engine side.

When swarmkit supports plugin in 1.13, we can have this PR addressed as well.

I will pull another PR with just the vendor-in of swarmkit and libnetwork to address this issue.

Lets keep this PR open and I will move the release milestone to 1.13

@mavenugo
Copy link
Contributor Author

mavenugo commented Jul 6, 2016

moby/swarmkit#1130 & moby/libnetwork#1310 should solve #23990

@mavenugo
Copy link
Contributor Author

Closing this PR as the issue was already handled by other fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/plugins status/failing-ci Indicates that the PR in its current state fails the test suite status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants