appliedmanifestwork eviction #190
Conversation
skeeey
force-pushed
the
amw-eviction
branch
2 times, most recently
from
70bb703
to
ea5e66c
Compare
|
refer to open-cluster-management-io/ocm#123 |
|
/assign @qiujian16 |
|
/hold |
| if evictionStartTime == nil { | ||
| copied := appliedManifestWork.DeepCopy() | ||
| copied.Status.EvictionStartTime = &metav1.Time{Time: now} | ||
| _, err := m.appliedManifestWorkClient.UpdateStatus(ctx, copied, metav1.UpdateOptions{}) |
| hubHash string | ||
| agentID string | ||
| evictionGracePeriod time.Duration | ||
| rateLimiter workqueue.RateLimiter | ||
| } | ||
|
|
||
| func NewUnManagedAppliedWorkController( |
There was a problem hiding this comment.
let's add some doc for this controller.
| } | ||
|
|
||
| return factory.New(). | ||
| WithInformersQueueKeyFunc(func(obj runtime.Object) string { |
There was a problem hiding this comment.
if we do not add this eventhandler, how could the controller know when a manifestwork is deleted?
There was a problem hiding this comment.
I think this controller will not handle the manifestwork deletion, it just need to know the relating manifestwork can be found from hub or not, right?
There was a problem hiding this comment.
added, and I also remove the https://github.com/open-cluster-management-io/work/pull/190/files#diff-7d50afd5fa68dd772f0ace48c6b0b06cb79e670b3604840100f9db9a89d6ed59R74, so that we have a uniform way to handle if the manifestwork is missing on the hub
| func (m *unmanagedAppliedWorkController) patchEvictionStartTime(ctx context.Context, | ||
| appliedManifestWork *workapiv1.AppliedManifestWork, evictionStartTime *metav1.Time) error { | ||
| oldData, err := json.Marshal(workapiv1.AppliedManifestWork{ | ||
| Status: appliedManifestWork.Status, |
There was a problem hiding this comment.
we should only patch the eviction time, right?
|
/unhold |
pkg/helper/helpers.go
Outdated
| @@ -378,6 +378,22 @@ func AppliedManifestworkHubHashFilter(hubHash string) factory.EventFilterFunc { | |||
| } | |||
| } | |||
|
|
|||
| // AppliedManifestWorkEvictionFilter filter the appliedmanifestwork belonging to this hub or this work agent | |||
| func AppliedManifestWorkEvictionFilter(hubHash, agentID string) factory.EventFilterFunc { | |||
There was a problem hiding this comment.
We need some explanation of this filter, why evictionfilter needs to consider hub hash and agent id. Or we name it as AppliedManifestWorkFilterByHubHashOrAgentID
| func(obj runtime.Object) string { | ||
| accessor, _ := meta.Accessor(obj) | ||
| return accessor.GetName() | ||
| }, helper.AppliedManifestWorkEvictionFilter(hubHash, agentID), appliedManifestWorkInformer.Informer()). |
There was a problem hiding this comment.
why we care about hubhash with different agent? Consider another agent with the same hubHash but different agent id.
There was a problem hiding this comment.
there is one case the hub is unchanged, but the klusterlet cr is recreated
There was a problem hiding this comment.
changed the filter to AppliedManifestworkAgentIDFilter, we only check the a work agent owned appliedmanifestwork
|
/approve |
Signed-off-by: Wei Liu <liuweixa@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, skeeey 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 |
|
/unhold |
openshift-merge-robot
merged commit ee923cd
into
open-cluster-management-io:main
Signed-off-by: Wei Liu <liuweixa@redhat.com>
No description provided.