Skip to content

Conversation

snarlysodboxer
Copy link
Contributor

See #1776.

Not sure if this is how you would want this implemented, but it didn't take long so I thought I'd get the conversation going. It's unclear to me how retries here might play into retries at higher levels.

Should the interval or timeout be configurable? Should it retry indefinitely?

@FxKu FxKu added this to the 1.8 milestone Feb 10, 2022
@snarlysodboxer
Copy link
Contributor Author

I see the e2e failure. Wondering if it's flakiness? The error doesn't seem related to my changes, but I could be wrong...

@FxKu
Copy link
Member

FxKu commented Feb 11, 2022

LGTM. Thanks. Yeah e2e tests on github are very flaky. You can ignore them.

@snarlysodboxer snarlysodboxer force-pushed the retry-get-secret branch 2 times, most recently from 2eea279 to 6246eb5 Compare February 17, 2022 18:01
@FxKu
Copy link
Member

FxKu commented Feb 18, 2022

Ok, one unit test is failing because the intervals must be set for it. Please add the following two lines (I mark then with +):

				Resources: config.Resources{
					PodEnvironmentSecret:  "idonotexist",
+					ResourceCheckInterval: time.Duration(3),
+					ResourceCheckTimeout:  time.Duration(10),
				},

here and here.

@snarlysodboxer
Copy link
Contributor Author

Ok, one unit test is failing because the intervals must be set for it. Please add the following two lines

Done.

I had a lot of trouble trying to run the tests. I couldn't get them to pass even in the master branch. GO111MODULE=on go test ./... returns undefined: mocks.NewMockVolumeResizer errors.

@snarlysodboxer snarlysodboxer force-pushed the retry-get-secret branch 2 times, most recently from f0f432b to c88aac5 Compare February 22, 2022 01:52
@snarlysodboxer
Copy link
Contributor Author

I was able to get the unit tests running after some trouble. - I added to the docs/developers.md to hopefully help others avoid the problems I ran into.

ResourceCheckTimeout: time.Duration(10),
},
},
err: fmt.Errorf(`could not read Secret PodEnvironmentSecretName: still failing after 3 retries: secret.core "idonotexist" not found`),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
err: fmt.Errorf(`could not read Secret PodEnvironmentSecretName: still failing after 3 retries: secret.core "idonotexist" not found`),
err: fmt.Errorf("could not read Secret PodEnvironmentSecretName: still failing after 3 retries: secret.core %q not found", "idonotexist"),

Copy link
Member

Choose a reason for hiding this comment

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

Would make sense to have a constant for the name, check #1794

Copy link
Contributor

Choose a reason for hiding this comment

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

Also would be nice to don't hardcode 3 as its 10/3, i.e. ResourceCheckTimeout/ResourceCheckInterval

Copy link
Contributor Author

@snarlysodboxer snarlysodboxer Feb 28, 2022

Choose a reason for hiding this comment

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

I've added this commit, is that what you were looking for?

@FxKu
Copy link
Member

FxKu commented Mar 1, 2022

👍

1 similar comment
@jopadi
Copy link
Member

jopadi commented Mar 1, 2022

👍

@FxKu FxKu merged commit ca0c27a into zalando:master Mar 1, 2022
@FxKu
Copy link
Member

FxKu commented Mar 1, 2022

Thanks @snarlysodboxer for your contribution!

@snarlysodboxer snarlysodboxer deleted the retry-get-secret branch March 1, 2022 17:11
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.

4 participants