Skip to content

Commit

Permalink
many: fix iface static attrs not properly updating (canonical#12878)
Browse files Browse the repository at this point in the history 
* o/ifacestate: fix typo

Signed-off-by: Alex Lewontin <[email protected]>

* o/ifacestate: autoconnection checking should not be task-bound

Signed-off-by: Alex Lewontin <[email protected]>

* o/ifacestate: update static attrs based on {auto,}connection policy

Signed-off-by: Alex Lewontin <[email protected]>

* tests: add spread test for static attr updating

Signed-off-by: Alex Lewontin <[email protected]>

* tests: add shared-memory static-attr update test

Signed-off-by: Alex Lewontin <[email protected]>

* o/ifacestate: add tests for policy-driven static attr updates

Signed-off-by: Alex Lewontin <[email protected]>

* o/ifacestate: simplify state error handling when reloading connections

Signed-off-by: Alex Lewontin <[email protected]>

* Revert "o/ifacestate: simplify state error handling when reloading connections"

This reverts commit d3f67c6.

* o/i: restore DeviceCtx on cleanup

Signed-off-by: Alex Lewontin <[email protected]>

* o/ifacestate: simpler state handling when reloading connections

Signed-off-by: Alex Lewontin <[email protected]>

* o/ifacestate: also check AutoConnect method when doing policy based
reloading

Signed-off-by: Alex Lewontin <[email protected]>

* o/i: use new Sequence helpers in test

Signed-off-by: Alex Lewontin <[email protected]>

* o/ifacestate: add missing appset parameter

* tests/main/upgrade-from-2.15: kill the test

The test is no longer useful. The oldest version we have in any distribution is
2.38 in Trusty, see https://launchpad.net/snapd/+packages but also 2.15 is very
ancient and completely unsupported at this time.

Signed-off-by: Maciej Borzecki <[email protected]>

* overlord: update managers test to account for preserved plug static attributes

Signed-off-by: Maciej Borzecki <[email protected]>

* tests/main/interface-static-attrs-update-on-refresh: clean up after the test

Signed-off-by: Maciej Borzecki <[email protected]>

---------

Signed-off-by: Alex Lewontin <[email protected]>
Signed-off-by: Maciej Borzecki <[email protected]>
Co-authored-by: Philip Meulengracht <[email protected]>
Co-authored-by: Maciej Borzecki <[email protected]>
  • Loading branch information
@alexclewontin @Meulengracht @bboozzoo
3 people authored Jul 4, 2024
1 parent 5291381 commit ccc5562
Show file tree
Hide file tree
Showing 13 changed files with 551 additions and 128 deletions.
14 changes: 7 additions & 7 deletions overlord/ifacestate/handlers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,7 @@ func (m *InterfaceManager) doConnect(task *state.Task, _ *tomb.Tomb) (err error)
// policy "connection" rules, other auto-connections obey the
// "auto-connection" rules
if autoConnect && !byGadget {
autochecker, err := newAutoConnectChecker(st, task, m.repo, deviceCtx)
autochecker, err := newAutoConnectChecker(st, m.repo, deviceCtx)
if err != nil {
return err
}
Expand Down Expand Up @@ -1360,7 +1360,7 @@ func (m *InterfaceManager) doAutoConnect(task *state.Task, _ *tomb.Tomb) error {

snapName := snapsup.InstanceName()

autochecker, err := newAutoConnectChecker(st, task, m.repo, deviceCtx)
autochecker, err := newAutoConnectChecker(st, m.repo, deviceCtx)
if err != nil {
return err
}
Expand Down Expand Up @@ -1431,7 +1431,7 @@ func (m *InterfaceManager) doAutoConnect(task *state.Task, _ *tomb.Tomb) error {
cannotAutoConnectLog := func(plug *snap.PlugInfo, candRefs []string) string {
return fmt.Sprintf("cannot auto-connect plug %s, candidates found: %s", plug, strings.Join(candRefs, ", "))
}
if err := autochecker.addAutoConnections(newconns, plugs, nil, conns, cannotAutoConnectLog, conflictError); err != nil {
if err := autochecker.addAutoConnections(task, newconns, plugs, nil, conns, cannotAutoConnectLog, conflictError); err != nil {
return err
}
// Auto-connect all the slots
Expand All @@ -1444,7 +1444,7 @@ func (m *InterfaceManager) doAutoConnect(task *state.Task, _ *tomb.Tomb) error {
cannotAutoConnectLog := func(plug *snap.PlugInfo, candRefs []string) string {
return fmt.Sprintf("cannot auto-connect slot %s to plug %s, candidates found: %s", slot, plug, strings.Join(candRefs, ", "))
}
if err := autochecker.addAutoConnections(newconns, candidates, filterForSlot(slot), conns, cannotAutoConnectLog, conflictError); err != nil {
if err := autochecker.addAutoConnections(task, newconns, candidates, filterForSlot(slot), conns, cannotAutoConnectLog, conflictError); err != nil {
return err
}
}
Expand Down Expand Up @@ -1681,7 +1681,7 @@ func (m *InterfaceManager) doHotplugConnect(task *state.Task, _ *tomb.Tomb) erro
conn := conns[id]
// device was not unplugged, this is the case if snapd is restarted and we enumerate devices.
// note, the situation where device was not unplugged but has changed is handled
// by hotlugDeviceAdded handler - updateDevice.
// by hotplugDeviceAdded handler - updateDevice.
if !conn.HotplugGone || conn.Undesired {
continue
}
Expand All @@ -1700,7 +1700,7 @@ func (m *InterfaceManager) doHotplugConnect(task *state.Task, _ *tomb.Tomb) erro
}

// find new auto-connections
autochecker, err := newAutoConnectChecker(st, task, m.repo, deviceCtx)
autochecker, err := newAutoConnectChecker(st, m.repo, deviceCtx)
if err != nil {
return err
}
Expand All @@ -1713,7 +1713,7 @@ func (m *InterfaceManager) doHotplugConnect(task *state.Task, _ *tomb.Tomb) erro
cannotAutoConnectLog := func(plug *snap.PlugInfo, candRefs []string) string {
return fmt.Sprintf("cannot auto-connect hotplug slot %s to plug %s, candidates found: %s", slot, plug, strings.Join(candRefs, ", "))
}
if err := autochecker.addAutoConnections(newconns, candidates, filterForSlot(slot), conns, cannotAutoConnectLog, conflictError); err != nil {
if err := autochecker.addAutoConnections(task, newconns, candidates, filterForSlot(slot), conns, cannotAutoConnectLog, conflictError); err != nil {
return err
}

Expand Down
91 changes: 66 additions & 25 deletions overlord/ifacestate/helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,28 @@ func (m *InterfaceManager) reloadConnections(snapName string) ([]string, error)
return nil, err
}

var policyChecker interfaces.PolicyFunc
var autoChecker *autoConnectChecker
var connChecker *connectChecker

deviceCtx, err := snapstate.DeviceCtx(m.state, nil, nil)
if errors.Is(err, state.ErrNoState) {
// everything else is a noop, as no model means no connections
// to reload
return nil, nil
} else if err != nil {
return nil, err
}
autoChecker, err = newAutoConnectChecker(m.state, m.repo, deviceCtx)
if err != nil {
return nil, err
}

connChecker, err = newConnectChecker(m.state, deviceCtx)
if err != nil {
return nil, err
}

connStateChanged := false
affected := make(map[string]bool)
ConnsLoop:
Expand Down Expand Up @@ -402,27 +424,48 @@ ConnsLoop:
var updateStaticAttrs bool
staticPlugAttrs := connState.StaticPlugAttrs
staticSlotAttrs := connState.StaticSlotAttrs
newStaticPlugAttrs := utils.NormalizeInterfaceAttributes(plugInfo.Attrs).(map[string]interface{})
newStaticSlotAttrs := utils.NormalizeInterfaceAttributes(slotInfo.Attrs).(map[string]interface{})

// if the interface was originally autoconnected, update the static attrs if it would
// still be allowed to autoconnect. Otherwise, update the static attrs if it would still
// be allowed to regular connect.
if connState.Auto && !connState.ByGadget {
policyChecker = func(cplug *interfaces.ConnectedPlug, cslot *interfaces.ConnectedSlot) (bool, error) {
iface, err := interfaces.ByName(cplug.Interface())
if err != nil {
return false, err
}

// XXX: Refresh the copy of the static connection attributes for "content"
// and "system-files" interfaces.
// This is a partial and temporary solution to https://bugs.launchpad.net/snapd/+bug/1825883
// and https://bugs.launchpad.net/snapd/+bug/1942266.
switch plugInfo.Interface {
case "content":
var plugContent, slotContent string
plugInfo.Attr("content", &plugContent)
slotInfo.Attr("content", &slotContent)

if plugContent != "" && plugContent == slotContent {
staticPlugAttrs = utils.NormalizeInterfaceAttributes(plugInfo.Attrs).(map[string]interface{})
staticSlotAttrs = utils.NormalizeInterfaceAttributes(slotInfo.Attrs).(map[string]interface{})
updateStaticAttrs = true
} else {
logger.Noticef("cannot refresh static attributes of the connection %q", connId)
if !iface.AutoConnect(plugInfo, slotInfo) {
return false, nil
}

ok, _, err := autoChecker.check(cplug, cslot)
return ok, err
}
case "system-files":
staticPlugAttrs = utils.NormalizeInterfaceAttributes(plugInfo.Attrs).(map[string]interface{})
staticSlotAttrs = utils.NormalizeInterfaceAttributes(slotInfo.Attrs).(map[string]interface{})
} else {
policyChecker = connChecker.check
}

plugAppSet, err := interfaces.NewSnapAppSet(plugInfo.Snap, nil)
if err != nil {
return nil, err
}
slotAppSet, err := interfaces.NewSnapAppSet(slotInfo.Snap, nil)
if err != nil {
return nil, err
}

cplug := interfaces.NewConnectedPlug(plugInfo, plugAppSet, newStaticPlugAttrs, connState.DynamicPlugAttrs)
cslot := interfaces.NewConnectedSlot(slotInfo, slotAppSet, newStaticSlotAttrs, connState.DynamicSlotAttrs)

ok, err := policyChecker(cplug, cslot)
if !ok || err != nil {
logger.Noticef("cannot refresh static attributes of the connection %q", connId)
} else {
staticPlugAttrs = newStaticPlugAttrs
staticSlotAttrs = newStaticSlotAttrs
updateStaticAttrs = true
}

Expand Down Expand Up @@ -704,22 +747,20 @@ var DebugAutoConnectCheck func(*policy.ConnectCandidate, interfaces.SideArity, e

type autoConnectChecker struct {
st *state.State
task *state.Task
repo *interfaces.Repository

deviceCtx snapstate.DeviceContext
cache map[string]*asserts.SnapDeclaration
baseDecl *asserts.BaseDeclaration
}

func newAutoConnectChecker(s *state.State, task *state.Task, repo *interfaces.Repository, deviceCtx snapstate.DeviceContext) (*autoConnectChecker, error) {
func newAutoConnectChecker(s *state.State, repo *interfaces.Repository, deviceCtx snapstate.DeviceContext) (*autoConnectChecker, error) {
baseDecl, err := assertstate.BaseDeclaration(s)
if err != nil {
return nil, fmt.Errorf("internal error: cannot find base declaration: %v", err)
}
return &autoConnectChecker{
st: s,
task: task,
repo: repo,
deviceCtx: deviceCtx,
cache: make(map[string]*asserts.SnapDeclaration),
Expand Down Expand Up @@ -833,7 +874,7 @@ func filterUbuntuCoreSlots(candidates []*snap.SlotInfo, arities []interfaces.Sid
// conns. cannotAutoConnectLog is called to build a log message in
// case no applicable pair was found. conflictError is called
// to handle checkAutoconnectConflicts errors.
func (c *autoConnectChecker) addAutoConnections(newconns map[string]*interfaces.ConnRef, plugs []*snap.PlugInfo, filter func([]*snap.SlotInfo) []*snap.SlotInfo, conns map[string]*schema.ConnState, cannotAutoConnectLog func(plug *snap.PlugInfo, candRefs []string) string, conflictError func(*state.Retry, error) error) error {
func (c *autoConnectChecker) addAutoConnections(task *state.Task, newconns map[string]*interfaces.ConnRef, plugs []*snap.PlugInfo, filter func([]*snap.SlotInfo) []*snap.SlotInfo, conns map[string]*schema.ConnState, cannotAutoConnectLog func(plug *snap.PlugInfo, candRefs []string) string, conflictError func(*state.Retry, error) error) error {
for _, plug := range plugs {
candSlots, arities := c.repo.AutoConnectCandidateSlots(plug.Snap.InstanceName(), plug.Name, c.check)

Expand Down Expand Up @@ -869,12 +910,12 @@ func (c *autoConnectChecker) addAutoConnections(newconns map[string]*interfaces.
for i, candidate := range candSlots {
crefs[i] = candidate.String()
}
c.task.Logf(cannotAutoConnectLog(plug, crefs))
task.Logf(cannotAutoConnectLog(plug, crefs))
continue
}

for _, slot := range applicable {
if err := addNewConnection(c.st, c.task, newconns, conns, plug, slot, conflictError); err != nil {
if err := addNewConnection(c.st, task, newconns, conns, plug, slot, conflictError); err != nil {
return err
}
}
Expand Down
22 changes: 22 additions & 0 deletions overlord/ifacestate/helpers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,12 @@ import (
"os"
"path/filepath"
"strings"
"time"

. "gopkg.in/check.v1"

"github.com/snapcore/snapd/asserts"
"github.com/snapcore/snapd/asserts/assertstest"
"github.com/snapcore/snapd/dirs"
"github.com/snapcore/snapd/interfaces"
"github.com/snapcore/snapd/interfaces/ifacetest"
Expand All @@ -45,6 +48,7 @@ import (
)

type helpersSuite struct {
testutil.BaseTest
st *state.State
}

Expand All @@ -53,12 +57,30 @@ var _ = Suite(&helpersSuite{})
func (s *helpersSuite) SetUpTest(c *C) {
s.st = state.New(nil)
dirs.SetRootDir(c.MkDir())

s.MockModel(c, nil)
}

func (s *helpersSuite) TearDownTest(c *C) {
dirs.SetRootDir("")
}

func (s *helpersSuite) MockModel(c *C, extraHeaders map[string]interface{}) {
model := assertstest.FakeAssertion(map[string]interface{}{
"type": "model",
"authority-id": "my-brand",
"series": "16",
"brand-id": "my-brand",
"model": "my-model",
"gadget": "gadget",
"kernel": "krnl",
"architecture": "amd64",
"timestamp": time.Now().Format(time.RFC3339),
}, extraHeaders).(*asserts.Model)

s.AddCleanup(snapstatetest.MockDeviceModel(model))
}

func (s *helpersSuite) TestIdentityMapper(c *C) {
var m ifacestate.SnapMapper = &ifacestate.IdentityMapper{}

Expand Down
Loading

0 comments on commit ccc5562

Please sign in to comment.