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

DpdkTestpmd: Installation path cleanup #3426

Merged
merged 10 commits into from
Oct 1, 2024

Conversation

mcgov
Copy link
Collaborator

@mcgov mcgov commented Sep 18, 2024

The DPDK installer path has grown to be unmaintainable, this PR series cleans it up using some classes instead of just a bunch of if/else trees. The PR looks scary; but, it is just moving code to the object-oriented replacements. This means when we get to Tool.install() we can just call dpdktestpmd.installer.run_installation()

My plan is to re-use the SourceInstaller and PackageManagerInstaller logic for rdma-core, though this series only implents the classes for DPDK.

For the DPDK and rdma-core installation we have to cover multiple cases:
-install from pkg manager
-install from source

  • install from git
  • install from tar
  • (implicit) use already installed version

And we cover these cases for multiple targets:

  • debian
    • ubuntu 18.04 special casing
  • suse
  • fedora

and now need to cover all of these scenarios for both DPDK and rdma-core.

Changes:

  • Adds a generic class 'Installer' class to dpdk/common.py
  • Adds child class SourceInstall and PackageManagerInstall to dpdk/common.py
  • Adds DpdkSourceInstall and DpdkPackageManagerInstall classes to dpdk/dpdktestpmd.py
  • Adds DpdkGitInstall and DpdkTarInstall implementations of the generic DpdkSourceInstall class to dpdk/dpdktestpmd.py

This required adding some other logic:

  • a Meson tool
  • Updating the python class to allow --user pip installation
  • Ddding the -f flag to the Ln tool.

@mcgov mcgov force-pushed the mcgov/test-install branch 2 times, most recently from b0a06fa to 0eecb81 Compare September 19, 2024 00:24
lisa/tools/python.py Outdated Show resolved Hide resolved
lisa/tools/meson.py Outdated Show resolved Hide resolved
@mcgov mcgov force-pushed the mcgov/test-install branch 5 times, most recently from 5468cae to cebb660 Compare September 19, 2024 16:52
@mcgov
Copy link
Collaborator Author

mcgov commented Sep 19, 2024

Making one more change to make Ninja a tool since it's used by two builds and annoying to install on some systems.

Edit: Change complete! Also fixed a bug when calling sudo ninja install on some systems by enforcing a consistent pythonpath

@mcgov mcgov force-pushed the mcgov/test-install branch 2 times, most recently from de4d216 to ceed53a Compare September 20, 2024 06:01
@mcgov
Copy link
Collaborator Author

mcgov commented Sep 20, 2024

Fixed a rebase issue, should be good to go

@mcgov mcgov marked this pull request as ready for review September 20, 2024 17:38
@squirrelsc
Copy link
Member

I read more on the code and the problem try to solve. It looks a valid scenario to a couple of tools, like some open source. There are some thoughts and draft design.

Thoughts

  • Two things here in different stages. If we split them into the two stages, the inherits chain is clear.
    • Download. The download ways include by wget, git.
    • Install. The install ways include copy, package, and build.
  • Reinstall. There may be different reason to reinstall the tool. For example, upgrade version, or build new temp version (xdp). It needs to define reinstall strategy in each tool.
  • Dependencies. Different distro has different dependent packages, or even extra commands to install correctly.
  • The composition of Downloader and Installer are implemented in the corresponding tool level. It can be detected by passed in parameters, os versions, etc.
  • It doesn't need to define methods to implement by subclass and subclass of subclass separately. The subclasses can override the same method in superclass, and call the method of superclass in the overridden version. It provides enough flexibility for most cases.
  • Don't inherits from two different superclasses. It's easy to cause issue. The workaround is either composite them, or use Mixin pattern to minimize conflicts.

Design

class Downloader:
  # base class of download. Due to different parameters, so use a class instead of a method.
  def __init__(self, **kwargs: Any) -> None:
    ...
  def download(self) -> path.PurePath:
    # download code including with wget or git
    raise NotImplementError()

class GitDownloader(Downloader):
  def __init__(self, repo: str, ref: str, **kwargs: Any) -> None:
    ...
  def download(self) -> path.PurePath:
    # imp git clone and checkout, if needed
    ...
class FileDownloader(Downloader):
  def __init__(self, url: str, **kwargs: Any) -> None:
    ...
  def download(self) -> path.PurePath:
    # imp wget download and return root path.
    ...
class TarFileDownloader(FileDownloader):
  def download(self) -> path.PurePath:
    path = super().download()
    # extra tar file and return the path
    ...


class Installer:
  def __init__(self, downloader: Downloader, **kwargs: Any) -> None:
    # downloader is initialized in corresponding tools by os version, passed in parameters etc.
    # kwargs can be passed through to subclasses
    ...
  def install(self) -> None:
    # implemented here to include reinstall for some reasons, like version mismatches.
    # below is the skeleton code, more error handling may be added.
    if self.need_install():
        self.downloader.download()
        self.uninstall()
        self._install()
  def uninstall(self) -> None:
    raise NotImplementedError()
  def need_install(self) -> bool:
    raise NotImplementedError()
  def _install(self) -> None:
    raise NotImplementedError()

class BinaryInstaller(Installer):
  # install by cp
   ...
class PackageInstaller(Installer):
  # install by packages
   ...
class SourceInstaller(Installer):
  # install by building binary.
   ...

@mcgov
Copy link
Collaborator Author

mcgov commented Sep 20, 2024

There are some thoughts and draft design.

Thanks @squirrelsc. I like the mix-in idea! I was trying to implement something like that but didn't know the name of it.

I'll implement that in the next PR.

@mcgov mcgov force-pushed the mcgov/test-install branch 2 times, most recently from 485127a to 37949ca Compare September 24, 2024 19:15
The installer path has grown to be unmaintainable.
We cover multiple cases:
-install from pkg manager
-install from source
   - install from git
   - install from tar
- (implicit) use already installed version

for multiple targets:
- debian
   - ubuntu 18.04 special casing
- suse
- fedora

and now need to allow all of these scenarios
for both DPDK and rdma-core (and the linux kernel).

SO... It's time for a refactor.

Commit makes the 'installer' concept object-oriented.

Adds a generic class 'Installer' into common.py
Adds child class SourceInstall and PackageManagerInstall.
Adds DpdkSourceInstall and DpdkPackageManagerInstall classes.
Adds DpdkGitInstall and DpdkTarInstall implementations of the generic
parent classes.
@mcgov
Copy link
Collaborator Author

mcgov commented Sep 25, 2024

@squirrelsc could I get one more look before Lili signs on 🥺🙏

@mcgov
Copy link
Collaborator Author

mcgov commented Sep 27, 2024

@LiliDeng can you give a final review once Chi resolves his comments? Please feel free to merge once you approve.

@mcgov
Copy link
Collaborator Author

mcgov commented Sep 30, 2024

@squirrelsc
last call on comments, anything else?

@squirrelsc
Copy link
Member

@squirrelsc last call on comments, anything else?

No others except one more polish. Please wait Lili for merging.

@LiliDeng LGTM.

@mcgov
Copy link
Collaborator Author

mcgov commented Oct 1, 2024

@squirrelsc Lili is OOF for the week.

@squirrelsc
Copy link
Member

@squirrelsc Lili is OOF for the week.

Please share test results on major versions of Ubuntu, SUSE, RedHat, I can merge code, if no regression.

@squirrelsc squirrelsc merged commit e9519d8 into microsoft:main Oct 1, 2024
45 checks passed
@squirrelsc
Copy link
Member

Checked test results, no failure happens on major distro with 3 typical dpdk test cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants