This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9679

sharesafe: introduce functionality to automatically upgrade shares
ClosedPublic

Authored by pulkit on Jan 6 2021, 8:21 AM.

Details

Reviewers
marmoute
Alphare
Group Reviewers
hg-reviewers
Commits
rHG0babe12ef35d: sharesafe: introduce functionality to automatically upgrade shares
Summary

In past few months, we have developed a share-safe mode for sharing repository
in which share source requirements and config values are shared with the shares.

To get it rolling, an important task is to get these shares automatically
upgraded. We are focusing on an installation where shares are created by scripts
and test jobs. It will be difficult to manually upgrade these and we need some
functionality to do so automatically.

This patch introduces a config option to deal with it. If all of the following
conditions are met, we upgrade the share repository automatically:

  • If the config option is enabled
  • Share source repository is share-safe enabled
  • Share is not share-safe enabled
  • Any command is run in the share

Upgrading the share is pretty easy as it involves only editing the requirements
file.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit created this revision.Jan 6 2021, 8:21 AM
baymax updated this revision to Diff 24620.Jan 6 2021, 10:14 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

marmoute requested changes to this revision.Jan 6 2021, 4:15 PM
marmoute added a subscriber: marmoute.
marmoute added inline comments.
mercurial/configitems.py
1077–1078

What's the final cofnig name you envision for this option ?

mercurial/localrepo.py
585–594

You should move most of that logic into the upgrade namespace is it small and critical enough to not be free floating inside a function.

mercurial/scmutil.py
1600–1615 ↗(On Diff #24620)

Lets move this to one of the upgrade module. scmutils has enough content in my opinion.

It looks like this function is doing both "writing to the repo" and returning data. I am not sure why we need both. Also, we probably should do this with a lock to prevent concurrent action. Or have clear docstring for why we do not need it.

tests/test-share-safe.t
512

Should the warning be an abort then ?

This revision now requires changes to proceed.Jan 6 2021, 4:15 PM
pulkit marked 3 inline comments as done.Jan 7 2021, 6:13 AM
pulkit added inline comments.
mercurial/configitems.py
1077–1078

You mean when we move the feature out of experimental? Then we can turn it into sharesafe.auto-updgrade-shares.

mercurial/scmutil.py
1600–1615 ↗(On Diff #24620)

It looks like this function is doing both "writing to the repo" and returning data. I am
not sure why we need both.

We need to return whether the writing to repo was successful or not. Actually it does not make sense in current context but with updated patch, it will.

Also we need to update the requirements at caller.

tests/test-share-safe.t
512

Nope, it's a run without the config option enabled. Since the share can still work as before, we let it work with a warning. This makes incremental upgrade possible.

Following are steps which should be performed:

  1. Upgrade the share source, shares will show a warning about source being upgraded but will still work
  2. set the config introduced in this patch globally and run any command

If we abort, we will need to instantly upgrade the shares else they will stop working.

pulkit marked 2 inline comments as done.Jan 7 2021, 6:47 AM
pulkit updated this revision to Diff 24624.
marmoute added inline comments.Jan 7 2021, 8:09 AM
mercurial/configitems.py
1077–1078

We don't have a sharesafe section (and I don't think we need one).

What about (eventually):

[format]
use-share-shafe.auto-upgrade = yes
mercurial/localrepo.py
586

I feel like this line could go in the helper too. Any reason it is isn't ?

mercurial/upgrade.py
247

My question about having it both modify -and- return something remains. Why do we need it (I am totally open to having a good reason to do so, I just want it documented)

261–265

Exception is extremely wide. Can we do something for the common case (like "lock acquisition failure ?)

tests/test-share-safe.t
502–503

Lets add a test for the case were we cannot lock the repository too. For coverage.

512

Okay ! lets add an option to silent the warning then (in another patch)

pulkit added inline comments.Jan 8 2021, 7:28 AM
mercurial/configitems.py
1077–1078

Sure, that sounds good.

baymax updated this revision to Diff 24630.Jan 8 2021, 7:56 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

pulkit updated this revision to Diff 24632.Jan 8 2021, 8:49 AM
pulkit marked 4 inline comments as done.Jan 8 2021, 8:52 AM

@marmoute incorporated all the suggestions.

pulkit updated this revision to Diff 24643.Jan 8 2021, 11:24 AM
baymax updated this revision to Diff 24693.Jan 9 2021, 8:03 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

Alphare accepted this revision.Jan 12 2021, 11:25 AM
pulkit updated this revision to Diff 24775.Jan 13 2021, 6:30 AM
pulkit added a commit: rHG0babe12ef35d: sharesafe: introduce functionality to automatically upgrade shares.Jan 13 2021, 2:39 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG0babe12ef35d: sharesafe: introduce functionality to automatically upgrade shares (authored by pulkit). · Explain Why
This revision was automatically updated to reflect the committed changes.
martinvonz added a subscriber: martinvonz.Jan 15 2021, 2:23 AM

Will this auto-upgrade feature be made to work for other upgrades too? Or is share-safe special?

marmoute added a comment.Jan 15 2021, 4:55 AM

(I just found out I had unsubmitted comments…)

mercurial/configitems.py
1077–1078

Lets add a comment with the planned final name next to the config declaration then.

mercurial/upgrade.py
248–250

This part is not very clear. Do you mean we can silently (from the code perspective) fail to upgrade ? And that depending of if this works or not, we use a different set of requirements for the share ?

253–254

What about we use try/else construct to restrict the try block to one actually trying to grab the lock ?

try:
            wlock = lockmod.trylock(ui, hgvfs, b'wlock', 0, 0)
except LockError as e:
    …
else:
    …
finally:
    …
marmoute added a comment.Jan 15 2021, 4:56 AM
In D9679#147731, @martinvonz wrote:

Will this auto-upgrade feature be made to work for other upgrades too? Or is share-safe special?

share-safe is special.

In this case we are talking about automatically upgrading/downgrading the share-repo to match the configuration in the source-repo. The source-repo still have to be manually upgraded, as for other format variant.

martinvonz added a comment.Jan 15 2021, 11:45 AM
In D9679#147736, @marmoute wrote:
In D9679#147731, @martinvonz wrote:

Will this auto-upgrade feature be made to work for other upgrades too? Or is share-safe special?

share-safe is special.
In this case we are talking about automatically upgrading/downgrading the share-repo to match the configuration in the source-repo. The source-repo still have to be manually upgraded, as for other format variant.

I see, that makes sense. I had missed that this was only about upgrading/downgrading the shares, not the source repo.

Revision Contents
Changeset List

Diff 24794

mercurial/configitems.py

) )
coreconfigitem( coreconfigitem(
b'experimental', b'experimental',
b'server.stream-narrow-clones', b'server.stream-narrow-clones',
default=False, default=False,
) )
coreconfigitem( coreconfigitem(
b'experimental', b'experimental',
b'sharesafe-auto-upgrade-shares',
default=False,
marmouteUnsubmitted
Not Done

What's the final cofnig name you envision for this option ?

marmoute: What's the final cofnig name you envision for this option ?
pulkitAuthorUnsubmitted
Done

You mean when we move the feature out of experimental? Then we can turn it into sharesafe.auto-updgrade-shares.

pulkit: You mean when we move the feature out of `experimental`? Then we can turn it into `sharesafe.
marmouteUnsubmitted
Not Done

We don't have a sharesafe section (and I don't think we need one).

What about (eventually):

[format]
use-share-shafe.auto-upgrade = yes
marmoute: We don't have a sharesafe section (and I don't think we need one). What about (eventually)…
pulkitAuthorUnsubmitted
Done

Sure, that sounds good.

pulkit: Sure, that sounds good.
marmouteUnsubmitted
Not Done

Lets add a comment with the planned final name next to the config declaration then.

marmoute: Lets add a comment with the planned final name next to the config declaration then.
)
coreconfigitem(
b'experimental',
b'single-head-per-branch', b'single-head-per-branch',
default=False, default=False,
) )
coreconfigitem( coreconfigitem(
b'experimental', b'experimental',
b'single-head-per-branch:account-closed-heads', b'single-head-per-branch:account-closed-heads',
default=False, default=False,
) )

mercurial/localrepo.py

raise error.Abort( raise error.Abort(
_(b"share source does not support exp-sharesafe requirement") _(b"share source does not support exp-sharesafe requirement")
) )
requirements |= _readrequires(storevfs, False) requirements |= _readrequires(storevfs, False)
elif shared: elif shared:
sourcerequires = _readrequires(sharedvfs, False) sourcerequires = _readrequires(sharedvfs, False)
if requirementsmod.SHARESAFE_REQUIREMENT in sourcerequires: if requirementsmod.SHARESAFE_REQUIREMENT in sourcerequires:
if ui.configbool(b'experimental', b'sharesafe-auto-upgrade-shares'):
# prevent cyclic import localrepo -> upgrade -> localrepo
marmouteUnsubmitted
Done

I feel like this line could go in the helper too. Any reason it is isn't ?

marmoute: I feel like this line could go in the helper too. Any reason it is isn't ?
from . import upgrade
upgrade.upgrade_share_to_safe(
ui,
hgvfs,
storevfs,
requirements,
)
marmouteUnsubmitted
Done

You should move most of that logic into the upgrade namespace is it small and critical enough to not be free floating inside a function.

marmoute: You should move most of that logic into the upgrade namespace is it small and critical enough…
else:
ui.warn( ui.warn(
_( _(
b'warning: source repository supports share-safe functionality.' b'warning: source repository supports share-safe functionality.'
b' Reshare to upgrade.\n' b' Reshare to upgrade.\n'
) )
) )
# The .hg/hgrc file may load extensions or contain config options # The .hg/hgrc file may load extensions or contain config options
# that influence repository construction. Attempt to load it and # that influence repository construction. Attempt to load it and
# process any new extensions that it may have pulled in. # process any new extensions that it may have pulled in.
if loadhgrc(ui, wdirvfs, hgvfs, requirements, sharedvfs): if loadhgrc(ui, wdirvfs, hgvfs, requirements, sharedvfs):
afterhgrcload(ui, wdirvfs, hgvfs, requirements) afterhgrcload(ui, wdirvfs, hgvfs, requirements)
extensions.loadall(ui) extensions.loadall(ui)
extensions.populateui(ui) extensions.populateui(ui)

mercurial/upgrade.py

# upgrade.py - functions for in place upgrade of Mercurial repository # upgrade.py - functions for in place upgrade of Mercurial repository
# #
# Copyright (c) 2016-present, Gregory Szorc # Copyright (c) 2016-present, Gregory Szorc
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
from .i18n import _ from .i18n import _
from . import ( from . import (
error, error,
hg, hg,
localrepo, localrepo,
lock as lockmod,
pycompat, pycompat,
requirements as requirementsmod,
scmutil,
) )
from .upgrade_utils import ( from .upgrade_utils import (
actions as upgrade_actions, actions as upgrade_actions,
engine as upgrade_engine, engine as upgrade_engine,
) )
from .utils import (
stringutil,
)
allformatvariant = upgrade_actions.allformatvariant allformatvariant = upgrade_actions.allformatvariant
def upgraderepo( def upgraderepo(
ui, ui,
repo, repo,
run=False, run=False,
optimize=None, optimize=None,
_( _(
b'the old repository will not be deleted; remove ' b'the old repository will not be deleted; remove '
b'it to free up disk space once the upgraded ' b'it to free up disk space once the upgraded '
b'repository is verified\n' b'repository is verified\n'
) )
) )
upgrade_op.print_post_op_messages() upgrade_op.print_post_op_messages()
def upgrade_share_to_safe(ui, hgvfs, storevfs, current_requirements):
"""Upgrades a share to use share-safe mechanism"""
wlock = None
try:
marmouteUnsubmitted
Done

My question about having it both modify -and- return something remains. Why do we need it (I am totally open to having a good reason to do so, I just want it documented)

marmoute: My question about having it both modify -and- return something remains. Why do we need it (I am…
wlock = lockmod.trylock(ui, hgvfs, b'wlock', 0, 0)
store_requirements = localrepo._readrequires(storevfs, False)
# after upgrade, store requires will be shared, so lets find
marmouteUnsubmitted
Not Done

This part is not very clear. Do you mean we can silently (from the code perspective) fail to upgrade ? And that depending of if this works or not, we use a different set of requirements for the share ?

marmoute: This part is not very clear. Do you mean we can silently (from the code perspective) fail to…
# the requirements which are not present in store and
# write them to share's .hg/requires
diffrequires = current_requirements - store_requirements
# add share-safe requirement as it will mark the share as share-safe
marmouteUnsubmitted
Not Done

What about we use try/else construct to restrict the try block to one actually trying to grab the lock ?

try:
            wlock = lockmod.trylock(ui, hgvfs, b'wlock', 0, 0)
except LockError as e:
    …
else:
    …
finally:
    …
marmoute: What about we use try/else construct to restrict the try block to one actually trying to grab…
diffrequires.add(requirementsmod.SHARESAFE_REQUIREMENT)
scmutil.writerequires(hgvfs, diffrequires)
current_requirements.add(requirementsmod.SHARESAFE_REQUIREMENT)
ui.warn(_(b'repository upgraded to use share-safe mode\n'))
except error.LockError as e:
ui.warn(
_(b'failed to upgrade share, got error: %s\n')
% stringutil.forcebytestr(e.strerror)
)
finally:
if wlock:
marmouteUnsubmitted
Done

Exception is extremely wide. Can we do something for the common case (like "lock acquisition failure ?)

marmoute: `Exception` is extremely wide. Can we do something for the common case (like "lock acquisition…
wlock.release()

tests/test-share-safe.t

Make sure existing shares still works Make sure existing shares still works
$ hg log -GT "{node}: {desc}\n" -R ../nss-share $ hg log -GT "{node}: {desc}\n" -R ../nss-share
@ f63db81e6dde1d9c78814167f77fb1fb49283f4f: added bar @ f63db81e6dde1d9c78814167f77fb1fb49283f4f: added bar
| |
o f3ba8b99bb6f897c87bbc1c07b75c6ddf43a4f77: added foo o f3ba8b99bb6f897c87bbc1c07b75c6ddf43a4f77: added foo
$ hg unshare -R ../nss-share
$ hg log -GT "{node}: {desc}\n" -R ../ss-share $ hg log -GT "{node}: {desc}\n" -R ../ss-share
abort: share source does not support exp-sharesafe requirement abort: share source does not support exp-sharesafe requirement
[255] [255]
Testing automatic upgrade of shares when config is set
$ hg debugupgraderepo -q --run --config format.exp-share-safe=True
upgrade will perform the following actions:
requirements
preserved: dotencode, fncache, generaldelta, revlogv1, sparserevlog, store
added: exp-sharesafe
processed revlogs:
- all-filelogs
- changelog
- manifest
repository upgraded to share safe mode, existing shares will still work in old non-safe mode. Re-share existing shares to use them in safe mode New shares will be created in safe mode.
$ hg debugrequirements
marmouteUnsubmitted
Done

Lets add a test for the case were we cannot lock the repository too. For coverage.

marmoute: Lets add a test for the case were we cannot lock the repository too. For coverage.
dotencode
exp-sharesafe
fncache
generaldelta
revlogv1
sparserevlog
store
$ hg log -GT "{node}: {desc}\n" -R ../nss-share
warning: source repository supports share-safe functionality. Reshare to upgrade.
marmouteUnsubmitted
Done

Should the warning be an abort then ?

marmoute: Should the warning be an abort then ?
pulkitAuthorUnsubmitted
Done

Nope, it's a run without the config option enabled. Since the share can still work as before, we let it work with a warning. This makes incremental upgrade possible.

Following are steps which should be performed:

  1. Upgrade the share source, shares will show a warning about source being upgraded but will still work
  2. set the config introduced in this patch globally and run any command

If we abort, we will need to instantly upgrade the shares else they will stop working.

pulkit: Nope, it's a run without the config option enabled. Since the share can still work as before…
marmouteUnsubmitted
Not Done

Okay ! lets add an option to silent the warning then (in another patch)

marmoute: Okay ! lets add an option to silent the warning then (in another patch)
@ f63db81e6dde1d9c78814167f77fb1fb49283f4f: added bar
|
o f3ba8b99bb6f897c87bbc1c07b75c6ddf43a4f77: added foo
Check that if lock is taken, upgrade fails but read operation are successful
$ touch ../nss-share/.hg/wlock
$ hg log -GT "{node}: {desc}\n" -R ../nss-share --config experimental.sharesafe-auto-upgrade-shares=true
failed to upgrade share, got error: Lock held
@ f63db81e6dde1d9c78814167f77fb1fb49283f4f: added bar
|
o f3ba8b99bb6f897c87bbc1c07b75c6ddf43a4f77: added foo
$ rm ../nss-share/.hg/wlock
$ hg log -GT "{node}: {desc}\n" -R ../nss-share --config experimental.sharesafe-auto-upgrade-shares=true
repository upgraded to use share-safe mode
@ f63db81e6dde1d9c78814167f77fb1fb49283f4f: added bar
|
o f3ba8b99bb6f897c87bbc1c07b75c6ddf43a4f77: added foo
Test that unshare works
$ hg unshare -R ../nss-share
$ hg log -GT "{node}: {desc}\n" -R ../nss-share
@ f63db81e6dde1d9c78814167f77fb1fb49283f4f: added bar
|
o f3ba8b99bb6f897c87bbc1c07b75c6ddf43a4f77: added foo