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

Replace maintenance with 'git maintenance run' #398

Merged
merged 13 commits into from Oct 8, 2020

Conversation

derrickstolee
Copy link
Contributor

@derrickstolee derrickstolee commented Jul 7, 2020

Scalar inherited its background maintenance model from VFS for Git, which does incremental maintenance by running some Git commands. Some of these steps have been implemented in Git proper, minus a few steps (such as verifying the file after writing it). We are working to move towards using the git maintenance builtin for all maintenance.

This PR is a step in that direction, but is in fact a half-measure.

  1. Some steps, such as CommitGraphStep and PackfileMaintenanceStep still verify the written files in the C# layer since Git doesn't do that (yet).
  2. Only Linux depends on git maintenance start to actually integrate with cron instead of using the Scalar.Service project.
  3. The macOS platform could use git maintenance start instead of Scalar.Service (as in [WIP] [DO NOT MERGE] Drop Scalar.Service from macOS #416), but we keep it around to minimize the risk in this release.

There was a late change from earlier WIP versions: we need to support Git versions that are not the latest .vfs. release! This adds a GitFeatureFlags.MaintenanceBuiltin flag that is checked only if we are in version v2.28.0.vfs.1.0 or v2.29.0.vfs.0.0 or later. When this builtin is fully integrated into later versions of Git (v2.30.0 at minimum) then we can expand the feature flag. At that point, we will drop the parallel implementation in the C# layer that calls the commit-graph and multi-pack-index builtins directly.

This update to the Git version also takes a lot of dependencies in at once, but all that are available in v2.29.0. The most troublesome is git/git@1eb22c7 which changes the behavior of git multi-pack-index repack --batch-size=<size> to repack frequently. Combining this with the fact that we no longer specify a batch size through git maintenance run --task=incremental-repack, some functional tests needed to change to accommodate this.

Finally, we remove a lot of tests that check that the PackfileMaintenanceStep clears up bad prefetch pack data. This was necessary in VFS for Git to clean up bad data from a previous version, but is less necessary now. We will want to be aware of anyone accumulating too many temp files (from early-exiting Git processes) to see if git gvfs-helper needs to take some of these features.

The commits are laid out in this sequence.

Update the version of Git:

  • Update Git to include maintenance builtin

Update some config schedules (only matters on Linux and future Scalar versions) and update GitFeatureFlags:

  • ConfigStep: prepare maintenance config for background runs
  • GitFeatureFlags: Maintenance builtin

Replace existing Git calls with git maintenance run --task=<task> when possible.

  • GitProcess: enable 'git maintenance run --task={task}'
  • CommitGraphStep: use the maintenance builtin, when available
  • PackfileMaintenanceStep: use incremental-repack task
  • LooseObjectsStep: use maintenance builtin
  • FetchStep: always use Git to fetch data
  • FetchStep: use maintenance builtin

On Linux, launch background maintenance with git maintenance start:

  • ConfigStep: launch background maintenance through Git

Fix the functional tests as necessary, due to new behavior:

  • FetchStep: remove tests that no longer apply
  • FunctionalTests: adapt tests to new maintenance behavior

@derrickstolee

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@derrickstolee
Copy link
Contributor Author

/azp run microsoft.scalar

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@derrickstolee derrickstolee force-pushed the maintenance branch 3 times, most recently from 0eb91c1 to e1709a9 Compare August 10, 2020 18:51
@derrickstolee derrickstolee mentioned this pull request Aug 10, 2020
derrickstolee added a commit that referenced this pull request Aug 12, 2020
These cleanups were discovered while working on #398:

1. Update Watchman download to get newer version. (Link was broken by recent change in release process.)
2. Use a better error when downloading the tip commit fails.
3. Use multi-valued config for `log.excludeDecoration` since it will need multiple values.
4. Do a better job interacting with `GIT_OBJECT_DIRECTORY` in `GitProcess`.
5. Stop exiting on an unknown exception during `GitMaintenanceStep`.
@derrickstolee
Copy link
Contributor Author

Note: this PR needs some decision points:

Do we only run maintenance if their Git version supports it? Or, do we keep the C# code around for a version? I think we should say "use our Git if you want maintenance."

@derrickstolee derrickstolee force-pushed the maintenance branch 5 times, most recently from d54887b to 1222e49 Compare October 7, 2020 18:46
@derrickstolee derrickstolee changed the title [WIP] Replace maintenance with 'git maintenance run' Replace maintenance with 'git maintenance run' Oct 7, 2020
@derrickstolee derrickstolee marked this pull request as ready for review October 7, 2020 19:17
The maintenance builtin is available in Git 2.28.0.vfs.1.0 or higher
(that is, the 'vfs' will be required until at least 2.30.0).

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
 * Disable the 'gc' task, enable the other tasks.

 * Disable all auto-checks for the other tasks.

 * Set proper schedules for these tasks.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Add unit tests to verify this works with the expected compatibility
versions.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
This adds extra dependencies on gvfs-helper.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
On Linux, when using Git v2.28.0.vfs.1.0 or later (and this will be
adjusted with the v2.29.0.vfs.0.0 release), we have access to the
cron-enabled background maintenance in Git. This enables background
maintenance for Scalar on Linux.

While we _could_ use this integration on macOS, we will still ship with
Scalar.Service on that platform for now to minimize possible issues.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
We removed all of the complicated logic from the C# FetchStep and
instead rely on gvfs-helper to do all of this work for us. Note that
these extra-careful steps are mostly due to previously-bad
implementations of these steps in an earlier version of VFS for Git.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
The 'git maintenance run' version of these steps behave slightly
differently and are tested in the Git codebase.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Copy link
Member

@mjcheetham mjcheetham left a comment

Choose a reason for hiding this comment

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

LGTM, just one question about git maintenance

Scalar.Common/Git/GitProcess.cs Outdated Show resolved Hide resolved
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee derrickstolee merged commit ec96219 into microsoft:main Oct 8, 2020
derrickstolee added a commit that referenced this pull request Oct 16, 2020
The ConfigStep makes a decision about calling 'git maintenance start'
depending on the platform (Linux only for now) and the Git version.
However, the code for computing the feature flags is in ScalarVerb and
needs to be injected by passing it through the ConfigStep constructor.
This was missed in the earlier work that inserted this into other
maintenance steps.

This fixes a feature that should have been introduced in #398.
derrickstolee added a commit that referenced this pull request Mar 31, 2021
…1.0+

Resolves #501.

The feature flag was originally introduced in cd0e476 (#398) but at that time we didn't know if the core Git client was ready, so we didn't enable it for versions of Git that did not include the `.vfs.` platform.

Fix this since background maintenance is now available on all platforms on v2.31.0 and later.
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.

None yet

2 participants