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

Move udev discovery handler from core Akri repo #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kate-goldenring
Copy link
Contributor

No description provided.

@kate-goldenring kate-goldenring force-pushed the initial-migration branch 3 times, most recently from 42b9780 to 30c3f67 Compare November 16, 2023 02:09
@kate-goldenring
Copy link
Contributor Author

@diconico07 I think I am going to need your help on how to configure just running the udev e2e test. It looks like conftest in not currently configured to run it

Copy link

@diconico07 diconico07 left a comment

Choose a reason for hiding this comment

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

I did not check the Rust code is the same, I only focused on build and test flow.

To answer your question for pytest, it should run all test_*.py files by default, so as the udev one is the only one here, it should work.

I put a lot of redundant suggestions, sorry for the amount of "comments" it creates 😇

Makefile Outdated Show resolved Hide resolved
.github/workflows/build-container.yml Outdated Show resolved Hide resolved
.github/workflows/build-container.yml Outdated Show resolved Hide resolved
.github/workflows/build-container.yml Outdated Show resolved Hide resolved
.github/workflows/build-container.yml Outdated Show resolved Hide resolved
build/Dockerfile.rust Outdated Show resolved Hide resolved
build/Dockerfile.rust Outdated Show resolved Hide resolved
build/Dockerfile.rust Outdated Show resolved Hide resolved
e2e/conftest.py Outdated Show resolved Hide resolved
e2e/conftest.py Outdated
Comment on lines 71 to 81
if pytestconfig.getoption("--use-local"):
local_tag = pytestconfig.getoption("--local-tag", "pr")
helm_install_command.extend(
[
Path(__file__).parent / "../../deployment/helm",
"--set",
"udev.image.pullPolicy=Never,"
f"udev.image.tag={local_tag}",
]
)
else:

Choose a reason for hiding this comment

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

As the helm chart is local there is no need for a difference in behavior here (the udev discovery handler image pull policy gets handled later on in the for discovery handler loop)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally we would make as minimal changes to this file as possible and pull it out into its own repository/library, but maybe we consider that next?

Choose a reason for hiding this comment

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

A longer term target could be to have the tests run as a helm test suite, embedded in the chart, this way running the tests for the enabled discovery handler would just be a matter of installing the chart and running helm test.

But this will always be difficult to correctly E2E test things on different repositories, as for example a change to the agent is likely to impact the tests for DHs (so an agent's PR would need to modify every DH test suite for these to pass), and a change to the DH is likely to require modifications to its test suite as well.

@kate-goldenring kate-goldenring force-pushed the initial-migration branch 4 times, most recently from 100ec88 to 59dff0d Compare November 17, 2023 23:36
@kate-goldenring
Copy link
Contributor Author

kate-goldenring commented Nov 17, 2023

@diconico07 do you have recommendations for running the e2e tests locally? My steps are as follows but i am hanging on poetry install --no-root. I'm on Ubuntu 22.04.3 LTS.

# The following hangs on pending deps. I also tried deleting the poetry.lock but gave up after 15 minutes of installing
$ poetry install --no-root
Installing dependencies from lock file

Package operations: 7 installs, 9 updates, 0 removals

  • Updating certifi (2023.7.22 -> 2023.5.7): Pending...
  • Updating charset-normalizer (3.3.2 -> 3.1.0): Pending...
  • Updating urllib3 (1.26.18 -> 2.0.3): Pending...

The next steps would be

# Build the container locally and load it into docker
$ PREFIX=[ghcr.io/project-akri/akri](http://ghcr.io/project-akri/akri) LABEL_PREFIX=pr LOAD=1 make
# Run specifying to pull from local
$ poetry run pytest -v --distribution k3s --test-version $(cat ../version.txt) —use-local

@diconico07
Copy link

@diconico07 do you have recommendations for running the e2e tests locally? My steps are as follows but i am hanging on poetry install --no-root. I'm on Ubuntu 22.04.3 LTS.

# The following hangs on pending deps. I also tried deleting the poetry.lock but gave up after 15 minutes of installing
$ poetry install --no-root
Installing dependencies from lock file

Package operations: 7 installs, 9 updates, 0 removals

  • Updating certifi (2023.7.22 -> 2023.5.7): Pending...
  • Updating charset-normalizer (3.3.2 -> 3.1.0): Pending...
  • Updating urllib3 (1.26.18 -> 2.0.3): Pending...

This looks like a cache issue I already encountered once in the past, you can try to do poetry cache clean --all for the PyPI and _default_cache caches, it should help with that issue.

The next steps would be

# Build the container locally and load it into docker
$ PREFIX=[ghcr.io/project-akri/akri](http://ghcr.io/project-akri/akri) LABEL_PREFIX=pr LOAD=1 make
# Run specifying to pull from local
$ poetry run pytest -v --distribution k3s --test-version $(cat ../version.txt) —use-local

Don't forget to import the images into your k3s, as the make will only load them into your docker, not into the k3s containerd.

@kate-goldenring
Copy link
Contributor Author

Tests were failing due to an agent/controller image not being available for the given version. I changed the tests to use latest prod release agent and controller instead of latest dev release

@kate-goldenring
Copy link
Contributor Author

@diconico07 I think this is ready for another round of review

Copy link

@diconico07 diconico07 left a comment

Choose a reason for hiding this comment

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

Look good to me, the only thing preventing me from just approving are the mistakenly committed files

_scratch/rune2e.md Outdated Show resolved Hide resolved
build/setup.sh Outdated Show resolved Hide resolved
udev-discovery.tar Outdated Show resolved Hide resolved
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.

2 participants