Skip to content

test: retry fqdn requests, increase curl timeout#11775

Merged
aanm merged 1 commit intomasterfrom
pr/increase-curl-timeouts
May 29, 2020
Merged

test: retry fqdn requests, increase curl timeout#11775
aanm merged 1 commit intomasterfrom
pr/increase-curl-timeouts

Conversation

@nebril
Copy link
Copy Markdown
Member

@nebril nebril commented May 29, 2020

No description provided.

@nebril nebril requested a review from a team as a code owner May 29, 2020 15:19
@maintainer-s-little-helper
Copy link
Copy Markdown

Please set the appropriate release note label.

@nebril
Copy link
Copy Markdown
Member Author

nebril commented May 29, 2020

test-focus K8sFQDNTest

@nebril nebril added the release-note/ci This PR makes changes to the CI. label May 29, 2020
Signed-off-by: Maciej Kwiek <maciej@isovalent.com>
@nebril nebril force-pushed the pr/increase-curl-timeouts branch from 5d4b069 to 3d4bb6f Compare May 29, 2020 15:22
@nebril
Copy link
Copy Markdown
Member Author

nebril commented May 29, 2020

test-focus K8sFQDNTest

Comment thread test/helpers/cons.go
// seconds longer than CurlConnectTimeout to provide some time to
// actually transfer data.
CurlMaxTimeout = 8
CurlMaxTimeout = 20
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The timeout is used by CurlFail(), which is sometimes supposed to fail. Won't that increase the waiting time for the tests when we expect the connexion to fail?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@qmonnet this should be handled by CurlConnectTimeout, which is set to 5 (it maps to --connect-timeout flag), so this change shouldn't affect failing connection.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, my bad, all good then!

Copy link
Copy Markdown
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Should we also add --retry 5 to the curl calls here

helpers.CurlFail(world2Target))

and

helpers.CurlFail(world1Target))

? (Sorry cannot comment inline, GH doesn't allow to add review comments to unchanged lines 😢 )

@nebril
Copy link
Copy Markdown
Member Author

nebril commented May 29, 2020

@tklauser these are supposed to fail, so it didn't flake on us (either there was a connectivity issue or Cilium worked as expected and blocked the request). I don't think there's need to add retries there.

@coveralls
Copy link
Copy Markdown

coveralls commented May 29, 2020

Coverage Status

Coverage increased (+0.02%) to 36.864% when pulling 3d4bb6f on pr/increase-curl-timeouts into 9735010 on master.

@tklauser
Copy link
Copy Markdown
Member

@nebril thanks for the explanation, makes sense.

@aanm aanm merged commit e884eae into master May 29, 2020
@aanm aanm deleted the pr/increase-curl-timeouts branch May 29, 2020 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note/ci This PR makes changes to the CI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants