-
Notifications
You must be signed in to change notification settings - Fork 128
Design proposal for the platform-orchestrator-abstraction #917
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
Design proposal for the platform-orchestrator-abstraction #917
Conversation
|
Thanks for your PR,
To skip the vendors CIs, Maintainers can use one of:
|
Pull Request Test Coverage Report for Build 19530595480Details
💛 - Coveralls |
|
Hi @zeeke @e0ne @adrianchiris please help move this one forward it's a critical feature I would like to get in as soon as possible. Thanks! |
| - **Comprehensive Testing**: Includes extensive unit tests with mocked HTTP calls | ||
| - **Error Handling**: Robust error handling for metadata service failures | ||
|
|
||
| This approach makes the SR-IOV Network Operator truly platform-agnostic while maintaining clean, maintainable code. No newline at end of file |
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.
nit: not really related to design doc
|
|
||
| ### Use Cases | ||
|
|
||
| 1. **Multi-Platform Support**: Enable the operator to run efficiently on different infrastructure platforms (bare metal, OpenStack, AWS,Oracle, etc.) with platform-specific optimizations |
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 think this is the only use-case here no ?
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 left both the platform and the orchestration as use-cases
| ### Goals | ||
|
|
||
| * Create a clean abstraction layer that separates platform-specific logic from orchestrator-specific logic | ||
| * Implement support for bare metal and OpenStack platforms |
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.
this is already implemented, you mean re-implement support for these platforms using the defined abstraction ?
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.
yep, I changed it
|
|
||
| * Create a clean abstraction layer that separates platform-specific logic from orchestrator-specific logic | ||
| * Implement support for bare metal and OpenStack platforms | ||
| * Implement support for Kubernetes and OpenShift orchestrators |
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.
same as above
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.
done
| ### Non-Goals | ||
|
|
||
| * Support all possible infrastructure platforms in the initial implementation | ||
| * Change existing API structures or user-facing interfaces |
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.
what does it mean ?
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.
no CRD changes neeeded. I updated to make it more clear.
| ClusterType() consts.ClusterType | ||
| Flavor() consts.ClusterFlavor | ||
| BeforeDrainNode(context.Context, *corev1.Node) (bool, error) | ||
| AfterCompleteDrainNode(context.Context, *corev1.Node) (bool, error) |
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.
nit: to be symmetric with BeforeDrainNode lets use AfterDrainNode
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 problem with AfterDrainNode that it shound like after the operator finish to drain a node.
but that case is for after the operator finish to release a node from a drain(uncordon)
I changed the function names let me know what you think about the new name
|
|
||
| ### Upgrade & Downgrade considerations | ||
|
|
||
| The abstraction layer is designed to be backward compatible. Existing configurations and behaviors are preserved, with the abstraction layer providing the same functionality through the new interface structure. |
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.
first sentence is irrelevant IMO.
|
|
||
| #### Factory Pattern | ||
|
|
||
| Both platform and orchestrator use factory patterns for instantiation: |
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.
facilitating easy extentions for new implementations
| ```golang | ||
| type Interface interface { | ||
| Init() error | ||
| GetHostHelpers() helper.HostHelpersInterface |
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 you have a usecase for different HostHelpersInterface implementations ?
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.
nope I just use that so the daemon can request the hostHelpers and I don't need to initiate it again and again in the code
adf2583 to
d12edf3
Compare
|
Hi @adrianchiris when you have time please take another look :) |
|
ping @zeeke @adrianchiris :) |
d12edf3 to
c7d1ef9
Compare
|
Hi @e0ne @adrianchiris I updated the design based on the actual implementation we just merged. please take a look when you have time :) |
adrianchiris
left a comment
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
Signed-off-by: Sebastian Sch <[email protected]>
c7d1ef9 to
48fb9f2
Compare
|
Merging this one |
No description provided.