stage0: prevent skipping some images in image gc

[sanicheev]

…ion is unable to process whole image tree

[iaguis]

About the first break, we're iterating over a slice that was obtained by calling

aciinfos, err := s.GetAllACIInfos([]string{"lastused"}, true)

This means the images will be sorted by lastused in ascending order so when we reach the grace period, we should break to avoid unnecessary processing.

About the second break, it seems like a bug, images can be in the running set even if they're older than the grace period so we might miss some of them.

[sanicheev]

@iaguis, i think 1st one should be using break and the 2nd one should be using continue. Should i adjust my PR?

[sanicheev]

Looks like semaphoreci has failed with an error: no disk space left on device. Build should be stable.

[iaguis]

This looks good. Do you mind adjusting the commit message to follow our commit convention?

It can start like this:

rkt: prevent skipping some images in image gc

We were using break when we found an image with a pod in running state...

Thanks a lot for the contribution!

[iaguis]

It'd be great if you could write a functional test for this. It's a more challenging task but I can try to guide you through it if you want to give it a try.

[lucab]

@sanicheev don't worry too much about temporary flakes, I re-triggered semaphore and it's green now.

[sanicheev]

@iaguis i'll try. is it ok if i'll create 'getRunningImages' test function which will return fake data?( i couldn't find if it's already exists)

[sanicheev]

guys give me some time plz to adjust my commit message and to write proper unit tests.

[iaguis]

I had in mind adding a functional test to https://github.com/rkt/rkt/blob/master/tests/rkt_image_gc_test.go but maybe a unit test is enough to cover this change. In that case a mock getRunningImages is fine.

[iaguis]

guys give me some time plz to adjust my commit message and to write proper unit tests.

No hurries :)

[sanicheev]

@iaguis , almost done. just not sure how to populate acinfo DB properly.

[iaguis]

@sanicheev any chance you can finish this test?

[sanicheev]

Sorry for a delay. Yes i'll push it during this weekend.

[sanicheev]

@iaguis am i going the right path here?(my go background is not strong enough at the moment and i want to fix that)

Because i'm thinking to use database/sql package functionality instead of hardcoding image info in a file. And it might be a good idea to put it under testutils/populateimagestore.go file and then call it during the test.

[sanicheev]

@iaguis , @lucab guys is everything ok with this unit test or should it be adjusted as i suggested?

[iaguis]

Sorry for the late reply.

What you did was copying the function logic to the test function, substituting the pieces that interact with the store with mock functions.

The mocking part is fine, what's problematic is copying the code to the test function. Keep in mind that unit tests need to test the actual functions that are present in the code, so that when the function changes, we can detect problems. The way you did it, you'd have to remember to change the code in the test when you change the function in the code.

What I'd do is extract the loop to its own function in rkt's code and then test that function. In this way you can keep using the mock functions you added in the test and you'll be testing the actual rkt code.

[sanicheev]

@iaguis, thank you i'll try to adjust