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

The shell command abstraction pull #1262

Merged

Conversation

MorningLightMountain713
Copy link
Contributor

@MorningLightMountain713 MorningLightMountain713 commented Mar 24, 2024

In terms of modifying current FluxOS functionality - this pull does nothing at all.

Background

IMO, running shell commands from FluxOS is a hack, and it should be a FluxOS aspiration to remove them eventually. (Obviously, this isn't possible right now, due to other issues around users and root) Shell commands exist outside FluxOS control and their output / side effects can be unknown.

Currently, shell comands are hardcoded everywhere in FluxOS, many of which are OS dependent, meaning that FluxOS is forced to run on Debian, (eg use of apt), or make other assumptions about the underlying OS. Since these commands are hardcoded, there is no ability to run different commands on different OSes.

Another obstacle Flux faces, the assumption has been made that FluxOS never stops, I.e., there is no provision to stop any of the "Flux Functions". There are many unreachable timers / intervals and it is impossible to shutdown gracefully. FluxOS should be able to shut down gracefully and manage this itself. This is where the FluxController (mentioned below) will come into effect in later pulls.

Use of Nodemon - Nodemon is a development tool, it shouldn't be used in production, it's a bit of a crutch. There are many tools available for FluxOS to manage itself - this issue is in conjunction with the above, as Flux can't close gracefully and reload, something like Nodemon has to be used.

What this pull does

  • Fixes a bug in the logging module where if you logged undefined, it would raise an error.
  • Implements the AsyncLock.
  • Implements the FluxController .
  • Implements a runCommand function in the serviceHelper module.
  • Adds tests to serviceHelper for the runCommand function
  • Adds a new test file for the AsyncLock
  • Adds a new test file for the FluxController

AsyncLock

This is useful as you can just wait for a lock to become available, instead of checking a variable every x seconds, and sleeping in a loop.

FluxController

Incorporates an asyncLock, and an abortController. Introduces the concept of an interruptable sleep. This allows for modules to instantiate the FluxController class, and make use of the abort controller and sleep methods, which can be controlled from outside the module. This makes stopping complex operations simple.

runCommand

const { stdout, stderr, error } = await serviceHelper.runCommand('rm', {
  runAsRoot: true,
  params: ['-rf', rootOwnedDir],
});

This is the abstraction layer where all commands are run through. It adds some structure around how a command is run. E.g you can pass in an option runAsRoot to run the command as root.

It handles error logging and catch errors etc. Error logging is on by default, but can be disabled with the logError option.

All commands are executed WITHOUT A SHELL. This offers performance improvements, as 99% of commands don't actually need a shell. Majority of the commands that are using pipes etc (that require a shell) can simply be rewritten to not use pipes and do the work in FluxOS itself. (I have done this already).

If necessary, you can pass shell option and it will run the command in a shell

This will make things orders of magnitude easier in the future, as we can just swap out the command executor for whatever.

Next steps

I have already rewritten EVERY shell command in FluxOS to use the runCommand function, as well as updated ALL tests to reflect this. They are in another pull, which has become unwieldly, so will slice them out and merge them seperately.

Of note, I've started writing tests for syncthing, as I have changed quite a bit of logic here around how it's started and managed, installed etc. Will probably do this as the next pull.

@MorningLightMountain713
Copy link
Contributor Author

Screenshot 2024-03-24 at 11 30 55 AM

@TheTrunk TheTrunk merged commit a363987 into RunOnFlux:development Mar 26, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants