Revert "Address a race condition in libevent select."

[awlauria]

We do not want to be patching upstream components anymore.
The proper method is to get this merged upstream, then
pull it in the next upstream release.

This reverts commit c39fb57.

Signed-off-by: Austen Lauria [email protected]

[bosilca]

This is bad policy. We can't accept to live with broken functionality until some external project deems our patch interesting enough to pull it in and propagate it through their own releases.

[rhc54]

I'm afraid we don't have a choice. With the move to git submodules for all external packages, we no longer have the ability to support OMPI-committed changes. Besides, downstream packagers routinely refuse to build with our internal versions - so if the code cannot work with the stock release of a package, then our users are trashed anyway. Best if we get the package fixed, or figure out how to work around the error.

[bwbarrett]

We could still make something work with submodules, although it would be more work.

The second (and a related third) aspect are more the issue, in my mind. We decided for 5.0 to get more aggressive about preferring external packages (of hwloc, pmix, libevent, prrte) rather than our internal packages. So if there's a bug in upstream, it is going to impact most of our users. When I surveyed distros for libevent, all but the soon to be EOLed CentOS/RHEL 6 had a recent enough libevent. So we need to think more around mitigations / avoidance and less about direct bug fixes, or we're not actually helping our customers.

[awlauria]

I'm kind of inbetween.

I think that we should try and patch external functionality for critical issues - something like it causing a wrong answer in OMPI or it segv'ing in a critical path. In those cases I think a good argument can be made that we can't wait - especially if the external project is dragging their feet (such as in this patch).

But patching every fix will be difficult to maintain with the new submodule system.

So I guess the question is, do we think this is a critical issue? Reading the PR it doesn't seem like one to me, but I could be wrong on that. It is a good fix though and we should try and get it in to libevent.

[rhc54]

I think @bwbarrett hit it correctly - we have to get off of customized embedded versions because it fools us into thinking all is okay...and then our downstream packagers connect to the official releases, and our users can't figure out why everything is broken.

[bosilca]

Right, instead we prefer to have our users figuring out why some capabilities described in the CHANGELOG are not available on their system, which is as bad.

I'm not arguing against trying to push the fix upstream in the correct package, that's a no-brainer, the desirable long term approach. I am mostly concerned about the timeframe of such code propagations and the status of our codebase meanwhile.

And here my concern is two-fold:

• In master, I think the model you propose makes it hard for developers to bring new code. It might work for PRRTE, HWLOC and PMIx as we are sharing infrastructure and developers. But for external packages such as libevent, this is more than cumbersome (as proof nobody cared to update the internal version, aka. 2.0.22 released in 2014-01-05, when libevent is now at 2.1.12). I don't see why we choose to have an internal package but we refuse a patch that would make it work better or provide additional functionality.
• stable releases. We will have a hard time figuring out what OMPI functionality we need to disable depending on the available packages on the target system. That will again require time and resources that someone will have to put in.

Last but not least, we are not very good at following our own time frame for releases. But if we start depending on external packages to release their own stable versions such that we can pull them in our stable releases and do the appropriate testing, I am not even sure we should try setting any timeframe.

@awlauria in particular this PR enables ULFM capabilities over TCP, by not generating spurious error events on remote socket closing. We might see this in master as well, but as we only close sockets on MPI_Finalize we don't really case.

[rhc54]

This is up to you OMPI folks, but I cannot help but laugh about the whole thing. The original rationale for including embedded code was that the packages weren't universally available and we wanted the user to be able to do one download and build - i.e., we didn't want to make them install other packages just to build/run OMPI.

Once OMPI became more accepted, we found that the downstream packagers refuse to package OMPI with the embedded packages, insisting instead that we operate with the external releases of those packages. So we adjusted and are moving towards removing the embedded packages.

Now you are arguing that we can only work with our embedded packages, which means that anyone installing from downstream is screwed - and that this is a better situation? I guess I don't get it.

🤷‍♂️ whatever you guys decide

[bosilca]

I don't think I said what is a better solution, I just make clear that none of the current proposals are good, and that we should decide what we want to sacrifice.

[jsquyres]

We talked about this today on the Webex:

• UTK is working on upstreaming a fix.
  • UTK notes that we are using a super old version of libevent.
  • It is currently unknown if the issue exists in newer version of libevent; UTK will investigate.
  • Meaning: if the solution is simply to upgrade to a newer version of libevent, this might be the easiest solution.
• If we have to carry a patched version of libevent (let's hope not, but if we do):
  • We'll need to make a mirror of the libevent git repo (e.g., under github.com/open-mpi/)
  • Make a branch in there with the UTK-suggested fix.
  • Re-orient Open MPI's git submodule to point to that.
  • Have some configure logic such that if ULFM is selected to be built:
    • Require the use of our internal (patched) libevent, or
    • Require a user-supplied "it's ok to use external libevent with ULFM; I promise it's safe" configure CLI flag
    • Check the external libevent version to make sure it's safe from this bug

[bwbarrett]

Hang on, how does this fit with the strategy of "use an external libevent if it is found"? It feels like the discussion went wrong somewhere (likely because I unexpectedly could not attend today).

[rhc54]

I think the conversation went okay - what we basically concluded was that IF someone configures OMPI with ULFM, then we would fall back to using the internal libevent. If they try to configure with ULFM and with an external libevent, then we would just error out. Someday, when we know an external libevent release version has the patch, we can modify that to allow the external libevent if it is at least the "known good" level.

Otherwise, we can only reject the ULFM patch - it cannot safely run with an unpatched libevent.

[abouteiller]

I ran with the latest libevent release 2.1.12 which is only a few weeks old.

1. The bug manifests only when using the SELECT lib event backend; the select backend libevent code is still incorrect in latest release/master.
2. This is not the preferred backend on any decently modern linux, where the EPOLL backend is preferred (the EPOLL backend is correct, and prior versions of libevent also were correct with EPOLL). There is no problem with Linux.
3. Thus, the bug does manifest only on OS-X (for some reason, the KQUEUE backend is not picked, although it is available on OS-X).
4. I could not test on real BSD variants for the KQUEUE backend, but the code looks legit for that case. There are actually quite a lot of cases and comments for dealing with EV_DELETE happening before/meanwhile kqueue is polling. If the KQUEUE backend is used, it should work properly.

As for mitigation on OS-X:

1. We already have a selection logic in the ULFM configury to force the internal lib event.
2. That is not satisfactory because that would contradict the 'prefer external libevent' policy in the default configure setup.
3. There is however a programmatic way to choose an appropriate backend; from the libevent changelog:

 Also, despite our best efforts, not every backend supports every
  operation we might like.  Some features (like edge-triggered events, or
  working with non-socket file descriptors) only work with some operating
  systems' fast backends.  Previously, programmers who cared about this
  needed to know which backends supported what.  This tended to get quite
  ungainly.

  There is now an API to choose backends, either by name or by feature.
  Here is an example:

      struct event_config_t *config;
      struct event_base *base;

      /* Create a new configuration object. */
      config = event_config_new();
      /* We don't want to use the "select" method. */
      event_config_avoid_method(config, "select");
      /* We want a method that can work with non-socket file descriptors */
      event_config_require_features(config, EV_FEATURE_FDS);

      base = event_base_new_with_config(config);
      if (!base) {
         /* There is no backend method that does what we want. */
         exit(1);
      }
      event_config_free(config);

  Supported features are documented in event2/event.h

4. OS X requires select
   1. Note that the libevent I compiled on OS-X (default options, so what would presumably be compiled by distributors) finds no available backend with the "EVENT_NOSELECT" environment set, which means that disabling the 'select' backend fails to initialize during MPI_INIT (no event_base created).
   2. However, that failure to initialize could be restricted to the case where we know that incompatible features are requested to be active on the mpiexec command line.
   3. Maybe the reason why kqueue is not used can be found. From the MPI_INIT output with "EVENT_SHOW_METHOD=1" we get for each rank the fact that the kqueue is indeed available in general, but not available for TCP BTL:

[msg] libevent using: select
[msg] libevent using: kqueue

[rhc54]

So it sounds like we should:

• update our libevent (either via submodule or our historical method) to 2.1.12
• add logic to the libevent init in our code bases to ensure we don't use select unless nothing else is available
• add some logic to disable incompatible features on Mac for now, hopefully in configure
• someone who has time can look into kqueue issue for the TCP BTL

Correct?

[jsquyres]

See #7666 for a discussion / handy table for minimum supported versions of libevent that we talked about for v5.0.

[awlauria]

This is going away when we update the libevent submodule and the new configury work coming in, but for completeness and git history, merging this.

[jsquyres]

Notes from Webex discussion on 28 July 2020:

• There is work coming in "soon" that will make libevent be a git submodule, so there really isn't much point in making local libevent changes. So we might as well go ahead and merge this revert, just to tie off the local mod in git history (i.e., pretty much what @awlauria said, above).
• @abouteiller doesn't need 2.1.12 for their work.
• @abouteiller described above the narrow case where the problem occurs. They're going to look at options in ULFM for mitigating / protecting against this issue with Libevent -- probably detecting and printing a "this libevent configuration is not supported with ULFM" friendly kind of error message and then cleanly aborting.
• Since this is all related to the libevent-becoming-a-submodule issue, we were reminded of Minimum external Libevent version for Open MPI 5.0? #7666 where we discussed the minimum supported libevent version, and the behavior of what OMPI will enact (i.e., selecting internal vs. external).