Description
When we added resourceVersion=0 to reflectors, we didn't properly reason about its impact on nodes. Its current behavior can cause two nodes to run a pod with the same name at the same time when using multiple api servers, which violates the pod safety guarantees on the cluster. Because a read serviced by the watch cache can be arbitrarily delayed, a client that connects to that api server can read an arbitrarily old history. We explicitly use quorum reads against etcd to prevent this.
Scenario:
- T1: StatefulSet controller creates
pod-0
(uid 1) which is scheduled tonode-1
- T2:
pod-0
is deleted as part of a rolling upgrade node-1
sees thatpod-0
is deleted and cleans it up, then deletes the pod in the api- The StatefulSet controller creates a second pod
pod-0
(uid 2) which is assigned tonode-2
node-2
sees thatpod-0
has been scheduled to it and startspod-0
- The kubelet on
node-1
crashes and restarts, then performs an initial list of pods scheduled to it against an API server in an HA setup (more than one API server) that is partitioned from the master (watch cache is arbitrarily delayed). The watch cache returns a list of pods from before T2 node-1
fills its local cache with a list of pods from before T2node-1
startspod-0
(uid 1) andnode-2
is already runningpod-0
(uid 2).
This violates pod safety. Since we support HA api servers, we cannot use resourceVersion=0
from reflectors on the node, and probably should not use it on the masters. We can only safely use resourceVersion=0
after we have retrieved at least one list, and only if we verify that resourceVersion is in the future.
@kubernetes/sig-apps-bugs @kubernetes/sig-api-machinery-bugs @kubernetes/sig-scalability-bugs This is a fairly serious issue that can lead to cluster identity guarantees being lost, which means clustered software cannot run safely if it has assumed the pod safety guarantee prevents two pods with the same name running on the cluster at the same time. The user impact is likely data loss of critical data.
This is also something that could happen for controllers - during a controller lease failover the next leader could be working from a very old cache and undo recent work done.
No matter what, the first list of a component with a clean state that must preserve "happens-before" must perform a live quorum read against etcd to fill their cache. That can only be done by omitting resourceVersion=0.
Fixes:
- Disable resourceVersion=0 from being used in reflector list, only use when known safe
- Disable resourceVersion=0 for first list, and optionally use resourceVersion=0 for subsequent calls if we know the previous resourceVersion is after our current version (assumes resource version monotonicity)
- Disable resourceVersion=0 for the first list from a reflector, then send resourceVersion= on subsequent list calls (which causes the watch cache to wait until the resource version shows up).
- Perform live reads from Kubelet on all new pods coming in to verify they still exist
1 is a pretty significant performance regression, but is the most correct and safest option (just like we enabled quorum reads everywhere). 2 is more complex, and there are a few people trying to remove the monotonicity guarantees from resource version, but would retain most of the performance benefits of using this in the reflector. 3 is probably less complex than 2, but i'm not positive it actually works. 4 is hideous and won't fix other usage.
Probably needs to be backported to 1.6.
Metadata
Metadata
Assignees
Labels
Type
Projects
Status
Activity
[-]Use of resourceVersion=0 in reflectors for initial sync breaks pod safety when more than one master[/-][+]Use of resourceVersion=0 in reflectors for initial sync breaks pod safety when more than one api server[/+]smarterclayton commentedon Feb 14, 2018
Note that we added this to improve large cluster performance, so we are almost certainly going to regress to some degree (how much has been mitigated by other improvements in the last year is uncertain)
roycaihw commentedon Feb 15, 2018
/sub
smarterclayton commentedon Feb 17, 2018
Disabling resourceVersion=0 for lists when more than one master is present is an option as well.
As Jordan noted the optimization here is impactful because we avoid having to fetch many full pod lists from etcd when we only return a subset to each node. It’s possible we could require all resourceVersion=0 calls to acquire a read lease on etcd which bounds the delay, but doesn’t guarantee happens before if the cache is delayed. If the watch returned a freshness guarantee we could synchronize on that as well.
smarterclayton commentedon Feb 17, 2018
We can do a synthetic write to the range and wait until it is observed by the cache, then service the rv=0. The logical equivalent is a serializable read on etcd for the range, but we need to know the highest observed RV on the range.
We can accomplish that by executing a range request with min create and mod revisions equal to the latest observed revision, and then performing the watch cache list at the highest RV on the range.
So:
We can mitigate the performance impact by ensuring that an initial list from the watch cache preserves happens before. We can safely serve historical lists (rv=N) at any time. The watch cache already handles rv=N mostly correctly.
So the proposed change here is:
That resolves this issue.
smarterclayton commentedon Feb 17, 2018
Serializable read over the range, that is.
smarterclayton commentedon Feb 17, 2018
Also:
jberkus commentedon Feb 21, 2018
if this MUST be resolved for 1.10, please add status/approved-for-milestone to it before Code Freeze. Thanks!
148 remaining items