Skip to content
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

Use ConfigMap to store supported NIC model list #145

Merged
merged 1 commit into from
Jun 22, 2021

Conversation

pliurh
Copy link
Collaborator

@pliurh pliurh commented Jun 9, 2021

Use a ConfigMap 'supported-nic-ids' to store the NIC model list
to replace the current hardcoded list. Deprecate the ConfigMap
'unsupported-nic-ids'.

Also deprecate the machanism which allow users can change the
content of ConfigMap on the fly. Now users have to restart the
webhook and config daemon pods after changing the ConfigMap.

api/v1/helper.go Outdated
sort.Slice(NicIdMap, func(i, j int) bool {
ip, _ := strconv.ParseInt(NicIdMap[i], 0, 32)
jp, _ := strconv.ParseInt(NicIdMap[j], 0, 32)
return ip < jp
Copy link
Member

Choose a reason for hiding this comment

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

ip & jp always resolve to 0
https://play.golang.org/p/rqy0xpHulpG

return err
}
for _, v := range cm.Data {
NicIdMap = append(NicIdMap, v)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we make the existing NicIdMap blank? Its currently populated on Ln 40.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh, forget to remove the old initialization code.

@zshi-redhat
Copy link
Collaborator

How will configmap be updated when new IDs are added in configmap during upgrade?

metadata:
name: supported-nic-ids
data:
I40e_XXV710: "8086 158a 154c"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we define a consistent name format for the data keys?
For example:
<vendor_driver_model_name>
Intel_ice_E810_Columbiaville
Nvidia_mlx5_MT42882_ConnectX-5

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we can do that. But keep in mind, the NIC name here is not used in the code logic, it is just for readability.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pliurh are you going to update the keys here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

@pliurh
Copy link
Collaborator Author

pliurh commented Jun 10, 2021

How will configmap be updated when new IDs are added in configmap during upgrade?

During an upgrade, the ConfigMap shall be updated first, then the pod images. So that we can guarantee pods start with the right ConfigMap.

@zshi-redhat
Copy link
Collaborator

How will configmap be updated when new IDs are added in configmap during upgrade?

During an upgrade, the ConfigMap shall be updated first, then the pod images. So that we can guarantee pods start with the right ConfigMap.

Who updates the configMap? and How?
Does it need to be deleted and re-created manually? The name of configMap doesn't change across upgrade and I don't see a logic in the controller to update the configMap.

@pliurh
Copy link
Collaborator Author

pliurh commented Jun 10, 2021

Users shall update the ConfigMap for the upstream version. For the downstream version, the OLM can update the ConfigMap. operator-framework/operator-lifecycle-manager#1434

Use a ConfigMap 'supported-nic-ids' to store the NIC model list
to replace the current hardcoded list. Deprecate the ConfigMap
'unsupported-nic-ids'.

Also deprecate the machanism which allow users can change the
content of ConfigMap on the fly. Now users have to restart the
webhook and config daemon pods after changing the ConfigMap.
@zshi-redhat
Copy link
Collaborator

/lgtm

@github-actions github-actions bot added the lgtm label Jun 17, 2021
@pliurh
Copy link
Collaborator Author

pliurh commented Jun 17, 2021

close #86

@pliurh
Copy link
Collaborator Author

pliurh commented Jun 17, 2021

@adrianchiris PTAL

@zshi-redhat
Copy link
Collaborator

close #86

I think this PR address part of issue #86: to move NIC id map to configMap.
There is still another part in the issue: allows more NICs to be used with SR-IOV Operator **directly**
I understand the second part is to not rely on the map for admitting use of new NICs.

@zshi-redhat
Copy link
Collaborator

/cc @akaris-redhat
This removes unsupported-nic-ids configmap.

@pliurh pliurh merged commit 0af44ee into k8snetworkplumbingwg:master Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants