hgtool should not blow away shared store if auto pooled storage is used

REOPENED
Assigned to

Status

Release Engineering
General Automation
REOPENED
2 years ago
2 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
The changes in bug 1270317 and the current implementation of hgtool will conflict with each other. Each tool will blow the others' checkout away due to each having its own opinion of how share should work.

I'll write more details in the commit message once I have a patch coded up.
(Assignee)

Comment 1

2 years ago
Created attachment 8749870 [details]
MozReview Request: Bug 1270951 - Don't verify "default" path matches; r?jlund

This verification was added in 437cce872cb7. We shouldn't need to verify
the "default" path matches expectations because the value of the
"default" path shouldn't matter: any command not using an explicit
URL to define which repo to interact with is buggy because it relies on
how a repo came into existence.

Distributed version control systems may pull changesets from various
sources. As long as the data is available, it shouldn't matter where
it came from. So paths shouldn't matter: only data does.

The test for recovering from a missing .hg/hgrc has been removed because
a .hg/hgrc file should not be required.

Review commit: https://reviewboard.mozilla.org/r/51203/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51203/
Attachment #8749870 - Flags: review?(jlund)
Attachment #8749871 - Flags: review?(jlund)
(Assignee)

Comment 2

2 years ago
Created attachment 8749871 [details]
MozReview Request: Bug 1270951 - Be less strict about auto pooled shared repos; r?jlund

mozharness was updated in bug 1270951 to always create and ensure auto
pooled storage. What this means is that .hg/sharedpath always points to
a path of the form "<sha1>/.hg" where <sha1> is a 40 character
hexidecimal changeset revision corresponding to the logical repo the
share is from.

What I didn't realize at the time I landed that was that this conflicts
with hgtool.

hgtool has code for verifying that .hg/sharedpath points to the base name
of the repository we're cloning from. In reality, this will likely never
match a 40 character hexidecimal changeset and hgtool will blow away the
working directory. This means that mozharness and hgtool will each step
on each other's toes, deleting working directories the other created.
This wouldn't be a problem if mozharness were the only game in town.
However, the new mozharness isn't deployed everywhere yet, so hgtool
will still run and blow away mozharness's auto pooled working directory.

The solution to this problem as implemented in this commit is to teach
hgtool just enough about auto pooled shared repos so it doesn't
interfere with mozharness-created repos. We add logic to look for
.hg/sharedpath pointing to a <sha1>/.hg path. If found, we assume
pooled storage and don't engage the strict share checking already
built into hgtool.

We could potentially remove the strict share checking in hgtool.
However, I don't want to do this out of an abundance of caution.
I'm not sure what all is still using hgtool and I don't want to take
a risk that changing hgtool's share behavior in the absence of
mozharness will lead to catastrophe.

Review commit: https://reviewboard.mozilla.org/r/51205/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51205/
(Assignee)

Comment 3

2 years ago
Comment on attachment 8749871 [details]
MozReview Request: Bug 1270951 - Be less strict about auto pooled shared repos; r?jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51205/diff/1-2/

Comment 4

2 years ago
(In reply to Gregory Szorc [:gps] from comment #0)
> The changes in bug 1270317 and the current implementation of hgtool will
> conflict with each other. Each tool will blow the others' checkout away due
> to each having its own opinion of how share should work.
> 
> I'll write more details in the commit message once I have a patch coded up.

oh! good point/catch

Comment 5

2 years ago
Comment on attachment 8749871 [details]
MozReview Request: Bug 1270951 - Be less strict about auto pooled shared repos; r?jlund

https://reviewboard.mozilla.org/r/51205/#review47911

::: lib/python/util/hg.py:505
(Diff revision 2)
> +        if re.search('[a-f0-9]{40}/\.hg$', store_path.replace('\\', '/')):
> +            log.info('pooled storage detected')
> +            if not os.path.exists(store_path):
> +                log.warning('store path %s does not exist; clobbering', store_path)
> +                remove_path(dest)
> +                return mercurial(repo, dest,

this method is getting long and complex..

how long will this code be used? Is the plan to fade it out as your patches from today ride trains and used everywhere? If so, I guess that's fine. If not, I think we should refactor this method and distingush `old_share` vs `new_share` more.

e.g. the blocks below your patch refers to the `old_share` style but that isn't clear. They use var names like shareBase and dest_sharedpath vars without much clear seperation.

but anyway, iiuc we:

1) check if the checkout dest was created via new native hg share (e.g. Mozharness)

2) if it was and is valid, we accept that and possibly purge and update||pull in a similar (identical) way to: http://hg.mozilla.org/build/tools/file/8c00f2d02981/lib/python/util/hg.py#l527

3) if it was and is _not_ valid, delete repo dest and clone a new repo using old style that tools hg.py knows about

4) if it wasn't, carry on as normal

is that accurate?
Attachment #8749871 - Flags: review?(jlund)

Updated

2 years ago
Attachment #8749870 - Flags: review?(jlund) → review+

Comment 6

2 years ago
Comment on attachment 8749870 [details]
MozReview Request: Bug 1270951 - Don't verify "default" path matches; r?jlund

https://reviewboard.mozilla.org/r/51203/#review47913
(Assignee)

Comment 7

2 years ago
Created attachment 8753517 [details]
MozReview Request: Bug 1270951 - Remove allowUnsharedLocalClones argument; r?jlund

AFAICT this argument is not being used. Nuke it.

Review commit: https://reviewboard.mozilla.org/r/53320/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53320/
Attachment #8753517 - Flags: review?(jlund)
Attachment #8753518 - Flags: review?(jlund)
Attachment #8753520 - Flags: review?(jlund)
(Assignee)

Comment 8

2 years ago
Created attachment 8753518 [details]
MozReview Request: Bug 1270951 - Remove Python 2.6, hg 2.6 from tox environments, add hg 3.7; r?jlund

I'm pretty sure we don't run Python 2.6 anywhere in automation any more.
We also have Mercurial 3.7.3 deployed nearly everywhere. So remove the
outdated environments and replace with Python 2.7 running Mercurial 3.7.

Review commit: https://reviewboard.mozilla.org/r/53322/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53322/
(Assignee)

Comment 9

2 years ago
Created attachment 8753519 [details]
MozReview Request: Bug 1270951 - Add robustcheckout.py extension; r?jlund

Copied from XXX

Review commit: https://reviewboard.mozilla.org/r/53324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53324/
(Assignee)

Comment 10

2 years ago
Created attachment 8753520 [details]
MozReview Request: Bug 1270951 - Use robustcheckout when possible; r?jlund

The robustcheckout extension/command provides most of the functionality
of hgtool's mercurial() function.

This commit integrates the robustcheckout extension/command into
hgtool for scenarios where it can be used. For all other scenarios
(no share directory, no update, no revision), we fall back to the
existing code.

In the ideal world, we'd only use robustcheckout. However, this
requires changing a number of consumers of hgtool and making invasive
changes to the tests. I started down this road, but it proved to be
a bit too much work. This commit strikes a compromise for quick wins
(using robustcheckout) without a massive refactor.

A test for clone by rev with share has been removed because the test
uses the new code path and the behavior with shared pooled storage
has changed. Rather than rewriting the test, I'm removing it: the
tests for the robustcheckout extension cover this use case.

Review commit: https://reviewboard.mozilla.org/r/53326/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53326/
(Assignee)

Updated

2 years ago
Attachment #8749870 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8749871 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Depends on: 1197879
(Assignee)

Comment 11

2 years ago
Derp.
Depends on: 1273305
No longer depends on: 1197879
(Assignee)

Comment 12

2 years ago
Comment on attachment 8753519 [details]
MozReview Request: Bug 1270951 - Add robustcheckout.py extension; r?jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53324/diff/1-2/
Attachment #8753519 - Attachment description: MozReview Request: Bug 1270951 - Add robustcheckout.py extension → MozReview Request: Bug 1270951 - Add robustcheckout.py extension; r?jlund
Attachment #8753519 - Flags: review?(jlund)
(Assignee)

