Skip to content
This repository has been archived by the owner on Sep 4, 2020. It is now read-only.

Upgrade kube dependency to 0.34.0 #539

Closed
wants to merge 9 commits into from
Closed

Upgrade kube dependency to 0.34.0 #539

wants to merge 9 commits into from

Conversation

messense
Copy link

@messense messense commented Feb 15, 2020

cargo build & cargo test passes on my local macOS.

Fixes #529

@messense
Copy link
Author

The Kubernetes E2E test output isn't very helpful.

Error: timed out waiting for the condition
make: *** [kind-e2e] Error 1
Makefile:37: recipe for target 'kind-e2e' failed

@wonderflow
Copy link
Member

wonderflow commented Feb 17, 2020

Yeah, it's currently hard to find out the reason. In my experience, this usually means the pod of rudr can't be running. Can you try to build the image , deploy and fix it locally.

@messense
Copy link
Author

messense commented Feb 17, 2020

Yeah, it's currently hard to find out the reason. In my experience, this usually means the pod of rudr can't be running. Can you try to build the image , deploy and fix it locally.

To be clear, this is just a drive-by PR.
I am not very familiar with k8s, and I tried but failed to start k8s with Docker Desktop on macOS.

@wonderflow
Copy link
Member

I tried your PR locally, but it fails on compiling:

  Compiling bytes v0.5.4
   Compiling futures-timer v3.0.1
   Compiling proc-macro2 v1.0.8
   Compiling libc v0.2.66
   Compiling syn v1.0.14
   Compiling indexmap v1.3.2
   Compiling num-traits v0.2.11
   Compiling num-integer v0.1.42
   Compiling num-iter v0.1.40
   Compiling memchr v2.3.2
   Compiling openssl-sys v0.9.54
   Compiling unicase v2.6.0
   Compiling backtrace-sys v0.1.32
   Compiling proc-macro-nested v0.1.3
   Compiling futures-channel v0.3.4
   Compiling unicode-normalization v0.1.12
   Compiling thread_local v1.0.1
   Compiling miniz_oxide v0.3.6
   Compiling serde v1.0.104
   Compiling rustversion v1.0.2
   Compiling encoding_rs v0.8.22
   Compiling k8s-openapi v0.7.1
   Compiling textwrap v0.11.0
   Compiling humantime v1.3.0
   Compiling want v0.3.0
error: `core::slice::<impl [T]>::len` is not yet stable as a const fn
   --> /home/jianbo/.cargo/registry/src/github.com-1ecc6299db9ec823/bytes-0.5.4/src/bytes.rs:130:18
    |
130 |             len: bytes.len(),
    |                  ^^^^^^^^^^^

   Compiling aho-corasick v0.7.8
error: aborting due to previous error

error: Could not compile `bytes`.
warning: build failed, waiting for other jobs to finish...

@messense
Copy link
Author

@wonderflow It requires rustc 1.41(current stable version).

@wonderflow
Copy link
Member

Thanks @messense, I have successfully build and it crashes as the following errors:

root@leijuan-thinkstation-p720:/home/jianbo/rudr# kubectl logs -f rudr-764f4c5d8d-tn7sh
2020-02-17 02:56:11 INFO [rudr:src/main.rs:73] starting server
2020-02-17 02:56:11 INFO [rudr:src/main.rs:33] Loading in-cluster config
2020-02-17 02:56:11 INFO [rudr:src/main.rs:77] apiserver:https://10.96.0.1:443
2020-02-17 02:56:11 INFO [kube::api::reflector:/home/jianbo/.cargo/registry/src/github.com-1ecc6299db9ec823/kube-0.25.0/src/api/reflector.rs:107] Starting Reflector for RawApi { resource: "componentschematics", group: "core.oam.dev", namespace: Some("default"), version: "v1alpha1", prefix: "apis" }
2020-02-17 02:56:11 INFO [rudr:src/main.rs:173] Watcher is running
2020-02-17 02:56:11 INFO [rudr:src/main.rs:33] Loading in-cluster config
2020-02-17 02:56:11 INFO [rudr:src/main.rs:177] Health server is running on 0.0.0.0:8080
thread '<unnamed>' panicked at 'there is no reactor running, must be called from the context of Tokio runtime', src/libcore/option.rs:1188:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Any', src/libcore/result.rs:1188:5

@messense
Copy link
Author

@wonderflow Great, I'll look into it later.

@wonderflow
Copy link
Member

You could rebase the master branch and the CI will give more detail logs

@messense
Copy link
Author

@wonderflow Still can't spot the problem in the detailed logs.

@wonderflow
Copy link
Member

Thanks so much for this great PR, but it's better if you could setup test environment.

I'm not very sure that will it possible that the health_server won't working?

@messense
Copy link
Author

Kubernetes E2E test passes.

Copy link
Member

@wonderflow wonderflow left a comment

Choose a reason for hiding this comment

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

Thanks so much for your effort and it's great to see such big contribution.

But as you can see, this PR is really complex. I don't know if these changes are all for upgrade dependency.
Hope you could give more information about this PR and help us understand better!

Thanks again!

uses: actions-rs/toolchain@v1
with:
toolchain: stable
override: true
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean this is a recommend way to always use latest rust version? Will we have risk to do so?

Dockerfile Outdated
@@ -1,4 +1,4 @@
ARG BUILDER_IMAGE=rust:1.38
ARG BUILDER_IMAGE=rust:1.41
Copy link
Member

Choose a reason for hiding this comment

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

please go through this repo and make sure all versions are updated to 1.41, thanks!

@@ -70,11 +66,12 @@ fn main() -> Result<(), Error> {
let client = APIClient::new(cfg_watch);
let mut cnt = 0;
loop {
let req = healthscope_resource.list(&ListParams::default())?;
match client.request::<ObjectList<HealthScopeObject>>(req) {
let req = healthscope_resource.list(&ListParams::default()).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

What will happen if it's None and causes panic?

Copy link
Author

Choose a reason for hiding this comment

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

I'll let you review the over all approach before addressing these small nits.

}
});

let server = std::thread::spawn(move || {
let server1 = tokio::spawn(async move {
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should rename server1 to main_server

});

std::thread::spawn(move || {
let server2 = tokio::spawn(async move {
Copy link
Member

Choose a reason for hiding this comment

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

rename server2 to health_server will be better

@@ -45,38 +45,38 @@ impl OAMScope {
}
}
/// create will create a real scope instance
pub fn create(&self, owner: meta::OwnerReference) -> Result<(), Error> {
pub async fn create(&self, owner: meta::OwnerReference) -> Result<(), Error> {
Copy link
Member

Choose a reason for hiding this comment

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

could you give some explanation that why we must change every function(including add\modify\delete\remove\status) to async?

Copy link
Author

Choose a reason for hiding this comment

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

kube-rs needs async/await, and async is invasive, which means that async fns needs to be called in async fns.

@messense
Copy link
Author

To upgrade kube-rs, we also need to upgrade reqwest to 0.10 which depends on hyper 0.13.

See https://github.com/dtolnay/async-trait for #[async_trait] usage.

@messense messense changed the title Upgrade kube dependency to 0.25.0 Upgrade kube dependency to 0.34.0 Jun 13, 2020
@messense messense closed this Jun 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade kube dependency to support GKE & macOS
2 participants