DIRAC_USE_TORNADO_IOLOOP set after Refresher is imported in tornado_start_all

[chrisburr]

In tornado_start_all.py DIRAC_USE_TORNADO_IOLOOP is set in the main function but the DIRACScript import triggers DIRAC.ConfigurationSystem.private.Refresher to be imported which reads this variable at import time.

In [1]: import sys
   ...: assert "DIRAC.FrameworkSystem.Client.MonitoringClient" not in sys.modules
   ...: assert "DIRAC.ConfigurationSystem.private.Refresher" not in sys.modules
   ...: from DIRAC.Core.Utilities.DIRACScript import DIRACScript
   ...: assert "DIRAC.FrameworkSystem.Client.MonitoringClient" not in sys.modules
   ...: assert "DIRAC.ConfigurationSystem.private.Refresher" not in sys.modules
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-1-8197cf08d195> in <module>
      4 from DIRAC.Core.Utilities.DIRACScript import DIRACScript
      5 assert "DIRAC.FrameworkSystem.Client.MonitoringClient" not in sys.modules
----> 6 assert "DIRAC.ConfigurationSystem.private.Refresher" not in sys.modules

AssertionError:

Originally posted by @chrisburr in #5214 (comment)

[chrisburr]

There are two places where this variable is read, can they be converted to read later than import time?

DIRAC/src/DIRAC/FrameworkSystem/Client/MonitoringClient.py

Lines 89 to 94 in 54d8988

	# DIRAC_USE_TORNADO_IOLOOP is defined by starting scripts
	if os.environ.get('DIRAC_USE_TORNADO_IOLOOP', 'false').lower() in ('yes', 'true'):
	from DIRAC.FrameworkSystem.Client.MonitoringClientIOLoop import MonitoringFlusherTornado
	gMonitoringFlusher = MonitoringFlusherTornado()
	else:
	gMonitoringFlusher = MonitoringFlusher()

This is only used in MonitoringClient.initialize so I think the initialisation of the global variable can be moved into there?

DIRAC/src/DIRAC/ConfigurationSystem/private/Refresher.py

Lines 110 to 115 in 8269146

	# DIRAC_USE_TORNADO_IOLOOP is defined by starting scripts
	if os.environ.get('DIRAC_USE_TORNADO_IOLOOP', 'false').lower() in ('yes', 'true'):
	from DIRAC.ConfigurationSystem.private.TornadoRefresher import TornadoRefresher
	gRefresher = TornadoRefresher()
	else:
	gRefresher = Refresher()

This one looks a little tricker.

[chaen]

I knew this would bite us https://github.com/DIRACGrid/DIRAC/pull/4865/files#r557381856

Again: why not putting the variable definition at the very top of the file and be done with it ?

[chrisburr]

As all modules containing a @DIRACScript decorated function are imported every time a dirac- command is ran in order to figure out which command is currently being ran so it can be replaced with the function from an DIRAC extension instead.

[chaen]

As all modules containing a @DIRACScript decorated function are imported every time a dirac- command is ran in order to figure out which command is currently being ran so it can be replaced with the function from an DIRAC extension instead.

Ah true... I guess it is due to this check that all the scripts use DIRACScript ?

DIRAC/src/DIRAC/Core/Utilities/DIRACScript.py

Lines 53 to 59 in cf55bc4

	entrypointFunc = entrypoint.load()
	if not isinstance(entrypointFunc, DIRACScript):
	raise ImportError(
	"Invalid dirac- console_scripts entry_point: " + repr(entrypoint) + "\n" +
	"All dirac- console_scripts should be wrapped in the DiracScript " +
	"decorator to ensure extension overlays are applied correctly."
	)

I'd be tempted to remove that check actually, and relying on the name... Importing hundreds of module every time we call a script is an overkill anyway isn't it ?

Another option would be to have that variable set in the run file, but that is really black magic, and means the installation cannot be automated with the current tools, so not so nice.
Another bad option: dedicated servers for https services, where we set that variable in the bashrc

Or, would by any chance adding the following at the very top of tornado-start-all solve our problem ?

if __name__ == "__main__":
  import os
  os.environ['DIRAC_USE_TORNADO_IOLOOP'] = 'Yes'

[chrisburr]

Another bad option: dedicated servers for https services, where we set that variable in the bashrc

It can also be added to the runit scripts:

DIRAC/src/DIRAC/FrameworkSystem/Client/ComponentInstaller.py

Lines 2357 to 2367 in 9515f7e

	with io.open(runFile, 'wt') as fd:
	fd.write(
	u"""#!/bin/bash
	rcfile=%(bashrc)s
	[ -e $rcfile ] && source $rcfile
	#
	exec 2>&1
	#
	#
	exec tornado-start-all
	""" % {'bashrc': os.path.join(self.instancePath, 'bashrc')})

(for tornado-start-all, I'm not sure where tornado-start-CS is used)

[chaen]

Yes, that's what I proposed (it's the run file), but I had forgotten that it's the ComponentManager writing this file.

tornado-start-CS is used for the master CS.

My choice goes to

if __name__ == "__main__":
  import os
  os.environ['DIRAC_USE_TORNADO_IOLOOP'] = 'Yes'

I think it's the best solution

[chrisburr]

When using entrypoints that will never trigger as __main__ is the wrapper script that pip generates at install time.

[chaen]

argh, yeah.... oh my...
OK, so ComponentInstaller it is, and for the custom scripts like tornado-start-CS, it's anyway a manual operation

[chrisburr]

Yeah...to stay safe shall we make the script exit with an error if the environment is invalid?

[chaen]

that is exactly what I have done. PR coming :)