Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

submit jobspec as JSON #1910

Merged
merged 9 commits into from Jan 9, 2019

Conversation

@garlick
Copy link
Member

commented Jan 6, 2019

This is a #1809, updated

  • to use PyYAML instead of ruamel.yaml (I think this is what we decided)
  • pass black formatting
  • fix travis Dockerfile to restore apt/yum cache prior to installing python-yaml

Whether this is the right direction may be an open question - #1909 was opened to track that decision.

(I still might need a bit of help understanding the proper procedure for submitting a PR with new package dependencies - though this seems to have worked for getting the initial PR through travis).

@codecov-io

This comment has been minimized.

Copy link

commented Jan 6, 2019

Codecov Report

Merging #1910 into master will decrease coverage by <.01%.
The diff coverage is 78.57%.

@@            Coverage Diff            @@
##           master   #1910      +/-   ##
=========================================
- Coverage   80.11%   80.1%   -0.01%     
=========================================
  Files         196     196              
  Lines       35124   35138      +14     
=========================================
+ Hits        28138   28147       +9     
- Misses       6986    6991       +5
Impacted Files Coverage Δ
src/modules/job-ingest/job-ingest.c 71.25% <78.57%> (+0.42%) ⬆️
src/common/libflux/response.c 81.48% <0%> (-1.24%) ⬇️
src/common/libjobspec/jobspec.cpp 84.46% <0%> (-0.8%) ⬇️
src/common/libflux/message.c 81.51% <0%> (+0.24%) ⬆️
@garlick garlick force-pushed the garlick:jobspec_json2 branch from 40527a3 to 6d2ce56 Jan 8, 2019
README.md Outdated Show resolved Hide resolved
@SteVwonder

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

(I still might need a bit of help understanding the proper procedure for submitting a PR with new package dependencies - though this seems to have worked for getting the initial PR through travis).

I think you will also want to add python-yaml to src/test/docker/bionic-base/Dockerfile and src/test/docker/centos7-base/Dockerfile. It won't affect this PR directly (which builds the travis image on top of the already built base images), but when the base images are re-built/pushed, they will then include python-yaml.

Install python-yaml packages in the travis Dockerfile.

Since the apt/yum cache is removed in the base images,
run apt/yum update before running the install commands.

Drop the packages listed there that are now in the base image.

Update block comment that was a cut&paste-o from RUN directive above.
@garlick garlick force-pushed the garlick:jobspec_json2 branch 2 times, most recently from 8f1d7af to e51686b Jan 8, 2019
@garlick

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2019

Thanks - rebased on current master and made those changes (squashing the doc one).

@garlick

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2019

Incidentally, we do have python-yaml (aka PyYAML) installed on TOSS 3 systems (just checked IPA), so this shouldn't create a new problem for building flux.

@SteVwonder

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

LGTM!

@garlick

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

Thanks!

I'm done working on this if someone wants to press the button.

@dongahn

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

Just a quick question. I assume that a jobspec will be stored as a JSON in KVS. How should flux-sched's resource matching service construct a jobspec out of the JSON?

https://github.com/flux-framework/flux-sched/blob/master/resource/modules/resource_match.cpp#L485

Do we need to extend https://github.com/flux-framework/flux-sched/blob/master/resource/libjobspec/jobspec.hpp to allow an JSON as an input as well?

Sorry I just don't recall the discussion we might have on this topic.

@grondo

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

I assume that a jobspec will be stored as a JSON in KVS. How should flux-sched's resource matching service construct a jobspec out of the JSON?

@dongahn, I think JSON is valid YAML and therefore libjobspec should be able to parse as JSON. We should give it a try though.

@grondo

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

Small typo in commit message for a396cb5: s/supplised/supplied/

@dongahn

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

@grondo: Ah good point. We should indeed try and test this.

@garlick

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

We should indeed try and test this.

t2200-job-ingest.t runs all the valid and invalid jobspec test input through the ingest module as JSON. The ingest module still validates jobspec using the C++ libjobspec, so I think the testing of this is already covered in this PR.

garlick added 8 commits Nov 5, 2018
Add y2j.py which reads YAML on stdin, and
writes JSON on stdout. It can be used to preprocess
YAML sample jobspec for input to existing tests,
in preparation for job-ingest requring jobs to
be submitted as signed JSON rather than allowing
either YAML or JSON.

Invalid YAML triggers a python exception and
a nonzero exit.

JSON output is compact, with no whitespace added
for readability.

Depends on python-yaml.
Rework sharness scripts to preprocess sample YAML
jobspec into JSON using y2j.py before signing and
submission, in preparation for job-ingest accepting
only signed JSON.
Add a test that submits signed YAML after job-ingest
was modified to reject YAML, and ensure we get the
expected result.
Add code to validate that submitted jobspec is
in JSON form.  If YAML is submitted (or any other
invalid JSON), an EINVAL error response is sent back
to the user, with a textual error message from the
JSON parser.

The jobspec is still passed through the C++ jobspec
validator, which accepts both JSON and YAML.
Update python/t0010-job.py test to internally
convert the testinput (basic.yaml) to JSON before
passing it to the submit API.
Add a configure check for PyYAML.  Require at
least the version supplied with CentOS 7,
which is 3.10.
@garlick garlick force-pushed the garlick:jobspec_json2 branch from e51686b to a03f9b0 Jan 9, 2019
@garlick

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

Small typo in commit message for a396cb5: s/supplised/supplied/

Thanks! Fixed and re-pushed.

@grondo

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

Ok, LGTM!

@grondo grondo merged commit e50e6a5 into flux-framework:master Jan 9, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garlick

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

Thanks!

@garlick garlick deleted the garlick:jobspec_json2 branch Jan 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.