This is an archive of the discontinued Mercurial Phabricator instance.

sharesafe: introduce functionality to automatically upgrade shares
ClosedPublic

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

Details

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
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.

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

(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:
    …

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.

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.