Skip to content

CA retries three times when requesting secret creation.#3909

Merged
myidpt merged 6 commits intoistio:masterfrom
incfly:master
Mar 5, 2018
Merged

CA retries three times when requesting secret creation.#3909
myidpt merged 6 commits intoistio:masterfrom
incfly:master

Conversation

@incfly
Copy link
Copy Markdown

@incfly incfly commented Mar 2, 2018

Also includes the unit test to inject the failure.

fix #3844

@incfly incfly requested review from a team, andraxylia and wattli March 2, 2018 19:55
@istio-merge-robot
Copy link
Copy Markdown

istio-merge-robot commented Mar 2, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: wenchenglu

Assign the PR to them by writing /assign @wenchenglu in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

wattli
wattli approved these changes Mar 2, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@wattli wattli left a comment

Choose a reason for hiding this comment

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

Thanks, a few minor suggestions

security/pkg/pki/ca/controller/secret.go Outdated
Copy link
Copy Markdown
Contributor

@wattli wattli Mar 2, 2018

Choose a reason for hiding this comment

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

Mention this is to reduce transient network issue?

Copy link
Copy Markdown
Author

@incfly incfly Mar 2, 2018

Choose a reason for hiding this comment

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

Done.

security/pkg/pki/ca/controller/secret.go Outdated
Copy link
Copy Markdown
Contributor

@wattli wattli Mar 2, 2018

Choose a reason for hiding this comment

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

Otherwise print out some logs for debugging purpose?

Copy link
Copy Markdown
Author

@incfly incfly Mar 2, 2018

Choose a reason for hiding this comment

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

Good point, done.

myidpt
myidpt reviewed Mar 2, 2018
View reviewed changes
security/pkg/pki/ca/controller/secret.go Outdated
Copy link
Copy Markdown

@myidpt myidpt Mar 2, 2018

Choose a reason for hiding this comment

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

Do you think it's better to add a delay between the retries?

Copy link
Copy Markdown
Author

@incfly incfly Mar 2, 2018

Choose a reason for hiding this comment

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

Changed it to sleep 1 second.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 2, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@be57dc6). Click here to learn what that means.
The diff coverage is 78%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #3909   +/-   ##
========================================
  Coverage          ?     76%           
========================================
  Files             ?     296           
  Lines             ?   27128           
  Branches          ?       0           
========================================
  Hits              ?   20486           
  Misses            ?    5338           
  Partials          ?    1304
Impacted Files Coverage Δ
security/pkg/pki/ca/controller/secret.go 71% <78%> (ø)

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 be57dc6...bc6ee18. Read the comment docs.

@incfly
Copy link
Copy Markdown
Author

incfly commented Mar 2, 2018

/test istio-presubmit

@incfly
Copy link
Copy Markdown
Author

incfly commented Mar 2, 2018 •
edited
Loading

/retest

1 similar comment
@incfly
Copy link
Copy Markdown
Author

incfly commented Mar 2, 2018

/retest

@incfly
Copy link
Copy Markdown
Author

incfly commented Mar 2, 2018

/test istio-presubmit

myidpt
myidpt approved these changes Mar 2, 2018
View reviewed changes
Copy link
Copy Markdown

@myidpt myidpt left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.

@incfly
Copy link
Copy Markdown
Author

incfly commented Mar 5, 2018

/retest

@myidpt
Copy link
Copy Markdown

myidpt commented Mar 5, 2018

Can you rebase to trigger the tests again?

@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Mar 5, 2018

@incfly: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/istio-presubmit.sh bc6ee18 link /test istio-presubmit
Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@myidpt myidpt merged commit 46d8682 into istio:master Mar 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andraxylia andraxylia Awaiting requested review from andraxylia

2 more reviewers

@myidpt myidpt myidpt approved these changes

@wattli wattli wattli approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

istio-ca fails to create secret (timeout from kube-apiserver)

6 participants

@incfly @istio-merge-robot @myidpt @istio-testing @wattli @googlebot