MULTIARCH-5058: Implement the node affinity scoring specs in the ClusterPodPlacementConfig #369
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pkenchap: This pull request references MULTIARCH-5058 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/assign @pkenchap |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
|
||
| const ( | ||
| // PluginName for NodeAffinityScoring. | ||
| PluginName = "NodeAffinityScoring" |
There was a problem hiding this comment.
Hi @pkenchap, a few initial notes of review:
- can we have two separate files in the common/plugins folder? One for the BasePlugin, another for the NodeAffinityScoring plugin.
- Can we also add an interface
IBasePluginwith one method for now,Name() string? - The interface IBasePlugin and struct BasePlugin can stay on the common/plugins/base_plugin.go
PluginNameshould be renamed toNodeAffinityScoringPluginNameso that other plugins can have their meaningful constant name accordingly.- I see all the fields (Enabled, Architecture, Weight) are tagged as
omitempty. I believe we should set them as always required. The onlyomitemptyfields should thepluginsstanza and each possible plugin's stanza's key within it. - Let's set
kubebuilderannotations to configure validation: https://book.kubebuilder.io/reference/markers/crd-validation
There was a problem hiding this comment.
Sure , I will make these changes . Thanks for the suggestions .
| } | ||
|
|
||
| // Score calculates the score of a node based on node affinity and architecture | ||
| func (n *NodeAffinityScoring) ArchWeight(ctx context.Context, pod *v1.Pod, node *v1.Node) int64 { |
There was a problem hiding this comment.
What's the purpose of this function?
| // of the pods. If left empty, all the namespaces are considered. | ||
| // The default sample allows to exclude all the namespaces where the | ||
| // +optional | ||
| Plugins plugins.NodeAffinityScoring `json:"plugins,omitempty"` |
There was a problem hiding this comment.
Plugins should be a more generic structure plugins.Plugins (and probably a pointer).
Something like
type Plugins struct {
NodeAffinityScoring *NodeAffinityScoring `json:"nodeAffinityScording,omitempty"
// Future plugins
}
|
|
||
| // NodeAffinityScoring is the plugin that implements the ScorePlugin interface | ||
| type NodeAffinityScoring struct { | ||
| Base PluginBase `json:"Base,omitempty" protobuf:"bytes,1,opt,name=Base"` |
There was a problem hiding this comment.
Let's use embedding to embed the PluginBase fields
| Base PluginBase `json:"Base,omitempty" protobuf:"bytes,1,opt,name=Base"` | |
| PluginBase `json:"Base,omitempty" protobuf:"bytes,1,opt,name=Base"` |
There was a problem hiding this comment.
pkenchap
force-pushed
the
MULTIARCH-5058
branch
from
2e21c43
to
2337fb3
Compare
…terPodPlacementConfig Signed-off-by: Punith Kenchappa <[email protected]>
pkenchap
force-pushed
the
MULTIARCH-5058
branch
from
2337fb3
to
7994305
Compare
|
@pkenchap: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| // NodeAffinityScoring is the plugin that implements the ScorePlugin interface. | ||
| type NodeAffinityScoring struct { | ||
| // Base is a required field for enabling/disabling the plugin. | ||
| Base PluginBase `json:"base" protobuf:"bytes,1,opt,name=base" kubebuilder:"validation:Required"` |
There was a problem hiding this comment.
| Base PluginBase `json:"base" protobuf:"bytes,1,opt,name=base" kubebuilder:"validation:Required"` | |
| PluginBase `json:"base" protobuf:"bytes,1,opt,name=base" kubebuilder:"validation:Required"` |
There was a problem hiding this comment.
Doesn't embedding suffice here?
| // Architecture must be a non-empty string. | ||
| Architecture string `json:"architecture" protobuf:"bytes,1,rep,name=architecture" kubebuilder:"validation:Required"` | ||
| // Weight must be a non-negative integer. | ||
| Weight int `json:"weight" protobuf:"bytes,2,rep,name=weight" kubebuilder:"validation:Required,validation:Minimum=0"` |
There was a problem hiding this comment.
are the kubebuilder tags verified?
We've always set them as comments above the field:
| Weight int `json:"weight" protobuf:"bytes,2,rep,name=weight" kubebuilder:"validation:Required,validation:Minimum=0"` | |
| // +kubebuilder:validation:Minimum:=0 | |
| // +kubebuilder:validation:Required | |
| Weight int `json:"weight" protobuf:"bytes,2,rep,name=weight"` |
(Look at similar ones in the code)
|
|
||
| const ( | ||
| // PluginName for NodeAffinityScoring. | ||
| PluginName = "NodeAffinityScoringPluginName" |
There was a problem hiding this comment.
| PluginName = "NodeAffinityScoringPluginName" | |
| NodeAffinityScoringPluginName = "NodeAffinityScoring" |
| ) | ||
|
|
||
| // Plugins represents the plugins configuration. | ||
| type Plugins struct { |
There was a problem hiding this comment.
Can this stay in a separate file too?
| } | ||
|
|
||
| // PluginBase is a common base for all plugins, requiring the Enabled field. | ||
| type PluginBase struct { |
There was a problem hiding this comment.
Task to add the nodeAffinityScoring for ClusterPodPlacemetnConfig API specs can be updated as in the following to host a skeleton of plugins, currently implementing only the nodeAffinityScoring.
Example: