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.

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

NAS-127426 / 24.10 / Installer rewrite #73

Merged
merged 2 commits into from
Apr 3, 2024
Merged

NAS-127426 / 24.10 / Installer rewrite #73

merged 2 commits into from
Apr 3, 2024

Conversation

themylogin
Copy link
Contributor

@themylogin themylogin commented Feb 21, 2024 •
edited
Loading

We chose to use asynchronous I/O for the installer for two reasons:

  • We plan to add HTTP REST API Server in the future here which must be either asynchronous or multi-threaded. Multi-threaded implementation will also require external multi-threaded application server and external HTTP-server, which will increase the size of the ISO.
  • We plan to also add more pseudo-GUI features to the installer. Using asyncio will allow us to use prompt-toolkit for more complex UI elements that we have today or even whole screens from https://github.com/truenas/midcli

The current code base does some blocking I/O in the main loop. However, we never do the I/O that can really pause the program execution (e.g. interacting with block devices or network connections). For sake of simplicity of the implementation we allow ourselves to:

  • Read from pseudo-filesystems (/proc, /sys) in the main loop. These are really CPU-bound tasks since the contents of those "files" is generated by kernel.
  • Read and write on /tmp (using tempfile module) which is a RAM-based file system and will never block.

@bugclerk bugclerk changed the title Installer rewrite NAS-127426 / 24.10 / Installer rewrite Feb 21, 2024
@bugclerk
Copy link

Jira URL: https://ixsystems.atlassian.net/browse/NAS-127426

@themylogin themylogin force-pushed the NAS-127426 branch 3 times, most recently from 593d363 to 877ab5a Compare February 21, 2024 11:36
yocalebo
yocalebo requested changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@yocalebo yocalebo left a comment

Choose a reason for hiding this comment

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

I know that you haven't asked for anyone to review this yet but I got excited seeing the PR show up 😄 .

.github/workflows/flake8.yml Outdated Show resolved Hide resolved
debian/control Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
truenas_installer/dialog.py Outdated Show resolved Hide resolved
truenas_installer/dialog.py Show resolved Hide resolved
truenas_installer/install.py Show resolved Hide resolved
@themylogin themylogin force-pushed the NAS-127426 branch 4 times, most recently from cd983c8 to 9106278 Compare February 22, 2024 20:15
@themylogin themylogin requested a review from yocalebo February 29, 2024 20:09
@yocalebo yocalebo requested a review from a team February 29, 2024 20:10
anodos325
anodos325 reviewed Mar 6, 2024
View reviewed changes
truenas_installer/installer_menu.py
}
)

if destination_disks is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

When will destination_disks be None as opposed to an empty list? Maybe add a comment about why this would happen.

@anodos325
Copy link
Contributor

Perhaps use a pidfile or some other mechanism to prevent two instances of installer from running simultaneously.

@anodos325
Copy link
Contributor

It looks like it might be easy to add i18n support for the installer menus. Would be a nice value-add for international community.

@yocalebo yocalebo requested review from anodos325, william-gr and a team March 12, 2024 15:32
@yocalebo
Copy link
Contributor

yocalebo commented Apr 3, 2024

ping @truenas/middleware please put some eyes on this.

@william-gr and @anodos325 any other comments from you all?

@william-gr
Copy link
Member

I glanced it over once again, looks good, apart from the "root" which I commented on.

Looking forward for the follow up PR with APIs ;).

@themylogin themylogin requested a review from william-gr April 3, 2024 14:39
yocalebo
yocalebo approved these changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@yocalebo yocalebo left a comment

Choose a reason for hiding this comment

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

Looks fine from my point of view. As discussed, we're going to have to change the expect scripts so our CI continues to function.

@themylogin themylogin merged commit ee845a5 into master Apr 3, 2024
2 checks passed
@themylogin themylogin deleted the NAS-127426 branch April 3, 2024 17:18
@bugclerk
Copy link

bugclerk commented Apr 3, 2024

This PR has been merged and conversations have been locked.
If you would like to discuss more about this issue please use our forums or raise a Jira ticket.

@truenas truenas locked as resolved and limited conversation to collaborators Apr 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@yocalebo yocalebo yocalebo approved these changes

@anodos325 anodos325 Awaiting requested review from anodos325

@william-gr william-gr Awaiting requested review from william-gr

Assignees
No one assigned
Labels
no-time-tracked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@themylogin @bugclerk @anodos325 @yocalebo @william-gr