Comment 13

2 years ago
Comment on attachment 8753520 [details]
MozReview Request: Bug 1270951 - Use robustcheckout when possible; r?jlund

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53326/diff/1-2/
Comment on attachment 8753517 [details]
MozReview Request: Bug 1270951 - Remove allowUnsharedLocalClones argument; r?jlund

https://reviewboard.mozilla.org/r/53320/#review50416
Attachment #8753517 - Flags: review?(jlund) → review+
Comment on attachment 8753518 [details]
MozReview Request: Bug 1270951 - Remove Python 2.6, hg 2.6 from tox environments, add hg 3.7; r?jlund

https://reviewboard.mozilla.org/r/53322/#review50420
Attachment #8753518 - Flags: review?(jlund) → review+
Comment on attachment 8753519 [details]
MozReview Request: Bug 1270951 - Add robustcheckout.py extension; r?jlund

https://reviewboard.mozilla.org/r/53324/#review50434

stamping purely as support to adding this extension and its location, not a review of the extention itself as it has been reviewed elsewhere.
Attachment #8753519 - Flags: review?(jlund) → review+
Comment on attachment 8753520 [details]
MozReview Request: Bug 1270951 - Use robustcheckout when possible; r?jlund

https://reviewboard.mozilla.org/r/53326/#review50440

see high level question in https://bugzilla.mozilla.org/show_bug.cgi?id=1270317#c123 to make sure I understand your impl strategy/scope

::: lib/python/util/hg.py:506
(Diff revision 2)
> +            cmd.append('--purge')
> +
> +        # We only take the first mirror because that's all robustcheckout can
> +        # take.
> +        if mirrors:
> +            cmd.extend(['--upstream', mirrors[0]])

hmm, I wonder if limiting mirrors will cause issues?

::: lib/python/util/hg.py:517
(Diff revision 2)
> +                if m:
> +                    return m.group(1)
> +
> +            raise HgUtilError('unable to find updated to revision')
> +        except subprocess.CalledProcessError as e:
> +            log.info('OUTPUT: %s' % e.output)

s/info/warning or error?
Attachment #8753520 - Flags: review?(jlund)
https://reviewboard.mozilla.org/r/53326/#review50440

it was mentioned there that we will be landing this after mozharness equivalent patch and also adressing edge cases in the future or at least getting bugs on file
Comment on attachment 8753520 [details]
MozReview Request: Bug 1270951 - Use robustcheckout when possible; r?jlund

https://reviewboard.mozilla.org/r/53326/#review50454
Attachment #8753520 - Flags: review+
(Assignee)

Comment 20

2 years ago
https://reviewboard.mozilla.org/r/53326/#review50440

> hmm, I wonder if limiting mirrors will cause issues?

I don't think it will. According to catlee in another bug, we don't use mirrors any more. So I'm hoping this doesn't break anything. I kept the code to be on the safe side.
(Assignee)

Comment 22

2 years ago
I just landed this. Much like bug 1270317, I expect random fallout from this. Hopefully nothing severe enough to warrant a backout. But if a backout is needed, we only need to back out the last commit - https://hg.mozilla.org/build/tools/rev/9fad28d27d67d25a48e2fa1c87177de96c1f2a33.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Comment 24

2 years ago
Backed out the last commit due to failures like http://archive.mozilla.org/pub/spidermonkey/try-builds/jolesen@mozilla.com-e4d3a9f3b8c1ccaab4d9fd8ae8df287adcf7cb62/try-linux64/try_linux64_spidermonkey-warnaserr-bm78-try1-build647.txt.gz.

The fix here is to ensure a mirror is defined for Try. Otherwise we try to pull all heads from the Try repo, which causes failure.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

2 years ago
Blocks: 1286336
You need to log in before you can comment on or make changes to this bug.