| Message ID | 20220302174932.2692378-12-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | tests: introduce testing coverage for TLS with migration | expand |
On Wed, Mar 02, 2022 at 05:49:25PM +0000, Daniel P. Berrangé wrote: > The migration precopy testing helper function always expects the > migration to run to a completion state. There will be test scenarios > for TLS where expect either the client or server to fail the migration. > This expands the helper to cope with these scenarios. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > tests/qtest/migration-test.c | 47 +++++++++++++++++++++++++++++------- > 1 file changed, 38 insertions(+), 9 deletions(-) > > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c > index 2082c58e8b..e40b408988 100644 > --- a/tests/qtest/migration-test.c > +++ b/tests/qtest/migration-test.c > @@ -827,17 +827,32 @@ typedef void (*TestMigrateFinishHook)(QTestState *from, > * @connect_uri: the URI for the src QEMU to connect to > * @start_hook: (optional) callback to run at start to set migration parameters > * @finish_hook: (optional) callback to run at finish to cleanup > + * @expect_fail: true if we expect migration to fail > + * @dst_quit: true if we expect the dst QEMU to quit with an > + * abnormal exit status on failure "dst_quit" sounds a bit confusing to me, as setting dst_quit=false seems to mean "dest qemu should not quit" but it's actually for checking an abnormal quit only. Rename may work. Or, IMHO it's nicer if we could merge the two parameters: @expected_result: What is the expectation of this migration test typedef enum { /* This test should succeed, the default */ MIG_TEST_SUCCEED = 0, /* This test should fail, dest qemu should keep alive */ MIG_TEST_FAIL, /* This test should fail, dest qemu should fail with abnormal status */ MIG_TEST_FAIL_DEST_QUIT_ERR, }; Because fundamentally the two parameters are correlated, e.g. there is no combination of expect_fail==false && dst_quit==true. No strong opinion, though. Thanks,
On Wed, Mar 02, 2022 at 05:49:25PM +0000, Daniel P. Berrangé wrote: > static void test_precopy_common(const char *listen_uri, > const char *connect_uri, > TestMigrateStartHook start_hook, > TestMigrateFinishHook finish_hook, > + bool expect_fail, > + bool dst_quit, > bool dirty_ring) > { > MigrateStart *args = migrate_start_new(); > @@ -875,24 +890,32 @@ static void test_precopy_common(const char *listen_uri, > > migrate_qmp(from, connect_uri, "{}"); > > - wait_for_migration_pass(from); > + if (expect_fail) { > + wait_for_migration_fail(from, !dst_quit); Two more thoughts.. (1) Shall we move MigrateStart creation to be even upper? Then we avoid passing over these parameters but merge these new parameters into MigrateStart too. After all we used to have similar long lists of params and we merged them into MigrateStart. (2) Shall we leverage MigrateStart.hide_stderr? I saw a bunch of errors dumped even if all things run as expected.
On Mon, Mar 07, 2022 at 03:39:22PM +0800, Peter Xu wrote: > On Wed, Mar 02, 2022 at 05:49:25PM +0000, Daniel P. Berrangé wrote: > > The migration precopy testing helper function always expects the > > migration to run to a completion state. There will be test scenarios > > for TLS where expect either the client or server to fail the migration. > > This expands the helper to cope with these scenarios. > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > tests/qtest/migration-test.c | 47 +++++++++++++++++++++++++++++------- > > 1 file changed, 38 insertions(+), 9 deletions(-) > > > > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c > > index 2082c58e8b..e40b408988 100644 > > --- a/tests/qtest/migration-test.c > > +++ b/tests/qtest/migration-test.c > > @@ -827,17 +827,32 @@ typedef void (*TestMigrateFinishHook)(QTestState *from, > > * @connect_uri: the URI for the src QEMU to connect to > > * @start_hook: (optional) callback to run at start to set migration parameters > > * @finish_hook: (optional) callback to run at finish to cleanup > > + * @expect_fail: true if we expect migration to fail > > + * @dst_quit: true if we expect the dst QEMU to quit with an > > + * abnormal exit status on failure > > "dst_quit" sounds a bit confusing to me, as setting dst_quit=false seems to > mean "dest qemu should not quit" but it's actually for checking an abnormal > quit only. > > Rename may work. Or, IMHO it's nicer if we could merge the two parameters: > > @expected_result: What is the expectation of this migration test > > typedef enum { > /* This test should succeed, the default */ > MIG_TEST_SUCCEED = 0, > /* This test should fail, dest qemu should keep alive */ > MIG_TEST_FAIL, > /* This test should fail, dest qemu should fail with abnormal status */ > MIG_TEST_FAIL_DEST_QUIT_ERR, > }; > > Because fundamentally the two parameters are correlated, e.g. there is no > combination of expect_fail==false && dst_quit==true. > > No strong opinion, though. Using enums is more clear to someone reading code, so a good idea. Regards, Daniel
On Mon, Mar 07, 2022 at 03:57:16PM +0800, Peter Xu wrote: > On Wed, Mar 02, 2022 at 05:49:25PM +0000, Daniel P. Berrangé wrote: > > static void test_precopy_common(const char *listen_uri, > > const char *connect_uri, > > TestMigrateStartHook start_hook, > > TestMigrateFinishHook finish_hook, > > + bool expect_fail, > > + bool dst_quit, > > bool dirty_ring) > > { > > MigrateStart *args = migrate_start_new(); > > @@ -875,24 +890,32 @@ static void test_precopy_common(const char *listen_uri, > > > > migrate_qmp(from, connect_uri, "{}"); > > > > - wait_for_migration_pass(from); > > + if (expect_fail) { > > + wait_for_migration_fail(from, !dst_quit); > > Two more thoughts.. > > (1) Shall we move MigrateStart creation to be even upper? Then we avoid > passing over these parameters but merge these new parameters into > MigrateStart too. After all we used to have similar long lists of > params and we merged them into MigrateStart. I don't to use MigrateStart as these new parameters are not common to all migration tests. I have come up with an equivalent approach though. > (2) Shall we leverage MigrateStart.hide_stderr? I saw a bunch of errors > dumped even if all things run as expected. Yes. Regards, Daniel
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index 2082c58e8b..e40b408988 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -827,17 +827,32 @@ typedef void (*TestMigrateFinishHook)(QTestState *from, * @connect_uri: the URI for the src QEMU to connect to * @start_hook: (optional) callback to run at start to set migration parameters * @finish_hook: (optional) callback to run at finish to cleanup + * @expect_fail: true if we expect migration to fail + * @dst_quit: true if we expect the dst QEMU to quit with an + * abnormal exit status on failure * @dirty_ring: true to use dirty ring tracking * * If @connect_uri is NULL, then it will query the dst * QEMU for its actual listening address and use that * as the connect address. This allows for dynamically * picking a free TCP port. + * + * If @expect_fail is true then we expect the migration process to + * fail instead of completing. There can be a variety of reasons + * and stages in which this may happen. If a failure is expected + * to happen at time of establishing the connection, then @dst_quit + * should be false to indicate that the dst QEMU is espected to + * stay running and accept future migration connections. If a + * failure is expected to happen while processing the migration + * stream, then @dst_quit should be true to indicate that the + * dst QEMU is expected to quit with non-zero exit status */ static void test_precopy_common(const char *listen_uri, const char *connect_uri, TestMigrateStartHook start_hook, TestMigrateFinishHook finish_hook, + bool expect_fail, + bool dst_quit, bool dirty_ring) { MigrateStart *args = migrate_start_new(); @@ -875,24 +890,32 @@ static void test_precopy_common(const char *listen_uri, migrate_qmp(from, connect_uri, "{}"); - wait_for_migration_pass(from); + if (expect_fail) { + wait_for_migration_fail(from, !dst_quit); - migrate_set_parameter_int(from, "downtime-limit", CONVERGE_DOWNTIME); + if (dst_quit) { + qtest_set_expected_status(to, 1); + } + } else { + wait_for_migration_pass(from); - if (!got_stop) { - qtest_qmp_eventwait(from, "STOP"); - } + migrate_set_parameter_int(from, "downtime-limit", CONVERGE_DOWNTIME); - qtest_qmp_eventwait(to, "RESUME"); + if (!got_stop) { + qtest_qmp_eventwait(from, "STOP"); + } - wait_for_serial("dest_serial"); - wait_for_migration_complete(from); + qtest_qmp_eventwait(to, "RESUME"); + + wait_for_serial("dest_serial"); + wait_for_migration_complete(from); + } if (finish_hook) { finish_hook(from, to, data_hook); } - test_migrate_end(from, to, true); + test_migrate_end(from, to, !expect_fail); } static void test_precopy_unix_common(bool dirty_ring) @@ -903,6 +926,8 @@ static void test_precopy_unix_common(bool dirty_ring) uri, NULL, /* start_hook */ NULL, /* finish_hook */ + false, /* expect_fail */ + false, /* dst_quit */ dirty_ring); } @@ -1012,6 +1037,8 @@ static void test_precopy_tcp(void) NULL, /* connect_uri */ NULL, /* start_hook */ NULL, /* finish_hook */ + false, /* expect_fail */ + false, /* dst_quit */ false /* dirty_ring */); } @@ -1079,6 +1106,8 @@ static void test_migrate_fd_proto(void) "fd:fd-mig", test_migrate_fd_start_hook, test_migrate_fd_finish_hook, + false, /* expect_fail */ + false, /* dst_quit */ false /* dirty_ring */); }
The migration precopy testing helper function always expects the migration to run to a completion state. There will be test scenarios for TLS where expect either the client or server to fail the migration. This expands the helper to cope with these scenarios. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tests/qtest/migration-test.c | 47 +++++++++++++++++++++++++++++------- 1 file changed, 38 insertions(+), 9 deletions(-)