| Message ID | 20201204121450.120730-1-cohuck@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] tests/acceptance: test hot(un)plug of ccw devices | expand |
Hi, On 12/4/20 9:14 AM, Cornelia Huck wrote: > Hotplug a virtio-net-ccw device, and then hotunplug it again. > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > > v1->v2: > - switch device id > - clear out dmesg before looking for CRW messages > > --- > tests/acceptance/machine_s390_ccw_virtio.py | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/tests/acceptance/machine_s390_ccw_virtio.py b/tests/acceptance/machine_s390_ccw_virtio.py > index 53b8484f8f9c..83c00190621b 100644 > --- a/tests/acceptance/machine_s390_ccw_virtio.py > +++ b/tests/acceptance/machine_s390_ccw_virtio.py > @@ -97,3 +97,19 @@ class S390CCWVirtioMachine(Test): > exec_command_and_wait_for_pattern(self, > 'cat /sys/bus/pci/devices/000a\:00\:00.0/function_id', > '0x0000000c') > + # add another device > + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') The problem is that `dmesg -c` will fail if you run the test with unprivileged user. - Wainer > + self.vm.command('device_add', driver='virtio-net-ccw', > + devno='fe.0.4711', id='net_4711') > + exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW') > + exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/', > + '0.0.4711') > + # and detach it again > + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') > + self.vm.command('device_del', id='net_4711') > + self.vm.event_wait(name='DEVICE_DELETED', > + match={'data': {'device': 'net_4711'}}) > + exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW') > + exec_command_and_wait_for_pattern(self, > + 'ls /sys/bus/ccw/devices/0.0.4711', > + 'No such file or directory')
On Fri, 4 Dec 2020 11:05:34 -0300 Wainer dos Santos Moschetta <wainersm@redhat.com> wrote: > Hi, > > On 12/4/20 9:14 AM, Cornelia Huck wrote: > > Hotplug a virtio-net-ccw device, and then hotunplug it again. > > > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > --- > > > > v1->v2: > > - switch device id > > - clear out dmesg before looking for CRW messages > > > > --- > > tests/acceptance/machine_s390_ccw_virtio.py | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/tests/acceptance/machine_s390_ccw_virtio.py b/tests/acceptance/machine_s390_ccw_virtio.py > > index 53b8484f8f9c..83c00190621b 100644 > > --- a/tests/acceptance/machine_s390_ccw_virtio.py > > +++ b/tests/acceptance/machine_s390_ccw_virtio.py > > @@ -97,3 +97,19 @@ class S390CCWVirtioMachine(Test): > > exec_command_and_wait_for_pattern(self, > > 'cat /sys/bus/pci/devices/000a\:00\:00.0/function_id', > > '0x0000000c') > > + # add another device > > + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') > > > The problem is that `dmesg -c` will fail if you run the test with > unprivileged user. Hm, why should that make a difference for a guest command? > > - Wainer > > > + self.vm.command('device_add', driver='virtio-net-ccw', > > + devno='fe.0.4711', id='net_4711') > > + exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW') > > + exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/', > > + '0.0.4711') > > + # and detach it again > > + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') > > + self.vm.command('device_del', id='net_4711') > > + self.vm.event_wait(name='DEVICE_DELETED', > > + match={'data': {'device': 'net_4711'}}) > > + exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW') > > + exec_command_and_wait_for_pattern(self, > > + 'ls /sys/bus/ccw/devices/0.0.4711', > > + 'No such file or directory')
On 12/4/20 11:08 AM, Cornelia Huck wrote: > On Fri, 4 Dec 2020 11:05:34 -0300 > Wainer dos Santos Moschetta <wainersm@redhat.com> wrote: > >> Hi, >> >> On 12/4/20 9:14 AM, Cornelia Huck wrote: >>> Hotplug a virtio-net-ccw device, and then hotunplug it again. >>> >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> >>> --- >>> >>> v1->v2: >>> - switch device id >>> - clear out dmesg before looking for CRW messages >>> >>> --- >>> tests/acceptance/machine_s390_ccw_virtio.py | 16 ++++++++++++++++ >>> 1 file changed, 16 insertions(+) >>> >>> diff --git a/tests/acceptance/machine_s390_ccw_virtio.py b/tests/acceptance/machine_s390_ccw_virtio.py >>> index 53b8484f8f9c..83c00190621b 100644 >>> --- a/tests/acceptance/machine_s390_ccw_virtio.py >>> +++ b/tests/acceptance/machine_s390_ccw_virtio.py >>> @@ -97,3 +97,19 @@ class S390CCWVirtioMachine(Test): >>> exec_command_and_wait_for_pattern(self, >>> 'cat /sys/bus/pci/devices/000a\:00\:00.0/function_id', >>> '0x0000000c') >>> + # add another device >>> + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') >> >> The problem is that `dmesg -c` will fail if you run the test with >> unprivileged user. > Hm, why should that make a difference for a guest command? Never mind, my brain mix host and guest very often.... Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com> > >> - Wainer >> >>> + self.vm.command('device_add', driver='virtio-net-ccw', >>> + devno='fe.0.4711', id='net_4711') >>> + exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW') >>> + exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/', >>> + '0.0.4711') >>> + # and detach it again >>> + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') >>> + self.vm.command('device_del', id='net_4711') >>> + self.vm.event_wait(name='DEVICE_DELETED', >>> + match={'data': {'device': 'net_4711'}}) >>> + exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW') >>> + exec_command_and_wait_for_pattern(self, >>> + 'ls /sys/bus/ccw/devices/0.0.4711', >>> + 'No such file or directory')
On Fri, 4 Dec 2020 17:13:22 -0300 Wainer dos Santos Moschetta <wainersm@redhat.com> wrote: > On 12/4/20 11:08 AM, Cornelia Huck wrote: > > On Fri, 4 Dec 2020 11:05:34 -0300 > > Wainer dos Santos Moschetta <wainersm@redhat.com> wrote: > > > >> Hi, > >> > >> On 12/4/20 9:14 AM, Cornelia Huck wrote: > >>> Hotplug a virtio-net-ccw device, and then hotunplug it again. > >>> > >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> > >>> --- > >>> > >>> v1->v2: > >>> - switch device id > >>> - clear out dmesg before looking for CRW messages > >>> > >>> --- > >>> tests/acceptance/machine_s390_ccw_virtio.py | 16 ++++++++++++++++ > >>> 1 file changed, 16 insertions(+) > >>> > >>> diff --git a/tests/acceptance/machine_s390_ccw_virtio.py b/tests/acceptance/machine_s390_ccw_virtio.py > >>> index 53b8484f8f9c..83c00190621b 100644 > >>> --- a/tests/acceptance/machine_s390_ccw_virtio.py > >>> +++ b/tests/acceptance/machine_s390_ccw_virtio.py > >>> @@ -97,3 +97,19 @@ class S390CCWVirtioMachine(Test): > >>> exec_command_and_wait_for_pattern(self, > >>> 'cat /sys/bus/pci/devices/000a\:00\:00.0/function_id', > >>> '0x0000000c') > >>> + # add another device > >>> + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') > >> > >> The problem is that `dmesg -c` will fail if you run the test with > >> unprivileged user. > > Hm, why should that make a difference for a guest command? > > > Never mind, my brain mix host and guest very often.... I'm sure you're not the only one :) > > Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com> Thanks! > > > > > >> - Wainer > >> > >>> + self.vm.command('device_add', driver='virtio-net-ccw', > >>> + devno='fe.0.4711', id='net_4711') > >>> + exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW') > >>> + exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/', > >>> + '0.0.4711') > >>> + # and detach it again > >>> + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') > >>> + self.vm.command('device_del', id='net_4711') > >>> + self.vm.event_wait(name='DEVICE_DELETED', > >>> + match={'data': {'device': 'net_4711'}}) > >>> + exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW') > >>> + exec_command_and_wait_for_pattern(self, > >>> + 'ls /sys/bus/ccw/devices/0.0.4711', > >>> + 'No such file or directory')
On 04/12/2020 13.14, Cornelia Huck wrote: > Hotplug a virtio-net-ccw device, and then hotunplug it again. > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > > v1->v2: > - switch device id > - clear out dmesg before looking for CRW messages > > --- > tests/acceptance/machine_s390_ccw_virtio.py | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/tests/acceptance/machine_s390_ccw_virtio.py b/tests/acceptance/machine_s390_ccw_virtio.py > index 53b8484f8f9c..83c00190621b 100644 > --- a/tests/acceptance/machine_s390_ccw_virtio.py > +++ b/tests/acceptance/machine_s390_ccw_virtio.py > @@ -97,3 +97,19 @@ class S390CCWVirtioMachine(Test): > exec_command_and_wait_for_pattern(self, > 'cat /sys/bus/pci/devices/000a\:00\:00.0/function_id', > '0x0000000c') > + # add another device > + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') > + self.vm.command('device_add', driver='virtio-net-ccw', > + devno='fe.0.4711', id='net_4711') > + exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW') Looking at this twice, I'm a little bit afraid that this could be racy - what if the kernel decides to emit the line with the "CRW" just after we executed the dmesg command? I'd maybe use something like this instead: exec_command_and_wait_for_pattern(self, 'while ! dmesg -c | grep CRW ; do sleep 1 ; done', '~ #') > + exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/', > + '0.0.4711') > + # and detach it again > + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') If adapt my above change, you could also get rid of this dmesg -c here (since it's done in the while loop already) > + self.vm.command('device_del', id='net_4711') > + self.vm.event_wait(name='DEVICE_DELETED', > + match={'data': {'device': 'net_4711'}}) > + exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW') dito > + exec_command_and_wait_for_pattern(self, > + 'ls /sys/bus/ccw/devices/0.0.4711', > + 'No such file or directory') With my suggestion applied: Reviewed-by: Thomas Huth <thuth@redhat.com>
On Mon, 7 Dec 2020 15:28:47 +0100 Thomas Huth <thuth@redhat.com> wrote: > On 04/12/2020 13.14, Cornelia Huck wrote: > > Hotplug a virtio-net-ccw device, and then hotunplug it again. > > > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > --- > > > > v1->v2: > > - switch device id > > - clear out dmesg before looking for CRW messages > > > > --- > > tests/acceptance/machine_s390_ccw_virtio.py | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/tests/acceptance/machine_s390_ccw_virtio.py b/tests/acceptance/machine_s390_ccw_virtio.py > > index 53b8484f8f9c..83c00190621b 100644 > > --- a/tests/acceptance/machine_s390_ccw_virtio.py > > +++ b/tests/acceptance/machine_s390_ccw_virtio.py > > @@ -97,3 +97,19 @@ class S390CCWVirtioMachine(Test): > > exec_command_and_wait_for_pattern(self, > > 'cat /sys/bus/pci/devices/000a\:00\:00.0/function_id', > > '0x0000000c') > > + # add another device > > + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') > > + self.vm.command('device_add', driver='virtio-net-ccw', > > + devno='fe.0.4711', id='net_4711') > > + exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW') > > Looking at this twice, I'm a little bit afraid that this could be racy - > what if the kernel decides to emit the line with the "CRW" just after we > executed the dmesg command? I'd maybe use something like this instead: > > exec_command_and_wait_for_pattern(self, > 'while ! dmesg -c | grep CRW ; do sleep 1 ; done', '~ #') Yes, you're right. Unless anyone can think of a better incantation? > > > + exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/', > > + '0.0.4711') > > + # and detach it again > > + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') > > If adapt my above change, you could also get rid of this dmesg -c here > (since it's done in the while loop already) I don't think so (there are two CRWs posted, and the loop might have caught the first one only.) > > > + self.vm.command('device_del', id='net_4711') > > + self.vm.event_wait(name='DEVICE_DELETED', > > + match={'data': {'device': 'net_4711'}}) > > + exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW') > > dito > > > + exec_command_and_wait_for_pattern(self, > > + 'ls /sys/bus/ccw/devices/0.0.4711', > > + 'No such file or directory') > > With my suggestion applied: > > Reviewed-by: Thomas Huth <thuth@redhat.com>
On 07/12/2020 17.30, Cornelia Huck wrote: > On Mon, 7 Dec 2020 15:28:47 +0100 > Thomas Huth <thuth@redhat.com> wrote: > >> On 04/12/2020 13.14, Cornelia Huck wrote: >>> Hotplug a virtio-net-ccw device, and then hotunplug it again. >>> >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> >>> --- >>> >>> v1->v2: >>> - switch device id >>> - clear out dmesg before looking for CRW messages >>> >>> --- >>> tests/acceptance/machine_s390_ccw_virtio.py | 16 ++++++++++++++++ >>> 1 file changed, 16 insertions(+) >>> >>> diff --git a/tests/acceptance/machine_s390_ccw_virtio.py b/tests/acceptance/machine_s390_ccw_virtio.py >>> index 53b8484f8f9c..83c00190621b 100644 >>> --- a/tests/acceptance/machine_s390_ccw_virtio.py >>> +++ b/tests/acceptance/machine_s390_ccw_virtio.py >>> @@ -97,3 +97,19 @@ class S390CCWVirtioMachine(Test): >>> exec_command_and_wait_for_pattern(self, >>> 'cat /sys/bus/pci/devices/000a\:00\:00.0/function_id', >>> '0x0000000c') >>> + # add another device >>> + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') >>> + self.vm.command('device_add', driver='virtio-net-ccw', >>> + devno='fe.0.4711', id='net_4711') >>> + exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW') >> >> Looking at this twice, I'm a little bit afraid that this could be racy - >> what if the kernel decides to emit the line with the "CRW" just after we >> executed the dmesg command? I'd maybe use something like this instead: >> >> exec_command_and_wait_for_pattern(self, >> 'while ! dmesg -c | grep CRW ; do sleep 1 ; done', '~ #') > > Yes, you're right. Unless anyone can think of a better incantation? You could maybe drop the sleep 1 to avoid delays... but that might burn more CPU cycles, so not sure what's better here? (Unfortunately, that "sleep" in the initrd does not support fractions, otherwise I'd suggest "sleep 0.1" instead) >> >>> + exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/', >>> + '0.0.4711') >>> + # and detach it again >>> + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') >> >> If adapt my above change, you could also get rid of this dmesg -c here >> (since it's done in the while loop already) > > I don't think so (there are two CRWs posted, and the loop might have > caught the first one only.) Oh, you're right. So let's better be safe than sorry and keep this dmesg -c. Thomas
On 07/12/2020 17.34, Thomas Huth wrote: > On 07/12/2020 17.30, Cornelia Huck wrote: >> On Mon, 7 Dec 2020 15:28:47 +0100 >> Thomas Huth <thuth@redhat.com> wrote: >> >>> On 04/12/2020 13.14, Cornelia Huck wrote: >>>> Hotplug a virtio-net-ccw device, and then hotunplug it again. >>>> >>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> >>>> --- > [...] >>>> + exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/', >>>> + '0.0.4711') >>>> + # and detach it again >>>> + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') >>> >>> If adapt my above change, you could also get rid of this dmesg -c here >>> (since it's done in the while loop already) >> >> I don't think so (there are two CRWs posted, and the loop might have >> caught the first one only.) > > Oh, you're right. So let's better be safe than sorry and keep this dmesg -c. Ok, as we had to discover during some testing, it's a bad idea to only wait for ' ' after the 'dmesg -c' since it matches too early, so that the device gets added while the dmesg command is still running. The following code is working for me instead: # add another device exec_command_and_wait_for_pattern(self, 'dmesg -c > /dev/null; echo dm-clear\ 1', 'dm-clear 1') self.vm.command('device_add', driver='virtio-net-ccw', devno='fe.0.4711', id='net_4711') exec_command_and_wait_for_pattern(self, 'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done', 'CRW reports') exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/', '0.0.4711') # and detach it again exec_command_and_wait_for_pattern(self, 'dmesg -c > /dev/null; echo dm-clear\ 2', 'dm-clear 2') self.vm.command('device_del', id='net_4711') self.vm.event_wait(name='DEVICE_DELETED', match={'data': {'device': 'net_4711'}}) exec_command_and_wait_for_pattern(self, 'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done', 'CRW reports') exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/0.0.4711', 'No such file or directory') HTH, Thomas
On Mon, 7 Dec 2020 19:40:36 +0100 Thomas Huth <thuth@redhat.com> wrote: > On 07/12/2020 17.34, Thomas Huth wrote: > > On 07/12/2020 17.30, Cornelia Huck wrote: > >> On Mon, 7 Dec 2020 15:28:47 +0100 > >> Thomas Huth <thuth@redhat.com> wrote: > >> > >>> On 04/12/2020 13.14, Cornelia Huck wrote: > >>>> Hotplug a virtio-net-ccw device, and then hotunplug it again. > >>>> > >>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> > >>>> --- > > > [...] > >>>> + exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/', > >>>> + '0.0.4711') > >>>> + # and detach it again > >>>> + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') > >>> > >>> If adapt my above change, you could also get rid of this dmesg -c here > >>> (since it's done in the while loop already) > >> > >> I don't think so (there are two CRWs posted, and the loop might have > >> caught the first one only.) > > > > Oh, you're right. So let's better be safe than sorry and keep this dmesg -c. > > Ok, as we had to discover during some testing, it's a bad idea to only wait > for ' ' after the 'dmesg -c' since it matches too early, so that the device > gets added while the dmesg command is still running. > > The following code is working for me instead: > > # add another device > exec_command_and_wait_for_pattern(self, > 'dmesg -c > /dev/null; echo dm-clear\ 1', 'dm-clear 1') > self.vm.command('device_add', driver='virtio-net-ccw', > devno='fe.0.4711', id='net_4711') > exec_command_and_wait_for_pattern(self, > 'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done', > 'CRW reports') > exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/', > '0.0.4711') > # and detach it again > exec_command_and_wait_for_pattern(self, > 'dmesg -c > /dev/null; echo dm-clear\ 2', 'dm-clear 2') > self.vm.command('device_del', id='net_4711') > self.vm.event_wait(name='DEVICE_DELETED', > match={'data': {'device': 'net_4711'}}) > exec_command_and_wait_for_pattern(self, > 'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done', > 'CRW reports') > exec_command_and_wait_for_pattern(self, > 'ls /sys/bus/ccw/devices/0.0.4711', > 'No such file or directory') Thanks for tracking this down, this works for me as well. I'll send a v3.
diff --git a/tests/acceptance/machine_s390_ccw_virtio.py b/tests/acceptance/machine_s390_ccw_virtio.py index 53b8484f8f9c..83c00190621b 100644 --- a/tests/acceptance/machine_s390_ccw_virtio.py +++ b/tests/acceptance/machine_s390_ccw_virtio.py @@ -97,3 +97,19 @@ class S390CCWVirtioMachine(Test): exec_command_and_wait_for_pattern(self, 'cat /sys/bus/pci/devices/000a\:00\:00.0/function_id', '0x0000000c') + # add another device + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') + self.vm.command('device_add', driver='virtio-net-ccw', + devno='fe.0.4711', id='net_4711') + exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW') + exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/', + '0.0.4711') + # and detach it again + exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ') + self.vm.command('device_del', id='net_4711') + self.vm.event_wait(name='DEVICE_DELETED', + match={'data': {'device': 'net_4711'}}) + exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW') + exec_command_and_wait_for_pattern(self, + 'ls /sys/bus/ccw/devices/0.0.4711', + 'No such file or directory')
Hotplug a virtio-net-ccw device, and then hotunplug it again. Signed-off-by: Cornelia Huck <cohuck@redhat.com> --- v1->v2: - switch device id - clear out dmesg before looking for CRW messages --- tests/acceptance/machine_s390_ccw_virtio.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)