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

ifacemgr: remove stale connections on startup #4968

Merged
merged 10 commits into from
May 16, 2018

Conversation

stolowski
Copy link
Contributor

@stolowski stolowski commented Apr 3, 2018

Remove stale conns left from old snapd releases at startup; I decided to do that in separate method rather than in reloadConnections to have it isolated.

}
if staleConns > 0 {
setConns(m.state, conns)
logger.Noticef("remove %d stale connections", staleConns)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: use past tense please, removed. Alternatively we could log the actually deleted connections because after seeing this message I will only wonder, "gee which connections were removed?"

Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

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

LGTM but please add a unit test first.

@zyga
Copy link
Contributor

zyga commented Apr 4, 2018

There are some real test failures from unit tests affected by this feature:

----------------------------------------------------------------------
FAIL: ifacestate_test.go:2147: interfaceManagerSuite.TestCoreConnectionsRenamed

ifacestate_test.go:2176:
    c.Assert(conns, DeepEquals, map[string]interface{}{
        "core:core-support-plug core:core-support": map[string]interface{}{
            "interface": "core-support", "auto": true,
        },
        "snap:unrelated core:unrelated": map[string]interface{}{
            "interface": "unrelated", "auto": true,
        },
    })
... obtained map[string]interface {} = map[string]interface {}{}
... expected map[string]interface {} = map[string]interface {}{"core:core-support-plug core:core-support":map[string]interface {}{"interface":"core-support", "auto":true}, "snap:unrelated core:unrelated":map[string]interface {}{"interface":"unrelated", "auto":true}}


----------------------------------------------------------------------
FAIL: ifacestate_test.go:1513: interfaceManagerSuite.TestDoDiscardConnsPlug

ifacestate_test.go:1514:
    s.testDoDicardConns(c, "consumer")
ifacestate_test.go:1563:
    c.Check(removed, DeepEquals, map[string]interface{}{
        "consumer:plug producer:slot": map[string]interface{}{"interface": "test"},
    })
... obtained map[string]interface {} = map[string]interface {}{}
... expected map[string]interface {} = map[string]interface {}{"consumer:plug producer:slot":map[string]interface {}{"interface":"test"}}


----------------------------------------------------------------------
FAIL: ifacestate_test.go:1517: interfaceManagerSuite.TestDoDiscardConnsSlot

ifacestate_test.go:1518:
    s.testDoDicardConns(c, "producer")
ifacestate_test.go:1563:
    c.Check(removed, DeepEquals, map[string]interface{}{
        "consumer:plug producer:slot": map[string]interface{}{"interface": "test"},
    })
... obtained map[string]interface {} = map[string]interface {}{}
... expected map[string]interface {} = map[string]interface {}{"consumer:plug producer:slot":map[string]interface {}{"interface":"test"}}


----------------------------------------------------------------------
FAIL: ifacestate_test.go:654: interfaceManagerSuite.TestStrayConnectionsIgnored

ifacestate_test.go:684:
    c.Assert(logLines, HasLen, 2)
... obtained []string = []string{"2018/04/03 13:01:38.510085 helpers.go:292: removed stale connections: consumer:plug producer:slot", "2018/04/03 13:01:38.510644 helpers.go:146: error trying to compare the snap system key: system-key missing on disk", ""}
... n int = 2


----------------------------------------------------------------------
FAIL: ifacestate_test.go:1521: interfaceManagerSuite.TestUndoDiscardConnsPlug

ifacestate_test.go:1522:
    s.testUndoDicardConns(c, "consumer")
ifacestate_test.go:1609:
    c.Check(conns, DeepEquals, map[string]interface{}{
        "consumer:plug producer:slot": map[string]interface{}{"interface": "test"},
    })
... obtained map[string]interface {} = map[string]interface {}{}
... expected map[string]interface {} = map[string]interface {}{"consumer:plug producer:slot":map[string]interface {}{"interface":"test"}}


----------------------------------------------------------------------
FAIL: ifacestate_test.go:1525: interfaceManagerSuite.TestUndoDiscardConnsSlot

ifacestate_test.go:1526:
    s.testUndoDicardConns(c, "producer")
ifacestate_test.go:1609:
    c.Check(conns, DeepEquals, map[string]interface{}{
        "consumer:plug producer:slot": map[string]interface{}{"interface": "test"},
    })
... obtained map[string]interface {} = map[string]interface {}{}
... expected map[string]interface {} = map[string]interface {}{"consumer:plug producer:slot":map[string]interface {}{"interface":"test"}}

@zyga
Copy link
Contributor

zyga commented Apr 4, 2018

We cannot land this before we fix the other bug (the one when snapd starts and there are no interfaces at all) because we would remove all of the stored connections in that case.

@stolowski stolowski changed the title RFC: ifacemgr: remove stale connections on startup ifacemgr: remove stale connections on startup Apr 6, 2018
@codecov-io
Copy link

codecov-io commented Apr 6, 2018

Codecov Report

Merging #4968 into master will decrease coverage by 0.02%.
The diff coverage is 53.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4968      +/-   ##
==========================================
- Coverage   79.05%   79.03%   -0.03%     
==========================================
  Files         489      489              
  Lines       36656    36684      +28     
==========================================
+ Hits        28980    28993      +13     
- Misses       5385     5394       +9     
- Partials     2291     2297       +6
Impacted Files Coverage Δ
overlord/ifacestate/helpers.go 65.38% <53.57%> (-1.29%) ⬇️
overlord/hookstate/hookmgr.go 74.87% <0%> (-1.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 761506d...99e81c0. Read the comment docs.

@zyga zyga removed the ⛔ Blocked label Apr 26, 2018
@zyga
Copy link
Contributor

zyga commented Apr 26, 2018

I don't think this is blocked anymore. We should merge mater to see that it passes and merge this branch.

@stolowski stolowski merged commit 7f52350 into canonical:master May 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants