taint fields: timeAdded not set

[justinsb]

I set taints on a node using the kubelet, but the timeAdded field was not set:

 taints:
  - effect: NoSchedule
    key: dedicated
    timeAdded: null
    value: master

In addition, I think we prefer to omit null fields.

[liggitt]

doc for that field says it is only written for NoExecute taints, and that field is omitempty

[liggitt]

ah, looks like omitempty will only work if timeAdded is a pointer

[liggitt]

opened speculative PR in #43016 to change it to a pointer (like other optional time fields like deletionTimestamp)

[timothysc]

/cc @kubernetes/sig-scheduling-bugs

[liggitt]

@davidopp what component is supposed to set timeAdded for NoExecute taints (and is validation supposed to prevent it for taints with effects other than NoExecute)? The only place I see it getting set is by the node controller

[davidopp]

As @liggitt alluded to, strictly speaking timeAdded is only needed for taints of effect NoExecute. The field is needed in order to make tolerationSeconds work in the face of component failure -- whichever component enforces the taint will evict the pod if timeAdded + pod.tolerationSeconds <= time.Now. (Yes, clock skew will break this if they are different components.) So only the components that add NoExecute taints (which is only NodeController) set timeAdded, and only for NoExecute taints.

#40586 was filed with the suggestion to add it for all taints. It's not strictly necessary, but since the field is user-visible (despite being purely an "internal" concept), it would probably be less confusing if we were consistent and set timeAdded for all taint types.

For 1.6 I think the current behavior is fine.

These comments are unrelated to #43016, they are just about whether/when we should be setting timeAdded.

Let me know if this is unclear.

[aveshagarwal]

Other components where taints can be added are: kubelet (-register-with-taints) and kubectl taints. Right now, taint controller enforces pod eviction for NoExecute taints.

[aveshagarwal]

Another component is node controller that adds taints for node problems (unreachable, notready) but clock skew is not an issue here, but only for kubelet, kubectl taint I think.

[liggitt]

If the switch to a pointer won't change the values servers will accept, then current behavior seems ok for 1.6 (for JSON, we'll need to handle null even when the value is a pointer, so that'll be fine... I'm less sure about protobuf)

[ethernetdan]

@davidopp @aveshagarwal @liggitt should this be blocking for 1.6?

[davidopp]

There are two issues here. One is just a dup of #40586. The main question is whether #43016 is blocking for 1.6. I don't know enough about the API stuff to know for sure.

[liggitt]

@smarterclayton says proto always tolerates missing objects, so I think #43016 is ok to move to 1.7

[k82cn]

The PR was moved to 1.8, should we also move this issue to 1.8?

[k82cn]

/priority important-soon

[spiffxp]

friendly v1.8 release ping, does this issue need to be in the v1.8 milestone? is this issue actively being worked?

it looks like #43016 which partially fixes this issue has been moved to v1.9

[liggitt]

no, moving to 1.9

[k82cn]

/status approved-for-milestone

We'd like to get this fixed in 1.9.

[k8s-ci-robot]

You must be a member of the @kubernetes/kubernetes-milestone-maintainers github team to add status labels.

[k8s-github-robot]

[MILESTONENOTIFIER] Milestone Issue Current

@davidopp @justinsb

Issue Labels

• sig/scheduling: Issue will be escalated to these SIGs if needed.
• priority/important-soon: Escalate to the issue owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
• kind/bug: Fixes a bug discovered during the current release.

Help

• Additional instructions
• Commands for setting labels

[dims]

@k82cn @justinsb move to 1.10?

[k82cn]

Fixed by #43016.