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

o/ifacestate{,apparmorprompting}: do not hold state lock during prompting startup #14413

Merged
merged 2 commits into from
Aug 23, 2024
Merged

o/ifacestate{,apparmorprompting}: do not hold state lock during prompting startup #14413

merged 2 commits into from
Aug 23, 2024

Conversation

olivercalder
Copy link
Member

@olivercalder olivercalder commented Aug 22, 2024 •
edited
Loading

Recording notices requires state lock, but recording notices asynchronously can cause unexpected behavior for the client. Thus, explicitly unlock state if the state lock is held when we know notices may be recorded; namely, during ifacemgr StartUp.

This reverts #14405. This is tracked internally by https://warthogs.atlassian.net/browse/SNAPDENG-31228

@olivercalder olivercalder added the Needs Samuele review Needs a review from Samuele before it can land label Aug 22, 2024
@olivercalder olivercalder added this to the 2.65 milestone Aug 22, 2024
olivercalder
olivercalder commented Aug 22, 2024
View reviewed changes
overlord/ifacestate/ifacemgr.go Outdated
@@ -273,6 +279,9 @@ func (m *InterfaceManager) Ensure() error {
// if running.
func (m *InterfaceManager) Stop() {
m.stopUDevMon()
// XXX: The state lock is *not* held during Stop(). If it were, we'd need
// to unlock state and re-lock it after the call to
// interfacesRequestsManagerStop, so that it can record notices if needed.
Copy link
Member Author

Choose a reason for hiding this comment

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

@pedronis please confirm that this is the case, from what I could tell this is true but not certain

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, afaik we don't hold the state lock when entering any of the manager methods driven by StateEngine, you could probably replace the XXX with a comment about that

@olivercalder olivercalder changed the title Prompting ensure notice order o/ifacestate{,apparmorprompting}: ensure notice order Aug 22, 2024
@olivercalder olivercalder reopened this Aug 22, 2024
@olivercalder olivercalder changed the title o/ifacestate{,apparmorprompting}: ensure notice order o/ifacestate{,apparmorprompting}: do not hold state lock during prompting startup Aug 22, 2024
pedronis
pedronis reviewed Aug 22, 2024
View reviewed changes
Copy link
Collaborator

@pedronis pedronis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I would suggest to also do this in the tests:

diff --git a/overlord/ifacestate/ifacestate_test.go b/overlord/ifacestate/ifacestate_test.go
index 6836cec88e..aa808bc362 100644
--- a/overlord/ifacestate/ifacestate_test.go
+++ b/overlord/ifacestate/ifacestate_test.go
@@ -378,12 +378,16 @@ func (s *interfaceManagerSuite) TestSmokeAppArmorPromptingEnabled(c *C) {
        fakeManager := &apparmorprompting.InterfacesRequestsManager{}
        restore = ifacestate.MockCreateInterfacesRequestsManager(func(s *state.State) (*apparmorprompting.InterfacesRequestsManager, error) {
                createCount++
+               s.Lock()
+               s.Unlock()
                return fakeManager, nil
        })
        defer restore()
        stopCount := 0
        restore = ifacestate.MockInterfacesRequestsManagerStop(func(m *apparmorprompting.InterfacesRequestsManager) error {
                stopCount++
+               s.st.Lock()
+               s.st.Unlock()
                return nil
        })
        defer restore()

Maybe there are other or better tests to do that in?

Also as suggested elsewhere we should have stop - start test for snapd with the apparmor prompting feature kept enabled

overlord/ifacestate/ifacemgr.go Outdated
@@ -273,6 +279,9 @@ func (m *InterfaceManager) Ensure() error {
// if running.
func (m *InterfaceManager) Stop() {
m.stopUDevMon()
// XXX: The state lock is *not* held during Stop(). If it were, we'd need
// to unlock state and re-lock it after the call to
// interfacesRequestsManagerStop, so that it can record notices if needed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, afaik we don't hold the state lock when entering any of the manager methods driven by StateEngine, you could probably replace the XXX with a comment about that

@olivercalder olivercalder requested a review from pedronis August 22, 2024 18:12
pedronis
pedronis approved these changes Aug 22, 2024
View reviewed changes
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

thank you

@olivercalder olivercalder force-pushed the prompting-ensure-notice-order branch from 8e79c7c to fb26ec0 Compare August 22, 2024 18:35
@olivercalder
Copy link
Member Author

Had a typo :(

@olivercalder
Copy link
Member Author

olivercalder commented Aug 22, 2024 •
edited
Loading

Client integration tests are passing again with this PR. Tested by rebasing #14387 on this PR.

ernestl
ernestl approved these changes Aug 22, 2024
View reviewed changes
Copy link
Collaborator

@ernestl ernestl left a comment

Choose a reason for hiding this comment

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

Thanks

@olivercalder olivercalder added the Squash-merge Please squash this PR when merging. label Aug 22, 2024
@olivercalder
Copy link
Member Author

This PR is ready to be merged when ready @ernestl . The core24 failure is main degraded, just flaky I believe, and the noble failure is upgrade-from-release, which is unrelated and fixed by #14416

@olivercalder
Copy link
Member Author

I don't believe recording notices in goroutines is actually incorrect, it I think it just may be confusing to the test runtime. Some investigation here: canonical/prompting-client#81

Regardless, I think it's clearer for the client if all notices occur in order and before API calls return, so this PR is still worth merging. In the future, we can explore ways to record notices synchronously without requiring the state lock.

@olivercalder
Revert "o/i/apparmorprompting: record notices in goroutines (canonica…
a7a2af9 
…l#14405)"

This reverts commit 2c10f5c.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
@olivercalder olivercalder force-pushed the prompting-ensure-notice-order branch from 5b5be0b to 3247a63 Compare August 23, 2024 00:46
@olivercalder
o/ifacestate: do not hold state lock while starting interfaces reques…
17fcbca 
…ts manager

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

o/ifacestate: test state lock during interfaces requests manager startup

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

tests: add test of snapd startup with apparmor prompting enabled

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

tests: check that snapd successfully shuts down and restarts with prompting enabled

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

tests: fix formatting of after arg in prompting notices query

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
@olivercalder olivercalder force-pushed the prompting-ensure-notice-order branch from 3247a63 to 17fcbca Compare August 23, 2024 00:48
@olivercalder
Copy link
Member Author

I rebased to clean up the history and preserve the revert commit.

@olivercalder olivercalder mentioned this pull request Aug 23, 2024
Closed
@ernestl ernestl merged commit b9d3b0a into canonical:master Aug 23, 2024
50 of 53 checks passed
@ernestl ernestl mentioned this pull request Aug 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ernestl ernestl ernestl approved these changes

@pedronis pedronis pedronis approved these changes

@andrewphelpsj andrewphelpsj Awaiting requested review from andrewphelpsj

Assignees
No one assigned
Labels
cherry-picked Needs Samuele review Needs a review from Samuele before it can land Squash-merge Please squash this PR when merging.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@olivercalder @pedronis @ernestl