Skip to content

Commit

Permalink
fix the pre-delete hook is recreated in hosted mode (#231)
Browse files Browse the repository at this point in the history
Signed-off-by: Zhiwei Yin <zyin@redhat.com>
  • Loading branch information
zhiweiyin318 authored Jan 23, 2024
1 parent bf8e38a commit 71f1b13
Show file tree
Hide file tree
Showing 3 changed files with 101 additions and 36 deletions.
46 changes: 25 additions & 21 deletions pkg/addonmanager/controllers/agentdeploy/hosted_hook_sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,20 +80,33 @@ func (s *hostedHookSyncer) sync(ctx context.Context,
return addon, nil
}

// will deploy the pre-delete hook manifestWork when the addon is deleting
if addon.DeletionTimestamp.IsZero() {
addonAddFinalizer(addon, addonapiv1alpha1.AddonHostingPreDeleteHookFinalizer)
return addon, nil
}

// the hook work is completed if there is no HostingPreDeleteHookFinalizer when the addon is deleting.
if !addonHasFinalizer(addon, addonapiv1alpha1.AddonHostingPreDeleteHookFinalizer) {
return addon, nil
}

hookWork, err = s.applyWork(ctx, addonapiv1alpha1.ManagedClusterAddOnHostingManifestApplied, hookWork, addon)
if err != nil {
return addon, err
// apply the pre-delete hook manifestWork when the addon is deleting and HookManifestCompleted condition is not true.
// there are 2 cases:
// 1. the HookManifestCompleted condition is false.
// 2. there is no this condition.
if !meta.IsStatusConditionTrue(addon.Status.Conditions, addonapiv1alpha1.ManagedClusterAddOnHookManifestCompleted) {
hookWork, err = s.applyWork(ctx, addonapiv1alpha1.ManagedClusterAddOnHostingManifestApplied, hookWork, addon)
if err != nil {
return addon, err
}
} else {
// cleanup is safe here since there is no case which HookManifestCompleted condition is changed from true to false.
if err = s.cleanupHookWork(ctx, addon); err != nil {
return addon, err
}
if addonRemoveFinalizer(addon, addonapiv1alpha1.AddonHostingPreDeleteHookFinalizer) {
return addon, err
}
return addon, nil
}

// TODO: will surface more message here
Expand All @@ -104,25 +117,16 @@ func (s *hostedHookSyncer) sync(ctx context.Context,
Reason: "HookManifestIsCompleted",
Message: fmt.Sprintf("hook manifestWork %v is completed.", hookWork.Name),
})

if err = s.cleanupHookWork(ctx, addon); err != nil {
return addon, err
}
if addonRemoveFinalizer(addon, addonapiv1alpha1.AddonHostingPreDeleteHookFinalizer) {
return addon, err
}
return addon, nil
} else {
meta.SetStatusCondition(&addon.Status.Conditions, metav1.Condition{
Type: addonapiv1alpha1.ManagedClusterAddOnHookManifestCompleted,
Status: metav1.ConditionFalse,
Reason: "HookManifestIsNotCompleted",
Message: fmt.Sprintf("hook manifestWork %v is not completed.", hookWork.Name),
})
}

meta.SetStatusCondition(&addon.Status.Conditions, metav1.Condition{
Type: addonapiv1alpha1.ManagedClusterAddOnHookManifestCompleted,
Status: metav1.ConditionFalse,
Reason: "HookManifestIsNotCompleted",
Message: fmt.Sprintf("hook manifestWork %v is not completed.", hookWork.Name),
})

return addon, nil

}

