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

dts-scripts refactoring #1067

Open
DaniilKl opened this issue Oct 1, 2024 · 13 comments
Open

dts-scripts refactoring #1067

DaniilKl opened this issue Oct 1, 2024 · 13 comments
Labels
DasharoToolsSuite enhancement New feature or request

Comments

@DaniilKl
Copy link
Contributor

DaniilKl commented Oct 1, 2024

The problem you're addressing (if any)

The scalability problem of a project might be caused not only by tools being used, but by the way the tools are being used as well.

  1. Bad scalability;
    a. No explicit coding rules and style -> the code is difficult to read and modify, the coding style of every contributor overlays with others, and the codebase shortly became messy;
    b. Not clearly separated systems -> the way the system works is not linear and, therefore, not clear, especially for novices;
    c. The language, which is not suitable for large codebases -> language issues and workarounds;
    d. Configuration and metadata directly stored in the codebase -> not convenient to scale and support;
  2. Awful testing and verification routine;
    a. No test cases prepared -> no empirical evidence of working features;
    b. No hardware separation -> QEMU cannot be used for testing and verification;
  3. High entry level;
    a. No documentation and comments;
    b. Highly non-linear structure;

Describe the solution you'd like

image

Nodes and paths description:

  • Deployment - common provisioning logic (not tools or calls to the tools, but decisions what to deploy and why), the only module writing to hardware (via HAL), the common logic makes calls to a hardware specific logic (functions; 8) held by HAL. In case the testing is going on, the common logic instead of communicating with hardware via HAL, communicates with mocking HAL functions (8), that emulate the hardware.
  • Configuration - logic responsible from getting and providing hardware information (2, 5). During the boot, it gets hardware inf. (7), and keeps it, then, according to the inf. gained, it chooses (6) the DPP configuration (links to artifacts, lists of hardware specific mocking functions, lists of variables that define deploying workflows, etc.) from Linux FS (or, theoretically, from cloud services (then, it will have to communicate with packaging node first)).
  • User interface - is responsible for communication with user. It has some hardware specific stuff (like, whether it is possible to update or to install, or both) on which it decides based on inf. provided by configuration node (5). It provides user following functionalities: start deploying (1), start backup or restore (9) and manage packages (10).
  • Backup and restore - logic, responsible for backing up and restoring firmware, takes full advantage of deploying logic (11) and its ability to communicate with hardware (8).
  • Packaging - responsible for checking DPP subscription with cloud services, logging in, downloading artifacts, and managing packages. Could be referenced by deploying logic (3) or explicitly by user (10).
  • Linux FS - holds some hardware-specific configs (that are currently held in board_config function) in .json files, as well as hardware-specific logic for HAL (some hardware-specific deploying functions and mock functions) and provides it on the start to configuration node (6).
  • HAL - Separates all logic from calls to tools, which manage hardware (13); holds some hardware-specific functions between calling to the tools (8) and the deploying logic, so to separate highly non-linear deploying logic from step-by-step hardware-specific logic. The hardware specific log is being dynamically linked depending on the hardware being used (12), or being replaced by hardware-specific mock functions for tests on QEMU.

Documentation:

  • Only the workflow of adding a new platform will be explicitly documented (these stuff on Linux FS on the diagram);
  • All subsystems must have comments for every function, documenting what it is doing and why.
  • Documentation should contain the diagram and a short description of the structure.

Tests:

  • Every tool which is being added into meta-dts explicitly should have two short tests:
    1. To verify that it is preset;
    2. To verify that it is working;
  • Every hardware-specific deploying workflow added to HAL should have a list of needed tests;
  • Every subsystem should be tested separately or by testing use-cases where every system participates.

Where is the value to a user, and who might that user be?

Pros:

  • Highly modular system:
    • Clear subsystems separation: no need to modify several subsystems in case only one subsystem functionality is being extended;
    • Better test coverage: every module can be tested separately enhancing unit-tests, the HAL will make testing on QEMU possible;
  • Separation of hardware-dependent logic from common logic:
    • Easier platform support integration: step-by-step hardware-dependent workflows will be separated from highly non-linear common logic;
  • Test coverage:
    • Easier changes verification
    • This will be a core argument on whether the changes made are working or not (we could ask for tests in every PR, and merge the PR basing on results).

Cons:

  • Adding platform support will require not only extending configuration, but adding mocking functions and tests as well;

Describe alternatives you've considered

No response

Additional context

No response

@DaniilKl DaniilKl added enhancement New feature or request DasharoToolsSuite labels Oct 1, 2024
@artur-rs
Copy link
Member

artur-rs commented Oct 6, 2024

@tym2k1 @m-iwanicki @PLangowski @macpijan marking for review, as agreed on the internal sync

@m-iwanicki
Copy link

@DaniilKl Am I right that Dasharo/dts-scripts#26 partialy addresses this issue?
It adds HAL and splits modules/functions based on functionality?
About coding style/rules those are at least partially forced by pre-commit.
Indentation will be when Dasharo/dts-scripts#36 is merged.

@DaniilKl
Copy link
Contributor Author

DaniilKl commented Dec 6, 2024

Am I right that Dasharo/dts-scripts#26 partialy addresses this issue?
It adds HAL and splits modules/functions based on functionality?

Yep, you are right.

@macpijan
Copy link
Contributor

macpijan commented Dec 6, 2024

No explicit coding rules and style -> the code is difficult to read and modify, the coding style of every contributor overlays with others, and the codebase shortly became messy;

We already have shfmt and shellcheck, right? That should help a bit and be mentioned in the presentation as one of the improvements introduced.

Going further in style topic, I tend to recommend https://google.github.io/styleguide/shellguide.html and been introducing it in a couple of scripts as well.

@macpijan
Copy link
Contributor

macpijan commented Dec 6, 2024

sed (12), or being replaced by hardware-specific mock functions for tests on QEMU.

There is no QEMU on the graphhic, maybe it should be next to the HW?

@macpijan
Copy link
Contributor

macpijan commented Dec 6, 2024

The general architecture makes sense from a high-level perspective.

I think what is still open, is whether (and how far) we want to improve things in the shell scripts, before moving on to another programming language.

To answer that question we would need to break down it into more concrete tasks - what would need to be done, and how much effort is needed to improve the bash solution, and compare it with the tasks breakdown of implementing another solution from scratch in parallel, keeping the current DTS in (ideally) feature-freeze state, mostly adding new firmware versions.

In our last meeting, we have definitely agreed that the priority is mostly on adding functional end-to-end testing, as otherwise introducing any greater changes will lead to a disaster. So one of the main points of the presentation should be about testing: showcase, listing what's been added, and what still can be added.

Ideally if we can reuse (most of) these tests, so we can prove that:

  • the bash solution still works as expected (even if we add new platforms, or redesigning it in bash),
  • the newly implemented solution developed in parallel works with these tests, giving us more control of the flow and higher quality, mainintaing the required feature set.

A general comment: if we have a such a complex proposal, it's rather difficult to review it in the issue. Perhaps RFC PR is a better idea, so we can use the review system.

@macpijan
Copy link
Contributor

macpijan commented Dec 6, 2024

Every tool which is being added into meta-dts explicitly should have two short tests:

That can be added to meta-dts guidelines right now, if not already enforced.

@m-iwanicki
Copy link

We already have shfmt and shellcheck, right? That should help a bit and be mentioned in the presentation as one of the improvements introduced.

currently shellcheck doesn't check info level warnings I think we should check at least Double quote to prevent globbing.
As we are using mostly [ ] and not [[ ]] empty string without "" results in error (or if we had multiple words in variable).
There is also chance of variable containing spaces.

@PLangowski
Copy link

I like this approach. Has anyone started a discussion on what language we will use in the new solution? I think we should conside choosing a compiled language, such as Go for 2 reasons:

  1. Performance will be much better.
  2. The compiler will validate some part of the code during compiling. This will reduce runtime errors, which would appear if we used an interpreted language such as Bash or Python.

@DaniilKl
Copy link
Contributor Author

DaniilKl commented Dec 9, 2024

Yep, GO is planned as a new language.

@PLangowski
Copy link

That's good to hear. I will be more than happy to contribute to the development.

@m-iwanicki
Copy link

@PLangowski

  1. Did we have problems with performance?
  2. We should probably find way to make changes locally for quick development (maybe dev image version with GO compiler and src code?)
    • additionally we would probably need a way to test unreleased binaries (currently I have to disable signature verification as those binaries are not signed)

@PLangowski
Copy link

PLangowski commented Dec 9, 2024

@m-iwanicki The performance is not terrible, but compiling should still speed it up. I agree that introducing local changes will not be as simple as it is now, but we can discuss solutions.
I think that despite the downside it's still worth it, especially considernig my second point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DasharoToolsSuite enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants