1121745 - MOZ_SIGN_CMD should only be managed by mozharness

Status: RESOLVED INVALID

(Release Engineering :: Applications: MozharnessCore, defect)

Comment 1

[Jordan Lund (:jlund)]

see https://bugzilla.mozilla.org/show_bug.cgi?id=1015018#c38 for details

Comment 2

[Jordan Lund (:jlund)]

Attachment: 150114_rm_moz_sign_env_if_enable_signing_is_false-mozharness.patch

untested but this should not be hit unless we configure buildbot incorrectly. It will help debug errors.

Comment 3

[[e-mail] (:bhearsum)]

Comment on attachment 8549255 [details] [diff] [review]
150114_rm_moz_sign_env_if_enable_signing_is_false-mozharness.patch

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

Is there a reason this is happening here rather than in buildbotcustom? That could be done with s/SigningScriptFactory/ScriptFactory/ or setting enableSigning=False.

Comment 4

[Jordan Lund (:jlund)]

(In reply to Ben Hearsum [:bhearsum] from comment #3)
> Comment on attachment 8549255 [details] [diff] [review]
> 150114_rm_moz_sign_env_if_enable_signing_is_false-mozharness.patch
> 
> Review of attachment 8549255 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there a reason this is happening here rather than in buildbotcustom? That
> could be done with s/SigningScriptFactory/ScriptFactory/ or setting
> enableSigning=False.

tl;dr I agress this could be improved. Part of the issue is we have so many things relying on SigningScriptFactory outside of desktop builds. My end goal is to have no logic in buildbot in terms of how a builder X should behave.

long answer: we already do rely on buildbotcustom/config. We use dep-signing and nightly-signing config items to determine whether or not we want signing (i.e. SigningScriptFactory vs ScriptFactory). However, in this case, we had a scenario where we accidentally, via buildbot-configs, configured a non signing builder as a signing builder. This left a rogue signing env var that got inherited in mozharness' os.env and mozharness didn't think it was doing signing but passed it on to 'mach build' anyway.

This bug is just a failsafe. It ensures we do not pass some arbitrary MOZ_SIGN_CMD in our os.env if it is not a signing builder. I like having mozharness decide if we are signing or not so it decouples our reliance on how we configure buildbot. My hope is to make it painless to use mozharness with buildbot, taskcluster, or locally. And to do that, I wanted mozharness to decide how a builder (e.g. osx st-an-debug) should behave. Does that somewhat answer your question. Obviously, I'm open to your preference.

Comment 5

[[e-mail] (:bhearsum)]

(In reply to Jordan Lund (:jlund) from comment #4)
> This bug is just a failsafe. It ensures we do not pass some arbitrary
> MOZ_SIGN_CMD in our os.env if it is not a signing builder. I like having
> mozharness decide if we are signing or not so it decouples our reliance on
> how we configure buildbot.

So, the thing that complicates this is that Buildbot *cannot* be decoupled from signing because of the necessary token generation. Unless we rework signing to not require that, Buildbot (and presumably task cluster) will always need to be involved with signing.

What I'm concerned about is this masking other issues. When you start deleting passed in configuration it gets a lot more difficult to see what the state is at any given time.

I really don't have any better suggestion than "make sure Buildbot does the right thing for jobs", unfortunately, so I'm not going to block you on this if you feel strongly, but this is a design flaw that we should clear up at some point.

Comment 6

[Jordan Lund (:jlund)]

> I really don't have any better suggestion than "make sure Buildbot does the
> right thing for jobs", unfortunately, so I'm not going to block you on this
> if you feel strongly, but this is a design flaw that we should clear up at
> some point.

True, I see what you mean now. The token gen can only be controlled by the master.

Maybe we should remove MOZ_SIGN_CMD from SigningScriptFactory since we already override or at least use mozharness' logic in a few places: http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/signing.py?rev=36c2a89a53bf#120

I'll leave this bug open and change the summary to do that but I'm not going to follow through as I think it will take time grasping the effect of removing MOZ_SIGN_CMD from buildbot. Until this design flaw is resolved we will have to make sure that buildbot and mozharness know how to handle individual parts of signing.

Comment 7

[Aki Sasaki (not active)]

I think our move to taskcluster, and moving signing into signingscript, makes this no longer valid.