Skip to content

refactor: separate eviction constraints to constraints.go #1693

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

Merged
merged 1 commit into from
May 19, 2025

Conversation

googs1025
Copy link
Member

  • separate eviction constraints to constraints.go

more info base on: #1665 (review)

@k8s-ci-robot k8s-ci-robot requested review from damemi and jklaw90 May 19, 2025 01:57
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 19, 2025
@googs1025 googs1025 force-pushed the constraints_refactor branch from 3bb2ec3 to 16a80b2 Compare May 19, 2025 01:59
@googs1025
Copy link
Member Author

/assign @ingvagabund

😄 ptal

if priorityThreshold != nil && (priorityThreshold.Value != nil || len(priorityThreshold.Name) > 0) {
thresholdPriority, err := utils.GetPriorityValueFromPriorityThreshold(context.TODO(), handle.ClientSet(), priorityThreshold)
if err != nil {
klog.Errorf("failed to get priority threshold: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to return the error so it can be properly handled.

return nil
}
}
return func(pod *v1.Pod) error { return nil }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function will be a no-op and will be invoked every time a pod is checked for eviction. What about to
change the function signature to return []constraint and return nil instead?

return nil
}
}
return func(pod *v1.Pod) error { return nil }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

return nil
}
}
return func(pod *v1.Pod) error { return nil }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

}
}
}
return func(pod *v1.Pod) error { return nil }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

if labelSelector != nil {
selector, err := metav1.LabelSelectorAsSelector(labelSelector)
if err != nil {
klog.Error(err, "could not get selector from label selector")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

indexName := "metadata.ownerReferences"
indexer, err := getPodIndexerByOwnerRefs(indexName, handle)
if err != nil {
klog.Error(err, "could not get pod indexer by ownerRefs")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

return nil
}
}
return func(pod *v1.Pod) error { return nil }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

return nil
}
}
return func(pod *v1.Pod) error { return nil }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

return nil
}
}
return func(pod *v1.Pod) error { return nil }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@googs1025 googs1025 force-pushed the constraints_refactor branch from 6e79122 to b3aeca7 Compare May 19, 2025 10:03
@ingvagabund
Copy link
Contributor

/approve
/lgtm

Thank you

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 19, 2025
@k8s-ci-robot k8s-ci-robot merged commit c8912ac into kubernetes-sigs:master May 19, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants