-
Notifications
You must be signed in to change notification settings - Fork 5
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
chore: remove the pod group #9
Conversation
Signed-off-by: Vacant2333 <vacant2333@gmail.com>
|
||
// This map contains the workload that we can handle now, the controllers will create PodGroup for them. | ||
var workloadGVKMap = map[schema.GroupVersionKind]struct{}{ | ||
var workloadGVKMap = map[schema.GroupVersionKind]NewWorkloadFunc{ |
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.
Do we need expose a method to let other workloads register themselves?
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 we can register each workload ourselves to the workloadGVKMap
, how do u think?
return nil, err | ||
} | ||
|
||
resource, err := dc.dynamicClient.Resource(mapping.Resource).Namespace(ref.Namespace).Get(context.Background(), ref.Name, metav1.GetOptions{}) |
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.
Is there a way that we can get resource directly and avoid API request, seems it's heavy.
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 didn't find a better way because we had to get the corresponding resources from rb, but in dispatcher we weren't sure which resources to handle
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.
OK.
It's better to add ut, can submit a seperate pr. |
@@ -77,7 +78,7 @@ func ResourceBindings(ar admissionv1.AdmissionReview) *admissionv1.AdmissionResp | |||
} | |||
|
|||
// Check if its workload, skip suspend if not. | |||
isWorkload, err := utils.IsWorkload(rb.Spec.Resource) | |||
isWorkload, _, err := workload.TryGetNewWorkloadFunc(rb.Spec.Resource) |
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 depend on this condition to check whether rb reference to a workload? In Karmada, based on rb.spec.Replicas>0 to determine whether it is a workload, but now this judgment is based on whether the workload has been registered, if there is no registration of workload judgment result is wrong.
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 workload
means the workloads we support, like volcano job, when we support the other workload like deployment, then we can handle it, else we should't suspend it
ok, i will submit a ut pr for it after this pr |
/lgtm |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: william-wang 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 |
for now, we didnt need the pod group, we can use resourcebinding to managet hte tasks, and implement the worklaod interface, we can supports more resource like deployment