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

broker: ensure parent-uri and parent-kvs-namespace are only set for jobs #6561

Merged
merged 4 commits into from
Jan 21, 2025

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Jan 17, 2025

This PR fixes #6560 by delaying initialization of the mentioned attributes until after bootstrap. Then the jobid attribute instead of FLUX_JOB_ID is used since this more correctly indicates if the current instance is a job or not.

Fixes #6560

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Problem: Some broker attributes that are initialized in init_attrs()
may be more properly initialized after bootstrap is complete (i.e.
after the call to boot_pmi() or boot_config() returns)

Split a new function init_attrs_post_boot() out of init_attrs() and
run it after bootstrap is complete. There are no other substantive
changes at this time.
Problem: The broker uses presence of FLUX_JOB_ID in the environment
to determine if it needs to set the parent-uri attribute, but this
ends up setting parent-uri in cases where the instance is not a job
such as `flux run flux start --test-size=1`.

Use the `jobid` broker attribute instead of FLUX_JOB_ID. This attribute
is only set by boot_pmi() if the instance was really started as a
job in another Flux instance.
Problem: The broker always sets the parent-kvs-namespace broker
attribute if FLUX_KVS_NAMESPACE is set in the environment, but this
doesn't make sense unless the current instance is a job.

Only set parent-kvs-namespace if the current instance is a job.
Problem: No tests in the testsuite ensure that parent-uri and
parent-kvs-namespace are set only for instance that are jobs.

Add a couple tests to t0001-basic.t.
@mergify mergify bot merged commit d32d543 into flux-framework:master Jan 21, 2025
35 checks passed
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.43%. Comparing base (482028e) to head (fb6f976).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
src/broker/broker.c 90.90% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6561   +/-   ##
=======================================
  Coverage   79.43%   79.43%           
=======================================
  Files         531      531           
  Lines       88280    88286    +6     
=======================================
+ Hits        70125    70133    +8     
+ Misses      18155    18153    -2     
Files with missing lines Coverage Δ
src/broker/broker.c 77.35% <90.90%> (+0.16%) ⬆️

... and 7 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

parent-uri attribute set with flux run flux start --test-size=
2 participants