Review OCI tests #269

yitsushi · 2021-11-17T15:09:56Z

What this PR does / why we need it:

use yitsushi/devmapper-containerd-action@v1
extends imageservice test
- invalid image pull
- double check if volume is mounted
- volume is not mounted test
- kernel volume mount test
full unit test on ImageService

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
fixes #15

Special notes for your reviewer:

That's the maximum I can do with integration tests.
There is one branch I couldn't test with unit test, it would require some magical refactor because I can't fake this private method to return true as it has an internal variable that's updated inside the the same private method.

func snapshotExists(ctx context.Context, key string, ss snapshots.Snapshotter) (bool, error) {
	snapshotExists := false

	err := ss.Walk(ctx, func(walkCtx context.Context, info snapshots.Info) error {
		if info.Name == key {
			snapshotExists = true
		}

		return nil
	})
	if err != nil {
		return false, fmt.Errorf("walking snapshots: %w", err)
	}

	return snapshotExists, nil
}

Checklist:

squashed commits into logical changes
includes documentation
adds unit tests
adds or updates e2e tests

codecov-commenter · 2021-11-17T15:43:50Z

Codecov Report

Merging #269 (d2418be) into main (aa5916e) will increase coverage by 15.17%.
The diff coverage is 45.13%.

@@             Coverage Diff             @@
##             main     #269       +/-   ##
===========================================
+ Coverage   40.29%   55.47%   +15.17%     
===========================================
  Files          46       46               
  Lines        2169     2248       +79     
===========================================
+ Hits          874     1247      +373     
+ Misses       1236      909      -327     
- Partials       59       92       +33

Impacted Files	Coverage Δ
core/application/reconcile.go	`0.00% <0.00%> (ø)`
core/models/vmid.go	`85.36% <0.00%> (-4.88%)`	⬇️
core/models/volumes.go	`0.00% <ø> (ø)`
infrastructure/controllers/microvm_controller.go	`70.00% <0.00%> (ø)`
infrastructure/firecracker/config.go	`0.00% <0.00%> (ø)`
pkg/planner/actuator.go	`83.33% <0.00%> (-2.88%)`	⬇️
core/plans/microvm_create_update.go	`50.00% <50.00%> (-7.54%)`	⬇️
infrastructure/containerd/lease.go	`66.66% <50.00%> (+66.66%)`	⬆️
core/application/commands.go	`72.58% <100.00%> (+0.91%)`	⬆️
core/steps/event/publish.go	`100.00% <100.00%> (ø)`
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78c369b...d2418be. Read the comment docs.

Callisto13

I'm struggling to run these integration tests locally a bit. I'm guessing we need to make sure devpool.sh has run, and that containerd is running, and that the CTR_SOCK_PATH is set. It would be good to document this, or have this setup and teardown automated (the teardown especially can be annoying to remember).

Also when I have that all set up and run make test-with-cov, the tests are still skipped, I think because the tests are executed with sudo in the make target and the env is not preserved (bear in mind that we cannot do sudo make test-with-cov in the workflow file because then they will be run with a different Go version 🤦‍♀️ )

infrastructure/containerd/image_service_integration_test.go

Callisto13 · 2021-11-22T09:58:46Z

Looking at the action logs these integration tests are not running:

    event_service_test.go:25: skipping containerd event service integration test
--- SKIP: TestEventService_Integration (0.00s)
=== RUN   TestImageService_Integration
    image_service_integration_test.go:31: skipping containerd image service integration test

Callisto13 · 2021-11-22T10:14:04Z

Can we add some comments or log lines to each stage of the test, just so that it is easier to tell at a glance what should be happenning (will be useful later when/if something breaks to know exactly what should be going on and if the test is legit doing that). Something like:

// When <condition X> <thing Y> should happen

yitsushi · 2021-11-22T11:06:53Z

Log line sounds a good idea, I'll add them. However I'm bad writing logs like that, I can write a long message or nothing. In this case the best I can do is simple form a sentence from the name of the function like

TestImageService_PullAndMount_failedLease

// to

Failed to PullAndMount because it was not able to reach leases.

Which I think gives not much more help than the name of the function.

Note from Slack: "oh i was talking about the integration tests"

Missing test run, interesting, maybe I don't set an environment variable with my action. (CTR_SOCK_PATH)

No it is there:

      - name: Test with coverage
        run: |
          export CTR_SOCK_PATH=/var/run/containerd/containerd.sock
          make test-with-cov

infrastructure/containerd/image_service_integration_test.go

* use yitsushi/devmapper-containerd-action@v1 * extends imageservice test * invalid image pull * double check if volume is mounted * volume is not mounted test * kernel volume mount test fixes liquidmetal-dev#15

infrastructure/containerd/image_service_integration_test.go

infrastructure/containerd/image_service_test.go

Callisto13 · 2021-11-22T14:10:46Z

infrastructure/containerd/image_service_test.go

+		ImageName: testImage,
+		Owner:     testOwner,
+	})
+	g.Expect(err).To(g.HaveOccurred())


let's check that the error is the one we expect here (ditto all the rest too probably)

I wouldn't go that deep as they can be very ugly by design as we just pass through the error without checking anything on them and at the end is a wrapped error wrapped into a new error that's wrapped into a new error that's wrapped ................. and who knows how deep is that. From the point we call client.Pull the error is basically the same containerd will throw back and we just wrap them with fmt.Errorf(..., err).

That would be cool if we can refactor this code to be less chaotic (and a bit more complex) on error handling, but with the code we have now, I don't think it has any use to test if the error is nope wrapped in N layers.

you can do like a ContainSubstring and look for a certain piece

but yeh LOTS of refactoring would be good in all this area

don't we wrap it? if we hit the failure at that point i would expect to have substring "listing existing containerd leases: nope"

yes we wrap, but we wrap all of them. so it will be always something: something: something: something: nope (and the amount of something changes based on what we called), but 100% it will contain nope.

so we can still do

Suggested change

g.Expect(err).To(g.HaveOccurred())

g.Expect(err).To(g.MatchError(ContainSubstring("listing existing containerd leases: nope")))

to match the tail end of the actual thing which caused the error and the point in our code where we caught the 3rd party fail

(pure personal opinion) In my eyes it falls into the "we can test it for coverage and because looks good, but useless" category.

i mean it doesn;t look great

but seriously 90% of the reviews i have done in my life have been "do we not want to catch that error?" followed by an "oh shit yeh!" response. so checking errors are returned when we expect and in the way we expect them is an unhealthy obsession of mine 😭

infrastructure/containerd/image_service_integration_test.go

infrastructure/containerd/image_service_test.go

Callisto13 · 2021-11-22T14:20:46Z

@yitsushi do you know why we call withOwnerLease on line 65 of PullAndMount and then pass the context into imageExists and then maybe pullImage which then both call withOwnerLease themselves again? Seems a bit redundant?

Co-authored-by: Claudia <[email protected]>

yitsushi · 2021-11-22T14:57:44Z

do you know why we call withOwnerLease on line 65 of PullAndMount and then pass the context into imageExists and then maybe pullImage which then both call withOwnerLease themselves again? Seems a bit redundant?

(pure assumption)
For those separate function, it's because imageExists and pullImage are called from other places like Pull for pullImage or IsMounted for imageExists.
Calling withOwnerLease in PullAndMount, I don't know maybe to make sure we can get the Lease and prevent it to take unnecessary steps. I have no idea.

Callisto13

LGTM, just some final trolls

infrastructure/containerd/image_service_integration_test.go

Callisto13 · 2021-11-22T15:52:25Z

infrastructure/containerd/image_service_integration_test.go

+		Use:          opts.Use,
+		OwnerUsageID: testOwnerUsageID,
+	})
+	Expect(err).ToNot(HaveOccurred())


Suggested change

Expect(err).ToNot(HaveOccurred())

Expect(err).NotTo(HaveOccurred())

But but but but bu but... to not have occurred vs not to have occurred, the first one sounds better to my ears.

honestly they both sound off to me, i have had this discussion like 100 times 😂 i have a vim shortcut which autocompletes to NotTo and I have no idea why

everywhere else (in this file) we have NotTo so for consistency i guess?

flintlock on  15-review-oci-tests via  v1.17.1 ❯ ag 'NotTo' | wc -l 97 flintlock on  15-review-oci-tests via  v1.17.1 ❯ ag 'ToNot' | wc -l 31 flintlock on  15-review-oci-tests via  v1.17.1 ❯ ag -l 'NotTo' core/models/vmid_test.go core/application/app_test.go core/steps/runtime/dir_create_test.go core/plans/microvm_delete_test.go core/plans/microvm_create_update_test.go test/e2e/utils/utils.go test/e2e/utils/runner.go pkg/wait/wait_test.go pkg/log/log_test.go pkg/validation/validate_test.go pkg/planner/actuator_test.go infrastructure/containerd/repo_test.go infrastructure/containerd/image_service_integration_test.go infrastructure/controllers/microvm_controller_test.go infrastructure/network/utils_test.go flintlock on  15-review-oci-tests via  v1.17.1 ❯ ag -l 'ToNot' core/models/vmid_test.go core/steps/runtime/kernel_mount_test.go core/steps/runtime/initrd_mount_test.go core/steps/runtime/repo_release_test.go core/steps/runtime/volume_mount_test.go core/steps/microvm/start_test.go core/steps/microvm/create_test.go core/steps/microvm/delete_test.go core/steps/network/interface_create_test.go core/steps/network/interface_delete_test.go core/steps/event/publish_test.go test/e2e/utils/utils.go test/e2e/e2e_test.go infrastructure/containerd/image_service_test.go infrastructure/containerd/image_service_integration_test.go

it's more like 2/3.

lool and some of those ToNots were mine 🤦‍♀️

Callisto13 · 2021-11-22T15:55:38Z

infrastructure/containerd/image_service_test.go

+		ImageName: testImage,
+		Owner:     testOwner,
+	})
+	g.Expect(err).To(g.HaveOccurred())


i mean it doesn;t look great

but seriously 90% of the reviews i have done in my life have been "do we not want to catch that error?" followed by an "oh shit yeh!" response. so checking errors are returned when we expect and in the way we expect them is an unhealthy obsession of mine 😭

yitsushi changed the title ~~try this~~ OCI tests Nov 17, 2021

yitsushi added area/testing Indicates an issue related to test kind/refactor Only refactoring changes labels Nov 17, 2021

yitsushi force-pushed the 15-review-oci-tests branch 3 times, most recently from d8182ff to 4c54640 Compare November 18, 2021 12:59

yitsushi changed the title ~~OCI tests~~ Review OCI tests Nov 18, 2021

yitsushi force-pushed the 15-review-oci-tests branch 4 times, most recently from 16ad25c to 3a8e5a8 Compare November 19, 2021 15:47

yitsushi marked this pull request as ready for review November 19, 2021 15:47

yitsushi force-pushed the 15-review-oci-tests branch from 3a8e5a8 to b7a52b7 Compare November 19, 2021 16:01

Callisto13 reviewed Nov 22, 2021

View reviewed changes

infrastructure/containerd/image_service_integration_test.go Show resolved Hide resolved

infrastructure/containerd/image_service_integration_test.go Outdated Show resolved Hide resolved

Callisto13 reviewed Nov 22, 2021

View reviewed changes

infrastructure/containerd/image_service_integration_test.go Show resolved Hide resolved

yitsushi added 3 commits November 22, 2021 12:23

Review OCI tests

0a526ce

* use yitsushi/devmapper-containerd-action@v1 * extends imageservice test * invalid image pull * double check if volume is mounted * volume is not mounted test * kernel volume mount test fixes liquidmetal-dev#15

Unit test on ImageService

cd599ee

Use getTestKernelImage and getTestVolumeImage

381f4e7

yitsushi force-pushed the 15-review-oci-tests branch from d8d24fb to 381f4e7 Compare November 22, 2021 11:23

yitsushi added 4 commits November 22, 2021 12:37

preserve CTR_SOCK_PATH EnvVar

870bca0

Add comments on test functions

1ee98e9

Add a nice and friendly comment on ImageService integration test

dd1c789

run tests on push, so we can get a better codecov on merge

113cb40

Callisto13 reviewed Nov 22, 2021

View reviewed changes

infrastructure/containerd/image_service_integration_test.go Outdated Show resolved Hide resolved

infrastructure/containerd/image_service_integration_test.go Show resolved Hide resolved

infrastructure/containerd/image_service_integration_test.go Outdated Show resolved Hide resolved

Callisto13 reviewed Nov 22, 2021

View reviewed changes

infrastructure/containerd/image_service_test.go Show resolved Hide resolved

Callisto13 reviewed Nov 22, 2021

View reviewed changes

yitsushi and others added 2 commits November 22, 2021 15:51

Update infrastructure/containerd/image_service_test.go

eb46100

Co-authored-by: Claudia <[email protected]>

Update infrastructure/containerd/image_service_integration_test.go

d2280a8

Co-authored-by: Claudia <[email protected]>

Add logging in cleanup and remove Chapter IV

d2418be

Callisto13 previously approved these changes Nov 22, 2021

View reviewed changes

ToNot -> NotTo

cf42b48

yitsushi dismissed Callisto13’s stale review via cf42b48 November 22, 2021 16:16

Callisto13 approved these changes Nov 22, 2021

View reviewed changes

yitsushi merged commit cd8062f into liquidmetal-dev:main Nov 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review OCI tests #269

Review OCI tests #269

yitsushi commented Nov 17, 2021 •

edited

Loading

codecov-commenter commented Nov 17, 2021 •

edited

Loading

Callisto13 left a comment •

edited

Loading

Callisto13 commented Nov 22, 2021

Callisto13 commented Nov 22, 2021

yitsushi commented Nov 22, 2021 •

edited

Loading

Callisto13 Nov 22, 2021

yitsushi Nov 22, 2021

yitsushi Nov 22, 2021

Callisto13 Nov 22, 2021

Callisto13 Nov 22, 2021

Callisto13 Nov 22, 2021

yitsushi Nov 22, 2021

Callisto13 Nov 22, 2021 •

edited

Loading

yitsushi Nov 22, 2021 •

edited

Loading

Callisto13 Nov 22, 2021

Callisto13 commented Nov 22, 2021

yitsushi commented Nov 22, 2021

Callisto13 left a comment

Callisto13 Nov 22, 2021

yitsushi Nov 22, 2021

Callisto13 Nov 22, 2021 •

edited

Loading

yitsushi Nov 22, 2021

Callisto13 Nov 22, 2021

Callisto13 Nov 22, 2021

	g.Expect(err).To(g.HaveOccurred())
	g.Expect(err).To(g.MatchError(ContainSubstring("listing existing containerd leases: nope")))

	Expect(err).ToNot(HaveOccurred())
	Expect(err).NotTo(HaveOccurred())

Review OCI tests #269

Review OCI tests #269

Conversation

yitsushi commented Nov 17, 2021 • edited Loading

codecov-commenter commented Nov 17, 2021 • edited Loading

Codecov Report

Callisto13 left a comment • edited Loading

Choose a reason for hiding this comment

Callisto13 commented Nov 22, 2021

Callisto13 commented Nov 22, 2021

yitsushi commented Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Callisto13 Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

yitsushi Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Callisto13 commented Nov 22, 2021

yitsushi commented Nov 22, 2021

Callisto13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Callisto13 Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yitsushi commented Nov 17, 2021 •

edited

Loading

codecov-commenter commented Nov 17, 2021 •

edited

Loading

Callisto13 left a comment •

edited

Loading

yitsushi commented Nov 22, 2021 •

edited

Loading

Callisto13 Nov 22, 2021 •

edited

Loading

yitsushi Nov 22, 2021 •

edited

Loading

Callisto13 Nov 22, 2021 •

edited

Loading