1087104 - mach build needs to create partial mars so it can upload and do sendchange before finishing

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Jordan Lund (:jlund)]

consists of ripping out partial gen logic from mozharness and adding it into mach build

Comment 1

[Michael Shal [:mshal]]

Attachment: partial.patch

This adds partial mar generation to the build. It's a temporary hack until funsize is ready, then it can be ripped out again.

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8509920 [details] [diff] [review]
partial.patch

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

::: build/gen_mach_buildprops.py
@@ +28,5 @@
>          pass
>  
>      return (sha1Hash, size)
>  
> +def getMarProperties(filename, martype):

I'd rather have a boolean flag like partial=False and have this function choose between 'complete' and 'partial' instead of relying on the caller getting it right.

@@ +88,5 @@
>      args = parser.parse_args()
>  
> +    json_data = getMarProperties(args.complete_mar_file, 'complete')
> +    if args.partial_mar_file:
> +        json_data.update(getMarProperties(args.partial_mar_file, 'partial'))

So, these calls would become
  getMarProperties(args.complete_mar_file)
and
  getMarProperties(args.partial_mar_file, partial=True)

::: tools/update-packaging/Makefile.in
@@ +99,5 @@
> +	  "$(STAGE_DIR)/$(PKG_UPDATE_BASENAME).partial.$${SRC_BUILD_ID}-$${DST_BUILD_ID}.mar" previous current && \
> +	$(if $(MOZ_SIGN_CMD),$(MOZ_SIGN_CMD) -f mar "$(STAGE_DIR)/$(PKG_UPDATE_BASENAME).partial.$${SRC_BUILD_ID}-$${DST_BUILD_ID}.mar" &&) \
> +	rm -f $(STAGE_DIR)/previous.mar; \
> +	else echo "No previous MAR found; not creating a partial MAR"; \
> +	fi

Where can I look for the original code for that?

Comment 3

[Michael Shal [:mshal]]

The code is in buildbotcustom/process/factory.py. See:

http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py#2142

through:

http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py#2314

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8509920 [details] [diff] [review]
partial.patch

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

::: tools/update-packaging/Makefile.in
@@ +80,5 @@
> +
> +automation-partial-patch: complete-patch
> +	rm -rf current current.work previous
> +	mkdir current previous
> +	latestmar=$$(ssh -l $(UPLOAD_USER) -i $(UPLOAD_SSH_KEY) $(UPLOAD_HOST) ls -1t $(LATEST_MAR_DIR) | grep $(MOZ_PKG_PLATFORM).complete.mar$$ | head -n 1); \

You want quotes around ls -1t through head -n 1, to make the whole command run on the upload host, instead of retrieving the whole list and then grepping locally.

@@ +82,5 @@
> +	rm -rf current current.work previous
> +	mkdir current previous
> +	latestmar=$$(ssh -l $(UPLOAD_USER) -i $(UPLOAD_SSH_KEY) $(UPLOAD_HOST) ls -1t $(LATEST_MAR_DIR) | grep $(MOZ_PKG_PLATFORM).complete.mar$$ | head -n 1); \
> +	  if test -n "$$latestmar"; then \
> +	  wget -O $(STAGE_DIR)/previous.mar --no-check-certificate http://$(UPLOAD_HOST)/$(LATEST_MAR_DIR)/$$latestmar && \

I know this comes from the current code, but that --no-check-certificate rubs me the wrong way.

@@ +84,5 @@
> +	latestmar=$$(ssh -l $(UPLOAD_USER) -i $(UPLOAD_SSH_KEY) $(UPLOAD_HOST) ls -1t $(LATEST_MAR_DIR) | grep $(MOZ_PKG_PLATFORM).complete.mar$$ | head -n 1); \
> +	  if test -n "$$latestmar"; then \
> +	  wget -O $(STAGE_DIR)/previous.mar --no-check-certificate http://$(UPLOAD_HOST)/$(LATEST_MAR_DIR)/$$latestmar && \
> +	(cd previous; \
> +	  MAR=$(LIBXUL_DIST)/host/bin/mar$(HOST_BIN_SUFFIX) perl $(topsrcdir)/tools/update-packaging/unwrap_full_update.pl '../$(STAGE_DIR)/previous.mar') && \

MAR=$(MAR_BIN)

@@ +88,5 @@
> +	  MAR=$(LIBXUL_DIST)/host/bin/mar$(HOST_BIN_SUFFIX) perl $(topsrcdir)/tools/update-packaging/unwrap_full_update.pl '../$(STAGE_DIR)/previous.mar') && \
> +	(cd current; \
> +	  MAR=$(MAR_BIN) perl $(topsrcdir)/tools/update-packaging/unwrap_full_update.pl '../$(DIST)/$(COMPLETE_MAR)') && \
> +	find current -name \*.pgc -print -delete && \
> +	find previous -name \*.pgc -print -delete && \

Both current and previous are first removed and unpacked. How can we end up with .pgc files there?

@@ +96,5 @@
> +	MAR=$(MAR_BIN) \
> +	MBSDIFF=$(MBSDIFF_BIN) \
> +	  $(srcdir)/make_incremental_update.sh \
> +	  "$(STAGE_DIR)/$(PKG_UPDATE_BASENAME).partial.$${SRC_BUILD_ID}-$${DST_BUILD_ID}.mar" previous current && \
> +	$(if $(MOZ_SIGN_CMD),$(MOZ_SIGN_CMD) -f mar "$(STAGE_DIR)/$(PKG_UPDATE_BASENAME).partial.$${SRC_BUILD_ID}-$${DST_BUILD_ID}.mar" &&) \

Why not invoke $(MAKE) partial-patch with the right values for SRC_BUILD and DST_BUILD?

Comment 5

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to Mike Hommey [:glandium] from comment #4)
> > +	  wget -O $(STAGE_DIR)/previous.mar --no-check-certificate http://$(UPLOAD_HOST)/$(LATEST_MAR_DIR)/$$latestmar && \
> 
> I know this comes from the current code, but that --no-check-certificate
> rubs me the wrong way.

Ah, old code, that does no good with http:// anyway. We can't switch to https://ftp-ssl just yet, the wget on windows doesn't have a cert bundle with the CA in it.

Comment 6

[Michael Shal [:mshal]]

Attachment: partial.patch

I'm not sure about the .pgc thing - looks like it was a workaround for bug 783784 / bug 785748. I don't see any pgc files in the tests that I've run, so maybe it's not needed anymore? This patch still has it, though all other review comments should be addressed.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8510640 [details] [diff] [review]
partial.patch

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

::: tools/update-packaging/Makefile.in
@@ +82,5 @@
> +	rm -rf current current.work previous
> +	mkdir current previous
> +	latestmar=$$(ssh -l $(UPLOAD_USER) -i $(UPLOAD_SSH_KEY) $(UPLOAD_HOST) 'ls -1t $(LATEST_MAR_DIR) | grep $(MOZ_PKG_PLATFORM).complete.mar$$ | head -n 1'); \
> +	  if test -n "$$latestmar"; then \
> +	  wget -O $(STAGE_DIR)/previous.mar http://$(UPLOAD_HOST)/$(LATEST_MAR_DIR)/$$latestmar && \

either do this now or in a followup bug, but it would be good to have the fact that it's downloading from http instead of https from something hardcoded in the tree. IOW, it would be better to allow switching to https without having to land something.

@@ +96,5 @@
> +	SRC_BUILD=previous DST_BUILD=current \
> +	  $(MAKE) partial-patch && \
> +	rm -f $(STAGE_DIR)/previous.mar; \
> +	else echo "No previous MAR found; not creating a partial MAR"; \
> +	fi

This would be nicer with proper indentation in the shell script.

Comment 8

[Michael Shal [:mshal]]

Attachment: partial.patch

Includes indentation fix, plus a one-liner addition to set partialMarUrl that I missed. r+ carried forward

Comment 9

[Michael Shal [:mshal]]

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=370700be12f0
inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/44aadcfb5f57

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/44aadcfb5f57

Comment 11

[Jordan Lund (:jlund)]

it turns out partials are not working. this is due to a missing buildbot property: previous_buildid

right now we have mach generating props for partials:
'partialMarFilename': u'firefox-36.0a1.en-US.mac.partial.20141101040202-20141102040206.mar',
'partialMarHash': u'8247923d6ba5faf6432bd12e004e27d3bb0a46fe7344a79a29dfa6ddab65670f74ad4f163ee18858f38ed6d54e086fbd18c978b3f0fb7b665dc19ad630aca800',
'partialMarSize': 168285,
'partialMarUrl': u'http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/11/2014-11-02-04-02-06-ash/firefox-36.0a1.en-US.mac.partial.20141101040202-20141102040206.mar'

Also, after looking at http://mxr.mozilla.org/build/source/tools/scripts/updates/balrog-submitter.py?rev=25f7fb5bab4f#66 it seems balrog doesn't care about these props individually but instead looks them up within the partialInfo key.

we need mach to:
    - add 'previous_buildid' to that mach_build_properties.json

we then need mozharness to:
    - add {'partialInfo': [ # all the partial props[1] including 'previous_buildid']} to buildbot props

[1] http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py?rev=0fefa828e5f9#2521

Comment 12

[Michael Shal [:mshal]]

Attachment: bug1087104-partialInfo

This should set the partialInfo property similar to the NightlyBuildFactory: http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py#2521

The same code from the ReleaseBuildFactory will need to be done as part of the releases followup in bug 1094363.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8517628 [details] [diff] [review]
bug1087104-partialInfo

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

::: build/gen_mach_buildprops.py
@@ +102,5 @@
>      if args.partial_mar_file:
>          json_data.update(getMarProperties(args.partial_mar_file, partial=True))
> +
> +        # Pull the previous buildid from the partial mar filename.
> +        res = re.match(r'.*\.([0-9]*)-[0-9]*.mar', args.partial_mar_file)

You want +es instead of *s.

Comment 14

[Michael Shal [:mshal]]

Ok, I used +es for the [0-9] parts.

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/edea136c8f99

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/edea136c8f99

Comment 16

[Michael Shal [:mshal]]

aurora uplift for bug 1093897:
https://hg.mozilla.org/releases/mozilla-aurora/rev/f81cf71ec0cd
https://hg.mozilla.org/releases/mozilla-aurora/rev/0ebb0de0bb58

Comment 17

[Michael Shal [:mshal]]

beta uplift for bug 1093897:
https://hg.mozilla.org/releases/mozilla-beta/rev/ee2c3cfb4a7b
https://hg.mozilla.org/releases/mozilla-beta/rev/dc18ad2b4816