Skip to content

BugFix: prevent sysctl reset from re-caching modified parameter values - #4857

Open
Austin Aviv (austinaviv) wants to merge 2 commits into
microsoft:mainfrom
austinaviv:fix-sysctl-reset
Open

Austin Aviv (austinaviv) wants to merge 2 commits into
microsoft:mainfrom
austinaviv:fix-sysctl-reset

Conversation

@austinaviv

@austinaviv Austin Aviv (austinaviv) commented Nov 3, 2026 •

Copy link
Copy Markdown

Description

In Sysctl.reset(), variables were popped from _original_values and restored via self.write(key, original_value). Because the key was already popped, write() encountered a KeyError and called self.get() to query the system value before writing. Since the system had not yet been restored, _original_values[key] was repopulated with the test/modified value rather than being cleared.

This caused _original_values to retain modified values after
eset(), making repeated
eset() calls write the test values back to the system and corrupting baseline tracking for subsequent tests on the node.

This PR introduces an internal _set() helper for executing sysctl -w commands. write() delegates to _set() while caching the original value if not present, and
eset() uses _set() to restore original values without re-populating _original_values.

Related Issue

Fixes #4856

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Refactoring
  • Documentation update

Checklist

  • Description is filled in above
  • No credentials, secrets, or internal details are included
  • Peer review requested (if not, add required peer reviewers after raising PR)
  • Tests executed and results posted below

Test Validation

Key Test Cases:
perf_sockperf_latency_tcp_sriov_busy_poll

Impacted LISA Features:
Sysctl

Tested Azure Marketplace Images:
N/A

Test Results

Image VM Size Result
local validation N/A PASSED

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues are reported for the sysctl caching and reset changes.

No open issues 🎉

What changed in this PR

Fixes #4856 by separating sysctl writes from baseline caching, so resetting node settings clears tracked values without caching modified values again.

Changes:

  • Adds _set() for writes that do not update the baseline cache.
  • Updates reset() to restore original values directly.
  • Adds unit tests for baseline caching, cache cleanup, repeated resets, and BSD polling cleanup.

Suggested integration validation (not run):

Key Test Cases:
perf_sockperf_latency_tcp_sriov_busy_poll

Impacted LISA Features:
None directly; Sysctl is a tool rather than a feature class.

Tested Azure Marketplace Images:

  • canonical 0001-com-ubuntu-server-jammy 22_04-lts-gen2 latest
File Description
selftests/​test_sysctl.py Adds regression tests for caching and reset behavior.
lisa/​tools/​sysctl.py Separates low-level writes from caching to make resets idempotent.

Review effort: Balanced

Comment thread selftests/test_sysctl.py Resolved
Copilot AI balanced review requested due to automatic review settings November 4, 2026 02:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The four claimed regression tests are absent from the PR, so the tests must be included or the description and validation results corrected to match what was delivered.

1 open issue

Review effort: Balanced

Comment thread lisa/tools/sysctl.py Resolved

This branch has not been deployed

No deployments
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.

Sysctl.reset re-caches modified parameter value instead of clearing original values

3 participants