// cleanupHookWork will delete the hosting pre-delete hook manifestWork and remove the finalizer,
Expand Down
31 changes: 18 additions & 13 deletions pkg/addonmanager/controllers/agentdeploy/hosted_hook_sync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ func TestHostingHookReconcile(t *testing.T) {
},
},
{
name: "deploy hook manifest for a deleting addon with 2 finalizer, completed",
name: "deploy hook manifest for a deleting addon with 2 finalizer, without completed condition",
key: "cluster1/test",
addon: []runtime.Object{
addontesting.SetAddonFinalizers(
Expand Down Expand Up @@ -296,30 +296,35 @@ func TestHostingHookReconcile(t *testing.T) {
}(),
},
validateWorkActions: func(t *testing.T, actions []clienttesting.Action) {
// hosted hook sync deletes the hook work in the hosting cluster ns
addontesting.AssertActions(t, actions, "delete")
if actions[0].(clienttesting.DeleteActionImpl).Name != constants.PreDeleteHookHostingWorkName("cluster1", "test") {
t.Errorf("should delete the hook work after completed")
}
addontesting.AssertNoActions(t, actions)
},
validateAddonActions: func(t *testing.T, actions []clienttesting.Action) {
// delete HostingPreDeleteHookFinalizer firstly
addontesting.AssertActions(t, actions, "update")
actual := actions[0].(clienttesting.UpdateActionImpl).Object
addOn := actual.(*addonapiv1alpha1.ManagedClusterAddOn)
addontesting.AssertActions(t, actions, "patch")
patch := actions[0].(clienttesting.PatchActionImpl).Patch
addOn := &addonapiv1alpha1.ManagedClusterAddOn{}
err := json.Unmarshal(patch, addOn)
if err != nil {
t.Fatal(err)
}
if addonHasFinalizer(addOn, addonapiv1alpha1.AddonHostingPreDeleteHookFinalizer) {
t.Errorf("expected no HostingManifestFinalizer on addon.")
t.Errorf("expected no HostingPreDeleteHookFinalizer on addon.")
}
if !meta.IsStatusConditionTrue(addOn.Status.Conditions, addonapiv1alpha1.ManagedClusterAddOnHookManifestCompleted) {
t.Errorf("HookManifestCompleted condition should be true, but got false.")
}
},
},
{
name: "deploy hook manifest for a deleting addon with 1 finalizer, completed",
name: "deploy hook manifest for a deleting addon with 1 finalizer, completed condition",
key: "cluster1/test",
addon: []runtime.Object{
addontesting.SetAddonFinalizers(
addontesting.SetAddonDeletionTimestamp(
addontesting.NewHostedModeAddon("test", "cluster1", "cluster2",
registrationAppliedCondition), time.Now()),
registrationAppliedCondition,
metav1.Condition{
Type: addonapiv1alpha1.ManagedClusterAddOnHookManifestCompleted,
Status: metav1.ConditionTrue}), time.Now()),
addonapiv1alpha1.AddonHostingPreDeleteHookFinalizer),
},
cluster: []runtime.Object{
Expand Down
60 changes: 58 additions & 2 deletions test/integration/agent_hosting_hook_deploy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ var _ = ginkgo.Describe("Agent hook deploy", func() {
Annotations: map[string]string{
addonapiv1alpha1.HostingClusterNameAnnotationKey: hostingClusterName,
},
// this finalizer is to prevent the addon from being deleted for test, it will be deleted at the end.
Finalizers: []string{"pending"},
},
Spec: addonapiv1alpha1.ManagedClusterAddOnSpec{
InstallNamespace: "default",
Expand Down Expand Up @@ -280,7 +282,9 @@ var _ = ginkgo.Describe("Agent hook deploy", func() {
return nil
}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())

// update hook manifest feedbackResult, addon will be deleted
// update hook manifest feedbackResult, addon status will be updated and finalizer and pre-delete manifestwork
// will be deleted.

work, err = hubWorkClient.WorkV1().ManifestWorks(hostingClusterName).
Get(context.Background(), constants.PreDeleteHookHostingWorkName(addon.Namespace, addon.Name), metav1.GetOptions{})
gomega.Expect(err).ToNot(gomega.HaveOccurred())
Expand Down Expand Up @@ -322,6 +326,59 @@ var _ = ginkgo.Describe("Agent hook deploy", func() {
_, err = hubWorkClient.WorkV1().ManifestWorks(hostingClusterName).
UpdateStatus(context.Background(), work, metav1.UpdateOptions{})
gomega.Expect(err).ToNot(gomega.HaveOccurred())

// addon only has pending finalizer, and status is updated
gomega.Eventually(func() error {
addon, err := hubAddonClient.AddonV1alpha1().ManagedClusterAddOns(managedClusterName).
Get(context.Background(), testHostedAddonImpl.name, metav1.GetOptions{})
if err != nil {
return err
}
if len(addon.Finalizers) != 1 {
return fmt.Errorf("addon is expected to only 1 finalizer,but got %v", len(addon.Finalizers))
}
if addon.Finalizers[0] != "pending" {
return fmt.Errorf("addon is expected to only pending finalizer,but got %v", len(addon.Finalizers))
}
if !meta.IsStatusConditionTrue(addon.Status.Conditions, addonapiv1alpha1.ManagedClusterAddOnHookManifestCompleted) {
return fmt.Errorf("addon HookManifestCompleted condition is expecte to true, but got false")
}
return nil
}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())

// update addon to trigger reconcile 3 times, and per-delete manifestwork should be deleted and not be re-created
for i := 0; i < 3; i++ {
gomega.Eventually(func() error {
addon, err := hubAddonClient.AddonV1alpha1().ManagedClusterAddOns(managedClusterName).
Get(context.Background(), testHostedAddonImpl.name, metav1.GetOptions{})
if err != nil {
return err
}
addon.Labels = map[string]string{"test": fmt.Sprintf("%d", i)}
_, err = hubAddonClient.AddonV1alpha1().ManagedClusterAddOns(managedClusterName).
Update(context.Background(), addon, metav1.UpdateOptions{})
return err
}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())

time.Sleep(2 * time.Second)
work, err = hubWorkClient.WorkV1().ManifestWorks(hostingClusterName).
Get(context.Background(), constants.PreDeleteHookHostingWorkName(addon.Namespace, addon.Name), metav1.GetOptions{})
gomega.Expect(errors.IsNotFound(err)).To(gomega.BeTrue())
}

// remove pending finalizer to delete addon
gomega.Eventually(func() error {
addon, err = hubAddonClient.AddonV1alpha1().ManagedClusterAddOns(managedClusterName).
Get(context.Background(), testHostedAddonImpl.name, metav1.GetOptions{})
if err != nil {
return err
}
addon.SetFinalizers([]string{})
_, err = hubAddonClient.AddonV1alpha1().ManagedClusterAddOns(managedClusterName).
Update(context.Background(), addon, metav1.UpdateOptions{})
return err
}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())

gomega.Eventually(func() error {
_, err := hubAddonClient.AddonV1alpha1().ManagedClusterAddOns(managedClusterName).
Get(context.Background(), testHostedAddonImpl.name, metav1.GetOptions{})
Expand All @@ -333,6 +390,5 @@ var _ = ginkgo.Describe("Agent hook deploy", func() {
}
return fmt.Errorf("addon is expceted to be deleted")
}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())

})
})

0 comments on commit 71f1b13

Please sign in to comment.