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

Derive pci device_spec edpm_derive_pci_device_spec #597

Merged

Conversation

vcandapp
Copy link
Contributor

@vcandapp vcandapp commented Mar 12, 2024

This role will generate the pci device_spec for sriov nic partitioning deployment.
The generated config file should be used to create device_spec configmap for custom nova dataplane service.

This has been re-factored from PR

@openshift-ci openshift-ci bot requested review from slagle and viroel March 12, 2024 17:09
@vcandapp vcandapp force-pushed the derive_device_spec branch 5 times, most recently from 83f2c57 to c79be83 Compare March 12, 2024 18:50
@vcandapp vcandapp changed the title [WIP] Derive pci device_spec edpm_derive_pci_device_spec Derive pci device_spec edpm_derive_pci_device_spec Mar 13, 2024
@vcandapp vcandapp force-pushed the derive_device_spec branch 2 times, most recently from f6cc507 to 5cf236a Compare March 13, 2024 07:58
# under the License.

- name: Derive the SR-IOV device_spec config map for nic-partitionin
ansible.builtin.import_tasks: derive_pci.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

@vcandapp please change derive_pci.yml file name like derive_pci_device_spec.yml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed file name for more clarity

when:
- not sriov_device_spec_conf_check.stat.exists
become: true
block:
Copy link
Contributor

@Jaganathancse Jaganathancse Mar 13, 2024

Choose a reason for hiding this comment

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

@vcandapp can we add one more task fof this block to print derived results. so that user can also use pod logs to create configmap for nova custom service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added output debug log

sriov_device_spec_map: "{{ edpm_derive_sriov_device_spec_list }}"
sriov_phydev_map: "{{edpm_derive_pci_device_spec_physmap_file }}"
sriov_device_spec_out_file: "{{ edpm_derive_pci_device_spec_conf_dir }}/{{ edpm_derive_pci_device_spec_conf_file }}"
register: derive_pci_devicespec
Copy link
Contributor

Choose a reason for hiding this comment

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

@vcandapp line 47 and 48 are not required.

Copy link
Contributor

Choose a reason for hiding this comment

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

derive_pci_devicespec.rc will throw error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be needed

- name: Check valid input for edpm_derive_sriov_device_spec_list
ansible.builtin.fail:
msg: "List of sriov device_spec cannot be empty - edpm_derive_sriov_device_spec_list"
when: edpm_derive_sriov_device_spec_list | length > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

@vcandapp Need to update condition length==0 for fail message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


.. code-block:: YAML

- name: "Configure DDP Package"
Copy link
Contributor

Choose a reason for hiding this comment

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

@vcandapp typo DDP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done



def get_allocated_pci_addresses(configs):
alloc_pci_info = []
Copy link
Contributor

Choose a reason for hiding this comment

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

same description missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done



def get_pci_passthrough_devicespec(user_config, pf, pci_addresses):
pci_passthrough_list = []
Copy link
Contributor

Choose a reason for hiding this comment

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

same description missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


def get_passthrough_config_for_all_pf(user_config,
system_configs,
allocated_pci):
Copy link
Contributor

Choose a reason for hiding this comment

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

same description missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

configs = []
src_config = ""
# Currently only "device_spec" is valid
valid_spec = ('device_spec')
Copy link
Contributor

Choose a reason for hiding this comment

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

@vcandapp can we reuse this variable PASSTHROUGH_DEVSPEC_KEY instead of valid_spec new variable again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No action, This function is removed now

sriov_phydev_map=None):
pci_passthrough = {}
system_configs = get_sriov_configs()
user_configs = sriov_device_spec
Copy link
Contributor

Choose a reason for hiding this comment

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

@vcandapp remove this line and specify user_configs as _run_derive_pci function argument sriov_device_spec,?
since no user_configs updates done. only for reading purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

- List of SR-IOV device_spec
type: list
required: true
default: ''
Copy link
Contributor

Choose a reason for hiding this comment

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

@vcandapp used required but default is empty. mutually exclusive and get internal error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

- ConfigMap file to be used in nic-partitioned deployment
type: str
required: true
default: '/var/lib/config-data/ansible-generated/derive_devicespec/20-sriov-nova.conf'
Copy link
Contributor

Choose a reason for hiding this comment

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

here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

type: str
required: false
default: ''
sriov_nova_conf_file:
Copy link
Contributor

Choose a reason for hiding this comment

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

sriov_device_spec_out_file is used in derive pci file and here sriov_nova_conf_file. need to update sync

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@vcandapp vcandapp force-pushed the derive_device_spec branch 6 times, most recently from a055be7 to cbce0c2 Compare March 14, 2024 12:23
changed_when: derive_pci_devicespec.rc == 0
- name: Print result of derived device_spec
debug:
msg: "{{ derive_pci_devicespec }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

@vcandapp Please use derive_pci_devicespec.device_spec instead of complete details

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated debug msg

@vcandapp vcandapp force-pushed the derive_device_spec branch 4 times, most recently from 04c1d18 to 02ec65d Compare March 14, 2024 14:08
@Jaganathancse
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Mar 14, 2024
Copy link
Contributor

@Jaganathancse Jaganathancse left a comment

Choose a reason for hiding this comment

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

Looks good.

@Jaganathancse
Copy link
Contributor

/approve

@vcandapp
Copy link
Contributor Author

vcandapp commented Mar 14, 2024

Did E-2-E testing with this PR in nic partitioned deployement, and here are the verified logs:

[WARNING]: running playbook inside collection osp.edpm

PLAY [Derive sriov device_spec] ************************************************

TASK [Gathering Facts] *********************************************************
Thursday 14 March 2024 16:49:17 +0000 (0:00:00.035) 0:00:00.035 ********
ok: [tigon30]
ok: [tigon29]

TASK [osp.edpm.edpm_derive_pci_device_spec : Validating arguments against arg spec 'main' - The main entry point for the edpm_derive_pci_device_spec role.] ***
Thursday 14 March 2024 16:49:19 +0000 (0:00:01.507) 0:00:01.542 ********
ok: [tigon29] => {"changed": false, "msg": "The arg spec validation passed", "validate_args_context": {"argument_spec_name": "main", "name": "edpm_derive_pci_device_spec", "path": "/usr/share/ansible/collections/ansible_collections/osp/edpm/roles/edpm_derive_pci_device_spec", "type": "role"}}
ok: [tigon30] => {"changed": false, "msg": "The arg spec validation passed", "validate_args_context": {"argument_spec_name": "main", "name": "edpm_derive_pci_device_spec", "path": "/usr/share/ansible/collections/ansible_collections/osp/edpm/roles/edpm_derive_pci_device_spec", "type": "role"}}

TASK [osp.edpm.edpm_derive_pci_device_spec : Check valid input for edpm_derive_sriov_device_spec_list] ***
Thursday 14 March 2024 16:49:19 +0000 (0:00:00.064) 0:00:01.606 ********
skipping: [tigon29] => {"changed": false, "skip_reason": "Conditional result was False"}
skipping: [tigon30] => {"changed": false, "skip_reason": "Conditional result was False"}

TASK [osp.edpm.edpm_derive_pci_device_spec : Create neutron-sriov-agent directories] ***
Thursday 14 March 2024 16:49:19 +0000 (0:00:00.052) 0:00:01.659 ********
changed: [tigon29] => {"changed": true, "gid": 1000, "group": "cloud-admin", "mode": "0755", "owner": "cloud-admin", "path": "/var/lib/config-data/ansible-generated/derive_devicespec", "secontext": "unconfined_u:object_r:container_file_t:s0", "size": 6, "state": "directory", "uid": 1000}
changed: [tigon30] => {"changed": true, "gid": 1000, "group": "cloud-admin", "mode": "0755", "owner": "cloud-admin", "path": "/var/lib/config-data/ansible-generated/derive_devicespec", "secontext": "unconfined_u:object_r:container_file_t:s0", "size": 6, "state": "directory", "uid": 1000}

TASK [osp.edpm.edpm_derive_pci_device_spec : Check if edpm node has the sriov nova conf] ***
Thursday 14 March 2024 16:49:19 +0000 (0:00:00.403) 0:00:02.063 ********
ok: [tigon29] => {"changed": false, "stat": {"exists": false}}
ok: [tigon30] => {"changed": false, "stat": {"exists": false}}

TASK [osp.edpm.edpm_derive_pci_device_spec : Run derive_pci_passthrough_devicespec] ***
Thursday 14 March 2024 16:49:20 +0000 (0:00:00.379) 0:00:02.442 ********
changed: [tigon30] => {"changed": true, "device_spec": {"device_spec": [{"address": "0000:17:04.6", "trusted": "true"}, {"address": "0000:17:04.4", "trusted": "true"}, {"address": "0000:17:04.2", "trusted": "true"}, {"address": "0000:17:04.3", "trusted": "true"}]}, "message": "PCI device_spec is generated", "rc": 0, "sucess": false}
changed: [tigon29] => {"changed": true, "device_spec": {"device_spec": [{"address": "0000:17:04.6", "trusted": "true"}, {"address": "0000:17:04.4", "trusted": "true"}, {"address": "0000:17:04.2", "trusted": "true"}, {"address": "0000:17:04.3", "trusted": "true"}]}, "message": "PCI device_spec is generated", "rc": 0, "sucess": false}

TASK [osp.edpm.edpm_derive_pci_device_spec : Print result of derived device_spec] ***
Thursday 14 March 2024 16:49:20 +0000 (0:00:00.394) 0:00:02.836 ********
ok: [tigon29] => {
"msg": {
"device_spec": [
{
"address": "0000:17:04.6",
"trusted": "true"
},
{
"address": "0000:17:04.4",
"trusted": "true"
},
{
"address": "0000:17:04.2",
"trusted": "true"
},
{
"address": "0000:17:04.3",
"trusted": "true"
}
]
}
}
ok: [tigon30] => {
"msg": {
"device_spec": [
{
"address": "0000:17:04.6",
"trusted": "true"
},
{
"address": "0000:17:04.4",
"trusted": "true"
},
{
"address": "0000:17:04.2",
"trusted": "true"
},
{
"address": "0000:17:04.3",
"trusted": "true"
}
]
}
}

PLAY RECAP
tigon29 : ok=6 changed=2 unreachable=0 failed=0 skipped=1 rescued=0 ignored=0
tigon30 : ok=6 changed=2 unreachable=0 failed=0 skipped=1 rescued=0 ignored=0
Thursday 14 March 2024 16:49:20 +0000 (0:00:00.146) 0:00:02.983

Gathering Facts --------------------------------------------------------- 1.51s
osp.edpm.edpm_derive_pci_device_spec : Create neutron-sriov-agent directories --- 0.40s
osp.edpm.edpm_derive_pci_device_spec : Run derive_pci_passthrough_devicespec --- 0.39s
osp.edpm.edpm_derive_pci_device_spec : Check if edpm node has the sriov nova conf --- 0.38s
osp.edpm.edpm_derive_pci_device_spec : Print result of derived device_spec --- 0.15s
osp.edpm.edpm_derive_pci_device_spec : Validating arguments against arg spec 'main' - The main entry point for the edpm_derive_pci_device_spec role. --- 0.06s
osp.edpm.edpm_derive_pci_device_spec : Check valid input for edpm_derive_sriov_device_spec_list --- 0.05s

This role will generate the pci device_spec for sriov
nic partitioning deployment.
The generated config file should be used to create device_spec
configmap for custom nova dataplane service.

*Added basic molecule test
*Unit tests
@openshift-ci openshift-ci bot added the lgtm label Mar 14, 2024
@Jaganathancse
Copy link
Contributor

/approve

Copy link
Contributor

@bshephar bshephar left a comment

Choose a reason for hiding this comment

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

Looks good. Glad to see some good test coverage and molecule tests. Be nice to have some end-to-end SR-IOV testing downstream to make sure we aren't missing anything.

Copy link
Contributor

openshift-ci bot commented Mar 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bshephar, Jaganathancse, vcandapp

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Jaganathancse Jaganathancse merged commit 12e8e16 into openstack-k8s-operators:main Mar 15, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants