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

fix(snapshots): Fixup for #3624. Shadow copy seems to extend time for snapshots. #3625

Merged
merged 11 commits into from
Feb 7, 2024

Conversation

aaron-kasten
Copy link
Collaborator

PR #3543 seems to put IO resource pressure on the system. Enough to cause TestServerScheduling to fail due to snapshot latencies - which, in turn, causes Windows tests to fail as a flake.

This PR embodies the fix suggested by @jkowalski.

This fix also fixes the SIGFAULTS caused by use of the -race flag, which points to a possible problem in dependent library.

Copy link

codecov bot commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b057b0b) 75.86% compared to head (3810e9f) 75.84%.

❗ Current head 3810e9f differs from pull request most recent head 61b4232. Consider uploading reports for the commit 61b4232 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3625      +/-   ##
==========================================
- Coverage   75.86%   75.84%   -0.03%     
==========================================
  Files         470      470              
  Lines       37301    37301              
==========================================
- Hits        28300    28291       -9     
- Misses       7073     7079       +6     
- Partials     1928     1931       +3     

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

@julio-lopez julio-lopez removed their assignment Feb 6, 2024
@Shrekster
Copy link
Collaborator

Shrekster commented Feb 6, 2024

@aaron-kasten , how about keeping the test enabled with a simple change of enabling the VSS policy just for the test and keeping it disabled by default otherwise as you already did:

diff --git a/cli/command_policy_set_os_snapshot_test.go b/cli/command_policy_set_os_snapshot_test.go
index 6165a09d..ac8e86f7 100644
--- a/cli/command_policy_set_os_snapshot_test.go
+++ b/cli/command_policy_set_os_snapshot_test.go
@@ -10,15 +10,19 @@
 )

 func TestSetOSSnapshotPolicy(t *testing.T) {
-       t.Skip("See ticket https://github.com/kopia/kopia/issues/3624")
        e := testenv.NewCLITest(t, testenv.RepoFormatNotImportant, testenv.NewInProcRunner(t))
-
        defer e.RunAndExpectSuccess(t, "repo", "disconnect")

        e.RunAndExpectSuccess(t, "repo", "create", "filesystem", "--path", e.RepoDir)

        lines := e.RunAndExpectSuccess(t, "policy", "show", "--global")
        lines = compressSpaces(lines)
+       require.Contains(t, lines, " Volume Shadow Copy: never (defined for this target)")
+
+       e.RunAndExpectSuccess(t, "policy", "set", "--global", "--enable-volume-shadow-copy=when-available")
+
+       lines = e.RunAndExpectSuccess(t, "policy", "show", "--global")
+       lines = compressSpaces(lines)
        require.Contains(t, lines, " Volume Shadow Copy: when-available (defined for this target)")

        // make some directory we'll be setting policy on

@aaron-kasten
Copy link
Collaborator Author

@aaron-kasten , how about keeping the test enabled with a simple change of enabling the VSS policy just for the test and keeping it disabled by default otherwise as you already did:

diff --git a/cli/command_policy_set_os_snapshot_test.go b/cli/command_policy_set_os_snapshot_test.go
index 6165a09d..ac8e86f7 100644
--- a/cli/command_policy_set_os_snapshot_test.go
+++ b/cli/command_policy_set_os_snapshot_test.go
@@ -10,15 +10,19 @@
 )

 func TestSetOSSnapshotPolicy(t *testing.T) {
-       t.Skip("See ticket https://github.com/kopia/kopia/issues/3624")
        e := testenv.NewCLITest(t, testenv.RepoFormatNotImportant, testenv.NewInProcRunner(t))
-
        defer e.RunAndExpectSuccess(t, "repo", "disconnect")

        e.RunAndExpectSuccess(t, "repo", "create", "filesystem", "--path", e.RepoDir)

        lines := e.RunAndExpectSuccess(t, "policy", "show", "--global")
        lines = compressSpaces(lines)
+       require.Contains(t, lines, " Volume Shadow Copy: never (defined for this target)")
+
+       e.RunAndExpectSuccess(t, "policy", "set", "--global", "--enable-volume-shadow-copy=when-available")
+
+       lines = e.RunAndExpectSuccess(t, "policy", "show", "--global")
+       lines = compressSpaces(lines)
        require.Contains(t, lines, " Volume Shadow Copy: when-available (defined for this target)")

        // make some directory we'll be setting policy on

@Shrekster that looks great. As long as this PR's test passes and other PRs are not blocked. I'm not so versed in VSS as to be able to make a judgement on the quality of the fix beyond that.

Copy link
Collaborator

@Shrekster Shrekster left a comment

Choose a reason for hiding this comment

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

LGTM; merge as tests pass

@Shrekster Shrekster enabled auto-merge (squash) February 7, 2024 00:16
@Shrekster Shrekster merged commit cb455c6 into kopia:master Feb 7, 2024
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants