Skip to content

Comments

Parameterize startup CRD wait retries#777

Merged
k8s-ci-robot merged 1 commit intokubernetes-csi:masterfrom
mattcary:wait
Nov 4, 2022
Merged

Parameterize startup CRD wait retries#777
k8s-ci-robot merged 1 commit intokubernetes-csi:masterfrom
mattcary:wait

Conversation

@mattcary
Copy link
Contributor

@mattcary mattcary commented Nov 3, 2022

Add a command line flag (retry-crd-interval-max) for the maximum interval to wait for CRDs to appear.

This allows providers to avoid unnecessary crashlooping on startup when it is not unusual for CRDs to take longer than 7 seconds to appear.

/kind feature

Add --retry-crd-interval-max flag to the snapshot-controller in order to allow customization of CRD detection on startup.

/assign @msau42

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Nov 3, 2022
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 3, 2022
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 3, 2022
}

// with a Factor of 1.5 we wait up to 7.5 seconds (the 10th attempt)
maxMs := retryCRDIntervalMax.Milliseconds()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add new comments here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// with a Factor of 1.5 we wait up to 7.5 seconds (the 10th attempt)
maxMs := retryCRDIntervalMax.Milliseconds()
if maxMs < 100 {
maxMs = 100
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the relationship between this 100 ms and Duration of 100 * time.Millisecond on line 115?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explained in the comment, LMK if it's not clear.

if maxMs < 100 {
maxMs = 100
}
steps := int(math.Ceil(math.Log(float64(maxMs)/100) / math.Log(1.5)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This 1.5 here is the same as Factor 1.5 on line 116? Can you use the same parameter if they are the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, explained in the comment.

@mattcary
Copy link
Contributor Author

mattcary commented Nov 4, 2022

Thanks @xing-yang , lmk if it's still unclear.

@mattcary mattcary force-pushed the wait branch 2 times, most recently from 28b1548 to 120721e Compare November 4, 2022 00:17
@xing-yang
Copy link
Collaborator

In the release note, can you add "in the snapshot-controller" at the end?

Can you also document this new flag in README? https://github.com/kubernetes-csi/external-snapshotter#snapshot-controller-command-line-options

Change-Id: I870a3d294b4d30abea3f6ecad951d68cfd5c9ceb
@mattcary
Copy link
Contributor Author

mattcary commented Nov 4, 2022

In the release note, can you add "in the snapshot-controller" at the end?

Can you also document this new flag in README? https://github.com/kubernetes-csi/external-snapshotter#snapshot-controller-command-line-options

Done & done!

@xing-yang
Copy link
Collaborator

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattcary, xing-yang

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

No open projects
Status: PRs Approved

Development

Successfully merging this pull request may close these issues.

4 participants