Add TPR to CRD migration helper. #46677
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-github-robot
added
size/L
enisoc
added
release-note-none
| // Freeze TPR data to prevent new writes via this apiserver process. | ||
| // Other apiservers can still write. This is best-effort because there | ||
| // are worse problems with TPR data than the possibility of going back | ||
| // in time when migrating to CRD [citation needed]. |
| frozen atomic.Value | ||
| } | ||
|
|
||
| func (r *REST) Freeze() { |
There was a problem hiding this comment.
nit: a comment that this is a one way trip.
|
This is a strong start. /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, enisoc
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
What if you detected that this was a migration (you're already doing that) and just didn't delete the original TPR data from etcd? We'd have dangling entries, but no one would be reading them anymore and no delete events would get sent. The watch would be dropped and the client's next list/watch would be satisfied by CRD. |
pkg/master/thirdparty/thirdparty.go
Outdated
| } | ||
|
|
||
| // Talk directly to CustomResource storage. | ||
| // We have to bypass the API server because TPR is shadowing CRD at this point. |
There was a problem hiding this comment.
will a client observe the deletions? Thinking about the etcd operator deleting its storage volumes.
There was a problem hiding this comment.
will a client observe the deletions? Thinking about the etcd operator deleting its storage volumes.
I think I would handle that as I indicated #46677 (comment) . I've also suggested manually stopping the controller here: kubernetes/website#3940
I think abandoning the etcd data can happen as a followup or bug fix if necessary.
There was a problem hiding this comment.
If I understand the PR correctly, you delete the TPR and it automatically is migrated (if successful) to the CRD (if it exists). This feels pretty counter-intuitive. I would have expected an start-cdr-migration annotation on the TPR, or even an automatic migration as soon as the CRD shows up.
There was a problem hiding this comment.
If I understand the PR correctly, you delete the TPR and it automatically is migrated (if successful) to the CRD (if it exists). This feels pretty counter-intuitive. I would have expected an start-cdr-migration annotation on the TPR, or even an automatic migration as soon as the CRD shows up.
I'm ok with either change as a followup/issue/bugfix if the TPR structure makes that possible. I don't know that updates are plumbed through.
enisoc
force-pushed
the
tpr-migrate-etcd
branch
2 times, most recently
from
6bcec9a to
73921cb
Compare
k8s-github-robot
removed
the
lgtm
enisoc
added
the
lgtm
|
@k8s-bot pull-kubernetes-federation-e2e-gce test this |
|
Automatic merge from submit-queue (batch tested with PRs 43505, 45168, 46439, 46677, 46623) |
This is a helper for migrating TPR data to CustomResource. It's rather hacky because it requires crossing apiserver boundaries, but doing it this way keeps the mess contained to the TPR code, which is scheduled for deletion anyway.
It's also not completely hands-free because making it resilient enough to be completely automated is too involved to be worth it for an alpha-to-beta migration, and would require investing significant effort to fix up soon-to-be-deleted TPR code. Instead, this feature will be documented as a best-effort helper whose results should be verified by hand.
The intended benefit of this over a totally manual process is that it should be possible to copy TPR data into a CRD without having to tear everything down in the middle. The process would look like this:
Note that the old TPR data is left behind by this process, so watchers should not receive DELETE events. This also means the user can revert to the pre-migration state by recreating the TPR definition.
Ref. #45728