example-cnf | Improvements for Mellanox support #661
Conversation
|
from change #661:
|
📝 WalkthroughWalkthroughThis change introduces a new variable, Changes
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
from change #661:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
roles/example_cnf_deploy/README.md (1)
117-119: Fix plural agreement and streamline phrasing.
Change "other type of cards" to "other types of cards" and tighten the Mellanox sentence for clarity.- This configuration is related to Intel cards. If you are using other type of cards, you need to apply the corresponding modifications (`device_id` and `vendor` will change, among others). For example, [for Mellanox cards](https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html/networking/hardware-networks#example-vf-use-in-dpdk-mode-mellanox_using-dpdk-and-rdma), you need to use `netdevice` instead of `vfio-pci`, and also enable RDMA (`is_rdma: true`). + This configuration is related to Intel cards. If you are using other types of cards, apply the corresponding modifications (`device_id` and `vendor` will change, among others`). For Mellanox cards, use `netdevice` instead of `vfio-pci` and enable RDMA (`is_rdma: true`) (see https://docs.redhat.com/.../example-vf-use-in-dpdk-mode-mellanox_using-dpdk-and-rdma).🧰 Tools
🪛 LanguageTool
[grammar] ~117-~117: In this context, ‘type’ should agree in number with the noun after ‘of’.
Context: ... to Intel cards. If you are using other type of cards, you need to apply the corresponding mo...(TYPE_OF_PLURAL)
[style] ~117-~117: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...mode-mellanox_using-dpdk-and-rdma), you need to usenetdeviceinstead ofvfio-pci, ...(REP_NEED_TO_VB)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
roles/example_cnf_deploy/README.md(2 hunks)roles/example_cnf_deploy/defaults/main.yml(1 hunks)roles/example_cnf_deploy/templates/grout-cr.yaml.j2(1 hunks)roles/example_cnf_deploy/templates/testpmd-cr.yaml.j2(1 hunks)roles/example_cnf_deploy/templates/trex-server-cr.yaml.j2(1 hunks)
🧰 Additional context used
🪛 LanguageTool
roles/example_cnf_deploy/README.md
[grammar] ~117-~117: In this context, ‘type’ should agree in number with the noun after ‘of’.
Context: ... to Intel cards. If you are using other type of cards, you need to apply the corresponding mo...
(TYPE_OF_PLURAL)
[style] ~117-~117: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...mode-mellanox_using-dpdk-and-rdma), you need to use netdevice instead of vfio-pci, ...
(REP_NEED_TO_VB)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Sanity Check (stable-2.17)
- GitHub Check: Sanity Check (stable-2.9)
- GitHub Check: Ansible-lint Check
🔇 Additional comments (6)
roles/example_cnf_deploy/defaults/main.yml (2)
70-74: Well-documented new feature for Mellanox supportThe comment clearly explains why privileged mode is necessary for Mellanox cards, providing good context about the netdevice vs vfio-pci difference and the requirement to access
/dev/vfio/vfio. The reference to the Red Hat solution article is particularly helpful.
74-74: Sensible security defaultSetting
ecd_enable_privileged_modetofalseby default is a good security practice - privileged mode should only be enabled when specifically needed.roles/example_cnf_deploy/templates/testpmd-cr.yaml.j2 (1)
7-7: Consistent implementation of privileged mode toggleThe template now properly uses the
ecd_enable_privileged_modevariable instead of a hardcoded value, making the TestPMD configuration consistent with the other components.roles/example_cnf_deploy/templates/trex-server-cr.yaml.j2 (1)
19-19: Good placement for privileged mode settingThe
privilegedfield is appropriately added to the spec section, making it clear how to configure the TRex server with privileged mode when needed for Mellanox cards.roles/example_cnf_deploy/templates/grout-cr.yaml.j2 (1)
7-7: Consistent implementation across all templatesThe Grout template now uses the same variable-based approach as the other templates, ensuring consistent behavior across all components when Mellanox cards are used.
roles/example_cnf_deploy/README.md (1)
48-48: Excellent addition ofecd_enable_privileged_mode.
This new variable clearly documents the need for privileged mode when using Mellanox cards and why a hostPath volume alone is insufficient.
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 3m 06s |
ramperher
force-pushed
the
example-cnf-nics
branch
from
b834ef8 to
fc4b8a9
Compare
|
from change #661:
|
1 similar comment
|
from change #661:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
roles/example_cnf_deploy/README.md (3)
48-48: Fix missing article for clarity.
Insert “the” before “host” for grammatical correctness:…requires access to /dev/vfio/vfio from the host. Using a volume…🧰 Tools
🪛 LanguageTool
[uncategorized] ~48-~48: You might be missing the article “the” here.
Context: ... requires access to /dev/vfio/vfio from host. Using a volume with hostPath is not en...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
116-116: Use plural for ‘types’ and refine phrasing.
Change “other type of cards” to “other types of cards” and consider rephrasing to reduce repetition (e.g., “If you are using cards from other vendors…”).🧰 Tools
🪛 LanguageTool
[grammar] ~116-~116: In this context, ‘type’ should agree in number with the noun after ‘of’.
Context: ... to Intel cards. If you are using other type of cards, you need to apply the corresponding mo...(TYPE_OF_PLURAL)
[style] ~116-~116: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...mode-mellanox_using-dpdk-and-rdma), you need to usenetdeviceinstead ofvfio-pci, ...(REP_NEED_TO_VB)
117-117: Improve reminder discoverability.
Consider moving the Mellanox privileged-mode note closer to theecd_enable_privileged_modevariable above or adding an inline link for easier reference.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
roles/example_cnf_deploy/README.md(2 hunks)roles/example_cnf_deploy/defaults/main.yml(1 hunks)roles/example_cnf_deploy/templates/grout-cr.yaml.j2(1 hunks)roles/example_cnf_deploy/templates/testpmd-cr.yaml.j2(1 hunks)roles/example_cnf_deploy/templates/trex-server-cr.yaml.j2(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- roles/example_cnf_deploy/templates/testpmd-cr.yaml.j2
- roles/example_cnf_deploy/templates/trex-server-cr.yaml.j2
- roles/example_cnf_deploy/templates/grout-cr.yaml.j2
- roles/example_cnf_deploy/defaults/main.yml
🧰 Additional context used
🪛 LanguageTool
roles/example_cnf_deploy/README.md
[uncategorized] ~48-~48: You might be missing the article “the” here.
Context: ... requires access to /dev/vfio/vfio from host. Using a volume with hostPath is not en...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~116-~116: In this context, ‘type’ should agree in number with the noun after ‘of’.
Context: ... to Intel cards. If you are using other type of cards, you need to apply the corresponding mo...
(TYPE_OF_PLURAL)
[style] ~116-~116: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...mode-mellanox_using-dpdk-and-rdma), you need to use netdevice instead of vfio-pci, ...
(REP_NEED_TO_VB)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Sanity Check (stable-2.17)
- GitHub Check: Sanity Check (stable-2.9)
- GitHub Check: Ansible-lint Check
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 56s |
ramperher
changed the title
|
from change #661: |
|
from change #661: |
|
recheck |
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 50s |
|
from change #661: |
|
from change #661: |
|
recheck |
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 45s |
|
from change #661: |
|
recheck |
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 46s |
|
from change #661: |
|
recheck |
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 52s |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
roles/example_cnf_deploy/README.md (2)
48-48: Nit: Improve grammar and clarity inecd_enable_privileged_modedescriptionThe sentence is a bit long and has a comma before “because,” plus “from host” should be “on the host.” Consider this more concise phrasing:
-| ecd_enable_privileged_mode | false | no | Enable container privileged mode, instead of setting specific capabilities. This is required when using Mellanox cards, because it requires access to /dev/vfio/vfio from host since it uses netdevice instead of vfio-pci. To achieve that, using a volume with hostPath is not enough, so that privileged mode is required (see [this](https://access.redhat.com/solutions/6560521) for an example of this). | +| ecd_enable_privileged_mode | false | no | Enable container privileged mode instead of setting specific capabilities. Required for Mellanox cards as they use netdevice instead of vfio-pci and need access to /dev/vfio/vfio on the host. A hostPath volume alone is insufficient, so privileged mode is necessary (see [this](https://access.redhat.com/solutions/6560521)). |🧰 Tools
🪛 LanguageTool
[formatting] ~48-~48: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...is is required when using Mellanox cards, because it requires access to /dev/vfio/vfio fr...(COMMA_BEFORE_BECAUSE)
[uncategorized] ~48-~48: You might be missing the article “the” here.
Context: ... requires access to /dev/vfio/vfio from host since it uses netdevice instead of vfio...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
116-116: Nit: Correct plural form for “type of cards”Use “types of cards” to agree in number:
-This configuration is related to Intel cards. If you are using other type of cards, you need to apply the corresponding modifications (`device_id` and `vendor` will change, among others). +This configuration applies to Intel cards. If you use other types of cards, adjust the corresponding parameters (`device_id`, `vendor`, etc.).🧰 Tools
🪛 LanguageTool
[grammar] ~116-~116: In this context, ‘type’ should agree in number with the noun after ‘of’.
Context: ... to Intel cards. If you are using other type of cards, you need to apply the corresponding mo...(TYPE_OF_PLURAL)
[style] ~116-~116: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...mode-mellanox_using-dpdk-and-rdma), you need to usenetdeviceinstead ofvfio-pci, ...(REP_NEED_TO_VB)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
roles/check_resource/tasks/wait-sriov.yml(1 hunks)roles/example_cnf_deploy/README.md(2 hunks)roles/example_cnf_deploy/defaults/main.yml(1 hunks)roles/example_cnf_deploy/templates/grout-cr.yaml.j2(1 hunks)roles/example_cnf_deploy/templates/testpmd-cr.yaml.j2(1 hunks)roles/example_cnf_deploy/templates/trex-server-cr.yaml.j2(1 hunks)roles/sriov_config/tasks/check_sriov_node_policy.yml(1 hunks)roles/sriov_config/templates/sriov-network.yml.j2(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- roles/example_cnf_deploy/templates/testpmd-cr.yaml.j2
- roles/example_cnf_deploy/templates/trex-server-cr.yaml.j2
- roles/example_cnf_deploy/templates/grout-cr.yaml.j2
- roles/example_cnf_deploy/defaults/main.yml
🧰 Additional context used
🪛 LanguageTool
roles/example_cnf_deploy/README.md
[formatting] ~48-~48: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...is is required when using Mellanox cards, because it requires access to /dev/vfio/vfio fr...
(COMMA_BEFORE_BECAUSE)
[uncategorized] ~48-~48: You might be missing the article “the” here.
Context: ... requires access to /dev/vfio/vfio from host since it uses netdevice instead of vfio...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~116-~116: In this context, ‘type’ should agree in number with the noun after ‘of’.
Context: ... to Intel cards. If you are using other type of cards, you need to apply the corresponding mo...
(TYPE_OF_PLURAL)
[style] ~116-~116: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...mode-mellanox_using-dpdk-and-rdma), you need to use netdevice instead of vfio-pci, ...
(REP_NEED_TO_VB)
[style] ~124-~124: Consider using “unable” to avoid wordiness.
Context: ...urrent setup, when launching TRex, it's not able to find the requested interfaces. Acco...
(NOT_ABLE_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Sanity Check (stable-2.17)
- GitHub Check: Sanity Check (stable-2.9)
- GitHub Check: Ansible-lint Check
🔇 Additional comments (5)
roles/example_cnf_deploy/README.md (1)
118-118: Docs: Emphasize Mellanox privileged mode requirementThe blockquote clearly highlights the need for privileged mode when using Mellanox cards. This callout aligns with the newly added variable and is easy to spot. Great addition!
roles/sriov_config/templates/sriov-network.yml.j2 (1)
20-20: Good fix for JSON serialization consistency!Adding
| to_jsonensures metaPlugins is properly serialized as JSON, consistent with other fields like capabilities and ipam in this template.roles/sriov_config/tasks/check_sriov_node_policy.yml (1)
35-43: Excellent addition for robust SR-IOV orchestration!This task properly handles the scenario where SR-IOV policy changes trigger node reboots. The retry/delay calculation matches the existing pattern, and the comments clearly explain the purpose.
roles/check_resource/tasks/wait-sriov.yml (2)
2-4: Smart addition of initial pause!The 60-second pause gives SriovNetworkNodeState time to initiate the update process before checking readiness conditions.
6-25: Great refactoring for improved clarity!The explicit JSON query variables and granular condition checks make this much more readable and maintainable than the previous complex filter approach. The underscore prefix for the register variable follows good conventions.
|
Testing is finished, this is ready for review. |
|
recheck |
|
from change #661:
|
1 similar comment
|
from change #661:
|
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 47s |
|
recheck |
|
from change #661:
|
1 similar comment
|
from change #661:
|
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 52s |
ramperher
force-pushed
the
example-cnf-nics
branch
from
db51d5a to
ec7cdb6
Compare
|
from change #661:
|
1 similar comment
|
from change #661:
|
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 51s |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
roles/example_cnf_deploy/README.md (2)
48-48: Remove unnecessary comma before "because".The clause after "because" is essential, so the comma before it can be dropped for smoother flow.
🧰 Tools
🪛 LanguageTool
[formatting] ~48-~48: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...is is required when using Mellanox cards, because it requires access to /dev/vfio/vfio fr...(COMMA_BEFORE_BECAUSE)
116-118: Streamline Mellanox guidance and fix grammar.
- Change "other type of cards" → "other types of cards".
- Consolidate the privileged-mode note into the main sentence and reduce repetition.
Example diff:
- This configuration is related to Intel cards. If you are using other type of cards, you need to apply the corresponding modifications (`device_id` and `vendor` will change, among others). For example, [for Mellanox cards](…), you need to use `netdevice` instead of `vfio-pci`, and also enable RDMA (`is_rdma: true`). It's also recommended to use `need_vhost_net: true`, and in case you want to include RDMA subsystem, you can do it with `meta_plugins: '{"type": "rdma"}'`. - > Remember that Mellanox requires containers to run in privileged mode. Enable it with `ecd_enable_privileged_mode: true`. + This configuration applies to Intel cards. For other types of cards, update `device_id`, `vendor`, etc. For Mellanox cards, use `netdevice` instead of `vfio-pci`, enable RDMA (`is_rdma: true`), set `need_vhost_net: true`, and optionally include the RDMA subsystem via `meta_plugins: '{"type": "rdma"}'`. Note that Mellanox NICs require privileged mode (`ecd_enable_privileged_mode: true`) to access `/dev/vfio/vfio`.🧰 Tools
🪛 LanguageTool
[grammar] ~116-~116: In this context, ‘type’ should agree in number with the noun after ‘of’.
Context: ... to Intel cards. If you are using other type of cards, you need to apply the corresponding mo...(TYPE_OF_PLURAL)
[style] ~116-~116: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...mode-mellanox_using-dpdk-and-rdma), you need to usenetdeviceinstead ofvfio-pci, ...(REP_NEED_TO_VB)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
roles/check_resource/tasks/wait-sriov.yml(1 hunks)roles/example_cnf_deploy/README.md(2 hunks)roles/example_cnf_deploy/defaults/main.yml(1 hunks)roles/example_cnf_deploy/templates/grout-cr.yaml.j2(1 hunks)roles/example_cnf_deploy/templates/testpmd-cr.yaml.j2(1 hunks)roles/example_cnf_deploy/templates/trex-server-cr.yaml.j2(1 hunks)roles/sriov_config/tasks/check_sriov_node_policy.yml(1 hunks)roles/sriov_config/templates/sriov-network.yml.j2(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- roles/example_cnf_deploy/templates/grout-cr.yaml.j2
- roles/sriov_config/tasks/check_sriov_node_policy.yml
- roles/example_cnf_deploy/defaults/main.yml
- roles/example_cnf_deploy/templates/testpmd-cr.yaml.j2
- roles/example_cnf_deploy/templates/trex-server-cr.yaml.j2
- roles/sriov_config/templates/sriov-network.yml.j2
- roles/check_resource/tasks/wait-sriov.yml
🧰 Additional context used
🪛 LanguageTool
roles/example_cnf_deploy/README.md
[formatting] ~48-~48: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...is is required when using Mellanox cards, because it requires access to /dev/vfio/vfio fr...
(COMMA_BEFORE_BECAUSE)
[grammar] ~116-~116: In this context, ‘type’ should agree in number with the noun after ‘of’.
Context: ... to Intel cards. If you are using other type of cards, you need to apply the corresponding mo...
(TYPE_OF_PLURAL)
[style] ~116-~116: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...mode-mellanox_using-dpdk-and-rdma), you need to use netdevice instead of vfio-pci, ...
(REP_NEED_TO_VB)
[style] ~124-~124: Consider using “unable” to avoid wordiness.
Context: ...urrent setup, when launching TRex, it's not able to find the requested interfaces. Acco...
(NOT_ABLE_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Ansible-lint Check
- GitHub Check: Sanity Check (stable-2.18)
- GitHub Check: Sanity Check (stable-2.9)
SUMMARY
ISSUE TYPE
Tests
Privileged mode is used, metaPlugins are appended to the SRIOV config of this setup, and the check for SriovNetworkNodeState is correctly integrated.
Tested the ping between the grout pod created by the automation and another grout instance created manually, since TRex cannot be used with Mellanox cards:
Automation with grout worked in a regular cluster. Changes were not affecting the default behavior.
Test-Hints: no-check