WIP: Generate gitlab ci yaml#8718
Conversation
|
ping @opadron I've been running this branch against my own local gitlab/ci instance, and though it needs adjustment, it's a decent (working) start. Currently the jobs in the pipeline are discovering they need a rebuild, then failing when my test Docker image doesn't have some of the build toolchain expected. |
|
ping @aashish24 |
|
thanks @scottwittenburg few things I have mentioned that you probably know already but I am documenting it here:
Great start.. thanks for pinging me on this. |
scottwittenburg
force-pushed
the
generate-gitlab-ci-yaml
branch
from
fc1ba49 to
9dda56c
Compare
tgamblin
left a comment
tgamblin
left a comment
There was a problem hiding this comment.
@scottwittenburg @opadron: This is starting to look good to me! See my review comments for details.
FYI: I found this promising GitLab issue on allowing seed jobs to generate gitlab-ci.yml dynamically. It seems like people like it. I put in a comment about our use case and a link to this PR, but I was struck by how similar the proposal is to what we ended up doing here. I hope it gets implemented.
| # full_hash doesn't match (or remote end doesn't know about | ||
| # the full_hash), then we trigger a rebuild. | ||
| remote_pkg_info = remote_pkg_index[pkg_short_hash] | ||
| if not 'full_hash' in remote_pkg_info or \ |
There was a problem hiding this comment.
Use parens around the condition instead of \
| for release_spec in release_spec_set: | ||
| pkg_name = release_spec.name | ||
| pkg_version = release_spec.version | ||
| pkg_short_hash = release_spec.dag_hash() |
There was a problem hiding this comment.
The name short_hash here is a little confusing, as Spack (like git) allows you to enter in uniqe prefixes of hashes and have them work. So I'd say the hash in your "short spec" is really a "short hash" 😄.
I think I see why we call it a short hash -- the full_hash is a SHA-256, while the dag_hash is a SHA-1, so it's shorter. That actually took me by surprise -- the full hash is really long and has a bunch of ==== padding at the end of it because SHA-256 length isn't evenly divisible by 5.
Can we do two things here?
- Since there's just going to be one hash in the end, just call this the
hash - change the full hash to use SHA-1/base32 like the
dag_hash, so at least they're the same length. I worry about transitioning full hash to be the main hash if they're different lengths. We can talk about making the hashes longer in Spack later, but I think things will be smoother if the full hash looks like the dag hash.
There was a problem hiding this comment.
#8911 represents my attempt to fulfill request (2) above.
| tty.msg(' %s -> %s' % (mirror, configured_mirrors[mirror])) | ||
| mirror_url = configured_mirrors[mirror] | ||
| mirror_rebuilds = get_mirror_rebuilds(mirror, mirror_url, release_spec_set) | ||
| if len(mirror_rebuilds) > 0: |
There was a problem hiding this comment.
more pythonic: if mirror_rebuilds:
| outf.write(json.dumps(rebuilds)) | ||
|
|
||
|
|
||
| def check_single(args): |
There was a problem hiding this comment.
seems like most of this function can be replaced with a call to check_all, if check_all took a spec set and/or list of specs as a parameter instead of opening a specific file.
Right now it duplicates a fair amount of logic.
| tty.msg(msg) | ||
|
|
||
|
|
||
| def check_binaries(parser, args): |
There was a problem hiding this comment.
Can this command be integrated with spack buildcache or with spack mirror? I think it makes more sense as a subcommand of one of those.
lib/spack/spack/cmd/release_jobs.py
Outdated
| share_path = os.path.join('.', 'share', 'spack') | ||
| common_scripts_dir = os.path.join(share_path, 'docker', 'build', 'common') | ||
|
|
||
| os_container_mapping = { |
There was a problem hiding this comment.
This probably belongs in a config file somewhere.
lib/spack/spack/cmd/release_jobs.py
Outdated
|
|
||
| release_spec_set = None | ||
|
|
||
| with open(release_specs_path, 'r') as fin: |
There was a problem hiding this comment.
this duplicates the logic in check_all -- why not a function like CombinatorialSpecSet.from_file(path)?
lib/spack/spack/cmd/release_jobs.py
Outdated
|
|
||
| with open(release_specs_path, 'r') as fin: | ||
| release_specs_contents = fin.read() | ||
| release_specs_yaml = yaml.load(release_specs_contents) |
There was a problem hiding this comment.
minor point: consider using spack_yaml instead of yaml. spack_yaml preserves file/line info from the file as attributes on the data structure it reads in (try typing spack config blame config if you are interested).
scottwittenburg
force-pushed
the
generate-gitlab-ci-yaml
branch
2 times, most recently
from
4a8dff1 to
3d130c7
Compare
|
I think this thing is ready for another review pass @tgamblin, at your convenience. Pinging @scheibelp for visibility as well. The jobs specified in the auto-generated Before I try pushing this branch to the gitlab instance set up by @opadron, I will need to regenerate the One thing I don't think I have seen yet is how dependencies will get pulled as binaries from the mirror if they exist. Is that something that should already exist if we just have a mirror configured? Or should I be thinking about how that will work? ping @aashish24 |
|
@scottwittenburg just as an FYI: the kubernetes cluster should be easier than ever to use! (#9018) |
|
fyi @zackgalbreath |
|
@scottwittenburg @opadron and @bryonbean: the S3 mirror should be set up in Kitware's AWS, maybe with Cloudflare but we can see if we really need that once the traffic starts coming in. There is some info here on hosting static stuff like this via S3. It would be nice if the bucket had a spack-ish domain name... maybe It is worth considering that as the bucket gets more distributed -- i.e. if we enable CDN and start having files propagated out all over -- I think the likelihood that binaries will appear there quickly goes down. So passing artifacts another way may be the better way to go. |
There was a problem hiding this comment.
@scottwittenburg: Thanks! This mostly looks good -- just a few comment inline. See above on the S3 mirror.
lib/spack/docs/testing_guide.rst
Outdated
| either of these options to ``spack install``: | ||
|
|
||
| * ``--log-format=cdash-simple`` | ||
| * ``--log-format=cdash-complete`` |
There was a problem hiding this comment.
There's actually just --log-format=cdash (and --log-format=junit) now. Sorry this is dated. Do you want me to fix it or maybe do you or @zackgalbreath want to take a stab?
lib/spack/docs/testing_guide.rst
Outdated
| ``report.configure.xml``, and ``report.test.xml``, for the build, | ||
| configure, and tests steps, respectively. | ||
|
|
||
| If you want to upload these fils to a CDash instance, you can use ``curl``: |
There was a problem hiding this comment.
should probably document @zackgalbreath's --cdash-upload-url here, before the curl stuff. Also @zackgalbreath, does that support authenticated submissions? Should that be documented?
lib/spack/docs/testing_guide.rst
Outdated
| .. _cmd-spack-test-suite: | ||
|
|
||
| --------------------- | ||
| CDash test suites |
There was a problem hiding this comment.
This whole section can be ripped out and replaced with dos on @scottwittenburg's new infrastructure when it's ready.
| 'Use -t to install all downloaded keys') | ||
|
|
||
|
|
||
| def read_from_url(file_uri): |
There was a problem hiding this comment.
This probably belongs in spack.util.web, and at least the urlopen part should be replaced with spack.util.web._urlopen for backward-compatibility with Python 2.6. I think you can factor out the logic in spack.util.web._spider() (the parallel web spider used by spack checksum) and use it here -- that stuff already handles things like disabling SSL verification for spack -k/--insecure, and this should respect -k.
Possibly useful (but maybe annoying) side note: Spack is a bit confused about how to fetch from URLs at the moment. fetch_strategy.URLFetchStrategy is probably the main tool for downloading, and it uses curl. It seems like system curl is the most likely tool on HPC systems to have SSL set up properly. Many HPC Pythons don't have SSL cert settings configured right, so using urlopen in Python can fail. curl can be set up wrong too, though, so that might fail as well. At the moment, we don't really have a good fallback system, or a way to try to automatically find certs in the places you might expect them to be.
Probably best not to worry about this too much here and just use _urlopen and the stuff already in spack.util.web, but in case it helps, now you have some more details.
| # full_hash doesn't match (or remote end doesn't know about | ||
| # the full_hash), then we trigger a rebuild. | ||
| remote_pkg_info = buildcache_index[pkg_hash] | ||
| if ('full_hash' not in remote_pkg_info or |
There was a problem hiding this comment.
this logic LGTM, but can you add a note that eventually it'll be less complicated (when the dag_hash is the full_hash)?
| except URLError as url_err: | ||
| msg = 'Unable to open url {0} due to {1}'.format( | ||
| file_uri, url_err.message) | ||
| raise spack.error.SpackError(msg) |
There was a problem hiding this comment.
probably ok not to wrap this here, as the function's only used by routines in this file -- the SpackError is immediately caught by needs_rebuild, anyway -- you could just let it catch the URLError.
| with open(output_file, 'w') as outf: | ||
| outf.write(json.dumps(rebuilds)) | ||
|
|
||
| return 1 if rebuilds else 0 |
There was a problem hiding this comment.
Not sure if it's clearer or not but int(bool(rebuilds)) would work too :).
| return json.loads(index_contents) | ||
|
|
||
|
|
||
| def check(mirrors, specs, no_index=False, output_file=None): |
There was a problem hiding this comment.
probably needs a more descriptive name -- I had to think about this a bit.
| remote_pkg_index = get_remote_index(mirror_url) | ||
|
|
||
| for spec in specs: | ||
| rebuild_spec = needs_rebuild(spec, mirror_url, remote_pkg_index) |
There was a problem hiding this comment.
I wonder if this could be made faster with logic like spack.util.web._spider or just its own multithreading.Pool -- right now it does a bunch of potentially slow web requests sequentially.
There was a problem hiding this comment.
My thought is that there are two potential use cases for this, 1) parallel jobs will run this with the --no-index flag, which means one job will fetch one remote .spec.yaml, or 2) if a single process runs this, it will not use --no-index, which means that it will fetch the remote package index from the mirror and then use that to look up the full hashes of all the specs it needs to check.
So given that we don't expect a single process to sequentially make a bunch of slow web requests (though it is possible through misuse/misunderstanding), do we still need to worry about making this faster?
lib/spack/spack/cmd/buildcache.py
Outdated
| specs = [Spec(args.spec)] | ||
| else: | ||
| release_specs_path = \ | ||
| os.path.join(etc_path, 'spack', 'defaults', 'release.yaml') |
There was a problem hiding this comment.
just break at the first ( instead of \
|
@tgamblin would another spack subdomain that redirects to the actual S3 bucket be desirable? |
scottwittenburg
force-pushed
the
generate-gitlab-ci-yaml
branch
3 times, most recently
from
d02fb63 to
5bcdde7
Compare
scottwittenburg
force-pushed
the
generate-gitlab-ci-yaml
branch
2 times, most recently
from
cc65500 to
44ef17c
Compare
scottwittenburg
force-pushed
the
generate-gitlab-ci-yaml
branch
3 times, most recently
from
003a079 to
02b0403
Compare
scottwittenburg
force-pushed
the
generate-gitlab-ci-yaml
branch
6 times, most recently
from
938da3f to
e5a37c5
Compare
scottwittenburg
force-pushed
the
generate-gitlab-ci-yaml
branch
from
3c47de9 to
ffc7325
Compare
Also provide the "all_deps" option when writing dependent spec to yaml
Since source package hashes are verified, this may be safe enough.
|
Heads up to reviewers (currently just @tgamblin or @opadron): The diff shown here on github does not match the diff I see locally between At any rate, as an example the diff shown here for This may indicate we should close this PR and start over. At which point it may make sense to re-factor into commits which more accurately reflect the logical changes. Let me know what you think. |
scottwittenburg
force-pushed
the
generate-gitlab-ci-yaml
branch
2 times, most recently
from
fa9d462 to
bb7cdfd
Compare
Also use presence of cdash build id in install output as condition for whether install succeeded or failed, as it seems a configure error may not result in a non-zero exit code from install command.
This reverts commit 075f898.
|
Scott, have you had a chance to include my changes to rebuild-package script? |
|
Closing this in favor of #10274, which is mostly the same but cleaned up and rebased on the latest |
8 participants
This presents another possible path to generating release binaries, similar to #8444, but with the jobs being created more statically (by a script run manually when release spec-set changes). Then each job (represented in the
rebuild_package.shshell script) can check for itself whether to build the package or whether that work is not needed.