-
Notifications
You must be signed in to change notification settings - Fork 258
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
Import populator #2690
Import populator #2690
Conversation
Skipping CI for Draft Pull Request. |
b7d10c9
to
d6e096b
Compare
d6e096b
to
770834f
Compare
@ShellyKa13 I cherry-picked some of your commits and tried to refactor common code so it's shared between populators in f51f2f7. Let me know what you think. |
ef256b6
to
644d6a4
Compare
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.
Great start! Here's my first quick pass...
// We still return the nil PVC' as we'll get called again once PVC' exists. | ||
// If target PVC is bound, we don't really need to populate anything. | ||
if pvcPrime == nil && cc.IsUnbound(pvc) { | ||
_, err := r.createPVCPrime(pvc, populationSource, waitForFirstConsumer, populator.updateAnnotations) |
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.
why pass populator.updateAnnotations
instead of just populator?
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.
In the case of createPVCPrime
I think it makes sense that we pass a single pvcModifier (as we do in the base DV controller) instead of the whole populator.
} | ||
|
||
// ImportSourceSpec defines the Spec field for ImportSource | ||
type ImportSourceSpec 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.
Just like UploadSource
I think this is a bad name. Source of what? Source to what? VolumeImportSource
is better but still not great IMO
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 I get your point... VolumeImportSource
works for me but I'm easy to convince. I'll try to think of some names with @ShellyKa13 so we can further discuss them.
11938e4
to
d37236d
Compare
de6316f
to
8cf8840
Compare
b17f57a
to
07879e4
Compare
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.
Couple little things
}, timeout, pollingInterval).Should(BeTrue()) | ||
}) | ||
}) | ||
|
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.
Can we add a test to make sure that the static binding works as expected? Create a PV with claimref set for PVC. Then create the PVC with populator. Make sure the PVC gets bound correctly and data is not overwritten
dvPopulatorLog = logf.Log.WithName("import-populator-test") | ||
) | ||
|
||
var _ = Describe("Import populator tests", func() { |
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.
Maybe I missed it, but how about a unit test that ignores/errors cross namespace datasource?
07879e4
to
1143aae
Compare
// VolumeImportSourceSpec defines the Spec field for VolumeImportSource | ||
type VolumeImportSourceSpec struct { | ||
ContentType DataVolumeContentType `json:"contentType,omitempty"` | ||
// Import sources |
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.
@mhenriks want to know your opinion on this, do you think it's better to have the sources embedded directly in the spec? Or should we add a new field to store them as we do with DataVolumes.
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.
Good question! Yeah I think it makes sense to have this pseudo-union defined in it's own struct
6cce0ac
to
3d4487a
Compare
Great work @alromeros! /approve Let''s let @ShellyKa13 do a final once over and 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 |
/test pull-containerized-data-importer-e2e-ceph |
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.
looks really good!
have several comments, mostly cosmetics, once fixed ill lgtm
pkg/controller/common/util.go
Outdated
@@ -79,6 +85,8 @@ const ( | |||
AnnPriorityClassName = AnnAPIGroup + "/storage.pod.priorityclassname" | |||
// AnnExternalPopulation annotation marks a PVC as "externally populated", allowing the import-controller to skip it | |||
AnnExternalPopulation = AnnAPIGroup + "/externalPopulation" | |||
// AnnOwnedByDataVolume annotation has the owner DataVolume name | |||
AnnOwnedByDataVolume = AnnAPIGroup + "/ownedByDataVolume" |
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.
same for this annotation I guess
pkg/controller/common/util.go
Outdated
|
||
// HasAnnOwnedByDataVolume returns if the obj has annotation | ||
// it is owned by DataVolume | ||
func HasAnnOwnedByDataVolume(obj metav1.Object) bool { |
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.
so eventually since we decided not to check if the pvc was owned by a dv to and we always inflate we dont actually use this function outside of the dv controllers, so might be worth it to put it back where it used to..
} | ||
|
||
// Import-specific implementation of preparePVCForPopulation | ||
func (r *ImportPopulatorReconciler) preparePVCForPopulation(pvc *corev1.PersistentVolumeClaim, source client.Object) { |
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 dont really like this name, it doesnt 'prepare' anything, it really just updates annotation on the pvc prime..
If you dont want to mention specifically the annotation part then can just call it modifyPVCPrime or something like that
}) | ||
|
||
// Common environment requirements | ||
scName := "testsc" |
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.
should be in the consts
Kind: cdiv1.VolumeImportSourceRef, | ||
Name: samplePopulatorName, | ||
} | ||
nsName := "test-import" |
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.
should be in the consts
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 one needs to be a variable to do &nsName.
reconciler = createImportPopulatorReconciler(targetPvc, pvcPrime, pod) | ||
err = reconciler.updateImportProgress(string(corev1.PodRunning), targetPvc, pvcPrime) | ||
Expect(err).ToNot(HaveOccurred()) | ||
Expect(targetPvc.Annotations[AnnImportProgressReporting]).To(BeEquivalentTo("13.45%")) |
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.
something the indentation of this Expect line seems wrong, maybe its just in the view in github
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 I'm sure it's a github thing, looks good when you open the file.
selectedNode := pvc.Annotations[cc.AnnSelectedNode] | ||
if isPopulator && selectedNode != "" { | ||
annotations[cc.AnnSelectedNode] = selectedNode | ||
} |
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.
maybe also put in a function? big chunk of code for this function which is very specific to populators :)
not a must though..
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.
hmmm I know I'm repeating some code here but I've tried some alternatives to reuse SetNodeNameIfPopulator
and I think it looks uglier.
tests/import_test.go
Outdated
Expect(err).ToNot(HaveOccurred()) | ||
} | ||
Eventually(func() bool { | ||
// Make sure pvcPrime was deleted after upload population |
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.
comment line wrong: "upload" population instead of import
This commit introduces the basic reconciler for populators with common function that can be used by the different populators. Signed-off-by: Shelly Kagan <skagan@redhat.com>
Signed-off-by: Shelly Kagan <skagan@redhat.com>
This commit adds the VolumeImportSource CRD into CDI. CRs created from this CRD will be referenced in the dataSourceRef field to populate PVCs with the import populator. Signed-off-by: Alvaro Romero <alromero@redhat.com>
This commit introduces and modifies several functions so we can reuse common code between all populators. Other than having a common reconcile function, a new populatorController interface has been introduced so we are able to call populator-specific methods from the populator-base reconciler. Signed-off-by: Alvaro Romero <alromero@redhat.com>
The import populator is a controller that handles the import of data in PVCs without the need of DataVolumes while still taking advantage of the import-controller flow. This controller creates an additional PVC' with import annotations. After the import process succeeds, the controller rebinds the PV to the original target PVc and deletes the PVC prime. Signed-off-by: Alvaro Romero <alromero@redhat.com>
This commit updates the import tests to cover the new import populator flow. Signed-off-by: Alvaro Romero <alromero@redhat.com>
Signed-off-by: Alvaro Romero <alromero@redhat.com>
* Modify indexes and other related code to support namespaced dataSourceRefs. Cross-namespace population is still not supported as it depends on alpha feature gates. * Add functional test to cover static binding. * Fix selected node annotation bug in scratch space PVCs * Fix linter alerts Signed-off-by: Alvaro Romero <alromero@redhat.com>
3d4487a
to
702ee39
Compare
/test pull-cdi-linter |
great work! |
/test pull-containerized-data-importer-e2e-nfs |
What this PR does / why we need it:
The import populator is a controller that handles the import of data in PVCs (still taking advantage of the import-controller flow) without the need for DataVolumes.
This controller creates an additional PVC' with import annotations. After the import process succeeds, the controller rebinds the PV to the original target PVC and deletes the PVC'.
Special notes for your reviewer: Still need to commit the unit tests.
Release note: