Skip to content

v4.1.x: Select all adapters with IPV4 addresses within specified subnet ranges. #9756

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Dec 17, 2021
Merged

v4.1.x: Select all adapters with IPV4 addresses within specified subnet ranges. #9756

merged 3 commits into from
Dec 17, 2021

Conversation

drwootton
Copy link
Contributor

@drwootton drwootton commented Dec 7, 2021 •
edited by jjhursey
Loading

This pull request is a cherry-pick of master branch pull requests #9681 and #9710 into the v4.1.x branch so that all interfaces within the specified subnet range can be selected by the the -mca btl_tcp_if_include and btl_tcp_if_exclude options. Including #9710 fixes for Coverity problems in #9681.

bot:notacherrypick

@ompiteam-bot
Copy link

Can one of the admins verify this patch?

@jjhursey
Copy link
Member

jjhursey commented Dec 7, 2021

ok to test

@jjhursey jjhursey added this to the v4.1.3 milestone Dec 7, 2021
@jjhursey jjhursey self-requested a review December 7, 2021 21:05
@jjhursey
Copy link
Member

jjhursey commented Dec 7, 2021

We will need a similar patch against orte/mca/oob/tcp in this PR.

David Wootton added 3 commits December 8, 2021 14:22
Allow --mca btl_tcp_if_include and --mca btl_tcp_if_exclude processin…
c4f047d 
…g to

select all interfaces within the specified subnet range.

Signed-off-by: David Wootton <[email protected]>
(cherry picked from commit 0b37cd2)
Signed-off-by: David Wootton <[email protected]>
Resolve Coverity problems reported in issue #9705
472d29c 
The Coverity CID numbers resolved by this pull request are
CID 1494435
CID 1494436

Signed-off-by: David Wootton <[email protected]>
(cherry picked from commit 82a868e)
Signed-off-by: David Wootton <[email protected]>
Make corresponding changes to oob_tcp_component.c so -mca
c16540b 
oob_tcp_if_include and oob_tcp_if_exclude options allow all
interfaces matching a subnet specification to be selected.

Signed-off-by: David Wootton <[email protected]>
@jjhursey
Copy link
Member

jjhursey commented Dec 8, 2021

This PR includes a cherry-pick of PR #9681 and additional fixes to ORTE and the embedded PMIx. The ORTE and PMIx changes are not cherry picks since they do not exist on master/v5.0.x.

@jjhursey jjhursey requested a review from jsquyres December 8, 2021 20:17
@jjhursey
Copy link
Member

jjhursey commented Dec 8, 2021

This is ready for review by the v4.1.x RMs.

A couple of notes:

The question to the RMs is if they want Commit 4 (modification to embedded OpenPMIx) or not. The OpenPMIx change openpmix/openpmix#2404 only went to master but we ask if they would entertain cherry-picking it to the v3 release. Alternatively, we could drop that 4th commit and keep OMPI and ORTE consistent. I'm not sure how many folks will notice the OpenPMIx change.

@rhc54
Copy link
Contributor

rhc54 commented Dec 8, 2021

Given that neither ORTE nor PMIx "stripe" messages across multiple interfaces, it's hard to see how those changes would impact anything. We only use one interface regardless of how many might be around.

@jsquyres
Copy link
Member

I'm ok with commits 1-3. I agree with @rhc54's point that ORTE doesn't ever use more than one interface, but the consistency between BTL and OOB is a good thing.

I'm conflicted about the 4th commit (the PMIX change), though.

  • Pros:
    • It went in on PMIx master.
    • Consistency with ORTE and BTL is a good thing.
  • Cons:
    • It makes the PMIx in v4.1.x be a bit different than the upstream PMIx 3.2.3 that 0e36e48 says that it is.

@bwbarrett Got an opinion here?

@jsquyres jsquyres requested a review from bwbarrett December 15, 2021 03:08
@rhc54
Copy link
Contributor

rhc54 commented Dec 15, 2021

Note: I'm not opposed to backporting it to PMIx 3.2.x and doing another 3.2 release. Just doesn't really do anything in terms of impacting behavior. But if it means something to you folks, we can certainly do it as it doesn't change the PMIx 3.2 series observable behavior.

jsquyres
jsquyres requested changes Dec 17, 2021
View reviewed changes
Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

We chatted offline about this.

Could you remove the PMIx commit from this PR?

I'm ambivalent about the ORTE commit -- feel free to keep it or feel free to delete it.

Thanks for the due diligence!

@jjhursey
Copy link
Member

@drwootton is out until after the new year. I went ahead and dropped the OpenPMIx commit on his branch. This should be ready for review.

@jsquyres jsquyres merged commit a287f09 into open-mpi:v4.1.x Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsquyres jsquyres jsquyres approved these changes

@jjhursey jjhursey jjhursey approved these changes

@bwbarrett bwbarrett Awaiting requested review from bwbarrett

Assignees

@jsquyres jsquyres

Labels
RM approved Target: v4.1.x
Projects
None yet
Milestone
v4.1.3
Development

Successfully merging this pull request may close these issues.
5 participants
@drwootton @ompiteam-bot @jjhursey @rhc54 @jsquyres