| Message ID | 20250329230141.3718282-7-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | debian / libvirt / devconfig fixes | expand |
On Sat, 29 Mar 2025, Luis Chamberlain wrote: > Folks trying to use kdevops and testing with fstests will quickly > find out a surprise that their config is not being parsed correctly > until later. > > Fix this by preventing bringup if the prefix has a dash. > > We use the dash to help parallelize testing filesystem profiles and > so the host prefix goes before the filesystem name and test profile. Where does that happen? All of my kdevops setups use multiple dashes in the host prefix (e.g. "kdevops-nfs-fstests-rhel-10-nightly"), so this breaks my setup, and I'd imagine it breaks Chuck's too. -Scott > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > playbooks/roles/gen_nodes/tasks/main.yml | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/playbooks/roles/gen_nodes/tasks/main.yml b/playbooks/roles/gen_nodes/tasks/main.yml > index d541dcbf1f54..8c6a1f705ee2 100644 > --- a/playbooks/roles/gen_nodes/tasks/main.yml > +++ b/playbooks/roles/gen_nodes/tasks/main.yml > @@ -18,6 +18,11 @@ > command: "id -g -n" > register: my_group > > +- name: Fail if kdevops_host_prefix contains a dash > + fail: > + msg: "Invalid kdevops_host_prefix '{{ kdevops_host_prefix }}'. The prefix cannot contain a dash ('-')." > + when: kdevops_host_prefix is search("-") > + > - name: Create guestfs directory > ansible.builtin.file: > path: "{{ guestfs_path }}" > -- > 2.47.2 > >
On Mon, Mar 31, 2025 at 01:35:41PM -0400, Scott Mayhew wrote: > On Sat, 29 Mar 2025, Luis Chamberlain wrote: > > > Folks trying to use kdevops and testing with fstests will quickly > > find out a surprise that their config is not being parsed correctly > > until later. > > > > Fix this by preventing bringup if the prefix has a dash. > > > > We use the dash to help parallelize testing filesystem profiles and > > so the host prefix goes before the filesystem name and test profile. > > Where does that happen? All of my kdevops setups use multiple dashes in > the host prefix (e.g. "kdevops-nfs-fstests-rhel-10-nightly"), so this > breaks my setup, and I'd imagine it breaks Chuck's too. Oh crap, sorry, does the NFS use of fstset not rely on the hostname to infer the target test section? If not then how about this: From a60443671181d889866265899ba8d1bc1d453d67 Mon Sep 17 00:00:00 2001 From: Luis Chamberlain <mcgrof@kernel.org> Date: Mon, 31 Mar 2025 11:28:14 -0700 Subject: [PATCH] gen_nodes: make prefix check only for fstests The oscheck script relies on the prefix heuristic to determine the target filesystem test section, but not all users of fstests use this. NFS uses its own heuristics. Fix this by localizing the prefix check for fstests dedicated tests only. Fixes: 2b61c9992d9ee9 ("gen_nodes: ensure kdevops prefix has no dashes") Reported-by: Scott Mayhew <smayhew@redhat.com> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- kconfigs/workflows/Kconfig | 1 + playbooks/roles/gen_nodes/defaults/main.yml | 1 + playbooks/roles/gen_nodes/tasks/main.yml | 4 +++- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/kconfigs/workflows/Kconfig b/kconfigs/workflows/Kconfig index 4f969c60cab8..3075cbc13a82 100644 --- a/kconfigs/workflows/Kconfig +++ b/kconfigs/workflows/Kconfig @@ -106,6 +106,7 @@ choice config KDEVOPS_WORKFLOW_DEDICATE_FSTESTS bool "fstests" select KDEVOPS_WORKFLOW_ENABLE_FSTESTS + output yaml help This will dedicate your configuration only to fstests. diff --git a/playbooks/roles/gen_nodes/defaults/main.yml b/playbooks/roles/gen_nodes/defaults/main.yml index 8ff9b87993a7..dff556c3b716 100644 --- a/playbooks/roles/gen_nodes/defaults/main.yml +++ b/playbooks/roles/gen_nodes/defaults/main.yml @@ -83,6 +83,7 @@ libvirt_largeio_base_size: 10240 libvirt_largeio_pow_limit: 12 kdevops_workflows_dedicated_workflow: False +kdevops_workflow_dedicate_fstests: False kdevops_workflow_enable_fstests: False kdevops_workflow_enable_blktests: False diff --git a/playbooks/roles/gen_nodes/tasks/main.yml b/playbooks/roles/gen_nodes/tasks/main.yml index 8c6a1f705ee2..f14f5879dc58 100644 --- a/playbooks/roles/gen_nodes/tasks/main.yml +++ b/playbooks/roles/gen_nodes/tasks/main.yml @@ -21,7 +21,9 @@ - name: Fail if kdevops_host_prefix contains a dash fail: msg: "Invalid kdevops_host_prefix '{{ kdevops_host_prefix }}'. The prefix cannot contain a dash ('-')." - when: kdevops_host_prefix is search("-") + when: + - kdevops_workflow_dedicate_fstests|bool + - kdevops_host_prefix is search("-") - name: Create guestfs directory ansible.builtin.file:
On Mon, 31 Mar 2025, Luis Chamberlain wrote: > On Mon, Mar 31, 2025 at 01:35:41PM -0400, Scott Mayhew wrote: > > On Sat, 29 Mar 2025, Luis Chamberlain wrote: > > > > > Folks trying to use kdevops and testing with fstests will quickly > > > find out a surprise that their config is not being parsed correctly > > > until later. > > > > > > Fix this by preventing bringup if the prefix has a dash. > > > > > > We use the dash to help parallelize testing filesystem profiles and > > > so the host prefix goes before the filesystem name and test profile. > > > > Where does that happen? All of my kdevops setups use multiple dashes in > > the host prefix (e.g. "kdevops-nfs-fstests-rhel-10-nightly"), so this > > breaks my setup, and I'd imagine it breaks Chuck's too. > > Oh crap, sorry, does the NFS use of fstset not rely on the hostname to > infer the target test section? If not then how about this: No, it still maps the hostname to the test section, and unless I'm missing something there's no special handling. I use the same naming scheme for SMB as well, although I don't have nearly as many SMB workflows running as NFS. Unfortunately I'm using dedicated workflows for all my stuff so this patch won't work. Maybe just add a KDEVOPS_HOSTS_PREFIX_ALLOW_DASHES config option to allow this check to be bypassed? > > From a60443671181d889866265899ba8d1bc1d453d67 Mon Sep 17 00:00:00 2001 > From: Luis Chamberlain <mcgrof@kernel.org> > Date: Mon, 31 Mar 2025 11:28:14 -0700 > Subject: [PATCH] gen_nodes: make prefix check only for fstests > > The oscheck script relies on the prefix heuristic to determine > the target filesystem test section, but not all users of fstests > use this. NFS uses its own heuristics. Fix this by localizing the > prefix check for fstests dedicated tests only. > > Fixes: 2b61c9992d9ee9 ("gen_nodes: ensure kdevops prefix has no dashes") > Reported-by: Scott Mayhew <smayhew@redhat.com> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > kconfigs/workflows/Kconfig | 1 + > playbooks/roles/gen_nodes/defaults/main.yml | 1 + > playbooks/roles/gen_nodes/tasks/main.yml | 4 +++- > 3 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/kconfigs/workflows/Kconfig b/kconfigs/workflows/Kconfig > index 4f969c60cab8..3075cbc13a82 100644 > --- a/kconfigs/workflows/Kconfig > +++ b/kconfigs/workflows/Kconfig > @@ -106,6 +106,7 @@ choice > config KDEVOPS_WORKFLOW_DEDICATE_FSTESTS > bool "fstests" > select KDEVOPS_WORKFLOW_ENABLE_FSTESTS > + output yaml > help > This will dedicate your configuration only to fstests. > > diff --git a/playbooks/roles/gen_nodes/defaults/main.yml b/playbooks/roles/gen_nodes/defaults/main.yml > index 8ff9b87993a7..dff556c3b716 100644 > --- a/playbooks/roles/gen_nodes/defaults/main.yml > +++ b/playbooks/roles/gen_nodes/defaults/main.yml > @@ -83,6 +83,7 @@ libvirt_largeio_base_size: 10240 > libvirt_largeio_pow_limit: 12 > > kdevops_workflows_dedicated_workflow: False > +kdevops_workflow_dedicate_fstests: False > kdevops_workflow_enable_fstests: False > kdevops_workflow_enable_blktests: False > > diff --git a/playbooks/roles/gen_nodes/tasks/main.yml b/playbooks/roles/gen_nodes/tasks/main.yml > index 8c6a1f705ee2..f14f5879dc58 100644 > --- a/playbooks/roles/gen_nodes/tasks/main.yml > +++ b/playbooks/roles/gen_nodes/tasks/main.yml > @@ -21,7 +21,9 @@ > - name: Fail if kdevops_host_prefix contains a dash > fail: > msg: "Invalid kdevops_host_prefix '{{ kdevops_host_prefix }}'. The prefix cannot contain a dash ('-')." > - when: kdevops_host_prefix is search("-") > + when: > + - kdevops_workflow_dedicate_fstests|bool > + - kdevops_host_prefix is search("-") > > - name: Create guestfs directory > ansible.builtin.file: > -- > 2.47.2 >
On Mon, Mar 31, 2025 at 03:14:28PM -0400, Scott Mayhew wrote: > On Mon, 31 Mar 2025, Luis Chamberlain wrote: > > > On Mon, Mar 31, 2025 at 01:35:41PM -0400, Scott Mayhew wrote: > > > On Sat, 29 Mar 2025, Luis Chamberlain wrote: > > > > > > > Folks trying to use kdevops and testing with fstests will quickly > > > > find out a surprise that their config is not being parsed correctly > > > > until later. > > > > > > > > Fix this by preventing bringup if the prefix has a dash. > > > > > > > > We use the dash to help parallelize testing filesystem profiles and > > > > so the host prefix goes before the filesystem name and test profile. > > > > > > Where does that happen? All of my kdevops setups use multiple dashes in > > > the host prefix (e.g. "kdevops-nfs-fstests-rhel-10-nightly"), so this > > > breaks my setup, and I'd imagine it breaks Chuck's too. > > > > Oh crap, sorry, does the NFS use of fstset not rely on the hostname to > > infer the target test section? If not then how about this: > > No, it still maps the hostname to the test section, and unless I'm > missing something there's no special handling. I use the same naming > scheme for SMB as well, although I don't have nearly as many SMB workflows > running as NFS. > > Unfortunately I'm using dedicated workflows for all my stuff so this > patch won't work. Maybe just add a KDEVOPS_HOSTS_PREFIX_ALLOW_DASHES > config option to allow this check to be bypassed? Yeah sure we can add KDEVOPS_HOSTS_PREFIX_ALLOW_DASHES, at least now we have a TODO item to review if in fact prefixes with dashes may break some existing heuristics for nfs/smb workoads. How is the configuration for fstests inferred for these workloads then? The exisitng 'make fstests-baseline' will use the hostname and extract the first part up to to the first dash, and then replace remaining "-" with "_", and we use that to infer what test section we will use from the /var/lib/xfstests/config/*.config file used. So for example, my extrafluff-ext4-4k host will use ext4_4k test section for its test when run. How is the test section inferred for NFS? Luis
On Mon, 31 Mar 2025, Luis Chamberlain wrote: > On Mon, Mar 31, 2025 at 03:14:28PM -0400, Scott Mayhew wrote: > > On Mon, 31 Mar 2025, Luis Chamberlain wrote: > > > > > On Mon, Mar 31, 2025 at 01:35:41PM -0400, Scott Mayhew wrote: > > > > On Sat, 29 Mar 2025, Luis Chamberlain wrote: > > > > > > > > > Folks trying to use kdevops and testing with fstests will quickly > > > > > find out a surprise that their config is not being parsed correctly > > > > > until later. > > > > > > > > > > Fix this by preventing bringup if the prefix has a dash. > > > > > > > > > > We use the dash to help parallelize testing filesystem profiles and > > > > > so the host prefix goes before the filesystem name and test profile. > > > > > > > > Where does that happen? All of my kdevops setups use multiple dashes in > > > > the host prefix (e.g. "kdevops-nfs-fstests-rhel-10-nightly"), so this > > > > breaks my setup, and I'd imagine it breaks Chuck's too. > > > > > > Oh crap, sorry, does the NFS use of fstset not rely on the hostname to > > > infer the target test section? If not then how about this: > > > > No, it still maps the hostname to the test section, and unless I'm > > missing something there's no special handling. I use the same naming > > scheme for SMB as well, although I don't have nearly as many SMB workflows > > running as NFS. > > > > Unfortunately I'm using dedicated workflows for all my stuff so this > > patch won't work. Maybe just add a KDEVOPS_HOSTS_PREFIX_ALLOW_DASHES > > config option to allow this check to be bypassed? > > Yeah sure we can add KDEVOPS_HOSTS_PREFIX_ALLOW_DASHES, at least now > we have a TODO item to review if in fact prefixes with dashes may > break some existing heuristics for nfs/smb workoads. > > How is the configuration for fstests inferred for these workloads then? > The exisitng 'make fstests-baseline' will use the hostname and extract > the first part up to to the first dash, and then replace remaining Maybe we're not looking at the same stuff. Where are you seeing "the first part up to the first dash"? I'm looking at this bit which starts on line 1241 in playbooks/roles/fstests/tasks/main.yml: ---8<--- - name: Run fstests using ./oscheck.sh --print-start --journal {{ fstests_journal }} --print-done --test-section {{ fstests_section }} {{ oscheck_extra_args }} {{ all_limit_tests }} vars: fstests_section: "{{ ansible_host | regex_replace(kdevops_host_prefix + '-') | regex_replace('-dev') | regex_replace('-', '_') }}" ---8<--- I see that as 1. strip off the entire host prefix *and* the first dash 2. strip off the '-dev' suffix 3. replace the remaining dashes with underscores If I boil that down to a simple test, that appears to be exactly what it does: $ cat test.yml --- - name: Just a test hosts: all tasks: - name: Import extra_vars.yaml include_vars: extra_vars.yaml - name: Just a task vars: fstests_section: "{{ ansible_host | regex_replace(kdevops_host_prefix + '-') | regex_replace('-dev') | regex_replace('-', '_') }}" ansible.builtin.debug: var: fstests_section $ ansible-playbook -i hosts test.yml ---8<--- TASK [Just a task] *************************************************************************************************************************************************************************************************************************** ok: [kdevops-nfs-fstests-rhel-10-nightly-nfs-tls] => { "fstests_section": "nfs_tls" } ok: [kdevops-nfs-fstests-rhel-10-nightly-nfs-v42] => { "fstests_section": "nfs_v42" } ok: [kdevops-nfs-fstests-rhel-10-nightly-nfs-v40] => { "fstests_section": "nfs_v40" } ok: [kdevops-nfs-fstests-rhel-10-nightly-nfs-v3] => { "fstests_section": "nfs_v3" } ok: [kdevops-nfs-fstests-rhel-10-nightly-nfsd] => { "fstests_section": "nfsd" } ---8<--- Those match the sections in playbooks/roles/fstests/templates/nfs/nfs.config -Scott > "-" with "_", and we use that to infer what test section we will use > from the /var/lib/xfstests/config/*.config file used. > > So for example, my extrafluff-ext4-4k host will use ext4_4k test section > for its test when run. How is the test section inferred for NFS? > > Luis >
diff --git a/playbooks/roles/gen_nodes/tasks/main.yml b/playbooks/roles/gen_nodes/tasks/main.yml index d541dcbf1f54..8c6a1f705ee2 100644 --- a/playbooks/roles/gen_nodes/tasks/main.yml +++ b/playbooks/roles/gen_nodes/tasks/main.yml @@ -18,6 +18,11 @@ command: "id -g -n" register: my_group +- name: Fail if kdevops_host_prefix contains a dash + fail: + msg: "Invalid kdevops_host_prefix '{{ kdevops_host_prefix }}'. The prefix cannot contain a dash ('-')." + when: kdevops_host_prefix is search("-") + - name: Create guestfs directory ansible.builtin.file: path: "{{ guestfs_path }}"
Folks trying to use kdevops and testing with fstests will quickly find out a surprise that their config is not being parsed correctly until later. Fix this by preventing bringup if the prefix has a dash. We use the dash to help parallelize testing filesystem profiles and so the host prefix goes before the filesystem name and test profile. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- playbooks/roles/gen_nodes/tasks/main.yml | 5 +++++ 1 file changed, 5 insertions(+)