Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kata-ctl: Disable network check on s390x #5439

Merged

Conversation

jodh-intel
Copy link
Contributor

s390x apparently does not support rust-tls, which is required by the
network check (due to the reqwest crate dependency).

Disable the network check on s390x until we can find a solution to the
problem.

Note:

This fix is assumed to be a temporary one until we find a solution.
Hence, I have not moved the network check code (which should be entirely
generic) into an architecture specific module.

Fixes: #5435.

Signed-off-by: James O. D. Hunt james.o.hunt@intel.com


Also fixes:

kata-ctl: arch: Improve check call

Rework the architecture-specific check() call by moving all the
conditional logic out of the function.

Fixes: #5402.

Signed-off-by: James O. D. Hunt james.o.hunt@intel.com

Rework the architecture-specific `check()` call by moving all the
conditional logic out of the function.

Fixes: kata-containers#5402.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
@katacontainersbot katacontainersbot added the size/medium Average sized task label Oct 15, 2022
@jodh-intel
Copy link
Contributor Author

/test

@liubin
Copy link
Member

liubin commented Oct 17, 2022

Some clippy warnings for s390x:

22:27:30 error[E0432]: unresolved import `serde_json`
22:27:30   --> src/check.rs:16:5
22:27:30    |
22:27:30 16 | use serde_json::Value;
22:27:30    |     ^^^^^^^^^^ use of undeclared crate or module `serde_json`
22:27:30 
22:27:30 error[E0432]: unresolved import `thiserror`
22:27:30  --> src/args.rs:8:5
22:27:30   |
22:27:30 8 | use thiserror::Error;
22:27:30   |     ^^^^^^^^^ use of undeclared crate or module `thiserror`
22:27:30 
22:27:30 error: cannot determine resolution for the derive macro `Error`
22:27:30   --> src/args.rs:44:23
22:27:30    |
22:27:30 44 | #[derive(Debug, Args, Error)]
22:27:30    |                       ^^^^^
22:27:30    |
22:27:30    = note: import resolution is stuck, try simplifying macro imports
22:27:30 
22:27:30 error: cannot find attribute `error` in this scope
22:27:30   --> src/args.rs:45:3
22:27:30    |
22:27:30 45 | #[error("Argument is not valid")]
22:27:30    |   ^^^^^
22:27:30 
22:27:30 error: unused import: `std::collections::HashMap`
22:27:30   --> src/check.rs:17:5
22:27:30    |
22:27:30 17 | use std::collections::HashMap;
22:27:30    |     ^^^^^^^^^^^^^^^^^^^^^^^^^
22:27:30    |
22:27:30    = note: `-D unused-imports` implied by `-D warnings`
22:27:30 

s390x apparently does not support rust-tls, which is required by the
network check (due to the `reqwest` crate dependency).

Disable the network check on s390x until we can find a solution to the
problem.

> **Note:**
>
> This fix is assumed to be a temporary one until we find a solution.
> Hence, I have not moved the network check code (which should be entirely
> generic) into an architecture specific module.

Fixes: kata-containers#5435.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
@hbrueckner
Copy link
Contributor

hbrueckner commented Oct 17, 2022

Hi @jodh-intel , @BbolroC ,

s390x apparently does not support rust-tls, which is required by the network check (due to the reqwest crate dependency).

Disable the network check on s390x until we can find a solution to the problem.

I would say, it is not rust-tls but rather its dependency ring where there is s390x basic support pending since last year: briansmith/ring#1297

Not sure if there are some alternatives for doing the tests or if we need to work and wait on above dependency.

@jodh-intel
Copy link
Contributor Author

/test

Copy link
Contributor

@dborquez dborquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks @jodh-intel nice fix

@dborquez
Copy link
Contributor

/test-ubuntu

@dborquez
Copy link
Contributor

looks like more use sections need to be skipped for s390x, so WIP

@liubin
Copy link
Member

liubin commented Oct 18, 2022

@jodh-intel @hbrueckner maybe native-tls is valued a try.

@jodh-intel
Copy link
Contributor Author

@liubin, @hbrueckner - possibly, but see #5438. I'm going to merge this PR to unblock the CI but I agree that someone with s390x experience could probably fix this in a better way (please do! ;)

@jodh-intel jodh-intel merged commit dd60a02 into kata-containers:main Oct 18, 2022
@BbolroC
Copy link
Member

BbolroC commented Oct 18, 2022

Hi, @jodh-intel. I still do not get the point to unblock the CI even if some of the CI jobs are failing (it has been merged, though). The errors never used will be fixed on another PR or you want me or @hbrueckner to fix the errors?

@jodh-intel
Copy link
Contributor Author

@BbolroC - I merged this PR because all of the required checks have passed. But yes, there are clearly still problems on s390x so if you could look into those, that would be great. I see that @dborquez has raised #5445, but even that is still failing on s390x.

@hbrueckner
Copy link
Contributor

Hi @jodh-intel ,

@BbolroC - I merged this PR because all of the required checks have passed. But yes, there are clearly still problems on s390x so if you could look into those, that would be great. I see that @dborquez has raised #5445, but even that is still failing on s390x.

I looked a bit into this issue and also to re-enable the CI tests on s390x. Two things that I need to sort out: there are some missing [cfg(test)] annotations leading to unused function etc. issues. Also cpu related tests do not (yet) exist on s390x. The most strange thing is that you proposed dependency solution in Cargo.toml does not work locally for me.

However, the good thing is that s390 use libc (not musl) so the native-tls approach is fine.

Let me know if I should open a PR for what I have?

@hbrueckner
Copy link
Contributor

@jodh-intel solved above issues, here we go #5447 . Please have a look. Thanks.

Comment on lines +28 to +34
#[cfg(not(any(
target_arch = "aarch64",
target_arch = "powerpc64le",
target_arch = "s390x",
target_arch = "x86_64"
)))]
compile_error!("unknown architecture");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jodh-intel I don't think this is needed. You will have a compile-time error anyway from arch_specific::check() for any non-supported platforms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/medium Average sized task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kata-ctl: Fix check on s390x kata-ctl: Simplify check function
8 participants