Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
overlord/ifacestate: fix auto-connect during core snap transition #3145
Conversation
zyga
added some commits
Apr 5, 2017
All checks have passed
zyga
requested review from
mvo5 and
fgimenez
Apr 5, 2017
zyga
added this to the
2.24 milestone
Apr 5, 2017
All checks have passed
fgimenez
reviewed
Apr 6, 2017
@zyga \o/ thanks a lot for fixing this! Could you please update tests/main/classic-ubuntu-core-transition with https://github.com/zyga/snapd/commit/097759295b71fb4b89f2aaf9e3d69ea6eeba7c0b.diff ? With those changes it currently fails in master and passes in your branch.
Cheers!
Some checks were not successful
|
@fgimenez done, thanks! |
|
@zyga thanks, it seems that with your changes we are losing the check for the |
Some checks were not successful
zyga
added some commits
Apr 6, 2017
All checks have passed
| @@ -550,7 +550,17 @@ func (r *Repository) connected(snapName, plugOrSlotName string) ([]ConnRef, erro | ||
| conns = append(conns, connRef) | ||
| } | ||
| } | ||
| - return conns, nil | ||
| + // remove duplicates, this is caused by duplicate plug/slot name on core | ||
| + // and missing validation on plug/snap name when adding core. |
niemeyer
Apr 6, 2017
Contributor
Validation of names is unrelated to this problem.
Then, rather than building a list with duplications and then building a map to then build a list without duplications, we can do it at once above by just skipping entries already seen in the first place.
| @@ -98,6 +101,24 @@ func (m *InterfaceManager) addSnaps() error { | ||
| return nil | ||
| } | ||
| +// fixDisconnectedCorePlugs will re-connect the network-bind plug to the | ||
| +// network-bind slot (on core). This cures the effects of bug | ||
| +// https://bugs.launchpad.net/snappy/+bug/1680097 |
| + if m.repo.Plug("core", "network-bind") != nil { | ||
| + // connect it to the slot (connect is a no-op if connected) | ||
| + connRef := interfaces.ConnRef{ | ||
| + PlugRef: interfaces.PlugRef{Snap: "core", Name: "network-bind"}, |
niemeyer
Apr 6, 2017
Contributor
This looks wrong and needs fixing right away. I have no idea how we allowed duplicated slot+plug names, but that's part of the foundation of the system. We cannot allow that to happen at all.
zyga
Apr 6, 2017
Contributor
Ok, I'll coordinate with @mvo5 so that the next core snap will use the new suffixed plug names.
zyga
Apr 7, 2017
Contributor
The core snap will be corrected on next upload but we also have this fix #3154
zyga
added some commits
Apr 6, 2017
Some checks were not successful
Some checks were not successful
|
I updated this slightly to connect both |
All checks have passed
| +func (m *InterfaceManager) fixDisconnectedCorePlugs() error { | ||
| + const coreName = "core" | ||
| + for _, slotName := range []string{"network-bind", "core-support"} { | ||
| + plugName := fmt.Sprintf("%s-plug", slotName) |
mvo5
Apr 7, 2017
Collaborator
Pardon my ignorance - this means we need to modify the core snap to have a new {network-bind,core-support}-plug name, correct?
mvo5
Apr 10, 2017
Collaborator
AIUI landing the core snap change is not mandatory for this so we should (IMO) leave it for now.
| +// fixDisconnectedCorePlugs will re-connect the network-bind-plug and | ||
| +// core-support-plug to the corresponding slots on the core snap. | ||
| +// This cures the effects of bug https://bugs.launchpad.net/snappy/+bug/1680097 | ||
| +func (m *InterfaceManager) fixDisconnectedCorePlugs() error { |
pedronis
Apr 10, 2017
Contributor
do I understand correctly that either this will do something or #3154, the latter will do something only if there are connections but with the old names?
pedronis
Apr 10, 2017
Contributor
after thinking with @mvo it seems we should not need this but only the bit in autoConnect, at some point we will run autoConnect with new core on the new core before trying to run the hook
| + case candidates[1].Snap.Name() == "ubuntu-core" && candidates[0].Snap.Name() == "core": | ||
| + candidates = candidates[0:1] | ||
| + } | ||
| + } |
pedronis
Apr 10, 2017
•
Edited 1 time
-
pedronis
Apr 10, 2017
Contributor
-
Apr 10, 2017
couldn't we do something directly in transitionConnectionsCoreMigration instead of keeping this special case here?
it's because it's too late there? we need again for the configure hook?
zyga
Apr 10, 2017
Contributor
that place is just dealing with connections in the state. We'd have to somehow convey (from the part that installs core) that ubuntu-core is special and we should not auto-connect to it. I don't think it would be cleaner.
I think we need epoch updates to be able to remove hacks like this.
pedronis
Apr 10, 2017
Contributor
as discussed with @mvo it seems once we fixe slot-side auto connect we need something like this for core-support anyway during the transition and as long as we have the transition, we will have a plug (on core) and two slots (on core and ubuntu-core) and sane autoconnect rules would/should refuse that independent on which side we look at (slots or plugs)
Some checks were not successful
| +// This cures the effects of bug https://bugs.launchpad.net/snappy/+bug/1680097 | ||
| +func (m *InterfaceManager) fixDisconnectedCorePlugs() error { | ||
| + const coreName = "core" | ||
| + for _, slotName := range []string{"network-bind", "core-support"} { |
pedronis
Apr 10, 2017
•
Edited 1 time
-
pedronis
Apr 10, 2017
Contributor
-
Apr 10, 2017
so we found out (and understood) that this is an issue only for network-bind
| @@ -537,17 +537,24 @@ func (r *Repository) connected(snapName, plugOrSlotName string) ([]ConnRef, erro | ||
| if r.plugs[snapName][plugOrSlotName] == nil && r.slots[snapName][plugOrSlotName] == nil { | ||
| return nil, fmt.Errorf("snap %q has no plug or slot named %q", snapName, plugOrSlotName) | ||
| } | ||
| - // Collect all the relevant connections | ||
| + // Collect all the relevant connections but be on the lookout for duplicates. |
niemeyer
Apr 10, 2017
Contributor
Given the fixes we discussed and are planning on merging imminently, we should never have duplicates here, so this doesn't look right.
Some checks were not successful
pedronis
reviewed
Apr 10, 2017
some comments after discussing, see also https://forum.snapcraft.io/t/duplicate-plug-slot-names-inside-the-core-snap/184/33?u=pedronis
| +// fixDisconnectedCorePlugs will re-connect the network-bind-plug and | ||
| +// core-support-plug to the corresponding slots on the core snap. | ||
| +// This cures the effects of bug https://bugs.launchpad.net/snappy/+bug/1680097 | ||
| +func (m *InterfaceManager) fixDisconnectedCorePlugs() error { |
pedronis
Apr 10, 2017
Contributor
do I understand correctly that either this will do something or #3154, the latter will do something only if there are connections but with the old names?
pedronis
Apr 10, 2017
Contributor
after thinking with @mvo it seems we should not need this but only the bit in autoConnect, at some point we will run autoConnect with new core on the new core before trying to run the hook
| + case candidates[1].Snap.Name() == "ubuntu-core" && candidates[0].Snap.Name() == "core": | ||
| + candidates = candidates[0:1] | ||
| + } | ||
| + } |
pedronis
Apr 10, 2017
•
Edited 1 time
-
pedronis
Apr 10, 2017
Contributor
-
Apr 10, 2017
couldn't we do something directly in transitionConnectionsCoreMigration instead of keeping this special case here?
it's because it's too late there? we need again for the configure hook?
zyga
Apr 10, 2017
Contributor
that place is just dealing with connections in the state. We'd have to somehow convey (from the part that installs core) that ubuntu-core is special and we should not auto-connect to it. I don't think it would be cleaner.
I think we need epoch updates to be able to remove hacks like this.
pedronis
Apr 10, 2017
Contributor
as discussed with @mvo it seems once we fixe slot-side auto connect we need something like this for core-support anyway during the transition and as long as we have the transition, we will have a plug (on core) and two slots (on core and ubuntu-core) and sane autoconnect rules would/should refuse that independent on which side we look at (slots or plugs)
mvo5
modified the milestones:
2.25,
2.24
Apr 11, 2017
|
Closing now as a simpler variant of this was merged. |
zyga commentedApr 5, 2017
We found that during the transition of the ubuntu-core snap to the new core snap the hook on the core snap is no longer auto-connected to core (this is for network-bind). This has negative side effects and it happens because during the transition process there are two candidates that supply the network-bind slot, one from the ubuntu-core and and one from the core snap.
This branch adds a quirk where we ignore candidates from ubuntu-core if candidates from core exist.