-
Notifications
You must be signed in to change notification settings - Fork 259
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
Allow the usage of AnnPodRetainAfterCompletion with populators #2873
Conversation
6261494
to
52a307d
Compare
52a307d
to
840d3a7
Compare
840d3a7
to
581a394
Compare
581a394
to
f7d44bc
Compare
c827df0
to
c11056b
Compare
/test pull-containerized-data-importer-e2e-hpp-latest |
// Avoiding cleanup so we can keep clone objects for debugging purposes. | ||
r.recorder.Eventf(pvc, corev1.EventTypeWarning, retainedPVCPrime, messageRetainedPVCPrime) | ||
} else { | ||
if err := r.planner.Cleanup(ctx, log, pvc); err != nil { |
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.
Hey @mhenriks, what do you think of skipping the cleanup when using AnnPodRetainAfterCompletion
? I think that for debugging purposes we can allow keeping all the clone objects (even the PVC prime in lost state).
Another possibility is to have the PVC Prime in terminating state until the user deletes the pod, but having the PVC stuck there seems awkward to me. Maybe we could also consider removing the PVC protection finalizer to delete the PVC prime.
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.
Only do this for host clone
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.
One thing to note here though. Pods will be in the source namespace. With cross namespace clone, the user may not have access to them. And I'm not sure we want to allow the user to do this in cross namespace case
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.
Yeah that's a good point. What about the check in the DV controller to see if we use populators or not?
containerized-data-importer/pkg/controller/datavolume/controller-base.go
Lines 1180 to 1184 in bd63352
// currently populators don't support retain pod annotation so don't use populators in that case | |
if retain := dv.Annotations[cc.AnnPodRetainAfterCompletion]; retain == "true" { | |
log.Info("Not using CDI populators, currently we don't support populators with retainAfterCompletion annotation") | |
return false, nil | |
} |
Are we ok with turning to populators even if we only use AnnPodRetainAfterCompletion
with host assisted clone?
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 guess we are since it's basically the same behavior without populators.
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.
Without populators, the target pod is in the target namespace. With populators both pods are in the source then rebind happens. The reason for having the target pod in the source is to better support k8s native cross namespace auth (when it goes live). In that case there will be no clone token. So can't do cross namespace host clone
@mhenriks This could be also an opportunity to add the importer pod name annotation to the target PVC. I personally liked the idea and find it better than expecting users to build the pod name with the UID. |
05905ce
to
2fa5fb0
Compare
/test pull-containerized-data-importer-e2e-istio |
// to keep the pods. | ||
// - Clone is host-assisted, which is the only clone type with transfer pods worth debugging. | ||
// - Clone occurs in a single namespace, so we avoid retaining pods in a namespace we don't have right to access. | ||
dataSourceNamespace, ok := pvc.Annotations[AnnDataSourceNamespace] |
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.
Let's future proof this for when cross namespace datasource goes live. In that case pvc.spec.dataCource.Namespace
may be set
This annotation causes CDI transfer pods (importer, uploader, cloner) to be retained after a successful or failed completion. This makes debugging and testing easier, as users can get the pod state and logs after completion. Signed-off-by: Alvaro Romero <[email protected]>
2fa5fb0
to
4c98be0
Compare
/test pull-containerized-data-importer-fossa |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mhenriks The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I find myself going back a lot to this function and I noticed some of the fallback reasons are outdated following: kubevirt#2765 kubevirt#2873 Signed-off-by: Alex Kalenyuk <[email protected]>
I find myself going back a lot to this function and I noticed some of the fallback reasons are outdated following: kubevirt#2765 kubevirt#2873 Signed-off-by: Alex Kalenyuk <[email protected]>
I find myself going back a lot to this function and I noticed some of the fallback reasons are outdated following: #2765 #2873 Signed-off-by: Alex Kalenyuk <[email protected]>
Backporting this as requested in Jira |
@alromeros: #2873 failed to apply on top of branch "release-v1.57":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it:
When introducing populators we somehow assumed that having the PVC' in Lost phase would interfere with the behavior of
AnnPodRetainAfterCompletion
. After better testing, the pod state and logs are still fetchable after completion.This PR allows the usage of this annotation.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer: Let's see how tests behave.
Release note: