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

test/functional: Add flag for container layer paths, remove containerd dependence #1536

Merged
merged 4 commits into from
Nov 15, 2022
Merged

test/functional: Add flag for container layer paths, remove containerd dependence #1536

merged 4 commits into from
Nov 15, 2022

Conversation

helsaawy
Copy link
Contributor

@helsaawy helsaawy commented Oct 6, 2022

Add lcow/wcow layer paths flag to allow providing image layer paths.
Remove dependence on containerd, and instead use github.com/google/go-containerregistry/ (which the dmverity exe uses) to download the images if none are provided.

The goal is to allow functional tests to be run without requiring containerd to be installed.

Signed-off-by: Hamza El-Saawy hamzaelsaawy@microsoft.com

@helsaawy helsaawy marked this pull request as ready for review October 6, 2022 19:59
@helsaawy helsaawy requested a review from a team as a code owner October 6, 2022 20:00
@dcantah
Copy link
Contributor

dcantah commented Oct 6, 2022 •
edited
Loading

I'd add a little blurb in the title stating that this is for tests (e.g. Tests: Add flag for container layer paths). Before clicking on this I was wondering what component this was going to be for 😆

@helsaawy helsaawy changed the title Add flag for container layer paths Add flag for container layer paths to functional tests binary Oct 6, 2022
@helsaawy helsaawy force-pushed the func-layer-path branch 2 times, most recently from e6c1beb to a98c246 Compare October 6, 2022 22:47
@kevpar
Copy link
Member

kevpar commented Oct 6, 2022

I'd add a little blurb in the title stating that this is for tests (e.g. Tests: Add flag for container layer paths). Before clicking on this I was wondering what component this was going to be for 😆

I was coming here to say the same thing. Maybe a test/cri-containerd: or similar prefix would be good.

@kevpar
Copy link
Member

kevpar commented Oct 6, 2022

I'd add a little blurb in the title stating that this is for tests (e.g. Tests: Add flag for container layer paths). Before clicking on this I was wondering what component this was going to be for 😆

I was coming here to say the same thing. Maybe a test/cri-containerd: or similar prefix would be good.

Just noticed it was actually updated already. It's a good practice to use a prefix like I had above to indicate the scope of a change. That makes it easier to tell what a change affects at a glance. So I'd still be in favor of that.

@helsaawy helsaawy changed the title Add flag for container layer paths to functional tests binary test/functional: Add flag for container layer paths, remove containerd dependence Oct 6, 2022
@helsaawy helsaawy added the tests Pull requests that modify tests label Oct 6, 2022
@ambarve
Copy link
Contributor

ambarve commented Oct 6, 2022

Replacing containerd with go-containerregistry also means that we will have to update these tests every time we make changes to the image pull code (like for cimfs). I wonder if we will always remember to change tests in this repo while changing code in containerd repo.
What do others think?

@helsaawy
Copy link
Contributor Author

Replacing containerd with go-containerregistry also means that we will have to update these tests every time we make changes to the image pull code (like for cimfs). I wonder if we will always remember to change tests in this repo while changing code in containerd repo. What do others think?

I think tests would start failing, so we would notice, but that is true.
The code just uses go-containerregistry to grab the OCI tar layers, and then hands off to tar2ext4 or wclayer, so if cimfs (or another differ is added), this would have to be updated.

The alternative is that our functional test, which only test portions code within the shim, rely on containerd to run, so we have to install it to unpack images. I feel like that complicates our testing pipelines

@helsaawy
Add flag for container layer paths
8abd641 
Add lcow/wcow layer paths flag to allow providing image layer paths
instead of requiring containerd to pull and unpack it.

The goal is to allow functional tests to be run without requiring
containerd to be installed.

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
@helsaawy
PR: logging, cleanup, naming.
075b315 
Changed `(LazyImageLayers).ImageLayers` to `(LazyImageLayers).Layers`.
`(LazyImageLayers).Close` now returns an error.
Comment cleanup.

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
@helsaawy
Copy link
Contributor Author

helsaawy commented Nov 1, 2022

rebased for merge conflicts

@helsaawy
PR: replace LayerFolders and docker
cda988d 
Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
@helsaawy helsaawy force-pushed the func-layer-path branch 2 times, most recently from f070297 to 9b65d58 Compare November 1, 2022 19:50
ambarve
ambarve approved these changes Nov 2, 2022
View reviewed changes
Copy link
Contributor

@ambarve ambarve left a comment

Choose a reason for hiding this comment

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

LGTM!

@katiewasnothere
Copy link
Contributor

Why do we want this? Is there some scenario we want to run this where we don't have containerd to connect to?

@helsaawy
Copy link
Contributor Author

helsaawy commented Nov 9, 2022

Why do we want this? Is there some scenario we want to run this where we don't have containerd to connect to?

To decouple our tests from containerd, mostly for CI/CD pipelines.
This way the ADO functional testing and benchmarking pipelines can run the tests without needing to create a new package and install it, which simplifies the pipelines significantly.
We can also theoretically look at running the functional tests here in github (at least, the WCOW process ones) as well ...

katiewasnothere
katiewasnothere approved these changes Nov 15, 2022
View reviewed changes
Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

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

Two comments that may require some additional code to add, but otherwise, LGTM.

@helsaawy
PR: unpack individual LCOW layers, comments
3ab6af7 
Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
@helsaawy helsaawy merged commit 1d141fa into microsoft:main Nov 15, 2022
@helsaawy helsaawy deleted the func-layer-path branch November 15, 2022 02:05
anmaxvl pushed a commit that referenced this pull request Feb 7, 2023
@kiashok
Merged PR 7167661: Update ADO hcsshim from github repo
be52222 
Updating the ADO repo from github hcsshim

Last commit - f83cc58

Related work items: #1536, #1562, #1563, #1564, #1565
helsaawy added a commit to helsaawy/hcsshim that referenced this pull request Sep 20, 2023
@helsaawy
[release/0.9] Backport test/pkg directory
2f99f1c 
Backport testing support functions in the `test/pkg` directory.
This includes changes from:
 - microsoft#1536
 - microsoft#1704
 - microsoft#1853
 - microsoft#1893

Rather than cherry-pick them, only changes to `test/pkg` are included,
since tests themselves will require significant changes to bring
up-to-date.

The goal is to expose testing functions so that tests can be moved out
of the repo.

Updated go version in `test` to 1.8, as required by `test/pkg/flag`.

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
@helsaawy helsaawy mentioned this pull request Sep 20, 2023
Merged
helsaawy added a commit that referenced this pull request Sep 28, 2023
@helsaawy
[release/0.9] Backport test/pkg directory (#1913)
e3d012a 
Backport testing support functions in the `test/pkg` directory.
This includes changes from:
 - #1536
 - #1704
 - #1853
 - #1893

Rather than cherry-pick them, only changes to `test/pkg` are included,
since tests themselves will require significant changes to bring
up-to-date.

The goal is to expose testing functions so that tests can be moved out
of the repo.

Updated go version in `test` to 1.8, as required by `test/pkg/flag`.

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@katiewasnothere katiewasnothere katiewasnothere approved these changes

@ambarve ambarve ambarve approved these changes

Assignees

@katiewasnothere katiewasnothere

@ambarve ambarve

Labels
tests Pull requests that modify tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@helsaawy @dcantah @kevpar @ambarve @katiewasnothere