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

Integrate jobspec into resource, etc #398

Merged
merged 9 commits into from Oct 25, 2018

Conversation

Projects
None yet
4 participants
@dongahn
Copy link
Contributor

dongahn commented Oct 24, 2018

This PR integrates jobspec internally into the resource infrastructure:

  1. Tightly managing libjobspec from within flux-sched solves odd make-check errors that we are seeing on some platforms (e.g., Sierra). The issues seem to arise due to C++ compiler version mismatches.

  2. flux-core will transition away from libjobspec anyway in a near future, and doing this now will help flux-core make this transition without having to worry about any breakage in flux-sched.

This PR also has a fix for pointer to 64 integer conversion for 32bit systems.

Resolve #393, #395 and #397.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 24, 2018

I think you forgot to add jobspec.hpp to libjobspec_conv_la_SOURCES and that's why distcheck failed in travis.

Does configure --disable-jobspec result in a working sched? If jobspec is required, that option should go away.

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 24, 2018

Yes, the jobspec is required. I will look at the new m4 file more closely. Thanks.

@dongahn dongahn force-pushed the dongahn:jobspec_copy branch from 8a2ac99 to d93ab6e Oct 24, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 24, 2018

Codecov Report

Merging #398 into master will increase coverage by 0.24%.
The diff coverage is 85.32%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #398      +/-   ##
=========================================
+ Coverage   75.45%   75.7%   +0.24%     
=========================================
  Files          65      67       +2     
  Lines       10512   10768     +256     
=========================================
+ Hits         7932    8152     +220     
- Misses       2580    2616      +36
Impacted Files Coverage Δ
resource/policies/base/matcher.hpp 100% <ø> (ø) ⬆️
resource/traversers/dfu_impl.hpp 100% <ø> (ø) ⬆️
resource/libjobspec/jobspec.hpp 100% <100%> (ø)
resource/planner/planner_multi.c 63.93% <33.33%> (ø) ⬆️
resource/libjobspec/jobspec.cpp 85.65% <85.65%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a8d570...e8850d9. Read the comment docs.

@dongahn dongahn force-pushed the dongahn:jobspec_copy branch 3 times, most recently from cda037e to 6aa1a75 Oct 24, 2018

dongahn added some commits Oct 23, 2018

config: Pull in checks for yamlcpp
In preparation for making a copy of libjobspec from flux-core.
jobspec: Copy libjobspec from flux-core
Pull in jobspec.cpp and jobspec.hpp. The goal is two-fold.

1. Tightly managing libjobspec from within flux-sched solves odd
make-check errors that we are seeing on some platforms (e.g., Sierra).
The issues seem to arise due to C++ compiler version mismatches.

2. flux-core will transition away from libjobspec anyway
in a near future, and doing this now will help flux-core
make this transition without having to worry about
any breakage in flux-sched.

Name the library as libjobspec_conv.la instead
of libjobspec.la to avoid any name collision
with flux-core's libjobspec.
jobspec: Integrate the copy of jobspec with the rest of resource
Integrate yamlcpp check into configure.ac

Drop flux-jobspec check in configure.ac.

Adjust header file inclusion path and makefiles.

Additionally, resource-query and grug2dot utilities commands
are now built in their own folder (instead of from within
their parent folder.
planner: Fix a pointer-to-integer conversion
Use intptr_t instead of int64_t to cast the span IDs
that were stored as (void *) in a zlist.

It turned out the int64_t conversion doesn't work for 32-bit
architecture such as armv7l:

$ dpkg-architecture -l | grep HOST_ARCH
DEB_HOST_ARCH=armhf
DEB_HOST_ARCH_ABI=eabihf
DEB_HOST_ARCH_BITS=32
DEB_HOST_ARCH_CPU=arm
DEB_HOST_ARCH_ENDIAN=little
DEB_HOST_ARCH_LIBC=gnu
DEB_HOST_ARCH_OS=linux

On 32-bit systems, it also is unsafe to assign int64_t span_id
directly into void * type.

When you have a very large span id, the value can be truncated
during this assignment.

But in practice, it is very unlikely the id will become larger than
2 billion, the largest value 32-bit integer (through
conversion) can represent. (In particular on 32-bit systems where
the amount of memory would be limited.)

Since we should optimize for 64-bit systems, I still keep
int64_t-to- void* conversion via intptr_t type for now.

If we need to be more serious about supporting 32-bit
architecture, we should take a deeper dive into this topic.

@dongahn dongahn force-pushed the dongahn:jobspec_copy branch from 6aa1a75 to c5c6b78 Oct 24, 2018

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 24, 2018

OK. I forced a push with fixes for both of the issues @garlick caught.

Travis CI is green.

I have to guess that the code coverage issue is the same side effect that @grondo's docker PR had?

From my perspective, this can be merged. Let me know, @garlick and @SteVwonder, if you need me to do more as part of this PR.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 24, 2018

On the coverage miss, actually you may want to pull in the tests too.

src/cmd/flux-jobspec-validate is a jobspec validator program written in c++.

t/jobspec contains various inputs including invalid jobspec, and jobspec from our RFC examples.

t/t0018-jobspec.t runs the inputs through the validator.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 24, 2018

(I would probably change flux-jobspec-validate to a test program rather than an installed command)

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 24, 2018

have to guess that the code coverage issue is the same side effect that @grondo's docker PR had?

No, this is because jobspec.cpp is mostly not covered by tests. We may also need to pull in the flux jobspec-validate command and associated sharness test (t0018-jobspec.t) to get good coverage.

Edit: sorry @garlick beat me to it.

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 24, 2018

Ugg... OK that makes sense. I will pull those in as well then.

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 25, 2018

OK. code coverage looks much better now. Let me know if there is anything else.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 25, 2018

Looks good. I'll go ahead and press the button (hope it's OK with sched people!)

@garlick garlick merged commit 2580f41 into flux-framework:master Oct 25, 2018

3 checks passed

codecov/patch 85.32% of diff hit (target 75.45%)
Details
codecov/project 75.7% (+0.24%) compared to 2a8d570
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.