overlord/state, tests: measure state lock #14874

sergiocazzolato · 2024-12-18T02:34:17Z

This change allows to collect the time the snapd state file is locked. To collect the results it is required to include the test snapd-state-lock and make sure just 1 worker is used.

SPREAD_SNAPD_STATE_LOCK_TRACE_THRESHOLD_MS=150 SPREAD_USE_SNAPD_SNAP_URL= SPREAD_USE_PREBUILT_SNAPD_SNAP=false ./run-spread -artifacts .artifacts google:ubuntu-22.04-64:tests/main/snapd-state-lock google:ubuntu-22.04-64:tests/smoke/

this is an example of the smoke suite with threshold=150ms: https://pastebin.canonical.com/p/T3bDG25f5q/
this is an example of the main suite with threshold=150ms: https://pastebin.canonical.com/p/sZjvcGdZFD/

pedronis

question

pedronis · 2024-12-20T13:15:19Z

overlord/state/state.go

@@ -139,6 +142,7 @@ func (s *State) Modified() bool {

 // Lock acquires the state lock.
 func (s *State) Lock() {
+	s.lockStart = osutil.GetLockStart()


shouldn't this be done with the lock taken? otherwise just the tentative themselves to lock will race/dirty lockStart

right, I'll update this

sergiocazzolato · 2025-01-06T14:04:46Z

These are the times after the fix proposed by @pedronis

lock-times-smoke.txt

I see it locks about 820ms when the snapd daemon is started (this happens in all the tests)

pedronis · 2025-01-06T17:16:25Z

overlord/state/state.go

@@ -159,6 +163,7 @@ func (s *State) writing() {
 func (s *State) unlock() {
 	atomic.AddInt32(&s.muC, -1)
 	s.mu.Unlock()
+	osutil.MaybeSaveLockTime(s.lockStart)


this only needs to happen with the lock taken I think

This may be ok to do with the state lock released if we flock the log file. With the state lock held, there would likely be no contention for flock on the log file. OTOH with the lock released, we'd rely on flock to serialize writes, but this would not keep the state lock unnecessarily held.

FWIW, unlocking may reschedule, so assuming we use flock for serializing writes, the timing may end up being incorrect given that the time is captured inside MaybeSaveLockTime(). The end time will likely need to be captured like so:

lockEnd := time.Now() s.mu.Unlock() osutil.MaybeSaveLockTime(s.lockStart, lockEnd)

note, I missed that s.lockStart is used after releasing the lock, it should ofc be read into a variable before the lock is released

sergiocazzolato · 2025-01-06T19:36:53Z

@pedronis This is the result of the main suite 145 tests (using a machine with 2 cores and threshold of 800ms),
https://pastebin.canonical.com/p/Km4DrSskYD/

I see some worst times doing:
github.com/snapcore/snapd/daemon.(*Daemon).Start
github.com/snapcore/snapd/overlord/assertstate.doValidateSnap

should we specially test those cases?

osutil/statelock.go

bboozzoo · 2025-01-07T15:34:33Z

osutil/statelock.go

+// MaybeSaveLockTime allows to save lock times when this overpass the threshold
+// defined by through the SNAPD_STATE_LOCK_THRESHOLD_MS environment settings.
+func MaybeSaveLockTime(lockStart int64) {
+	lockEnd := time.Now().UnixNano() / int64(time.Millisecond)


this can prob be done after both getenvs

bboozzoo · 2025-01-07T15:39:26Z

overlord/state/state.go

@@ -159,6 +163,7 @@ func (s *State) writing() {
 func (s *State) unlock() {
 	atomic.AddInt32(&s.muC, -1)
 	s.mu.Unlock()
+	osutil.MaybeSaveLockTime(s.lockStart)


This may be ok to do with the state lock released if we flock the log file. With the state lock held, there would likely be no contention for flock on the log file. OTOH with the lock released, we'd rely on flock to serialize writes, but this would not keep the state lock unnecessarily held.

FWIW, unlocking may reschedule, so assuming we use flock for serializing writes, the timing may end up being incorrect given that the time is captured inside MaybeSaveLockTime(). The end time will likely need to be captured like so:

lockEnd := time.Now() s.mu.Unlock() osutil.MaybeSaveLockTime(s.lockStart, lockEnd)

osutil/statelock.go

bboozzoo · 2025-01-07T15:45:06Z

osutil/statelock.go

+		f := runtime.FuncForPC(pc[i])
+		file, line := f.FileLine(pc[i])
+		formattedLine = fmt.Sprintf("%s:%d %s\n", file, line, f.Name())
+		if _, err = lockfile.WriteString(formattedLine); err != nil {


I wonder if we could maybe use runtime.Profile to achieve the same thing but do it in more standard way.

osutil/statelock.go

overlord/state/state.go

bboozzoo · 2025-01-07T15:59:37Z

osutil/statelock.go

+
+// MaybeSaveLockTime allows to save lock times when this overpass the threshold
+// defined by through the SNAPD_STATE_LOCK_THRESHOLD_MS environment settings.
+func MaybeSaveLockTime(lockStart int64) {


Suggested change

func MaybeSaveLockTime(lockStart int64) {

func MaybeSaveLockTime(lockWait, lockStart, lockEnd int64) {

overlord/state/state.go

bboozzoo · 2025-01-07T16:01:31Z

overlord/state/state.go

@@ -159,6 +163,7 @@ func (s *State) writing() {
 func (s *State) unlock() {
 	atomic.AddInt32(&s.muC, -1)
 	s.mu.Unlock()
+	osutil.MaybeSaveLockTime(s.lockStart)


note, I missed that s.lockStart is used after releasing the lock, it should ofc be read into a variable before the lock is released

osutil/statelock.go

codecov · 2025-01-08T04:21:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (master@79632c6). Learn more about missing BASE report.
Report is 17 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #14874   +/-   ##
=========================================
  Coverage          ?   78.26%           
=========================================
  Files             ?     1154           
  Lines             ?   153335           
  Branches          ?        0           
=========================================
  Hits              ?   120015           
  Misses            ?    25920           
  Partials          ?     7400

Flag	Coverage Δ
unittests	`78.26% <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.

sergiocazzolato · 2025-01-08T12:20:32Z

@bboozzoo @pedronis these are the results for the smoke suite after the latest changes
https://pastebin.canonical.com/p/wFdxyZYQ48/

Those were executed in a vm with 2 cpus

osutil/flock.go

osutil/statelock.go

bboozzoo · 2025-01-08T13:28:07Z

osutil/statelock.go

+	"time"
+)
+
+func traceCallers(description string) {


Suggested change

func traceCallers(description string) {

func traceCallers(description string) error {

and then you can update MaybeSave to do:

if heldMs > threshold || waitMs > threshold { formattedLine := fmt.Sprintf("lock: held %d ms wait %d ms", heldMs, waitMs) if err := traceCallers(formattedLine); err != { logger.Infof("cannot trace callers: %v", err) } }

and change fmt.Fprintf() to use return fmt.Errorf("cannot this or that: %w", err)

I tried that initially bet realized that we cannot use logger in utils, is there any other way to use it?

I implemented using fmt.Fprintf() because I saw faultinject does that too.

.github/workflows/test.yaml

pedronis

thank you, some questions, some comments

pedronis · 2025-01-10T16:52:12Z

osutil/statelock.go

+	}()
+
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "cannot enable state lock tracing: %v\n", err)


I'm not sure it make sense to have all the testing snapds print this?

pedronis · 2025-01-10T16:53:13Z

osutil/statelock.go

+		return
+	}
+
+	now := LockTimestamp()


wasn't there a discussion that should be passed in too?

pedronis · 2025-01-10T16:59:46Z

osutil/statelock.go

what's the reason of having this in osutil? at face value is a bit of a odd place for this

per mm discussion, moved the code to overlord/state

pedronis

thanks for the changes, one question still

pedronis · 2025-01-13T13:51:51Z

overlord/state/state.go

 	s.mu.Unlock()
-	osutil.MaybeSaveLockTime(lockWait, lockStart)
+	osutil.MaybeSaveLockTime(lockWaitStart, lockHoldStart, osutil.LockTimestamp())


shouldn't the LockTimestamp be captured before the Unlock, assuming the Unlock anyway is quick?

good point, I made a similar comment on the original code and then forgot to update it.

pedronis

thanks everybody, can we produce the result with the latest changes?

miguelpires

LGTM, just a few nitpicks

miguelpires · 2025-01-15T15:41:35Z

overlord/state/locktrace.go

+//go:build statelocktrace
+
+/*
+ * Copyright (C) 2021 Canonical Ltd


Suggested change

* Copyright (C) 2021 Canonical Ltd

* Copyright (C) 2025 Canonical Ltd

miguelpires · 2025-01-15T16:07:29Z

overlord/state/locktrace.go

+	}
+
+	pc := make([]uintptr, 10)
+	// avoid 3 first callers on the stack: runtime.Callers(), this function and the parent


nitpick

Suggested change

// avoid 3 first callers on the stack: runtime.Callers(), this function and the parent

// ignore the first 3 callers on the stack: runtime.Callers(), this function and the parent

miguelpires · 2025-01-15T17:02:13Z

overlord/state/locktrace_fake.go

+//go:build !statelocktrace
+
+/*
+ * Copyright (C) 2021 Canonical Ltd


Suggested change

* Copyright (C) 2021 Canonical Ltd

* Copyright (C) 2024 Canonical Ltd

miguelpires · 2025-01-15T17:11:41Z

overlord/state/locktrace_fake.go

+func lockTimestamp() int64 {
+	return int64(0)
+}
+
+func maybeSaveLockTime(lockWaitStart, lockHoldStart, now int64) {
+}


tiny nitpick, we can simplify the no-ops

Suggested change

func lockTimestamp() int64 {

return int64(0)

}

func maybeSaveLockTime(lockWaitStart, lockHoldStart, now int64) {

}

func lockTimestamp() int64 { return 0 }

func maybeSaveLockTime(int64, int64, int64) {}

miguelpires · 2025-01-15T17:17:07Z

overlord/state/locktrace_fake.go

+//go:build !statelocktrace
+
+/*
+ * Copyright (C) 2021 Canonical Ltd


Suggested change

* Copyright (C) 2021 Canonical Ltd

* Copyright (C) 2024 Canonical Ltd

miguelpires · 2025-01-15T17:23:35Z

overlord/state/locktrace.go

+// maybeSaveLockTime allows to save lock times when this overpass the threshold
+// defined by through the SNAPD_STATE_LOCK_THRESHOLD_MS environment settings.


minor suggestion for clarity's sake

Suggested change

// maybeSaveLockTime allows to save lock times when this overpass the threshold

// defined by through the SNAPD_STATE_LOCK_THRESHOLD_MS environment settings.

// maybeSaveLockTime logs the lock hold and wait times when either exceeds the

// threshold defined in the SNAPD_STATE_LOCK_THRESHOLD_MS environment variable.

sergiocazzolato · 2025-01-15T23:48:15Z

this is an example of the smoke suite with threshold=150ms: https://pastebin.canonical.com/p/T3bDG25f5q/
this is an example of the main suite with threshold=150ms: https://pastebin.canonical.com/p/sZjvcGdZFD/

sergiocazzolato · 2025-01-15T23:49:06Z

@pedronis

this is an example of the smoke suite with threshold=150ms: https://pastebin.canonical.com/p/T3bDG25f5q/
this is an example of the main suite (~300 tests) with threshold=150ms: https://pastebin.canonical.com/p/sZjvcGdZFD/

github-actions · 2025-01-16T21:51:23Z

Thu Jan 23 14:49:31 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/12886246871

Failures:

pedronis · 2025-01-17T09:18:12Z

@sergiocazzolato thanks, the result look interesting/great, it seems maybe we should try the main suite run with threshold 200ms, also it would be good to have a tool that goes over the result and find repeated call stacks and present them only once with range of times instead of having them repeated many times and thenn also sort the call stacks by the one taking the longest time, probably having a first the long held and then the long waits

bboozzoo · 2025-01-17T09:19:54Z

I'll push the changes suggested by @miguelpires

bboozzoo · 2025-01-17T09:25:42Z

Updated and rebased. @sergiocazzolato please fetch before pushing any changes.

maykathm

Looks good

pedronis · 2025-01-17T10:26:37Z

@sergiocazzolato @maykathm to be clear the tool I mentioned can come later, is not for this PR

This will prevent raises.

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

…uild tag Move state lock tracing from osutil into overlord/state. Tweak the code to not export functions since there aren't any external consumers. Signed-off-by: Maciej Borzecki <[email protected]>

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

The code is already executed in spread tests. Signed-off-by: Maciej Borzecki <[email protected]>

sergiocazzolato marked this pull request as draft December 18, 2024 02:34

sergiocazzolato requested a review from pedronis December 18, 2024 02:34

sergiocazzolato force-pushed the tests-measure-state-lock branch from 0d1c64f to f46e537 Compare December 18, 2024 17:40

sergiocazzolato added the Needs Samuele review Needs a review from Samuele before it can land label Dec 19, 2024

sergiocazzolato requested a review from maykathm December 19, 2024 16:51

sergiocazzolato marked this pull request as ready for review December 19, 2024 16:51

pedronis reviewed Dec 20, 2024

View reviewed changes

pedronis requested a review from bboozzoo December 20, 2024 14:03

pedronis self-requested a review January 6, 2025 15:54

pedronis reviewed Jan 6, 2025

View reviewed changes

bboozzoo reviewed Jan 7, 2025

View reviewed changes

bboozzoo reviewed Jan 8, 2025

View reviewed changes

pedronis self-requested a review January 8, 2025 14:04

sergiocazzolato requested a review from bboozzoo January 9, 2025 19:28

maykathm reviewed Jan 10, 2025

View reviewed changes

.github/workflows/test.yaml Outdated Show resolved Hide resolved

bboozzoo force-pushed the tests-measure-state-lock branch from f4fa741 to 431e59d Compare January 10, 2025 15:59

pedronis reviewed Jan 10, 2025

View reviewed changes

bboozzoo changed the title ~~tests: measure state lock~~ overlord/state, tests: measure state lock Jan 13, 2025

bboozzoo requested review from miguelpires, pedronis and maykathm January 13, 2025 12:56

pedronis reviewed Jan 13, 2025

View reviewed changes

pedronis approved these changes Jan 14, 2025

View reviewed changes

miguelpires approved these changes Jan 15, 2025

View reviewed changes

sergiocazzolato closed this Jan 15, 2025

sergiocazzolato reopened this Jan 16, 2025

bboozzoo force-pushed the tests-measure-state-lock branch from 3e28324 to c5a0532 Compare January 17, 2025 09:25

maykathm approved these changes Jan 17, 2025

View reviewed changes

sergiocazzolato and others added 16 commits January 21, 2025 08:44

spike to measure locking time os snapd state file

c184bff

Changes required to collect the state locks for the whole run

092e768

fix shellcheck error

e8c7afe

measure the lock time after the lock is done

ecaa4b5

Save unlock times before the unlock

8efed33

This will prevent raises.

Added wait time and several improvements

9b6ee68

update unit test a minor fix on comments

cecc657

fix flock unit test

2c4bfdb

osutil: tweak env vars for state lock tracing, improve tracing

d78c20a

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

tests: update for new env varibles names

dc9bef0

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

osutil: simplify init, tweak naming and lock tracking API

fad4adc

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

overlord/state: tweak naming and match lock tracing API

c1659c1

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

overlord/state: move state lock tracing, unexport functions, change b…

2057242

…uild tag Move state lock tracing from osutil into overlord/state. Tweak the code to not export functions since there aren't any external consumers. Signed-off-by: Maciej Borzecki <[email protected]>

many: update state lock trace build tag use locations

a652c02

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

overlord/state: take timestamp before unlocking

41e2f8a

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

github: do not collect coverage for statelocktrace tag

d13f58d

The code is already executed in spread tests. Signed-off-by: Maciej Borzecki <[email protected]>

sergiocazzolato force-pushed the tests-measure-state-lock branch from c5a0532 to d13f58d Compare January 21, 2025 11:45

sergiocazzolato merged commit 953a5d8 into canonical:master Jan 24, 2025
65 of 66 checks passed

sergiocazzolato deleted the tests-measure-state-lock branch January 24, 2025 14:07

	func MaybeSaveLockTime(lockStart int64) {
	func MaybeSaveLockTime(lockWait, lockStart, lockEnd int64) {

	func traceCallers(description string) {
	func traceCallers(description string) error {

	* Copyright (C) 2021 Canonical Ltd
	* Copyright (C) 2025 Canonical Ltd

	// avoid 3 first callers on the stack: runtime.Callers(), this function and the parent
	// ignore the first 3 callers on the stack: runtime.Callers(), this function and the parent

	* Copyright (C) 2021 Canonical Ltd
	* Copyright (C) 2024 Canonical Ltd

		// maybeSaveLockTime allows to save lock times when this overpass the threshold
		// defined by through the SNAPD_STATE_LOCK_THRESHOLD_MS environment settings.

overlord/state, tests: measure state lock #14874

overlord/state, tests: measure state lock #14874

Conversation

sergiocazzolato commented Dec 18, 2024 • edited Loading

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

sergiocazzolato commented Jan 6, 2025 • edited Loading

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

sergiocazzolato commented Jan 6, 2025 • edited Loading

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

codecov bot commented Jan 8, 2025 • edited Loading

Codecov Report

sergiocazzolato commented Jan 8, 2025 • edited Loading

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

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

miguelpires 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

sergiocazzolato commented Jan 15, 2025

sergiocazzolato commented Jan 15, 2025

github-actions bot commented Jan 16, 2025 • edited Loading

Failures:

pedronis commented Jan 17, 2025 • edited Loading

bboozzoo commented Jan 17, 2025

bboozzoo commented Jan 17, 2025

maykathm left a comment

Choose a reason for hiding this comment

pedronis commented Jan 17, 2025

sergiocazzolato commented Dec 18, 2024 •

edited

Loading

sergiocazzolato commented Jan 6, 2025 •

edited

Loading

sergiocazzolato commented Jan 6, 2025 •

edited

Loading

codecov bot commented Jan 8, 2025 •

edited

Loading

sergiocazzolato commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

pedronis commented Jan 17, 2025 •

edited

Loading