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

Update Akri Kubernetes and Runtime Dependencies #361

Merged
merged 22 commits into from Aug 24, 2021

Conversation

kate-goldenring
Copy link
Contributor

@kate-goldenring kate-goldenring commented Aug 12, 2021

What this PR does / why we need it:
This PR updates our major dependencies. Most importantly, it updates tokio to 1.0 and to the latest kube-rs and k8s-openapi versions.

Special notes for your reviewer:

  • Resolves all of ref Increase dependency versions where not covered by auto-update #324 excepts for udev and any dependencies in the Webhook (See next points)
  • The Configuration validating webhook tests failed after updating to latest kube-rs and k8s-openapi. It may be due to the need to possibly regenerate the Configuration OpenAPI API that @DazWilkin generated and hosts here. We could also switch to using the kube-rs Admission Controller type now that they support one. For now, the Webhook dependencies are left un-updated and point at the akri-shared library from the latest release. It was due to tests expecting the metadata.generation to be f64; however, the Kubernetes API defines it as an integer, since this field is incremented every time a CustomResource is modified.
  • Updates version.sh to not update or check the version Akri packages that have a git source (aka akri-shared in the Admission Controller).
  • Fixes cargo clippy warnings that had been hanging around. Some still are not fixed, such as functions with too many parameters
  • Points to updated h2 patch
  • udev crate needs more work to update

closes #223

The following will be closed once the Webhook dependencies are also updated.
closes #349
closes #358
closes #359

If applicable:

  • added code adheres to standard Rust formatting (cargo fmt)
  • code builds properly (cargo build)
  • code is free of common mistakes (cargo clippy)
  • all Akri tests succeed (cargo test)
  • inline documentation builds (cargo doc)
  • version has been updated appropriately (./version.sh)

shared/src/k8s/mod.rs Outdated Show resolved Hide resolved
@bfjelds
Copy link
Collaborator

bfjelds commented Aug 12, 2021

there is sooo much code change in this PR ... thanks for sorting through all the ramifications of updating these!

agent/Cargo.toml Outdated Show resolved Hide resolved
version.txt Show resolved Hide resolved
Copy link
Contributor

@romoh romoh left a comment

Choose a reason for hiding this comment

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

With the exception of locking the h2 version, the change looks good.

@bfjelds
Copy link
Collaborator

bfjelds commented Aug 13, 2021

might not be for this PR, but when i run the tests on my work machine i get failures in test_timeout_for_simple_onvif_discover. it seems to be that i'm finding 4 cameras but get_responsive_uris is only returning for 2 of them, so simple_onvif_discover is taking longer than the expected time. maybe we should mock OnvifQuery in the test?

@kate-goldenring
Copy link
Contributor Author

might not be for this PR, but when i run the tests on my work machine i get failures in test_timeout_for_simple_onvif_discover. it seems to be that i'm finding 4 cameras but get_responsive_uris is only returning for 2 of them, so simple_onvif_discover is taking longer than the expected time. maybe we should mock OnvifQuery in the test?

Interesting. I tested it locally too using RUST_LOG=trace cargo test -- test_timeout_for_simple_onvif_discover --nocapture and am just under the time. Like you said, its probably taking too long because the get_reponsive_uri call lasts long enough so the next time check is over X seconds. We should add a timeout on that call like we do with try_recv_string. That could be a "good first issue" issue.

@kate-goldenring
Copy link
Contributor Author

kate-goldenring commented Aug 17, 2021

Out tarpaulin code coverage tests are failing with:

   Compiling tower-http v0.1.1
error: Broken pipe (os error 32)
warning: build failed, waiting for other jobs to finish...
thread 'main' panicked at 'already borrowed: BorrowMutError', src/tools/cargo/src/cargo/util/config/mod.rs:302:20
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Aug 17 23:13:30.930 ERROR cargo_tarpaulin: Failed to compile tests! Error: the trait bound `ResBody: http_body::Body` is not satisfied
Error: "Failed to compile tests! Error: the trait bound `ResBody: http_body::Body` is not satisfied"

However, when I ran RUST_LOG=trace cargo tarpaulin -v --all-features --out Xml locally with the same tarpaulin version 0.16.0, it succeeds. It seems we are running into this bug xd009642/tarpaulin#463, but there doesn't seem to be a clear solution. I am going to try to update the version and see what happens

@bfjelds
Copy link
Collaborator

bfjelds commented Aug 18, 2021

Interesting. I tested it locally too using RUST_LOG=trace cargo test -- test_timeout_for_simple_onvif_discover --nocapture and am just under the time. Like you said, its probably taking too long because the get_reponsive_uri call lasts long enough so the next time check is over X seconds. We should add a timeout on that call like we do with try_recv_string. That could be a "good first issue" issue.

i think the test has lost its meaning a bit. previously, the intent was to see if the udp discovery stopped ~2 seconds when the timeout was set to 2.

now, with the timeout set to 2 seconds, for me, the discovery takes ~3.1 seconds (but for you it takes less than 2.2 seconds). i can make the test pass if i change what wait_for_call_millis is set to: let wait_for_call_millis = 4000; i'm not sure that is a good solution though because it seems to depend on how many cameras are actually discovered.

in any case, maybe this is a test improvement that can be made later (or if tests fail for other people).

@kate-goldenring
Copy link
Contributor Author

Out tarpaulin code coverage tests are failing with:

   Compiling tower-http v0.1.1
error: Broken pipe (os error 32)
warning: build failed, waiting for other jobs to finish...
thread 'main' panicked at 'already borrowed: BorrowMutError', src/tools/cargo/src/cargo/util/config/mod.rs:302:20
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Aug 17 23:13:30.930 ERROR cargo_tarpaulin: Failed to compile tests! Error: the trait bound `ResBody: http_body::Body` is not satisfied
Error: "Failed to compile tests! Error: the trait bound `ResBody: http_body::Body` is not satisfied"

However, when I ran RUST_LOG=trace cargo tarpaulin -v --all-features --out Xml locally with the same tarpaulin version 0.16.0, it succeeds. It seems we are running into this bug xd009642/tarpaulin#463, but there doesn't seem to be a clear solution. I am going to try to update the version and see what happens

increasing the tarpaulin version to the latest seems to have done the trick

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants