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
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

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–589

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

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

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
280 ↗(On Diff #24624)

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)

294–298 ↗(On Diff #24624)

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
284–286 ↗(On Diff #24643)

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 ?

289–290 ↗(On Diff #24643)

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 24612

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'):
storerequires = _readrequires(storevfs, False)
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 ?
requirements = scmutil.upgrade_share_to_safe(
hgvfs, requirements, storerequires
)
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…
ui.warn(_(b'repository upgraded to use share-safe mode\n'))
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/scmutil.py

def writerequires(opener, requirements): def writerequires(opener, requirements):
with opener(b'requires', b'w', atomictemp=True) as fp: with opener(b'requires', b'w', atomictemp=True) as fp:
for r in sorted(requirements): for r in sorted(requirements):
fp.write(b"%s\n" % r) fp.write(b"%s\n" % r)
def upgrade_share_to_safe(hgvfs, current_requirements, store_requirements):
"""Upgrades a share to use share-safe mechanism
Returns the set of new requirements for the repository
"""
# after upgrade, store requires will be shared, so lets find
# 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
diffrequires.add(requirementsmod.SHARESAFE_REQUIREMENT)
writerequires(hgvfs, diffrequires)
current_requirements.add(requirementsmod.SHARESAFE_REQUIREMENT)
return current_requirements
marmouteUnsubmitted
Done

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.

marmoute: Lets move this to one of the upgrade module. scmutils has enough content in my opinion. It…
pulkitAuthorUnsubmitted
Done

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.

pulkit: > It looks like this function is doing both "writing to the repo" and returning data. I am >…
class filecachesubentry(object): class filecachesubentry(object):
def __init__(self, path, stat): def __init__(self, path, stat):
self.path = path self.path = path
self.cachestat = None self.cachestat = None
self._cacheable = None self._cacheable = None
if stat: if stat:
self.cachestat = filecachesubentry.stat(self.path) self.cachestat = filecachesubentry.stat(self.path)

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
$ 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