overlord/fdestate/fdestate.go: fix path to data mount point on hybrid #14734

valentindavid · 2024-11-18T11:27:33Z

We used a path that is not a mount point on hybrid to discover the data disk. Instead we now use / for hybrid, and /writable on core.

valentindavid · 2024-11-18T11:28:34Z

That should make tests/nested/manual/muinstaller-real work again.

codecov · 2024-11-18T12:28:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (fde-manager-features@544bb48). Learn more about missing BASE report.

Additional details and impacted files

@@                   Coverage Diff                   @@
##             fde-manager-features   #14734   +/-   ##
=======================================================
  Coverage                        ?   78.84%           
=======================================================
  Files                           ?     1093           
  Lines                           ?   147888           
  Branches                        ?        0           
=======================================================
  Hits                            ?   116599           
  Misses                          ?    24010           
  Partials                        ?     7279

Flag	Coverage Δ
unittests	`78.84% <100.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

bboozzoo

Can you update the PR description?

bboozzoo · 2024-11-18T12:35:37Z

overlord/fdestate/fdestate.go

@@ -218,7 +219,12 @@ func initializeState(st *state.State) error {

 	// FIXME mount points will be different in recovery or factory-reset modes
 	// either inspect degraded.json, or use boot.HostUbuntuDataForMode()
-	dataUUID, dataErr := disksDMCryptUUIDFromMountPoint(dirs.SnapdStateDir(dirs.GlobalRootDir))
+	dataDir := dirs.GlobalRootDir


isn't this true on hybrid as well? Unless, I'm misunderstanding what the problem is and which path exists in which scenario.

Suggested change

dataDir := dirs.GlobalRootDir

dataDir := dirs.SnapdStateDir(dirs.GlobalRootDir)

I do not think it is a mount point on hybrid. On core it is a bind mount.

Also tests/nested/manual/muinstaller-real fails with this.

bboozzoo · 2024-11-18T12:36:28Z

overlord/fdestate/fdestate.go

+	if osutil.FileExists(writable) {
+		dataDir = writable
+	}


Suggested change

if osutil.FileExists(writable) {

dataDir = writable

}

if osutil.FileExists(writable) {

// appears be an Ubuntu Core device, where /writable exists

dataDir = writable

}

Thinking of it, I will test it is a mount point. Because someone could create /writable and easily break it.

could we check whether release indicates we are on Core and so choose the right path?

Sure. That might be just simpler.

bboozzoo

LGTM, some suggestions about tweaks to the unit tests

bboozzoo · 2024-11-20T10:39:31Z

overlord/fdestate/fdemgr_test.go

 	defer fdestate.MockDMCryptUUIDFromMountPoint(func(mountpoint string) (string, error) {
+		fmt.Printf("%s %s %s\n", mountpoint, dirs.GlobalRootDir, filepath.Join(dirs.GlobalRootDir, "writable"))


c.Logf() ?

Suggested change

fmt.Printf("%s %s %s\n", mountpoint, dirs.GlobalRootDir, filepath.Join(dirs.GlobalRootDir, "writable"))

c.Logf("%s %s %s\n", mountpoint, dirs.GlobalRootDir, filepath.Join(dirs.GlobalRootDir, "writable"))

Ah, that was some debug thing. I can just remove it.

bboozzoo · 2024-11-20T10:40:09Z

overlord/fdestate/fdemgr_test.go

@@ -101,10 +103,17 @@ func (u *instrumentedUnlocker) Relock() {
 	u.relocked += 1
 }

-func (s *fdeMgrSuite) startedManager(c *C) *fdestate.FDEManager {
+func (s *fdeMgrSuite) startedManager(c *C, onClassic bool) *fdestate.FDEManager {
+	s.AddCleanup(release.MockOnClassic(onClassic))


I think I'd move the mocking back to the actual test and have a MockOnClassic(true) in SetUpTest() so that we have a clear 'view' of the system state

ZeyadYasser

Thank you!

Some small nitpicks

ZeyadYasser · 2024-11-20T13:01:03Z

overlord/fdestate/fdestate.go

@@ -218,7 +220,12 @@ func initializeState(st *state.State) error {

 	// FIXME mount points will be different in recovery or factory-reset modes
 	// either inspect degraded.json, or use boot.HostUbuntuDataForMode()
-	dataUUID, dataErr := disksDMCryptUUIDFromMountPoint(dirs.SnapdStateDir(dirs.GlobalRootDir))
+	dataDir := dirs.GlobalRootDir


What makes / the correct mount point to look at to find the data disk on classic? maybe a small comment is needed to explain this.

ZeyadYasser · 2024-11-20T13:07:01Z

overlord/fdestate/fdemgr_test.go

+	c.Check(primaryKey.Digest.Digest, DeepEquals, []byte{5, 6, 7, 8})
+}
+
+func (s *fdeMgrSuite) TestGetManagerFromStateWithWritable(c *C) {


Nitpick, both tests seem identical, maybe the could be merged

Suggested change

func (s *fdeMgrSuite) TestGetManagerFromStateWithWritable(c *C) {

func (s *fdeMgrSuite) testGetManagerFromState(c *C, onClassic bool)

st := s.st

defer release.MockOnClassic(onClassic)()

manager := s.startedManager(c, onClassic)

st.Lock()

defer st.Unlock()

foundManager := fdestate.FdeMgr(st)

c.Check(foundManager, Equals, manager)

var fdeSt fdestate.FdeState

err := st.Get("fde", &fdeSt)

c.Assert(err, IsNil)

primaryKey, hasPrimaryKey := fdeSt.PrimaryKeys[0]

c.Assert(hasPrimaryKey, Equals, true)

c.Check(crypto.Hash(primaryKey.Digest.Algorithm), Equals, crypto.Hash(crypto.SHA256))

c.Check(primaryKey.Digest.Salt, DeepEquals, []byte{1, 2, 3, 4})

c.Check(primaryKey.Digest.Digest, DeepEquals, []byte{5, 6, 7, 8})

}

func (s *fdeMgrSuite) TestGetManagerFromState(c *C) {

const onClassic = true

s.testGetManagerFromState(c, onClassic)

}

func (s *fdeMgrSuite) TestGetManagerFromStateWithWritable(c *C) {

const onClassic = false

s.testGetManagerFromState(c, onClassic)

}

ZeyadYasser · 2024-11-20T13:07:17Z

overlord/fdestate/fdemgr_test.go

@@ -82,6 +84,8 @@ func (s *fdeMgrSuite) SetUpTest(c *C) {
 	}
 	err := m.WriteTo(dirs.GlobalRootDir)
 	c.Assert(err, IsNil)
+
+	s.AddCleanup(release.MockOnClassic(true))


Why is this needed?

@bboozzoo asked for it.

#14734 (comment)

pedronis

nitpick

pedronis · 2024-11-21T11:24:09Z

overlord/fdestate/fdestate.go

+	if !release.OnClassic {
+		// If on Core /writable is a bind mount from data dir
+		dataDir = filepath.Join(dirs.GlobalRootDir, "writable")
+	}


nitpick: it feels like this logic should be a function of the dirs package

pedronis

thank you

We used a path that is not a mount point on hybrid to discover the data disk. Instead we now use / for hybrid, and /writable on core.

valentindavid added Run nested The PR also runs tests inluded in nested suite FDE Manager Pull requests that target FDE manager branch labels Nov 18, 2024

valentindavid closed this Nov 18, 2024

valentindavid reopened this Nov 18, 2024

valentindavid force-pushed the valentindavid/fix-hybrid-data-path branch 2 times, most recently from 6f19bb3 to 39ad1ff Compare November 18, 2024 12:11

valentindavid marked this pull request as ready for review November 18, 2024 12:29

valentindavid requested review from bboozzoo and ZeyadYasser November 18, 2024 12:30

bboozzoo reviewed Nov 18, 2024

View reviewed changes

valentindavid requested a review from bboozzoo November 19, 2024 10:35

bboozzoo approved these changes Nov 20, 2024

View reviewed changes

ZeyadYasser approved these changes Nov 20, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/fix-hybrid-data-path branch from 61223f5 to a80d97c Compare November 20, 2024 13:15

pedronis reviewed Nov 21, 2024

View reviewed changes

valentindavid requested a review from pedronis November 21, 2024 11:48

pedronis approved these changes Nov 21, 2024

View reviewed changes

overlord/fdestate/fdestate.go: fix path to data mount point on hybrid

1d6746f

We used a path that is not a mount point on hybrid to discover the data disk. Instead we now use / for hybrid, and /writable on core.

valentindavid force-pushed the valentindavid/fix-hybrid-data-path branch from 491569b to 1d6746f Compare November 22, 2024 09:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

overlord/fdestate/fdestate.go: fix path to data mount point on hybrid #14734

overlord/fdestate/fdestate.go: fix path to data mount point on hybrid #14734

valentindavid commented Nov 18, 2024 •

edited

Loading

valentindavid commented Nov 18, 2024

codecov bot commented Nov 18, 2024 •

edited

Loading

bboozzoo left a comment

bboozzoo Nov 18, 2024

valentindavid Nov 18, 2024

valentindavid Nov 18, 2024

bboozzoo Nov 18, 2024

valentindavid Nov 18, 2024

bboozzoo Nov 18, 2024

valentindavid Nov 18, 2024

bboozzoo left a comment

bboozzoo Nov 20, 2024

valentindavid Nov 20, 2024

bboozzoo Nov 20, 2024

valentindavid Nov 20, 2024

ZeyadYasser left a comment

ZeyadYasser Nov 20, 2024

ZeyadYasser Nov 20, 2024

ZeyadYasser Nov 20, 2024

valentindavid Nov 20, 2024

valentindavid Nov 20, 2024

pedronis left a comment

pedronis Nov 21, 2024

valentindavid Nov 21, 2024

pedronis left a comment

	dataDir := dirs.GlobalRootDir
	dataDir := dirs.SnapdStateDir(dirs.GlobalRootDir)

		defer fdestate.MockDMCryptUUIDFromMountPoint(func(mountpoint string) (string, error) {
		fmt.Printf("%s %s %s\n", mountpoint, dirs.GlobalRootDir, filepath.Join(dirs.GlobalRootDir, "writable"))

	fmt.Printf("%s %s %s\n", mountpoint, dirs.GlobalRootDir, filepath.Join(dirs.GlobalRootDir, "writable"))
	c.Logf("%s %s %s\n", mountpoint, dirs.GlobalRootDir, filepath.Join(dirs.GlobalRootDir, "writable"))

-func (s *fdeMgrSuite) TestGetManagerFromStateWithWritable(c *C) {
+func (s *fdeMgrSuite) testGetManagerFromState(c *C, onClassic bool)
+	st := s.st
+	defer release.MockOnClassic(onClassic)()
+	manager := s.startedManager(c, onClassic)
+	st.Lock()
+	defer st.Unlock()
+	foundManager := fdestate.FdeMgr(st)
+	c.Check(foundManager, Equals, manager)
+	var fdeSt fdestate.FdeState
+	err := st.Get("fde", &fdeSt)
+	c.Assert(err, IsNil)
+	primaryKey, hasPrimaryKey := fdeSt.PrimaryKeys[0]
+	c.Assert(hasPrimaryKey, Equals, true)
+	c.Check(crypto.Hash(primaryKey.Digest.Algorithm), Equals, crypto.Hash(crypto.SHA256))
+	c.Check(primaryKey.Digest.Salt, DeepEquals, []byte{1, 2, 3, 4})
+	c.Check(primaryKey.Digest.Digest, DeepEquals, []byte{5, 6, 7, 8})
+}
+func (s *fdeMgrSuite) TestGetManagerFromState(c *C) {
+    	const onClassic = true
+    	s.testGetManagerFromState(c, onClassic)
+}
+func (s *fdeMgrSuite) TestGetManagerFromStateWithWritable(c *C) {
+    	const onClassic = false
+    	s.testGetManagerFromState(c, onClassic)
+}

overlord/fdestate/fdestate.go: fix path to data mount point on hybrid #14734

Are you sure you want to change the base?

overlord/fdestate/fdestate.go: fix path to data mount point on hybrid #14734

Conversation

valentindavid commented Nov 18, 2024 • edited Loading

valentindavid commented Nov 18, 2024

codecov bot commented Nov 18, 2024 • edited Loading

Codecov Report

bboozzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeyadYasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

valentindavid commented Nov 18, 2024 •

edited

Loading

codecov bot commented Nov 18, 2024 •

edited

Loading