929093 - vcs-sync: 2013.10.18 github hiccup

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Aki Sasaki (not active)]

09:44:57    ERROR -   ! [remote rejected] fx-team -> fx-team (pre-receive hook declined)
09:44:57    ERROR -  error: failed to push some refs to '[e-mail]:mozilla/integration-gecko-dev.git'
09:44:57    ERROR - Return code: 256
09:44:57    ERROR - fx-team: Can't push /opt/vcs2vcs/build/conversion/beagle to github-beagle!
09:44:57    ERROR - Unable to push fx-team. failed.
09:44:57    FATAL - Unable to push these repos:
09:44:57    FATAL - fx-team: Can't push /opt/vcs2vcs/build/conversion/beagle to github-beagle!
09:44:57    FATAL - 
09:44:57    FATAL - Running post_fatal callback...

We should probably detect this line:
09:44:57     INFO -  remote: /data/github/current/bin/safe-ruby: line
20: /data/ruby/bin/ruby-jemalloc: No such file or directory

and we should mark a repo as failed, so we try again next time even if |hg incoming| says no change:

[10:06]    <hwine>    aki|backoct21: how soon does it retry? I.e. is no
2nd email within 10 minutes mean self cleared?
[10:07]    <aki|backoct21>    hwine: nope.
http://escapewindow.dreamwidth.org/240333.html
[10:07]    <aki|backoct21>    if it's check_incoming then it doesn't try
again until there's a change in fx-team
[10:08]    <aki|backoct21>    i made this local change to the config to
force fx-team to convert every minute https://pastebin.mozilla.org/3286224
[10:08]    <aki|backoct21>    (and not spam the team). once it finished
successfully i backed that out
[10:09]    <aki|backoct21>    ideally i mark a repo as bad, try again
next time if it fails anywhere, and we ignore the check_incoming if
that's true
[10:09]    <aki|backoct21>    that would probably go in the repo_update.json

Comment 1

[Aki Sasaki (not active)]

Attachment: denydeletes

This patch:

* removes the unused "bare_checkout" flag
* inits the test_push repos with denyDeletes
* nukes failed clones, so future clones have a chance to succeed
* allows for remote configs to be defined in-place, rather than in a separate dict.  This will be important when we generate conversion_repos dicts dynamically for the build/* repos.
* and most importantly, catches us up so my dev branch isn't too far ahead of what's live.

Comment 2

[Hal Wine [:hwine] use NI!]

Comment on attachment 824487 [details] [diff] [review]
denydeletes

Review of attachment 824487 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm -- assuming you're going to manually update all existing repos with the receive.denyDeletes configuration option.

I made a few non-blocking comments -- they're just feedback.

::: scripts/vcs-sync/vcs_sync.py
@@ +135,5 @@
>              error_level=FATAL,
>              error_message="Can't set up %s!" % path
>          )
> +        if deny_deletes:
> +            return self.run_command(

style preference - I'd rather see this as "status = ...", and "return status" at the end of the function outside the conditional. Easier to correctly modify in the future, imo.

@@ +664,4 @@
>                  if not os.path.exists(target_dest):
>                      self.info("Creating local target repo %s." % target_dest)
>                      if target_config.get("vcs", "git") == "git":
> +                        self.init_git_repo(target_dest, additional_args=['--bare', '--shared=true'],

Not related to this change, but '--shared=all' might make investigating easier/safer, as it could be done by users without write permission to the repositories.

If we had unit tests for this, it'd be great to have one to explicitly check that setting --shared=true does, in fact, also set denyNonFastForwards.

I don't find the man page wording ("by default") comforting on the connection between --share & denyNonFastForwards, and am thinking of qualifying a new git version down the road.

Comment 3

[Aki Sasaki (not active)]

Comment on attachment 824487 [details] [diff] [review]
denydeletes

http://hg.mozilla.org/build/mozharness/rev/63d722874209 with comments addressed.

Comment 4

[Aki Sasaki (not active)]

(In reply to Hal Wine [:hwine] (use needinfo) from comment #2)
> If we had unit tests for this, it'd be great to have one to explicitly check
> that setting --shared=true does, in fact, also set denyNonFastForwards.
> 
> I don't find the man page wording ("by default") comforting on the
> connection between --share & denyNonFastForwards, and am thinking of
> qualifying a new git version down the road.

Hm.
If we're worried, we can also explicitly run |git config receive.denyNonFastForwards true|.

I'm not as worried about that as you seem to be, but I have no objections to adding it... should come in the next patch review.

Comment 5

[Aki Sasaki (not active)]

Attachment: previous_repo_status

This patch:

* adds the check specified in comment 0
* adds an explicit denyNonFastForwards
* adds methods to set & query previous status of a repo in the json
* skips the |hg incoming| check if the previous status isn't True
* sets the previous status to False whenever we bail out
* add_summary()'s if the previous_status wasn't True, but we pushed successfully
** this will send non-empty successful email, so we can start sending successful, non-empty emails to release+vcs2vcs

Once this patch lands, we should be able to resolve this bug.

Comment 6

[Hal Wine [:hwine] use NI!]

Comment on attachment 826214 [details] [diff] [review]
previous_repo_status

Review of attachment 826214 [details] [diff] [review]:
-----------------------------------------------------------------

Two comments -- the first isn't blocking if my analysis was correct (otherwise needs a comment).

The second is non blocking.

r+ if the first doesn't block.

::: scripts/vcs-sync/vcs_sync.py
@@ +293,5 @@
>  
> +    def _query_repo_previous_status(self, repo_name, repo_map=None):
> +        if repo_map is None:
> +            repo_map = self._read_repo_update_json()
> +        return repo_map.get('repos', {}).get(repo_name, {}).get('previous_push_successful')

Since the final 'get()' doesn't have a default value, it will return None if the value has not yet been set. While the code effectively treats that as 'False', both line 338 and line 865 make a distinction between 'None' and 'False', apparently only for the purpose of a slightly different log message the very first time a repo is successfully pushed.

Those two conditionals and 2 unique logging messages could be removed by changing this final get() to have a default of False. And, if the first time event is critical to log, log that here.

If I'm missing something, then I think a comment is needed. :)

@@ +383,5 @@
>              if retry:
>                  return self._update_stage_repo(
>                      repo_config, retry=False, clobber=True)
>              else:
> +                self._update_repo_previous_status(repo_name, write_update=True)

nit - it's not obvious that the status being updated is "successful_flag=False" from the method name. It'd be easier for a maintainer if the call was explicit about setting that value here, instead of requiring they read the method signature.

Ditto for several places below.

Comment 7

[Aki Sasaki (not active)]

Thanks!

(In reply to Hal Wine [:hwine] (use needinfo) from comment #6)
> ::: scripts/vcs-sync/vcs_sync.py
> @@ +293,5 @@
> >  
> > +    def _query_repo_previous_status(self, repo_name, repo_map=None):
> > +        if repo_map is None:
> > +            repo_map = self._read_repo_update_json()
> > +        return repo_map.get('repos', {}).get(repo_name, {}).get('previous_push_successful')
> 
> Since the final 'get()' doesn't have a default value, it will return None if
> the value has not yet been set. While the code effectively treats that as
> 'False', both line 338 and line 865 make a distinction between 'None' and
> 'False', apparently only for the purpose of a slightly different log message
> the very first time a repo is successfully pushed.

Correct.

> Those two conditionals and 2 unique logging messages could be removed by
> changing this final get() to have a default of False. And, if the first time
> event is critical to log, log that here.
> 
> If I'm missing something, then I think a comment is needed. :)

Yeah, except it's two different sets of log messages, and an add_summary() in one case and an info() in the other.  I started refactoring,

def _query_repo_previous_status(self, repo_name, repo_map=None, log_level=INFO, summary=False, log=False, new_message=None, previous_fail_message=None):

and it seemed like a little too much to get rid of two similar if/else blocks.

Added some more docstrings.

> @@ +383,5 @@
> >              if retry:
> >                  return self._update_stage_repo(
> >                      repo_config, retry=False, clobber=True)
> >              else:
> > +                self._update_repo_previous_status(repo_name, write_update=True)
> 
> nit - it's not obvious that the status being updated is
> "successful_flag=False" from the method name. It'd be easier for a
> maintainer if the call was explicit about setting that value here, instead
> of requiring they read the method signature.
> 
> Ditto for several places below.

Done.

Comment 8

[Aki Sasaki (not active)]

Comment on attachment 826214 [details] [diff] [review]
previous_repo_status

https://hg.mozilla.org/build/mozharness/rev/5dcffed15b28

Comment 9

[Aki Sasaki (not active)]

Attachment: more_release_email

(In reply to Aki Sasaki [:aki] from comment #5)
> Once this patch lands, we should be able to resolve this bug.

I lied, I want to land this two-line email config patch before resolving.

This will get more useful email to release+vcs2vcs, hopefully (can tune more, but I think this is a good level to start with).

Comment 10

[Hal Wine [:hwine] use NI!]

Comment on attachment 827110 [details] [diff] [review]
more_release_email

Review of attachment 827110 [details] [diff] [review]:
-----------------------------------------------------------------

love to see it getting liver ;)

Comment 11

[John Hopkins (:jhopkins)]

in production

Comment 12

[Aki Sasaki (not active)]

Comment on attachment 827110 [details] [diff] [review]
more_release_email

http://hg.mozilla.org/build/mozharness/rev/52bc2a8490cc
Merged to production.