Alternate, Simplified Async Pull #137

imuni4fun · 2024-02-05T20:41:32Z

This PR implements an alternative, simplified approach for sync and async image pulling of images. It leverages the previous, synchronous code and behavior as much as possible.

Effectively, only the actual pull activity is affected. Before v0.7.0, the pull activity was entirely synchronous and a timeout meant the pull was terminated. Now, the same flow is preserved except that after the existing logic is evaluated and a decision to pull is made, a check is done to see if if the pull is already occurring because it was previously tried and timed out.

Many factors complicate the simple timeout-based cancellation that this asynchronous pull feature addresses. Proper modularity allows for more robust and simple tests of individual logic. The changes to existing code are very minimal. The new behavior is implemented in a modular way in new files (a new package). The behavior is more easily tested because more of the system can be mocked.

For another comparison which does not have to back out as many new changes, see https://github.com/warm-metal/container-image-csi-driver/compare/v0.7.0...imuni4fun:container-image-csi-driver:async-pull-simplified-diff?expand=1
That comparison is missing a few minor tweaks but it illustrates the minimalism of the change to existing code.

Created issue #147 per request in community standup. It shares a little more background on this PR.

imuni4fun · 2024-02-05T20:59:37Z

Note that I still need to update logging to use the chosen logger rather than fmt.Printf() which I had used in my linked example for the pattern. Logging has also changed since the initial integration that I did pre-v0.7.0. Everything other than a few logging lines are ready for evaluation.

imuni4fun · 2024-02-06T03:17:52Z

logging is now updated to use klog. verbose logging of internals of the AsyncPuller can be enabled by increasing log verbosity, though i was having trouble trying to pass this through go test to get those higher verbosity outputs.

imuni4fun · 2024-02-06T03:50:26Z

go test -count=1 -v ./pkg/remoteimageasync/...
=== RUN TestPullDuration
pullerMock: starting to pull image nginx:6
pullerMock: starting to pull image nginx:2
pullerMock: starting to pull image nginx:7
pullerMock: starting to pull image nginx:1
pullerMock: starting to pull image nginx:4
pullerMock: starting to pull image nginx:3
pullerMock: starting to pull image nginx:8
pullerMock: finshed pulling image nginx:1
pullerMock: finshed pulling image nginx:2
pullerMock: finshed pulling image nginx:3
pullerMock: finshed pulling image nginx:4
pullerMock: context cancelled
pullerMock: context cancelled
pullerMock: context cancelled
--- PASS: TestPullDuration (5.00s)
=== RUN TestParallelPull
pullerMock: starting to pull image nginx:2
pullerMock: finshed pulling image nginx:2
--- PASS: TestParallelPull (3.00s)
=== RUN TestSerialResumedSessions
pullerMock: starting to pull image nginx:A
pullerMock: finshed pulling image nginx:A
--- PASS: TestSerialResumedSessions (6.00s)
=== RUN TestParallelResumedSessions
pullerMock: starting to pull image nginx:A
pullerMock: finshed pulling image nginx:A
--- PASS: TestParallelResumedSessions (6.00s)
PASS
ok github.com/warm-metal/container-image-csi-driver/pkg/remoteimageasync 20.607s

mugdha-adhav · 2024-02-08T11:21:24Z

Node plugin pod seems to be crashing due to -

container-image-csi-driver-warm-metal-csi-driver-nodeplugizd67g csi-plugin I0208 11:19:14.556750       1 provider.go:82] Docker config file not found: couldn't find valid .dockercfg after checking in [  /root /]
container-image-csi-driver-warm-metal-csi-driver-nodeplugizd67g csi-plugin panic: runtime error: invalid memory address or nil pointer dereference
container-image-csi-driver-warm-metal-csi-driver-nodeplugizd67g csi-plugin [signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x19f7b2c]
container-image-csi-driver-warm-metal-csi-driver-nodeplugizd67g csi-plugin
container-image-csi-driver-warm-metal-csi-driver-nodeplugizd67g csi-plugin goroutine 44 [running]:
container-image-csi-driver-warm-metal-csi-driver-nodeplugizd67g csi-plugin github.com/warm-metal/container-image-csi-driver/pkg/remoteimage.puller.Pull({{0x0, 0x0}, {0x22a1800, 0x400017c8c0}, {0x2299df8, 0x40007317a0}}, {0x22b3d90, 0x4000a25770})
container-image-csi-driver-warm-metal-csi-driver-nodeplugizd67g csi-plugin 	/go/src/container-image-csi-driver/pkg/remoteimage/pull.go:47 +0x13c

vadasambar · 2024-02-09T13:25:01Z

cmd/plugin/main.go

@@ -51,8 +51,8 @@ var (
 	enableCache = flag.Bool("enable-daemon-image-credential-cache", true,
 		"Whether to save contents of imagepullsecrets of the daemon ServiceAccount in memory. "+
 			"If set to false, secrets will be fetched from the API server on every image pull.")
-	asyncImagePullMount = flag.Bool("async-pull-mount", false,
-		"Whether to pull images asynchronously (helps prevent timeout for larger images)")
+	asyncImagePullTimeout = flag.Duration("async-pull-timeout", 0,


…act that pull was not occurring.

…et.yaml to load multiple jobs in parallel

…c pull logic functions properly via fully integrated kind test

pkg/remoteimageasync/puller.go

…er result type checks with method calls since i felt this was becoming too rigid... especially when considering logging

imuni4fun · 2024-02-22T20:01:01Z

@mugdha-adhav could you kick integration tests again? thanks

…not updated to remove completionChan depth and the build targets still succeeded. should have failed integration tests if executed.

vadasambar · 2024-02-23T13:44:06Z

pkg/remoteimageasync/synchronizer_test.go

+	"github.com/stretchr/testify/assert"
+)
+
+type chanTestStruct struct {


Can we use the actual session object here?

vadasambar · 2024-02-23T13:50:22Z

pkg/remoteimageasync/synchronizer_test.go

+}
+
+// demonstrates session channel structure's pass-by-reference is appropriate
+func TestChannelStructContent(t *testing.T) {


Same comment as above. If these tests are not directly or indirectly related by code to synchronizer, I think we should put it in a separate file.

vadasambar · 2024-02-23T15:03:42Z

Overall LGTM provided we

address Alternate, Simplified Async Pull #137 (comment)
address Alternate, Simplified Async Pull #137 (comment)
add tests for NodePublishVolume (in node_server_test.go)

imuni4fun · 2024-02-26T13:34:52Z

summary of discussion with @vadasambar:

I agree that it would be good to build out additional regression tests at higher layers of the onion... in this case at the call to PublishNodeVolume() with mocked containerd being called over GRPC.

I believe this additional testing does not exercise additional code paths beyond one case where there is interplay between caller timeout (kubelet) and actual pull time when pull policy is always. In this case, an image pull that takes too long to pull and exceeds the 2m caller timeout but ALSO completes the pull BEFORE the next call to continue (completes during backoff waiting period) will start a new pull on next call. The image in this scenario will never succeed. Because it would never succeed previously, and just timeout and cancel, I consider this the same behavior.

The above scenario tests the pre-existing pull-always logic in a new context where now many more scenarios CAN succeed. Anything that cannot succeed after the change would also have not succeeded prior.

I am happy to grow the regression tests in a future change but don't think adding these new layers should block this feature.

imuni4fun · 2024-02-27T14:43:43Z

@vadasambar @mugdha-adhav could you review? thx

imuni4fun · 2024-02-27T22:14:00Z

@mugdha-adhav created meeting to wrap up tomorrow

could you kick off integration tests again? it looks like it might need another run but it’s not clear from my end

last commit was just comments. previous commit had passing integration tests

imuni4fun · 2024-02-27T22:17:52Z

Also why did we remove the metrics test?

I didn’t remove the test. I removed the “rendered” version which is rendered from https://github.com/imuni4fun/container-image-csi-driver/blob/4c6d7160fa72408225477aad4b061765753095e8/test/integration/metrics-manifests/test.yaml#L1

rendered test changes each time and should be ignored. i added it to .gitignore file in different PR/issue.
☝️ https://github.com/warm-metal/container-image-csi-driver/pull/148/files#diff-bc37d034bad564583790a46f19d807abfe519c5671395fd494d8cce506c42947

mugdha-adhav · 2024-02-26T13:26:06Z

charts/warm-metal-csi-driver/templates/nodeplugin.yaml

-            {{- if .Values.enableAsyncPullMount }}
-            - --async-pull-mount=true
+            {{- if .Values.enableAsyncPull }}
+            - --async-pull-timeout=10m


Could we pass this timeout value from helm values?

After discussing with @imuni4fun, we decided that we want to keep the enableAsyncPull and adding default timeout value to be passed in from values.yaml. Something like --async-pull-timeout={{ .Values.asyncPullTimeout }}

…er.Named struct

pkg/remoteimage/pull.go

…ller getter method Puller.ImageWithTag()

imuni4fun · 2024-02-29T21:36:47Z

@mugdha-adhav pushed changes called out in review plus from @woehrl01 above. also one small replacement of docker.Named.String() with Puller.ImageWithTag()

ephemeral-volumet-set test passed in local kind cluster

ready for last integration tests and merge

imuni4fun added 2 commits February 5, 2024 14:45

rebased updated async implementation onto main, review "//TODO:" entries

941d2ee

simplified RunCompletionChecker() loop exit

cfc23bd

imuni4fun requested a review from a team as a code owner February 5, 2024 20:41

updated logging

c98e7d6

imuni4fun added 2 commits February 6, 2024 10:52

updated chart to use new flag

b9b5962

removed previous async files

d65aef7

mbtamuli mentioned this pull request Feb 7, 2024

Update CI tests to have better logging #138

Closed

vadasambar reviewed Feb 9, 2024

View reviewed changes

imuni4fun added 7 commits February 9, 2024 08:26

changed to sync from async for this test which may match original intent

b6b702d

rebased updated async implementation onto main, review "//TODO:" entries

7a688f2

simplified RunCompletionChecker() loop exit

c497347

updated logging

75b2517

updated chart to use new flag

8374809

removed previous async files

6ff665b

changed to sync from async for this test which may match original intent

fbd31d5

mugdha-adhav force-pushed the async-pull-jk branch from b6b702d to fbd31d5 Compare February 9, 2024 14:21

imuni4fun added 10 commits February 9, 2024 13:54

added async puller startup log messages

c206d9d

fixed omission of setting imageSvc

fdef232

updated ephemeral volume yaml to always pull. not doing this masked f…

22b486f

…act that pull was not occurring.

added initial kind doc and Makefile targets, added ephemeral-volume-s…

d805aeb

…et.yaml to load multiple jobs in parallel

updated doc

7a7b0f0

added final steps to fully flush the ephemeral volume and verify asyn…

1aece15

…c pull logic functions properly via fully integrated kind test

Merge remote-tracking branch 'fork/async-pull-jk' into async-pull-jk

6974562

resolved session state bug, reintroduced metrics, updated metrics test

a937ec6

fixed elapsed calc

89adc0e

added removal of pull time info after 1 minute expiration

d31d47d

imuni4fun added 2 commits February 21, 2024 14:31

reverted Makefile change... this was extracted to another PR

88f6a39

added integration test for async pull feature

62fc0d0

vadasambar reviewed Feb 22, 2024

View reviewed changes

pkg/remoteimageasync/puller.go Show resolved Hide resolved

imuni4fun added 4 commits February 22, 2024 11:38

removed missed status definitions from previous async implementation

1879256

replaced completionChan and loop with completionFunc

48fcd61

removed isComplete, did not make changes to replace isTimeout and oth…

17c1c87

…er result type checks with method calls since i felt this was becoming too rigid... especially when considering logging

updated timeout to be more verbose

8b54915

fixed bug where one call to StartAsyncPuller() in node_server.go was …

b5d6c03

…not updated to remove completionChan depth and the build targets still succeeded. should have failed integration tests if executed.

vadasambar reviewed Feb 23, 2024

View reviewed changes

implemented test organization improvements from discussion

b824954

captured comment updates

4c6d716

mugdha-adhav reviewed Feb 28, 2024

View reviewed changes

imuni4fun added 3 commits February 28, 2024 09:40

implemented change for exposing Image() on PullSession

a75a0be

added configurability of asyncPullTimeout to helm deployment yaml

6f13aba

fixed buG resulting from uniform extraction of image string from dock…

f3cd52d

…er.Named struct

mugdha-adhav reviewed Feb 29, 2024

View reviewed changes

pkg/remoteimage/pull.go Outdated Show resolved Hide resolved

woehrl01 mentioned this pull request Feb 29, 2024

Volumes are suddenly empty #151

Open

woehrl01 reviewed Feb 29, 2024

View reviewed changes

pkg/remoteimage/pull.go Outdated Show resolved Hide resolved

addressed two comments, replaced use of docker.Named.String() with Pu…

6c28744

…ller getter method Puller.ImageWithTag()

mugdha-adhav approved these changes Mar 4, 2024

View reviewed changes

Updating Chart.yaml and Makefile

2ec4f6f

mugdha-adhav merged commit ade923f into warm-metal:main Mar 4, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alternate, Simplified Async Pull #137

Alternate, Simplified Async Pull #137

imuni4fun commented Feb 5, 2024 •

edited

Loading

imuni4fun commented Feb 5, 2024

imuni4fun commented Feb 6, 2024

imuni4fun commented Feb 6, 2024

mugdha-adhav commented Feb 8, 2024 •

edited

Loading

vadasambar Feb 9, 2024

imuni4fun commented Feb 22, 2024

vadasambar Feb 23, 2024

vadasambar Feb 23, 2024

vadasambar commented Feb 23, 2024

imuni4fun commented Feb 26, 2024 •

edited

Loading

imuni4fun commented Feb 27, 2024

imuni4fun commented Feb 27, 2024

imuni4fun commented Feb 27, 2024 •

edited

Loading

mugdha-adhav Feb 26, 2024

mugdha-adhav Feb 28, 2024

imuni4fun commented Feb 29, 2024 •

edited

Loading

Alternate, Simplified Async Pull #137

Alternate, Simplified Async Pull #137

Conversation

imuni4fun commented Feb 5, 2024 • edited Loading

imuni4fun commented Feb 5, 2024

imuni4fun commented Feb 6, 2024

imuni4fun commented Feb 6, 2024

mugdha-adhav commented Feb 8, 2024 • edited Loading

vadasambar Feb 9, 2024

Choose a reason for hiding this comment

imuni4fun commented Feb 22, 2024

vadasambar Feb 23, 2024

Choose a reason for hiding this comment

vadasambar Feb 23, 2024

Choose a reason for hiding this comment

vadasambar commented Feb 23, 2024

imuni4fun commented Feb 26, 2024 • edited Loading

imuni4fun commented Feb 27, 2024

imuni4fun commented Feb 27, 2024

imuni4fun commented Feb 27, 2024 • edited Loading

mugdha-adhav Feb 26, 2024

Choose a reason for hiding this comment

mugdha-adhav Feb 28, 2024

Choose a reason for hiding this comment

imuni4fun commented Feb 29, 2024 • edited Loading

imuni4fun commented Feb 5, 2024 •

edited

Loading

mugdha-adhav commented Feb 8, 2024 •

edited

Loading

imuni4fun commented Feb 26, 2024 •

edited

Loading

imuni4fun commented Feb 27, 2024 •

edited

Loading

imuni4fun commented Feb 29, 2024 •

edited

Loading