-
Notifications
You must be signed in to change notification settings - Fork 40
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
fix the pre-delete hook is recreated in hosted mode #231
fix the pre-delete hook is recreated in hosted mode #231
Conversation
/assign @qiujian16 |
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 think you need to add an integration test. e.g. add another finalizer to pend the addon deletion, and check if manifestwork recreation.
bc69d8d
to
5e3c4f8
Compare
@qiujian16 have added. |
} | ||
return nil | ||
}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred()) | ||
|
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 we wait for 5 seconds and check if work is not recreated?
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.
yes, updated the addon cr label to trigger reconcile 3 times to check if the per-delete manifestwork is not re-created.
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, zhiweiyin318 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 |
5e3c4f8
to
263cd26
Compare
leave to @zhujian7 for final review |
return addon, err | ||
} | ||
} else { | ||
// cleanup is safe here since there is no case which HookManifestCompleted condition is changed from true to false. |
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 it fails to cleanup hook work/remove finalizer here, it also could result in creating the hook again, right?
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.
no, if go to clean part, that means the hookcompleted condition is true, so that means the addon after this in the queue has the true hookcompleted condition, that will never go to apply manifestwork part.
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 it fails at the cleanup part, it returns, so no chance to add the hook completed condition right?
But it is a corner 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.
no, if it fails at the cleanup part, that means the hookcompleted condition has been true. no place to change the condition to false.
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.
got it!
@@ -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, completed,without completed condition", |
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.
the name is confusing, I do not know if it is complete or not.
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.
done
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.") |
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.
t.Errorf("HookManifestCompleted condition should be true,but got false.") | |
t.Errorf("HookManifestCompleted condition should be true, but got false.") |
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.
done
} | ||
addon.Labels = map[string]string{"test": fmt.Sprintf("%d", i)} | ||
_, err = hubAddonClient.AddonV1alpha1().ManagedClusterAddOns(managedClusterName). | ||
Update(context.Background(), addon, metav1.UpdateOptions{}) |
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.
consider putting the getting/updating into a retry block since it may fail with a conflict.
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.
done
b8b2b00
to
5c56dde
Compare
Signed-off-by: Zhiwei Yin <zyin@redhat.com>
5c56dde
to
4299e17
Compare
/lgtm |
71f1b13
into
open-cluster-management-io:main
No description provided.