[libvirt] [PATCH rust 0/5] ci related plus some small updates
Hello Daniel, When you have a moment, can you please merge this serie. It fixes CI issue, switch to bionic, and updates the tested versions. Results: https://travis-ci.org/sahid/libvirt-rust/builds/554170698 Thanks, s. Sahid Orentino Ferdjaoui (5): fix bug integration test with rust multithreading make lookup_by_id() test more robust update tested versions from 2.5.0 to 5.5.0 switch to the last ubuntu lts bionic fix code formating in README .travis.yml | 10 +++++----- README.md | 6 +++++- tests/domain.rs | 19 +++++++++---------- tests/integration_qemu.rs | 15 +++++++++++---- tests/libvirtd.sasl | 2 ++ 5 files changed, 32 insertions(+), 20 deletions(-) create mode 100644 tests/libvirtd.sasl -- 2.20.1
The open_auth test cases were randomly failling. It seems that because tests are executed in parallel if a sasl connection is open when an other happen in same time from the same libvirt connection an error happens: "Failed to start SASL negotiation: -4 (SASL(-4): no mechanism available: No worthy mechs found)". Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- tests/integration_qemu.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/tests/integration_qemu.rs b/tests/integration_qemu.rs index 49e07c4..7db43d5 100644 --- a/tests/integration_qemu.rs +++ b/tests/integration_qemu.rs @@ -89,6 +89,16 @@ fn test_create_storage_pool_and_vols() { #[test] #[ignore] fn test_connection_with_auth() { + // Rust is excecuting tests in parallel (threads), if a sasl + // connection is open when an other happen in same time from the + // same libvirt connection an error happens: "Failed to start SASL + // negotiation: -4 (SASL(-4): no mechanism available: No worthy + // mechs found)". + connection_with_auth_ok(); + connection_with_auth_wrong(); +} + +fn connection_with_auth_ok() { fn callback(creds: &mut Vec<ConnectCredential>) { for cred in creds { match cred.typed { @@ -118,10 +128,7 @@ fn test_connection_with_auth() { } } - -#[test] -#[ignore] -fn test_connection_with_auth_wrong() { +fn connection_with_auth_wrong() { fn callback(creds: &mut Vec<ConnectCredential>) { for cred in creds { match cred.typed { -- 2.20.1
On Thu, Jul 04, 2019 at 12:15:25PM +0200, Sahid Orentino Ferdjaoui wrote:
The open_auth test cases were randomly failling. It seems that because tests are executed in parallel if a sasl connection is open when an other happen in same time from the same libvirt connection an error happens: "Failed to start SASL negotiation: -4 (SASL(-4): no mechanism available: No worthy mechs found)".
This doesn't really make sense. Libvirt itself is fully multi-threaded so it should be fine to be opening connections in parallel. If there's a non-deterministic failure happening this suggests a race condition bug somewhere, which shouldn't just be ignored by changing the tests to avoid hitting the race,
Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- tests/integration_qemu.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/tests/integration_qemu.rs b/tests/integration_qemu.rs index 49e07c4..7db43d5 100644 --- a/tests/integration_qemu.rs +++ b/tests/integration_qemu.rs @@ -89,6 +89,16 @@ fn test_create_storage_pool_and_vols() { #[test] #[ignore] fn test_connection_with_auth() { + // Rust is excecuting tests in parallel (threads), if a sasl + // connection is open when an other happen in same time from the + // same libvirt connection an error happens: "Failed to start SASL + // negotiation: -4 (SASL(-4): no mechanism available: No worthy + // mechs found)". + connection_with_auth_ok(); + connection_with_auth_wrong(); +} + +fn connection_with_auth_ok() { fn callback(creds: &mut Vec<ConnectCredential>) { for cred in creds { match cred.typed { @@ -118,10 +128,7 @@ fn test_connection_with_auth() { } }
- -#[test] -#[ignore] -fn test_connection_with_auth_wrong() { +fn connection_with_auth_wrong() { fn callback(creds: &mut Vec<ConnectCredential>) { for cred in creds { match cred.typed { -- 2.20.1
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Jul 05, 2019 at 09:47:14AM +0100, Daniel P. Berrangé wrote:
On Thu, Jul 04, 2019 at 12:15:25PM +0200, Sahid Orentino Ferdjaoui wrote:
The open_auth test cases were randomly failling. It seems that because tests are executed in parallel if a sasl connection is open when an other happen in same time from the same libvirt connection an error happens: "Failed to start SASL negotiation: -4 (SASL(-4): no mechanism available: No worthy mechs found)".
This doesn't really make sense. Libvirt itself is fully multi-threaded so it should be fine to be opening connections in parallel.
If there's a non-deterministic failure happening this suggests a race condition bug somewhere, which shouldn't just be ignored by changing the tests to avoid hitting the race,
You are completely right I did lot of tests and I was not able to find the root cause. I was only able to reproduce the problem when I'm using the test framework and even with that if I'm using test-threads=1, no problems, or if I'm executing only one of them several times, no problems. Even, running 2 of example/auth.rs in parallel lot of time does not cause any issues. That is the leak which sometime happens: ==20734== ==20734== HEAP SUMMARY: ==20734== in use at exit: 165,843 bytes in 852 blocks ==20734== total heap usage: 4,141 allocs, 3,289 frees, 1,923,649 bytes allocated ==20734== ==20734== 544 bytes in 1 blocks are definitely lost in loss record 103 of 121 ==20734== at 0x4C2CE8E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==20734== by 0xA780E5E: _plug_buf_alloc (in /usr/lib/x86_64-linux-gnu/sasl2/libdigestmd5.so.2.0.25) ==20734== by 0xA77A53D: ??? (in /usr/lib/x86_64-linux-gnu/sasl2/libdigestmd5.so.2.0.25) ==20734== by 0xA77F017: ??? (in /usr/lib/x86_64-linux-gnu/sasl2/libdigestmd5.so.2.0.25) ==20734== by 0xA77F8B0: ??? (in /usr/lib/x86_64-linux-gnu/sasl2/libdigestmd5.so.2.0.25) ==20734== by 0x7DF85EF: sasl_client_step (in /usr/lib/x86_64-linux-gnu/libsasl2.so.2.0.25) ==20734== by 0x7DF896D: sasl_client_start (in /usr/lib/x86_64-linux-gnu/libsasl2.so.2.0.25) ==20734== by 0x4FF0756: virNetSASLSessionClientStart (in /usr/lib/libvirt.so.0.1002.2) ==20734== by 0x4FD1AD4: ??? (in /usr/lib/libvirt.so.0.1002.2) ==20734== by 0x4FD27E5: ??? (in /usr/lib/libvirt.so.0.1002.2) ==20734== by 0x4F6C4E5: ??? (in /usr/lib/libvirt.so.0.1002.2) ==20734== by 0x4F6F00C: virConnectOpenAuth (in /usr/lib/libvirt.so.0.1002.2) ==20734== ==20734== LEAK SUMMARY: ==20734== definitely lost: 544 bytes in 1 blocks ==20734== indirectly lost: 0 bytes in 0 blocks ==20734== possibly lost: 0 bytes in 0 blocks ==20734== still reachable: 165,299 bytes in 851 blocks ==20734== suppressed: 0 bytes in 0 blocks ==20734== Reachable blocks (those to which a pointer was found) are not shown. ==20734== To see them, rerun with: --leak-check=full --show-leak-kinds=all ==20734== ==20734== For counts of detected and suppressed errors, rerun with: -v ==20734== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0) I also have no clues why the heap is not completely freed. I will try to investigate a bit more. Thanks, s.
Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- tests/integration_qemu.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/tests/integration_qemu.rs b/tests/integration_qemu.rs index 49e07c4..7db43d5 100644 --- a/tests/integration_qemu.rs +++ b/tests/integration_qemu.rs @@ -89,6 +89,16 @@ fn test_create_storage_pool_and_vols() { #[test] #[ignore] fn test_connection_with_auth() { + // Rust is excecuting tests in parallel (threads), if a sasl + // connection is open when an other happen in same time from the + // same libvirt connection an error happens: "Failed to start SASL + // negotiation: -4 (SASL(-4): no mechanism available: No worthy + // mechs found)". + connection_with_auth_ok(); + connection_with_auth_wrong(); +} + +fn connection_with_auth_ok() { fn callback(creds: &mut Vec<ConnectCredential>) { for cred in creds { match cred.typed { @@ -118,10 +128,7 @@ fn test_connection_with_auth() { } }
- -#[test] -#[ignore] -fn test_connection_with_auth_wrong() { +fn connection_with_auth_wrong() { fn callback(creds: &mut Vec<ConnectCredential>) { for cred in creds { match cred.typed { -- 2.20.1
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- tests/domain.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/tests/domain.rs b/tests/domain.rs index 5a64a75..a70139e 100644 --- a/tests/domain.rs +++ b/tests/domain.rs @@ -89,26 +89,25 @@ fn test_get_vcpus_flags() { } #[test] -fn test_lookup_domain_by_id() { +fn test_lookup_domain_by_name() { let c = common::conn(); - let v = c.list_domains().unwrap_or(vec![]); - assert!(0 < v.len(), "At least one domain should exist"); - for domid in v { - match Domain::lookup_by_id(&c, domid) { - Ok(mut dom) => dom.free().unwrap_or(()), - Err(e) => panic!("failed with code {}, message: {}", e.code, e.message), - } + match Domain::lookup_by_name(&c, "test") { + Ok(mut r) => r.free().unwrap_or(()), + Err(e) => panic!("failed with code {}, message: {}", e.code, e.message), } common::close(c); } #[test] -fn test_lookup_domain_by_name() { +fn test_lookup_domain_by_id() { let c = common::conn(); - match Domain::lookup_by_name(&c, "test") { + let d = common::build_test_domain(&c, "by_id", true); + let id = d.get_id().unwrap_or(0); + match Domain::lookup_by_id(&c, id) { Ok(mut r) => r.free().unwrap_or(()), Err(e) => panic!("failed with code {}, message: {}", e.code, e.message), } + common::clean(d); common::close(c); } -- 2.20.1
On Thu, Jul 04, 2019 at 12:15:26PM +0200, Sahid Orentino Ferdjaoui wrote:
Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- tests/domain.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/tests/domain.rs b/tests/domain.rs index 5a64a75..a70139e 100644 --- a/tests/domain.rs +++ b/tests/domain.rs @@ -89,26 +89,25 @@ fn test_get_vcpus_flags() { }
#[test] -fn test_lookup_domain_by_id() { +fn test_lookup_domain_by_name() { let c = common::conn(); - let v = c.list_domains().unwrap_or(vec![]); - assert!(0 < v.len(), "At least one domain should exist"); - for domid in v { - match Domain::lookup_by_id(&c, domid) { - Ok(mut dom) => dom.free().unwrap_or(()), - Err(e) => panic!("failed with code {}, message: {}", e.code, e.message), - } + match Domain::lookup_by_name(&c, "test") { + Ok(mut r) => r.free().unwrap_or(()), + Err(e) => panic!("failed with code {}, message: {}", e.code, e.message), } common::close(c); }
#[test] -fn test_lookup_domain_by_name() { +fn test_lookup_domain_by_id() { let c = common::conn(); - match Domain::lookup_by_name(&c, "test") { + let d = common::build_test_domain(&c, "by_id", true); + let id = d.get_id().unwrap_or(0); + match Domain::lookup_by_id(&c, id) { Ok(mut r) => r.free().unwrap_or(()), Err(e) => panic!("failed with code {}, message: {}", e.code, e.message), } + common::clean(d); common::close(c); }
Why did you change the order of the two test methods ? It has created a big diff that hides what is actually being changed. Can you put them back to the original order, so we just see the relevant change. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Jul 05, 2019 at 09:49:02AM +0100, Daniel P. Berrangé wrote:
On Thu, Jul 04, 2019 at 12:15:26PM +0200, Sahid Orentino Ferdjaoui wrote:
Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- tests/domain.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/tests/domain.rs b/tests/domain.rs index 5a64a75..a70139e 100644 --- a/tests/domain.rs +++ b/tests/domain.rs @@ -89,26 +89,25 @@ fn test_get_vcpus_flags() { }
#[test] -fn test_lookup_domain_by_id() { +fn test_lookup_domain_by_name() { let c = common::conn(); - let v = c.list_domains().unwrap_or(vec![]); - assert!(0 < v.len(), "At least one domain should exist"); - for domid in v { - match Domain::lookup_by_id(&c, domid) { - Ok(mut dom) => dom.free().unwrap_or(()), - Err(e) => panic!("failed with code {}, message: {}", e.code, e.message), - } + match Domain::lookup_by_name(&c, "test") { + Ok(mut r) => r.free().unwrap_or(()), + Err(e) => panic!("failed with code {}, message: {}", e.code, e.message), } common::close(c); }
#[test] -fn test_lookup_domain_by_name() { +fn test_lookup_domain_by_id() { let c = common::conn(); - match Domain::lookup_by_name(&c, "test") { + let d = common::build_test_domain(&c, "by_id", true); + let id = d.get_id().unwrap_or(0); + match Domain::lookup_by_id(&c, id) { Ok(mut r) => r.free().unwrap_or(()), Err(e) => panic!("failed with code {}, message: {}", e.code, e.message), } + common::clean(d); common::close(c); }
Why did you change the order of the two test methods ? It has created a big diff that hides what is actually being changed. Can you put them back to the original order, so we just see the relevant change.
No ideas I will rearrange that.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- .travis.yml | 7 +++---- README.md | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/.travis.yml b/.travis.yml index c52f745..2267a8f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -13,11 +13,10 @@ matrix: - rust: nightly env: - - LIBVIRT=1.2.0 EXT=gz - - LIBVIRT=1.2.10 EXT=gz - - LIBVIRT=1.2.20 EXT=gz - LIBVIRT=2.5.0 EXT=xz - - LIBVIRT=3.3.0 EXT=xz + - LIBVIRT=3.10.0 EXT=xz + - LIBVIRT=4.10.0 EXT=xz + - LIBVIRT=5.5.0 EXT=xz install: - sudo apt-get -qqy build-dep libvirt diff --git a/README.md b/README.md index 5643f74..a359574 100644 --- a/README.md +++ b/README.md @@ -21,7 +21,7 @@ The bindings use standard errors handling from Rust. Each method ## Tests/Exercises -CI is executing tests automatically from libvirt 1.2.0 to 3.3.0. Using +CI is executing tests automatically from libvirt 2.5.0 to 5.5.0. Using Rust from stable, beta to nightly. * https://travis-ci.org/libvirt/libvirt-rust -- 2.20.1
On Thu, Jul 04, 2019 at 12:15:27PM +0200, Sahid Orentino Ferdjaoui wrote:
Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- .travis.yml | 7 +++---- README.md | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Not sure what the problem is by using 'scram-sha-1' with ubuntu: cannot list SASL mechanisms -4 (SASL(-4): no mechanism available: Internal Error -4 in ../../lib/server.c near line 1762) So we currently switch the mech to digest-md5. Seems that libvirt-go is doing same. Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- .travis.yml | 3 ++- tests/libvirtd.sasl | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 tests/libvirtd.sasl diff --git a/.travis.yml b/.travis.yml index 2267a8f..bea4496 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,6 +1,6 @@ language: rust os: linux -dist: trusty +dist: bionic sudo: require rust: @@ -33,6 +33,7 @@ install: - make - sudo make install - popd + - sudo cp tests/libvirtd.sasl /etc/sasl2/libvirt.conf - sudo libvirtd -d -l -f tests/libvirtd.conf - sudo virtlogd -d || true - sudo chmod a+rwx /var/run/libvirt/libvirt-sock* diff --git a/tests/libvirtd.sasl b/tests/libvirtd.sasl new file mode 100644 index 0000000..ab609e0 --- /dev/null +++ b/tests/libvirtd.sasl @@ -0,0 +1,2 @@ +mech_list: digest-md5 +sasldb_path: /etc/libvirt/passwd.db \ No newline at end of file -- 2.20.1
On Thu, Jul 04, 2019 at 12:15:28PM +0200, Sahid Orentino Ferdjaoui wrote:
Not sure what the problem is by using 'scram-sha-1' with ubuntu:
cannot list SASL mechanisms -4 (SASL(-4): no mechanism available: Internal Error -4 in ../../lib/server.c near line 1762)
For some strange reason Debian decided to put the scram plugin in the libsasl2-modules-gssapi-mit package so i expect you're missing that in the test env.
So we currently switch the mech to digest-md5. Seems that libvirt-go is doing same.
That's a historical accident, not good practice ! Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Jul 05, 2019 at 09:55:37AM +0100, Daniel P. Berrangé wrote:
On Thu, Jul 04, 2019 at 12:15:28PM +0200, Sahid Orentino Ferdjaoui wrote:
Not sure what the problem is by using 'scram-sha-1' with ubuntu:
cannot list SASL mechanisms -4 (SASL(-4): no mechanism available: Internal Error -4 in ../../lib/server.c near line 1762)
For some strange reason Debian decided to put the scram plugin in the libsasl2-modules-gssapi-mit package so i expect you're missing that in the test env.
I tried but that does not work either. Also with libsasl2-modules-gssapi-heimda.
So we currently switch the mech to digest-md5. Seems that libvirt-go is doing same.
That's a historical accident, not good practice !
I reported the issue in ubuntu [0] and it looks like it's actually libvirt which does not support it without TLS. So I imagine digest-md5 is fine for the purpose of testing. Perhaps you can comment. Thanks, s. [0] https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1835427
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Jul 05, 2019 at 12:46:44PM +0200, Sahid Orentino Ferdjaoui wrote:
On Fri, Jul 05, 2019 at 09:55:37AM +0100, Daniel P. Berrangé wrote:
On Thu, Jul 04, 2019 at 12:15:28PM +0200, Sahid Orentino Ferdjaoui wrote:
Not sure what the problem is by using 'scram-sha-1' with ubuntu:
cannot list SASL mechanisms -4 (SASL(-4): no mechanism available: Internal Error -4 in ../../lib/server.c near line 1762)
For some strange reason Debian decided to put the scram plugin in the libsasl2-modules-gssapi-mit package so i expect you're missing that in the test env.
I tried but that does not work either. Also with libsasl2-modules-gssapi-heimda.
So we currently switch the mech to digest-md5. Seems that libvirt-go is doing same.
That's a historical accident, not good practice !
I reported the issue in ubuntu [0] and it looks like it's actually libvirt which does not support it without TLS.
Oh yes, I missed that you're not using TLS here. the scram-sha-1 auth requires an external encryption layer, while digest-md5 includes (broken) encryption.
So I imagine digest-md5 is fine for the purpose of testing.
It is horribly insecure, but yes it is fine for testing. So for this patch Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index a359574..6e1ad64 100644 --- a/README.md +++ b/README.md @@ -76,14 +76,18 @@ at any time. The preferred submission method is to use git send-email to submit patches to the libvir-list@redhat.com mailing list. eg. to send a single patch +``` git send-email --to libvir-list@redhat.com --subject-prefix "PATCH rust" \ --smtp-server=$HOSTNAME -1 +``` Or to send all patches on the current branch, against master +``` git send-email --to libvir-list@redhat.com --subject-prefix "PATCH rust" \ --smtp-server=$HOSTNAME --no-chain-reply-to --cover-letter --annotate \ master.. +``` Note the master GIT repository is at -- 2.20.1
On Thu, Jul 04, 2019 at 12:15:29PM +0200, Sahid Orentino Ferdjaoui wrote:
Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- README.md | 4 ++++ 1 file changed, 4 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Daniel P. Berrangé -
Sahid Orentino Ferdjaoui