CNF-15754: nodegroups refactor #1142
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Tal-or 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 |
openshift-ci
bot
added
the
approved
Tal-or
force-pushed
the
nodegroups_refactor
branch
from
9c0890f to
6d0b0c3
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@Tal-or: This pull request references CNF-15754 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.19.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. |
|
@Tal-or: This pull request references CNF-15754 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.19.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. |
Tal-or
force-pushed
the
nodegroups_refactor
branch
from
6d0b0c3 to
a34236e
Compare
A common interface for managing nodegroups. This interface will be inplemented for OpenShift and HyperShift platforms. Signed-off-by: Talor Itzhak <[email protected]>
Add common validation functions that will be used for both implementations of HyperShift and OpenShift. Signed-off-by: Talor Itzhak <[email protected]>
Implement the `Manager` infterface for OpenShift. The functionality is copied from validation package, with slight changes in the functions names and signatures, so no intended changes in the behavior. Signed-off-by: Talor Itzhak <[email protected]>
Implement the `Manager` infterface for HyperShift. The functionality is copied from validation package, with slight changes in the functions names and signatures, so no intended changes in the behavior. Signed-off-by: Talor Itzhak <[email protected]>
Consume the manager as part of the reconciler struct. In future commit we'll use the actual methods provided by the manager. Signed-off-by: Talor Itzhak <[email protected]>
replace the existing functions with the methods provided by the manager. No intended changes in the behavior. Signed-off-by: Talor Itzhak <[email protected]>
Now that we have `nodegroups` package it seems more appropriate to have a specific error for validation of nodegroups under a package with the same name. Signed-off-by: Talor Itzhak <[email protected]>
In hypershift case it will exit the functions doing nothing becuase the mcp slice it empty. In case that the user does provide an mcp selector on hypershift, we'll have a validation that will report about that. So we won't reach this code flow anyway. Signed-off-by: Talor Itzhak <[email protected]>
Signed-off-by: Talor Itzhak <[email protected]>
Now that each platform is implementing it own validation, we can remove the old validation which used to branch per each platform. Signed-off-by: Talor Itzhak <[email protected]>
Tal-or
force-pushed
the
nodegroups_refactor
branch
from
a34236e to
45a3060
Compare
|
/cc @ffromani |
|
/cc @ffromani PTAL this is a perquisite for the abstraction of the e2e serial suite so it will support HyperShift |
|
@Tal-or: GitHub didn't allow me to request PR reviews from the following users: a, e2e, PTAL, abstraction, it, will, HyperShift, this, of, suite, so, serial, is, perquisite, for, the. Note that only openshift-kni members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
pkg/nodegroups/nodegroups.go
Outdated
| import ( | ||
| "context" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
|
|
||
| nropv1 "github.com/openshift-kni/numaresources-operator/api/numaresourcesoperator/v1" | ||
| nodegroupv1 "github.com/openshift-kni/numaresources-operator/api/numaresourcesoperator/v1/helper/nodegroup" | ||
| ) | ||
|
|
||
| // Manager hold different operations that can be done against nodegroups | ||
| // TODO better name? | ||
| type Manager interface { | ||
| Validate(nodeGroups []nropv1.NodeGroup) error | ||
| FetchTrees(ctx context.Context, cli client.Client, nodeGroups []nropv1.NodeGroup) ([]nodegroupv1.Tree, error) | ||
| } |
There was a problem hiding this comment.
introducing a nodegroups package with specialization (and possibly subpackages) for openshift andhypershift is totally fine and the expected general direction. I don't think however we should add interfaces, and most notably this catchall interface.
In general (and I'm guilty myself plenty of times) if it's hard to give a name to an abstraction, this means it's likely the wrong abstraction.
In go interfaces are better defined in the consumer side rather than in the producer side, so I'd just aim for a code reshaping and probably keep a trivial if and a function pointer in the consumer side (controller)
refs:
https://bryanftan.medium.com/accept-interfaces-return-structs-in-go-d4cab29a301b
https://go.dev/wiki/CodeReviewComments#interfaces
There was a problem hiding this comment.
code reshaping and probably keep a trivial if and a function pointer in the consumer side (controller)
We're having 2 functions here Validate and FetchTrees, so it means 2 functions pointer which is less nicer.
Moving the interface to the controller package is fine by me, but how this is going to work when there's more than a single consumer?
Shall we have a defined interface for every consumer?
There was a problem hiding this comment.
I think 2 function pointers is actually better: there's no commonality in them at interface level, thus conjoining them in a single interface is a questionable abstraction.
The producer vs consumer story highlights a general golang guideline: packages should provide (return) concrete types, which in our case are function types.
If we look for a straight answer than yes: in the current code layout every hypotethical consumer would need to define its interfaces
There was a problem hiding this comment.
Elaborating further, we can totally define interfaces in the producer package (io is a glaring easy example) but still we would need to be careful and pursue the general direction of accept interfaces, return concrete types
|
Let's start with #1154 which is less invasive |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
Moving validation and tree fetching logic into helper functions which are per platform use.
Fixes: #1086 and #1085