Message ID | 20210224212654.1146167-1-wainersm@redhat.com (mailing list archive) |
---|---|
Headers | show |
Series | tests/acceptance: Handle tests with "cpu" tag | expand |
On Wed, Feb 24, 2021 at 06:26:51PM -0300, Wainer dos Santos Moschetta wrote: > Currently the acceptance tests tagged with "machine" have the "-M TYPE" > automatically added to the list of arguments of the QEMUMachine object. > In other words, that option is passed to the launched QEMU. On this > series it is implemented the same feature but instead for tests marked > with "cpu". > Good! > There is a caveat, however, in case the test needs additional arguments to > the CPU type they cannot be passed via tag, because the tags parser split > values by comma. For example, in tests/acceptance/x86_cpu_model_versions.py, > there are cases where: > > * -cpu is set to "Cascadelake-Server,x-force-features=on,check=off,enforce=off" > * if it was tagged like "cpu:Cascadelake-Server,x-force-features=on,check=off,enforce=off" > then the parser would break it into 4 tags ("cpu:Cascadelake-Server", > "x-force-features=on", "check=off", "enforce=off") > * resulting on "-cpu Cascadelake-Server" and the remaining arguments are ignored. > > For the example above, one should tag it (or not at all) as "cpu:Cascadelake-Server" > AND self.vm.add_args('-cpu', "Cascadelake-Server,x-force-features=on,check=off,enforce=off"), > and that results on something like: > > "qemu-system-x86_64 (...) -cpu Cascadelake-Server -cpu Cascadelake-Server,x-force-features=on,check=off,enforce=off". > There are clearly two problems here: 1) the tag is meant to be succinct, so that it can be used by users selecting which tests to run. At the same time, it's a waste to throw away the other information or keep it duplicate or incosistent. 2) QEMUMachine doesn't keep track of command line arguments (add_args() makes it pretty clear what's doing). But, on this type of use case, a "set_args()" is desirable, in which case it would overwrite the existing arguments for a given command line option. > QEMU is going to ignore the first -cpu argument. See the patch 0003 for a reference. > But this would still be creating a QEMU command line with multiple '-cpu' arguments, right? I understand this could be useful for testing the behavior of the parameter parsing (if that's intended) but it's bad practice to be generating incorrect command line in tests. Maybe just by tackling issue #2 this could be avoided. Cheers, - Cleber.
Added John and Eduardo, On 3/9/21 3:52 PM, Cleber Rosa wrote: > On Wed, Feb 24, 2021 at 06:26:51PM -0300, Wainer dos Santos Moschetta wrote: >> Currently the acceptance tests tagged with "machine" have the "-M TYPE" >> automatically added to the list of arguments of the QEMUMachine object. >> In other words, that option is passed to the launched QEMU. On this >> series it is implemented the same feature but instead for tests marked >> with "cpu". >> > Good! > >> There is a caveat, however, in case the test needs additional arguments to >> the CPU type they cannot be passed via tag, because the tags parser split >> values by comma. For example, in tests/acceptance/x86_cpu_model_versions.py, >> there are cases where: >> >> * -cpu is set to "Cascadelake-Server,x-force-features=on,check=off,enforce=off" >> * if it was tagged like "cpu:Cascadelake-Server,x-force-features=on,check=off,enforce=off" >> then the parser would break it into 4 tags ("cpu:Cascadelake-Server", >> "x-force-features=on", "check=off", "enforce=off") >> * resulting on "-cpu Cascadelake-Server" and the remaining arguments are ignored. >> >> For the example above, one should tag it (or not at all) as "cpu:Cascadelake-Server" >> AND self.vm.add_args('-cpu', "Cascadelake-Server,x-force-features=on,check=off,enforce=off"), >> and that results on something like: >> >> "qemu-system-x86_64 (...) -cpu Cascadelake-Server -cpu Cascadelake-Server,x-force-features=on,check=off,enforce=off". >> > There are clearly two problems here: > > 1) the tag is meant to be succinct, so that it can be used by users > selecting which tests to run. At the same time, it's a waste > to throw away the other information or keep it duplicate or > incosistent. > > 2) QEMUMachine doesn't keep track of command line arguments > (add_args() makes it pretty clear what's doing). But, on this type > of use case, a "set_args()" is desirable, in which case it would > overwrite the existing arguments for a given command line option. I like the idea of a "set_args()" to QEMUMachine as you describe above but it needs further discussion because I can see at least one corner case; for example, one can set the machine type as either -machine or -M, then what key it should be searched-and-replaced (if any) on the list of args? Unlike your suggestion, I thought on implement the method to deal with a single argument at time, as: def set_arg(self, arg: Union[str, list], value: str) -> None: """ Set the value of an argument from the list of extra arguments to be given to the QEMU binary. If the argument does not exist then it is added to the list. If the ``arg`` parameter is a list then it will search and replace all occurencies (if any). Otherwise a new argument is added and it is used the first value of the ``arg`` list. """ pass Does it sound good to you? Thanks! Wainer >> QEMU is going to ignore the first -cpu argument. See the patch 0003 for a reference. >> > But this would still be creating a QEMU command line with multiple > '-cpu' arguments, right? I understand this could be useful for > testing the behavior of the parameter parsing (if that's intended) but > it's bad practice to be generating incorrect command line in tests. > > Maybe just by tackling issue #2 this could be avoided. > > Cheers, > - Cleber.
On 3/17/21 3:16 PM, Wainer dos Santos Moschetta wrote: > Added John and Eduardo, > > On 3/9/21 3:52 PM, Cleber Rosa wrote: >> On Wed, Feb 24, 2021 at 06:26:51PM -0300, Wainer dos Santos Moschetta >> wrote: >>> Currently the acceptance tests tagged with "machine" have the "-M TYPE" >>> automatically added to the list of arguments of the QEMUMachine object. >>> In other words, that option is passed to the launched QEMU. On this >>> series it is implemented the same feature but instead for tests marked >>> with "cpu". >>> >> Good! >> >>> There is a caveat, however, in case the test needs additional >>> arguments to >>> the CPU type they cannot be passed via tag, because the tags parser >>> split >>> values by comma. For example, in >>> tests/acceptance/x86_cpu_model_versions.py, >>> there are cases where: >>> >>> * -cpu is set to >>> "Cascadelake-Server,x-force-features=on,check=off,enforce=off" >>> * if it was tagged like >>> "cpu:Cascadelake-Server,x-force-features=on,check=off,enforce=off" >>> then the parser would break it into 4 tags >>> ("cpu:Cascadelake-Server", >>> "x-force-features=on", "check=off", "enforce=off") >>> * resulting on "-cpu Cascadelake-Server" and the remaining >>> arguments are ignored. >>> >>> For the example above, one should tag it (or not at all) as >>> "cpu:Cascadelake-Server" >>> AND self.vm.add_args('-cpu', >>> "Cascadelake-Server,x-force-features=on,check=off,enforce=off"), >>> and that results on something like: >>> >>> "qemu-system-x86_64 (...) -cpu Cascadelake-Server -cpu >>> Cascadelake-Server,x-force-features=on,check=off,enforce=off". >>> >> There are clearly two problems here: >> >> 1) the tag is meant to be succinct, so that it can be used by users >> selecting which tests to run. At the same time, it's a waste >> to throw away the other information or keep it duplicate or >> incosistent. >> >> 2) QEMUMachine doesn't keep track of command line arguments >> (add_args() makes it pretty clear what's doing). But, on this type >> of use case, a "set_args()" is desirable, in which case it would >> overwrite the existing arguments for a given command line option. > > I like the idea of a "set_args()" to QEMUMachine as you describe above > but it needs further discussion because I can see at least one corner > case; for example, one can set the machine type as either -machine or > -M, then what key it should be searched-and-replaced (if any) on the > list of args? > > Unlike your suggestion, I thought on implement the method to deal with a > single argument at time, as: > > def set_arg(self, arg: Union[str, list], value: str) -> None: > """ > Set the value of an argument from the list of extra arguments > to be > given to the QEMU binary. If the argument does not exist then > it is > added to the list. > > If the ``arg`` parameter is a list then it will search and > replace all > occurencies (if any). Otherwise a new argument is added and it is > used the first value of the ``arg`` list. > """ > pass > > Does it sound good to you? > > Thanks! > > Wainer > A little hokey, but I suppose that's true of our CLI interface in general. I'd prefer not get into the business of building a "config" inside the python module if we can help it right now, but if "setting" individual args is something you truly need to do, I won't stand in the way. Do what's least-gross. --js >>> QEMU is going to ignore the first -cpu argument. See the patch 0003 >>> for a reference. >>> >> But this would still be creating a QEMU command line with multiple >> '-cpu' arguments, right? I understand this could be useful for >> testing the behavior of the parameter parsing (if that's intended) but >> it's bad practice to be generating incorrect command line in tests. >> >> Maybe just by tackling issue #2 this could be avoided. >> >> Cheers, >> - Cleber.
On Tue, Mar 23, 2021 at 05:01:09PM -0400, John Snow wrote: > On 3/17/21 3:16 PM, Wainer dos Santos Moschetta wrote: > > Added John and Eduardo, > > > > On 3/9/21 3:52 PM, Cleber Rosa wrote: > > > On Wed, Feb 24, 2021 at 06:26:51PM -0300, Wainer dos Santos > > > Moschetta wrote: > > > > Currently the acceptance tests tagged with "machine" have the "-M TYPE" > > > > automatically added to the list of arguments of the QEMUMachine object. > > > > In other words, that option is passed to the launched QEMU. On this > > > > series it is implemented the same feature but instead for tests marked > > > > with "cpu". > > > > > > > Good! > > > > > > > There is a caveat, however, in case the test needs additional > > > > arguments to > > > > the CPU type they cannot be passed via tag, because the tags > > > > parser split > > > > values by comma. For example, in > > > > tests/acceptance/x86_cpu_model_versions.py, > > > > there are cases where: > > > > > > > > * -cpu is set to > > > > "Cascadelake-Server,x-force-features=on,check=off,enforce=off" > > > > * if it was tagged like > > > > "cpu:Cascadelake-Server,x-force-features=on,check=off,enforce=off" > > > > then the parser would break it into 4 tags > > > > ("cpu:Cascadelake-Server", > > > > "x-force-features=on", "check=off", "enforce=off") > > > > * resulting on "-cpu Cascadelake-Server" and the remaining > > > > arguments are ignored. > > > > > > > > For the example above, one should tag it (or not at all) as > > > > "cpu:Cascadelake-Server" > > > > AND self.vm.add_args('-cpu', > > > > "Cascadelake-Server,x-force-features=on,check=off,enforce=off"), > > > > and that results on something like: > > > > > > > > "qemu-system-x86_64 (...) -cpu Cascadelake-Server -cpu > > > > Cascadelake-Server,x-force-features=on,check=off,enforce=off". > > > > > > > There are clearly two problems here: > > > > > > 1) the tag is meant to be succinct, so that it can be used by users > > > selecting which tests to run. At the same time, it's a waste > > > to throw away the other information or keep it duplicate or > > > incosistent. > > > > > > 2) QEMUMachine doesn't keep track of command line arguments > > > (add_args() makes it pretty clear what's doing). But, on this type > > > of use case, a "set_args()" is desirable, in which case it would > > > overwrite the existing arguments for a given command line option. > > > > I like the idea of a "set_args()" to QEMUMachine as you describe above > > but it needs further discussion because I can see at least one corner > > case; for example, one can set the machine type as either -machine or > > -M, then what key it should be searched-and-replaced (if any) on the > > list of args? > > > > Unlike your suggestion, I thought on implement the method to deal with a > > single argument at time, as: > > > > def set_arg(self, arg: Union[str, list], value: str) -> None: > > """ > > Set the value of an argument from the list of extra arguments > > to be > > given to the QEMU binary. If the argument does not exist then > > it is > > added to the list. > > > > If the ``arg`` parameter is a list then it will search and > > replace all > > occurencies (if any). Otherwise a new argument is added and it is > > used the first value of the ``arg`` list. > > """ > > pass > > > > Does it sound good to you? > > > > Thanks! > > > > Wainer > > > > A little hokey, but I suppose that's true of our CLI interface in general. > > I'd prefer not get into the business of building a "config" inside the > python module if we can help it right now, but if "setting" individual args > is something you truly need to do, I won't stand in the way. > > Do what's least-gross. I don't have any specific suggestions on how the API should look like, but I'm having trouble understanding the documentation above. I don't know what "it will search and replace all occurrences" means. Occurrences of what? I don't understand what "it is used the first value of the `arg` list" means, either. I understand you are going to use the first value of the list, but you don't say what you are going to do with it.
Hi, On 4/7/21 5:01 PM, Eduardo Habkost wrote: > On Tue, Mar 23, 2021 at 05:01:09PM -0400, John Snow wrote: >> On 3/17/21 3:16 PM, Wainer dos Santos Moschetta wrote: >>> Added John and Eduardo, >>> >>> On 3/9/21 3:52 PM, Cleber Rosa wrote: >>>> On Wed, Feb 24, 2021 at 06:26:51PM -0300, Wainer dos Santos >>>> Moschetta wrote: >>>>> Currently the acceptance tests tagged with "machine" have the "-M TYPE" >>>>> automatically added to the list of arguments of the QEMUMachine object. >>>>> In other words, that option is passed to the launched QEMU. On this >>>>> series it is implemented the same feature but instead for tests marked >>>>> with "cpu". >>>>> >>>> Good! >>>> >>>>> There is a caveat, however, in case the test needs additional >>>>> arguments to >>>>> the CPU type they cannot be passed via tag, because the tags >>>>> parser split >>>>> values by comma. For example, in >>>>> tests/acceptance/x86_cpu_model_versions.py, >>>>> there are cases where: >>>>> >>>>> * -cpu is set to >>>>> "Cascadelake-Server,x-force-features=on,check=off,enforce=off" >>>>> * if it was tagged like >>>>> "cpu:Cascadelake-Server,x-force-features=on,check=off,enforce=off" >>>>> then the parser would break it into 4 tags >>>>> ("cpu:Cascadelake-Server", >>>>> "x-force-features=on", "check=off", "enforce=off") >>>>> * resulting on "-cpu Cascadelake-Server" and the remaining >>>>> arguments are ignored. >>>>> >>>>> For the example above, one should tag it (or not at all) as >>>>> "cpu:Cascadelake-Server" >>>>> AND self.vm.add_args('-cpu', >>>>> "Cascadelake-Server,x-force-features=on,check=off,enforce=off"), >>>>> and that results on something like: >>>>> >>>>> "qemu-system-x86_64 (...) -cpu Cascadelake-Server -cpu >>>>> Cascadelake-Server,x-force-features=on,check=off,enforce=off". >>>>> >>>> There are clearly two problems here: >>>> >>>> 1) the tag is meant to be succinct, so that it can be used by users >>>> selecting which tests to run. At the same time, it's a waste >>>> to throw away the other information or keep it duplicate or >>>> incosistent. >>>> >>>> 2) QEMUMachine doesn't keep track of command line arguments >>>> (add_args() makes it pretty clear what's doing). But, on this type >>>> of use case, a "set_args()" is desirable, in which case it would >>>> overwrite the existing arguments for a given command line option. >>> I like the idea of a "set_args()" to QEMUMachine as you describe above >>> but it needs further discussion because I can see at least one corner >>> case; for example, one can set the machine type as either -machine or >>> -M, then what key it should be searched-and-replaced (if any) on the >>> list of args? >>> >>> Unlike your suggestion, I thought on implement the method to deal with a >>> single argument at time, as: >>> >>> def set_arg(self, arg: Union[str, list], value: str) -> None: >>> """ >>> Set the value of an argument from the list of extra arguments >>> to be >>> given to the QEMU binary. If the argument does not exist then >>> it is >>> added to the list. >>> >>> If the ``arg`` parameter is a list then it will search and >>> replace all >>> occurencies (if any). Otherwise a new argument is added and it is >>> used the first value of the ``arg`` list. >>> """ >>> pass >>> >>> Does it sound good to you? >>> >>> Thanks! >>> >>> Wainer >>> >> A little hokey, but I suppose that's true of our CLI interface in general. >> >> I'd prefer not get into the business of building a "config" inside the >> python module if we can help it right now, but if "setting" individual args >> is something you truly need to do, I won't stand in the way. >> >> Do what's least-gross. > I don't have any specific suggestions on how the API should look > like, but I'm having trouble understanding the documentation > above. > > I don't know what "it will search and replace all occurrences" > means. Occurrences of what? > > I don't understand what "it is used the first value of the `arg` > list" means, either. I understand you are going to use the first > value of the list, but you don't say what you are going to do > with it. The documentation was indeed confusing but, please, disregard it. Based on John's comments on this thread I decided to not introduce yet another specialized function to QEMUMachine class. Instead I added the "args" property so that users will have access to QEMUMachine._args to change it whatever they like. You will find that implemented on the v2 of this series: '[PATCH v2 0/7] tests/acceptance: Handle tests with "cpu" tag' Thanks! - Wainer >
On 4/9/21 10:53 AM, Wainer dos Santos Moschetta wrote: > Hi, > > On 4/7/21 5:01 PM, Eduardo Habkost wrote: >> On Tue, Mar 23, 2021 at 05:01:09PM -0400, John Snow wrote: >>> On 3/17/21 3:16 PM, Wainer dos Santos Moschetta wrote: >>>> Added John and Eduardo, >>>> >>>> On 3/9/21 3:52 PM, Cleber Rosa wrote: >>>>> On Wed, Feb 24, 2021 at 06:26:51PM -0300, Wainer dos Santos >>>>> Moschetta wrote: >>>>>> Currently the acceptance tests tagged with "machine" have the "-M >>>>>> TYPE" >>>>>> automatically added to the list of arguments of the QEMUMachine >>>>>> object. >>>>>> In other words, that option is passed to the launched QEMU. On this >>>>>> series it is implemented the same feature but instead for tests >>>>>> marked >>>>>> with "cpu". >>>>>> >>>>> Good! >>>>> >>>>>> There is a caveat, however, in case the test needs additional >>>>>> arguments to >>>>>> the CPU type they cannot be passed via tag, because the tags >>>>>> parser split >>>>>> values by comma. For example, in >>>>>> tests/acceptance/x86_cpu_model_versions.py, >>>>>> there are cases where: >>>>>> >>>>>> * -cpu is set to >>>>>> "Cascadelake-Server,x-force-features=on,check=off,enforce=off" >>>>>> * if it was tagged like >>>>>> "cpu:Cascadelake-Server,x-force-features=on,check=off,enforce=off" >>>>>> then the parser would break it into 4 tags >>>>>> ("cpu:Cascadelake-Server", >>>>>> "x-force-features=on", "check=off", "enforce=off") >>>>>> * resulting on "-cpu Cascadelake-Server" and the remaining >>>>>> arguments are ignored. >>>>>> >>>>>> For the example above, one should tag it (or not at all) as >>>>>> "cpu:Cascadelake-Server" >>>>>> AND self.vm.add_args('-cpu', >>>>>> "Cascadelake-Server,x-force-features=on,check=off,enforce=off"), >>>>>> and that results on something like: >>>>>> >>>>>> "qemu-system-x86_64 (...) -cpu Cascadelake-Server -cpu >>>>>> Cascadelake-Server,x-force-features=on,check=off,enforce=off". >>>>>> >>>>> There are clearly two problems here: >>>>> >>>>> 1) the tag is meant to be succinct, so that it can be used by users >>>>> selecting which tests to run. At the same time, it's a waste >>>>> to throw away the other information or keep it duplicate or >>>>> incosistent. >>>>> >>>>> 2) QEMUMachine doesn't keep track of command line arguments >>>>> (add_args() makes it pretty clear what's doing). But, on this >>>>> type >>>>> of use case, a "set_args()" is desirable, in which case it would >>>>> overwrite the existing arguments for a given command line option. >>>> I like the idea of a "set_args()" to QEMUMachine as you describe above >>>> but it needs further discussion because I can see at least one corner >>>> case; for example, one can set the machine type as either -machine or >>>> -M, then what key it should be searched-and-replaced (if any) on the >>>> list of args? >>>> >>>> Unlike your suggestion, I thought on implement the method to deal >>>> with a >>>> single argument at time, as: >>>> >>>> def set_arg(self, arg: Union[str, list], value: str) -> None: >>>> """ >>>> Set the value of an argument from the list of extra arguments >>>> to be >>>> given to the QEMU binary. If the argument does not exist then >>>> it is >>>> added to the list. >>>> >>>> If the ``arg`` parameter is a list then it will search and >>>> replace all >>>> occurencies (if any). Otherwise a new argument is added >>>> and it is >>>> used the first value of the ``arg`` list. >>>> """ >>>> pass >>>> >>>> Does it sound good to you? >>>> >>>> Thanks! >>>> >>>> Wainer >>>> >>> A little hokey, but I suppose that's true of our CLI interface in >>> general. >>> >>> I'd prefer not get into the business of building a "config" inside the >>> python module if we can help it right now, but if "setting" >>> individual args >>> is something you truly need to do, I won't stand in the way. >>> >>> Do what's least-gross. >> I don't have any specific suggestions on how the API should look >> like, but I'm having trouble understanding the documentation >> above. >> >> I don't know what "it will search and replace all occurrences" >> means. Occurrences of what? >> >> I don't understand what "it is used the first value of the `arg` >> list" means, either. I understand you are going to use the first >> value of the list, but you don't say what you are going to do >> with it. > > > The documentation was indeed confusing but, please, disregard it. Based > on John's comments on this thread I decided to not introduce yet another > specialized function to QEMUMachine class. Instead I added the "args" > property so that users will have access to QEMUMachine._args to change > it whatever they like. You will find that implemented on the v2 of this > series: > > '[PATCH v2 0/7] tests/acceptance: Handle tests with "cpu" tag' > > Thanks! > > - Wainer > > >> It would truly be very cool if we had a QEMUMachineConfig class that we could build out properly. In the hypothetical perfect future world where we have a json-based config file format that mapped perfectly to QMP commands, we could create a class that represents loading and representing such a format, and allow callers to change values at runtime, like: config.machine = q35 but this treads so absurdly close to what libvirt already does that I am hesitant to work on it without addressing some of the core organizational problems with our CLI first. A frequent source of anguish is how we treat multiple or repeating values on the CLI, which we do not treat consistently. Many options use their own parsers and consistent behavior at the API level here requires a lot of special-casing. Trying to enumerate the cases like -m/-machine and other conflicting things like -drive/-blockdev and so on seems difficult to get right and manage correctly. Smarter move is not to try, I think. For now, it's sadly likely best that the caller simply reaches in and fiddles with the args according to its whims like Wainer has suggested. Caller knows best. If you are seeing lots of repeated boilerplate though, feel free to add helper functions outside of the class for various test-users to use. Document their behavior rigorously. thanks! --js