-
Notifications
You must be signed in to change notification settings - Fork 215
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
Add support for deleting resources in reverse order by stage. #2437
Conversation
/assign @barney-s |
c63bb7f
to
b17f448
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
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 curious to see how this code will behave with Updates to composition.
Like an user changes composition to add or remove stages.
Or even re-order stages !!
@@ -154,11 +154,11 @@ func NewKCCSample(t *testing.T, sample Sample, dependentSamples []Sample) *Scena | |||
} | |||
|
|||
func (s *Scenario) Cleanup() { | |||
defer cluster.ReleaseCluster(s.T, s.cluster) | |||
defer s.GatherLogs() |
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.
GatherLogs and then release clusters ?
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 I'm not mistaken, defers are executed in LIFO order, so gather will be called before release.
t.Fatalf("failed removing finalizer from configmap: %v", err) | ||
} | ||
|
||
// Verify the remaining objects have been deleted |
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
s.C.MustNotExist([]*unstructured.Unstructured{cms[1]}, scenario.DeleteTimeout) ?
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.
Line 479 will handle that check -- should I modify it to match this?
s.C.MustNotExist(cms, scenario.DeleteTimeout) | ||
|
||
cr = utils.GetUnstructuredObj("facade.foocorp.com", "v1alpha1", "DConfig", "configs", "config-team-b") | ||
s.C.Delete(context.Background(), cr) |
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 split this into 2 separate test cases ?
experiments/compositions/composition/internal/controller/expander_reconciler.go
Show resolved
Hide resolved
@@ -645,6 +701,8 @@ func (r *ExpanderReconciler) enqueueAllFromGVK(ctx context.Context, _ client.Obj | |||
|
|||
// SetupWithManager sets up the controller with the Manager. | |||
func (r *ExpanderReconciler) SetupWithManager(mgr ctrl.Manager, cr *unstructured.Unstructured) error { | |||
r.appliedObjects = map[types.NamespacedName][]*appliedStage{} |
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.
iam not so sure of caching objects in the reconciler for all input CRs.
Cant find a more elegant alternative.
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 have an idea, we can discuss offline.
Foreground with ownership may may not solve ordering deletion issue: |
- Add finalizers to expanded resources and process deletion requests - Track expanded objects by stage - When deleting, iterate over the stages in reverse order and delete all objects. Once those objects no longer exist, proceed to the next stage - Remove owner reference injection on expanded resources
b17f448
to
1afa3cd
Compare
Change description