New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kubeadm: add a Timeouts struct to v1beta4 #122529
kubeadm: add a Timeouts struct to v1beta4 #122529
Conversation
Skipping CI for Draft Pull Request. |
/triage accepted |
3cd2fac
to
781ba9b
Compare
601d0ad
to
06eafbe
Compare
LGTM label has been added. Git tree hash: 4045e3de72b42b3f2dda4bb97612287691857a85
|
ControlPlaneComponentHealthCheck: &metav1.Duration{Duration: 4 * time.Minute}, | ||
KubeletHealthCheck: &metav1.Duration{Duration: 4 * time.Minute}, | ||
KubernetesAPICall: &metav1.Duration{Duration: 1 * time.Minute}, | ||
EtcdAPICall: &metav1.Duration{Duration: 2 * time.Minute}, | ||
TLSBootstrap: &metav1.Duration{Duration: 5 * time.Minute}, | ||
Discovery: &metav1.Duration{Duration: 5 * time.Minute}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether it is better to use defined constants instead of literals?
for example:
...
Discovery: &metav1.Duration{Duration: constants.DefaultDiscoveryTimeout},
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added as part of commit:
kubeadm: start using the Timeouts struct values
this is where all constants were refactored and defaults started to be used in the public v1beta4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to clarify: i had to add this defaulter util function because the converter for v1beta3 and the fuzzer needed it.
we might be able to drop it once v1beta3 is removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
f9f8c8e
to
d6b4b4d
Compare
LGTM label has been added. Git tree hash: ef81e8c89b99dcd9a26e011d24cee5d07f882ce3
|
Replace the usage of the deprecated wait.Poll() and wait.PollImmediate() functions with wait.PollUntilContextTimeout(). Since we don't have piping of context around kubeadm, use context.Background() everywhere. Some wait.Poll() functions were converted to "immediate" as there is no point for them to not be. This is done for consistency. Replace the only instance of wait.JitterUntil with wait.PollUntilContextTimeout. JitterUntil is not deprecated but this is also done for consistency.
Propagate usage of the Timeout struct values. Apply sanitazation to timeout constants in contants.go.
Switch to PollUntilContextTimeout() everywhere to allow usage of the exposed timeouts in the kubeadm API. Exponential backoff options are more difficult to expose in this regard and a bit too detailed for the common user - i.e. have "steps", "factor" and so on.
d6b4b4d
to
5f876b9
Compare
pushed minor diff to fix bug and resolve this actual flake |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/retest
LGTM label has been added. Git tree hash: 4bc10264575887e8a4e1f2e343d2f5e617861b0b
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neolit123, SataQiu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold cancel |
@@ -332,15 +336,17 @@ func PatchNode(client clientset.Interface, nodeName string, patchFn func(*v1.Nod | |||
func GetConfigMapWithRetry(client clientset.Interface, namespace, name string) (*v1.ConfigMap, error) { | |||
var cm *v1.ConfigMap | |||
var lastError error | |||
err := wait.ExponentialBackoff(clientsetretry.DefaultBackoff, func() (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// DefaultBackoff is the recommended backoff for a conflict where a client
// may be attempting to make an unrelated modification to a resource under
// active management by one or more controllers.
var DefaultBackoff = wait.Backoff{
Steps: 4,
Duration: 10 * time.Millisecond,
Factor: 5.0,
Jitter: 0.1,
}
KubernetesAPICallRetryInterval, kubeadmapi.GetActiveTimeouts().KubernetesAPICall.Duration
= 500ms \ 1m
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other places: KubernetesAPICall
replaced kubeadmconstants.APICallWithWriteTimeout
, they are correct.
This one we need another timeout setting or just keep it as.
What type of PR is this?
/kind feature
What this PR does / why we need it:
add common Timeout structs in the kubeadm API that allows the user to configure various timeouts.
perform a number of side cleanups and sanitizations that further enable this functionality.
please see all commit messages, best to review one commit at a time.
Which issue(s) this PR fixes:
xref kubernetes/kubeadm#2463
xref kubernetes/kubeadm#2890
Special notes for your reviewer:
NOTE: didn't use existing commits and previous work from
@wangyysde
and@chendave
.started the code work from scratch.
the plan is still roughly based on prior discussions in kubernetes/kubeadm#2463
of course.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: