feat: add status back-reporting support (1/, work applier side) #327
Conversation
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| // OwnerReference in the Namespace object (https://book.kubebuilder.io/reference/envtest.html#testing-considerations). | ||
| checkNSOwnerReferences(workName, nsName) | ||
|
|
||
| // Ensure that the AppliedWork object has been removed. |
There was a problem hiding this comment.
if the AppliedWork object is supposed to be gone at this point. And you check the owner reference of the namespace right before, which includes getting the applied work from memberClient1.
What happened between checkNSOwnerReferences(workName, nsName) and appliedWorkRemovedActual := appliedWorkRemovedActual(workName, nsName) that caused the AppliedWork to be removed between these 2 statements?
There was a problem hiding this comment.
Hi Wei! This was kind of related to an earlier issue we discussed a while ago. The integration test uses the envtest package, which runs a somewhat trimmed down Kubernetes environment. Specifically in this environment, IIRC when we do the foreground deletion there is no controller to actually delete the child objects, and owner references will not be updated as well (even if the owner itself is already gone), which is the reason why the corrent code formation works. I am happy to explain this a bit further if needed -> Britania might have a bit more context though as she added this part of the code.
There was a problem hiding this comment.
My confusion is that this function
func appliedWorkRemovedActual(workName, nsName string) func() error {
return func() error {
// Retrieve the AppliedWork object.
appliedWork := &fleetv1beta1.AppliedWork{}
if err := memberClient1.Get(ctx, client.ObjectKey{Name: workName}, appliedWork); err != nil {
if errors.IsNotFound(err) {
// The AppliedWork object has been deleted, which is expected.
return nil
}
return fmt.Errorf("failed to retrieve the AppliedWork object: %w", err)
}
if !appliedWork.DeletionTimestamp.IsZero() && controllerutil.ContainsFinalizer(appliedWork, metav1.FinalizerDeleteDependents) {
// The AppliedWork object is being deleted, but the finalizer is still present. Remove the finalizer as there
// are no real built-in controllers in this test environment to handle garbage collection.
controllerutil.RemoveFinalizer(appliedWork, metav1.FinalizerDeleteDependents)
Expect(memberClient1.Update(ctx, appliedWork)).To(Succeed(), "Failed to remove the finalizer from the AppliedWork object")
}
return fmt.Errorf("appliedWork object still exists")
}
}doesn't delete anything unless it is the removal of the finalizer that resulted in a successful deletion?
|
|
||
| It("should back-report refreshed deployment status to the Work object", func() { | ||
| Eventually(func() error { | ||
| workObj := &fleetv1beta1.Work{} |
There was a problem hiding this comment.
is this chunk of code exactly the same as the chunk that starts at line 9820?
There was a problem hiding this comment.
Ah, yeah; let me add a common block in this test spec and simplify things a bit. Thanks.
| // a) the ReportBackStrategy is of the type Mirror; and | ||
| // b) the manifest object has been applied successfully. | ||
| if isStatusBackReportingOn && isManifestObjectApplied(bundle.applyOrReportDiffResTyp) { | ||
| backReportStatus(bundle.inMemberClusterObj, manifestCond, now, klog.KObj(work)) |
There was a problem hiding this comment.
can we get into situations that the combined resource status size is over the etcd limit?
There was a problem hiding this comment.
Ah, the 1(.5) MB size limit can be a concern, let me add some logic to double-check -> this can be fixed by further sharding but we do not have that protocol yet...
3 participants
Description of your changes
This PR adds support for status back-reporting on the work applier side.
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer
E2E tests are only possible when the full flow has been implemented.