Page MenuHomePhabricator

[zorg] Add support for uploading artifacts to the 'llvmlab bisect' bucket and enable this for clang-cmake-mips.
ClosedPublic

Authored by dsanders on Nov 20 2015, 3:27 AM.

Details

Summary

Adds a new keyword argument to getClangCMakeBuildFactory():

stage1_upload_directory - This is the bucket subdirectory to upload to and should match the builder name.

The buildslave must have gsutil installed to use this argument.

Diff Detail

Event Timeline

dsanders updated this revision to Diff 40760.Nov 20 2015, 3:27 AM
dsanders retitled this revision from to [zorg] Add support for uploading artifacts to the 'llvmlab bisect' bucket and enable this for clang-cmake-mips..
dsanders updated this object.

Once our little-endian builder+buildslave is added to the buildbot I'll enable this there too.
Also, once I've configured the necessary machine I'll follow up with a further patch to add an lnt builder factory that takes pre-built toolchains from this bucket.

Could you consider subclassing and creating a separate build factory for this kind of builders instead, since you do not expect all the existing builders upload to the Google Cloud Storage, please?

The current getClangCMakeBuildFactory is consistent with the build performance data collection and analyses. With the uploading steps it wouldn't be any more. I prefer having a separate build factory for this case.
Thanks for working on this!

Thanks

Galina

zorg/buildbot/builders/ClangBuilder.py
689

Please define this as a builder env from the beginning, without introducing a new builder param.

Could you consider subclassing and creating a separate build factory for this kind of builders instead

I'm not sure what you mean by subclassing in this context since it's a plain function. I can duplicate the function or split it into separate stage1/stage2 parts and add the upload between stage1+stage2 if that helps.

since you do not expect all the existing builders upload to the Google Cloud Storage, please?

When Chris announced the bisection tool (http://lists.llvm.org/pipermail/llvm-dev/2015-October/091140.html) he invited other bot owners to upload to this area so it may turn out that others are interested too. I haven't asked around though.

The current getClangCMakeBuildFactory is consistent with the build performance data collection and analyses. With the uploading steps it wouldn't be any more. I prefer having a separate build factory for this case.

I'm not sure I understand this. Adding an upload step doesn't prevent the collection of data in the (optional) lnt step as far as I can tell. Am I missing something?

Thanks for working on this!

Thanks

Galina

zorg/buildbot/builders/ClangBuilder.py
689

Sure

dsanders updated this revision to Diff 41226.Nov 26 2015, 3:53 AM

Dropped boto_config_file in favour of 'env'.

dsanders updated this object.Nov 26 2015, 5:56 AM

Hi Daniel,

FWIW, I had a near identical patch locally (making the same decision to add an extra keyword argument, etc...) so it LGTM. My only interpretation of Galina's suggestion was to create a new function called getClangGCSBuildFactory(...) that calls getClangBuildFactory(...) and then adds the extra upload step. I don't see what benefit that would bring TBH. Hopefully Galina can clarify.

Have you acquired the relevant credentials to use gsutil like this? I asked a while back, and was told I need to make sure everything works OK with my own GCS account before pushing to the official GCS. Unfortunately I'm blocked getting a GCS internally, I'd be interested in hearing what policy you followed to get this commit-ready. If you've demonstrated this works, then that should allow the rest of us Buildbot users to follow your lead and get our builds into the cloud as well.

dsanders marked 2 inline comments as done.Nov 27 2015, 6:54 AM

Hi Daniel,

FWIW, I had a near identical patch locally (making the same decision to add an extra keyword argument, etc...) so it LGTM.
My only interpretation of Galina's suggestion was to create a new function called getClangGCSBuildFactory(...) that calls
getClangBuildFactory(...) and then adds the extra upload step. I don't see what benefit that would bring TBH. Hopefully Galina can clarify.

As far as I know it's only possible to append steps to a factory so I think I'd have to duplicate or split the function.

Have you acquired the relevant credentials to use gsutil like this? I asked a while back, and was told I need to make sure
everything works OK with my own GCS account before pushing to the official GCS. Unfortunately I'm blocked getting a GCS
internally, I'd be interested in hearing what policy you followed to get this commit-ready.
If you've demonstrated this works, then that should allow the rest of us Buildbot users to follow your lead and get our builds into the cloud as well.

I've got the config and I've checked it works by uploading a file and deleting it again.

To get it ready, I tested it with a private buildbot using the zorg scripts and modifications to disable mail and most of the builders/slaves. I'm using a newer version of buildbot but I don't think I've used anything new. On the GCS side, it turned out I didn't need to go through an approval process so long as I didn't spend any money :-). I therefore used the free trial. I've tested phased builders and some of the 'llvmlab bisect' commands. I haven't tried an actual bisect, but I have confirmed the filenames and tarball contents are the same as the two builders that are already uploading.

Sorry for not being clear from the beginning.
Anyway, your interpretation is correct. I meant having a separate get<something>BuildFactory(...) function for the builders which would upload blobs.

My only interpretation of Galina's suggestion was to create a new function called getClangGCSBuildFactory(...) that calls
getClangBuildFactory(...) and then adds the extra upload step.

Unfortunately, with the current getClangBuildFactory design this is not that simple. To do that right would require a relatively large refactoring of the getClangBuildFactory, which I don't want to force Daniel do just to get this patch in.

So, adding a new get<something>BuildFactory(...) which would simply call the changed getClangBuildFactory or copying and changing the actually needed code from the getClangBuildFactory would do for now.

I don't see what benefit that would bring TBH. Hopefully Galina can clarify.

There are 2 issues here:

  1. Having the getClangBuildFactory changed this way, we could all of a sudden get a lot of builders uploading or trying to upload blobs by just changing the default value of one of the many optional parameters. This might be something the buildslave owners didn't consider and have setup for in the first place.
  1. The getClangBuildFactory already due for refactoring, and adding more parameters there is not a good idea in general. Though, this is acceptable in this particular case as a "till-the-next-refactoring" solution, but I'd keep the dependencies as small and clear as possible.

Hope this make sense.

dsanders updated this revision to Diff 41749.Dec 3 2015, 6:40 AM
  • Renamed getClangCMakeBuilder() to _getClangCMakeBuilder()
  • Added getClangCMakeBuildFactory() with the old set of arguments and getClangCMakeGCSBuildFactory() with the additional one.
  • Split additions to _getClangCMakeBuilder() into a separate addGCSUploadSteps() to be more in line with the likely result of refactoring getClangCMakeBuilder(). Also generalized it enough to be reusable for stage 2 uploads.
  • Small cosmetic tweaks (e.g. 'upload' -> 'uploading' for the in-progress description) and tidy up.

Thanks, I think I understand now. Is the updated diff what you have in mind? I've taken the easy option in this patch but it should be easy to extract some of the duplication using Bicycle Repair Man or a similar tool.

Am I right in thinking that long term you want to end up with something like:

getClangCMakeBuildFactory(...) \
    .addStage1Steps(clean=..., test=..., install=...) \
    .addStage2Steps(clean=..., test=..., install=...) \
    .addGCSUploadSteps(...) \
    .addTestsuiteSteps(...)
gkistanova accepted this revision.Dec 3 2015, 1:59 PM
gkistanova added a reviewer: gkistanova.

Thanks, Daniel!

Is the updated diff what you have in mind?

This is exactly what I have in mind.

Am I right in thinking that long term you want to end up with something like:

Yes, I'm thinking along those lines. Maybe having more universal "bricks" which could be used for both stages, and such. Grouping steps and re-using the groups would help adding custom actions in the middle of the build flow, and keep specific factories separated, rather than having one large factory heavily parametric.

This revision is now accepted and ready to land.Dec 3 2015, 1:59 PM
dsanders closed this revision.Dec 4 2015, 6:33 AM

Thanks. I've committed it but it looks like the buildmaster hasn't picked up the changes automatically. It might need a restart.