-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
RFC: Coordinated dynamic storage version #1201
Conversation
|
||
### Curating a list of participating API servers in HA master | ||
|
||
Currently, API servers maintain such a list in the "kubernetes" endpoints. See |
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.
We have a way to do that - this doesn't mean we always do.
For example in GKE, this endpoint contains only IP of the LB in front of apiservers (whether that's good or bad is a separate discussion, but I guess we may not be the only one doing something like that).
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.
I agree that other operators might have other forms of deployments that also hide the individual apiserver endpoints, so we can't rely on the current "kubernetes" service endpoints to get a reliable list of apiservers.
As I mentioned in the KEP, I will send another KEP to formalize the procedure of apiserver census. Operators will need to configure an ID for each apiserver to make it work.
participating API server. This means the API server has gone and the entry | ||
is stale. Removes such entries. | ||
* checks if all participating API server votes for the same version. If so, | ||
sets the version as the Selectedversion. |
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.
What if not all of them are voting for the same version?
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.
The preferred version in the first vote becomes the first selected version.
Afterwards, only after all apiservers prefer a new version, then it becomes the new selected version.
checks if key does not already exist before committing a create operation. As | ||
another example, kube-apiserver checks that the resourceVersion is the latest | ||
before committing an update operation. Adding another transaction condition does | ||
not add extra RPC call. |
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.
It would be good to run some benchmark of it to understand the impact of it. I hope it should be relatively small, but I don't know etcd internals well enough to know exactly what would happen.
@jpbetz - FYI
version. The storage version in the CRD spec is the "selected storage version". | ||
|
||
The current implementation of the apiextension-apiserver already provides the | ||
"loose guarantee". |
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.
Do we have any signals from users that it's not enough?
If not, maybe it's not worth changing that (and maybe that also means it should be good enough for builtin resources).
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.
No, I haven't heard any complain.
I'm willing to pursue the "loose guarantee" path if all reviewers think it's good enough.
* gets the StorageVersion object for this resource, | ||
* if the object does not exist, creates one, setting this kube-apiserver's | ||
preferred storage version as the Selectedversion, also adding its preferred | ||
storage version to the CandidateVersions list. Then jumps to the last step. |
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.
last step is which step? probably better to number the steps
* gets the list of all participating API servers, | ||
* gets all the StorageVersion objects, | ||
* removes the StorageVersion object if none of its CandidateVersions.APIServerID | ||
is a participating API server. |
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.
usually api servers are independent, unaware of the existence of the other api servers. what about using keep-alive have each api server to tell it is still alive, participating
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.
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.
Yes, that is it.
exists in all parts of Kubernetes API. Client-side coordination is necessary | ||
to overcome the race. Let's take the storage migrator as an example. For the | ||
storage migrator to work properly, the required client-side coordination is | ||
that at most one master version change could happen in any 5 minute window. |
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.
i don't think this is client-side coordination, this is an assumption / guarantee made on the server side, i.e., cluster admins do not do version upgrade or downgrade more than once every 5 minutes.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caesarxuchao The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
b6cd34b
to
ff8ad27
Compare
ff8ad27
to
6700cbb
Compare
/assign @deads2k |
server will change the selected version to the new storage version, and all API | ||
servers start to use the new version. | ||
|
||
### The selected storage version is supported by all API servers during upgrades/downgrades |
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.
This describes selecting which version to use for a type which all API servers support, but at different versions.
Would the same mechanism correctly handle a type which some API servers do not support at all (e.g. avoid serving a new beta type while there are joined API servers that do not make that type available, xref kubernetes/kubernetes#81286)
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.
I think so. It's mentioned in the "future work" section: "For example,
kube-apiservers can vote on what API is served, and only serve the API that is
supported by all servers."
|
||
During the rolling update of an HA master, API server instances *i)* use | ||
different storage versions for a built-in API resource, and *ii)* show different | ||
storageVersionHash in the discovery documents. These facts make the storage |
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.
elaborate here. I suspect it's because etcd may contain records written in different storage versions.
[storageVersionHash]:https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L979 | ||
[details]:https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/35-storage-version-hash.md#ha-masters | ||
|
||
We propose a mechanism for HA API servers to vote on the storage version to use, |
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.
I don't think that we need to have all apiserver allow configurable storage. Instead, an apiserver can indicate which storage version it writes (hardcoded like today) and the entity driving the storage migrator can decide to wait until all apiservers agree. That builds one system on another instead of changing the underlying system.
|
||
```golang | ||
// Storage version for a specific resource. | ||
type StorageVersion struct { |
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.
I would like to see a spec/status split here. apiservers write the status of what is
- groupversions this server can read
- groupversions this server will write
I don't see any fields for spec yet.
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.
Is "groupversions this server can read" essential to this API? That information is beyond "storageVersion". I'll need to come up with a new API name.
### Storage Version API | ||
|
||
We introduce a new API `StorageVersion`, in a new API group | ||
`kube-apiserver.internal.k8s.io/v1alpha1`. |
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.
this isn't specific to kube-apiserver. This is generic for any aggregated apiserver as well.
|
||
type VersionCandidate struct { | ||
// The ID of the reporting API server. | ||
APIServerID string |
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.
how will old/outdated servers be stripped removed? A server can crash and be replaced by a different server with a different ID. Imagine using an autoscaling deployment as a for instance.
this [design doc][] for details. | ||
|
||
We will formalize the API in another KEP. For the purpose of this KEP, all we | ||
need to know is that HA master is able to maintain a list of participating API |
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.
ah, neat. This is how you handle pruning. Combined with visibility into apiservices, this is sufficient to selectively prune apiservers.
|
||
[design doc]:https://github.com/kubernetes/community/pull/939 | ||
|
||
### Voting for Storage Version |
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.
I'm not seeing why voting is necessary for the storage migrator to function correctly. It seems like being able to see the current state and knowing whether it is consistent across the cluster is sufficient to make a choice about migrating that cluster. That, in combination with a resource version seems fully sufficient to know if storage migration should commence and whether it was successful.
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.
Overall I think this proposal should be about discovery, not configuration, enabling other processes to drive the storage migration.
[storageVersionHash]:https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L979 | ||
[details]:https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/35-storage-version-hash.md#ha-masters | ||
|
||
We propose a mechanism for HA API servers to vote on the storage version to use, |
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.
The StorageVersionHash
problem linked above is about being able to detect when its safe to trigger the storage migratior, whereas here you are suggesting you intent to actually configure which storage version to use. Please clarify the intended use. I would suggest this proposal should only be about discovering which storage versions are in use.
ObjectMeta | ||
// The selected storage version. All API server intances encode the resource | ||
// objects in this version when committing it to etcd. | ||
SelectedVersion string |
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.
If we are not configuring storage (I don't think this is what we want), this field is unnecessary.
APIServerID string | ||
// The preferred storage version of the reporting API server. The preferred | ||
// storage version is hardcoded in each Kubernetes release. | ||
PreferredVersion string |
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.
This should simply be the storage version in use by this api server.
Superseded by #1220
An alternative (hopefully a better one) to #1176.
We propose a mechanism for HA API servers to vote on the storage version to use, then show the selected storage version in the discovery document and switch to use the selected version atomically.
/assign @yliaog @wojtek-t @lavalamp
Thank you for you previous comments in #1176
/sig api-machinery