if we calculate the sleep duration first, we can log the sleep time in the error message which is sometimes useful to see how long something is in a backoff loop.

done

only if the feature is enabled

done

would we only put leases in this namespace? would this be a logical place to put configmaps with node config as well?

whatever name is selected here needs to be communicated well in advance, so cluster operators can reserve the namespace for system use. it does raise an interesting question about what namespaces the kube system components are entitled to claim in the future. anything prefixed with kube-? anything prefixed with kube-system?

also, this doesn't really belong in meta/v1. it's fundamental to kube-apiserver and kubelet, but not to apimachinery (same really goes for kube-system and kube-public, but we can deal with those separately)

kube-node would make sense to store node configmaps and leases. Thoughts?

would we only put leases in this namespace?

My understanding is just leases. This is what the KEP says:

for each Node there will be a corresponding Lease object with Name equal to Node name in a newly created dedicated namespace (we considered using kube-system namespace but decided that it's already too overloaded). That namespace should be created automatically (similarly to "default" and "kube-system", probably by NodeController) and never be deleted (so that nodes don't require permission for it).

Which implies you just use it for node leases, because not prefixing the lease names means you can't cleanly add any other type of lease to that namespace in the future.

needs to be communicated well in advance

My understanding is that we had already reserved the kube- prefix for namespaces? Maybe this isn't true? @thockin do you know?

this doesn't really belong in meta/v1

That's fair, but then where should we put the constants for standard system namespaces? Multiple consumers need them, so they should be in a common place.

Maybe just core/v1?

looks like the others are already in pkg/apis/core/types.go... there or in core/v1/types.go is better than meta/v1

I talked to @bgrant0607 about kube-, given that this is the convention we think we should just explicitly reserve it: kubernetes/community#2379

Why aren't the other ones in core/v1 in addition to core? Just because they're in meta/v1 instead (odd, as you said)?
Should we put a constant for the node lease namespace in both core and core/v1?

moved to both for now

is there a reason we're not using the injected clock here?

oops, good catch!

done

7 seconds hard-coded? this looks weird...

I can make it a constant. I'm just keeping it consistent with what node registration does, for lack of a concrete principle to actually base the number on.

Not sure these need to match the node registration exactly, but if you want to, factoring them out to use in both places sounds good.

I'd rather keep them decoupled, but just set to the same number for now.

is this working today in a test cluster with authz enabled? I expected to see modifications to the Node authorizer and NodeRestriction admission plugin to let kubelets mess with their own leases

Good point, I forgot about that. I will add those changes to this PR, and it probably makes sense to ensure the e2e test also runs in the standard e2e suite, instead of just the e2e node suite.

We also don't currently run the node authorizer/node restriction in the e2e node tests. Maybe I should revisit #60172?

Do these client calls include a request timeout?

heartbeatClient is explicitly constructed with a timeout to avoid eternal hangs, and a distinct QPS to avoid starvation by other kube API calls, but is currently fixed to corev1. need to adapt/widen that for use here

I believe that is configured on the client before it is passed to the controller.

If we want the timeout to match the heartbeat frequency, then we need a distinct client, since the Lease-based heartbeat potentially has a different frequency than the node status update, right?

Taking the shorter of the two would probably be fine

done

this is expecting to use the node name as the name of the lease object. Is that compatible with the currently allowed names of the lease objects? I can't remember if the initial PR limited them to single dns labels (e.g. no . characters). If so, we likely want to relax that

// ValidateLease validates a Lease.
func ValidateLease(lease *coordination.Lease) field.ErrorList {
	allErrs := validation.ValidateObjectMeta(&lease.ObjectMeta, true, validation.NameIsDNSSubdomain, field.NewPath("objectMeta"))
	allErrs = append(allErrs, ValidateLeaseSpec(&lease.Spec, field.NewPath("spec"))...)
	return allErrs
}

Looks like we just require the name to be a DNS subdomain, via validation.NameIsDNSSubdomain.