-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat: MOFED subscription mounts #946
Conversation
Pull Request Test Coverage Report for Build 9208587816Details
💛 - Coveralls |
Related Red Hat Knowledgebase : https://access.redhat.com/solutions/5870841 |
@@ -127,6 +129,8 @@ func (p *provider) GetNodePools(filters ...Filter) []NodePool { | |||
} | |||
nodePool.Kernel = kernel | |||
|
|||
nodePool.ContainerRuntime = getContainerRuntime(node) |
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 assume that container runtime is the same for all nodes that are part of the pool ?
technically every attribute that is not part of the key in nodePoolMap may be different and you just save the last node that match.
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.
Yes, I assume all nodes in the same pool have the same container runtime.
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.
ack. technically i think we can have several runtimes. moreover it can be set dynamically with RuntimeClass[1] but i dont think we have a requirement to support it.
[1] https://kubernetes.io/docs/concepts/containers/runtime-class/
func (s *stateOFED) handleSubscriptionVolumes( | ||
ctx context.Context, osname string, runtime string, mounts *additionalVolumeMounts) error { | ||
reqLogger := log.FromContext(ctx) | ||
if osname == "rhel" && runtime != nodeinfo.CRIO { |
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 want to ignore an empty runtime value ?
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.
Can an empty runtime happen?
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.
field should be set. just wondering if we should add check for completness.
Add needed mounts for subscription in RHEL with non CRIO container runtime. Signed-off-by: Fred Rolland <frolland@nvidia.com>
/retest-image_scan |
/retest-nic_operator_helm |
Tested manually on cluster with RHEL 8.8, containerd 1.7.16 |
Add needed mounts for subscription in RHEL with non CRIO container runtime.