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

Fix unix domain socket (uds) gRPC-client implementation compatibility with Googles' C based gRPC (reference) implementation (closes #497) #498

Merged
merged 2 commits into from
Sep 2, 2022

Conversation

bitmeal
Copy link
Contributor

@bitmeal bitmeal commented Aug 16, 2022

What this PR does / why we need it:
closes #497 (at least fixes it)

Special notes for your reviewer:

  • has been tested in isolation, and in a cluster with a custom discovery handler using the google C++ gRPC implementation
  • test discovery::server::tests::test_run_discovery_server_error_invalid_ip_addr is failing (for me) even without the modifications and a clean master branch.

If applicable:

  • this PR has an associated PR with documentation in akri-docs
  • this PR contains unit tests
  • 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)
  • all commits pass the DCO bot check by being signed off -- see the failing DCO check for instructions on how to retroactively sign commits

@kate-goldenring
Copy link
Contributor

kate-goldenring commented Aug 17, 2022

This seems like a great fix to me. I just kicked off the end to end tests. Assuming those pass, I think it is safe assume that going from dummy to http is a fair change.

test discovery::server::tests::test_run_discovery_server_error_invalid_ip_addr is failing (for me) even without the modifications and a clean master branch.

@bitmeal what errors are you getting when you run the test?

@bitmeal
Copy link
Contributor Author

bitmeal commented Aug 17, 2022

cargo test fails discovery::server::tests::test_run_discovery_server_error_invalid_ip_addr from master branch (and with the modifications from the pull request) on

  • Ubuntu 20.04 on WSL2 with latest rust version
  • Ubuntu 18.04 with rust version set by the setup script

@kate-goldenring
Copy link
Contributor

cargo test fails discovery::server::tests::test_run_discovery_server_error_invalid_ip_addr from master branch (and with the modifications from the pull request) on

  • Ubuntu 20.04 on WSL2 with latest rust version
  • Ubuntu 18.04 with rust version set by the setup script

@bitmeal does it give any extra output? Can you run it with RUST_LOG=trace and -- --nocapture

@bitmeal
Copy link
Contributor Author

bitmeal commented Aug 17, 2022

Seems to be my fault: Rust toolchain versions have not been set correctly while running tests!
Tests succeed without any issues when using 1.61.0 version (tested @ Ubuntu Bionic, Focal and Jammy).

I will edit the PR text to reflect the info


@bitmeal does it give any extra output? Can you run it with RUST_LOG=trace and -- --nocapture

Using latest stable toolchain does yield the error though. While this is then not related to this PR anymore, I uploaded the requested logs here: https://gist.githubusercontent.com/bitmeal/e04816d479fe34a8beb0b252a23ee42f/raw/0d5b656806ee5be17c397d679019be0a4a74624f/akri_cargo_test_trace.log (view raw, as gists seem to be capped on lines)

  • produced with: RUST_LOG=trace cargo test -- --nocapture 2>&1 | tee cargo_test_trace.log
  • cargo --versioncargo 1.63.0 (fd9c4297c 2022-07-01)
  • uname -aLinux 5a3c77f245ae 4.15.0-191-generic #202-Ubuntu SMP Thu Aug 4 01:49:29 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

@kate-goldenring
Copy link
Contributor

Seems to be my fault: Rust toolchain versions have not been set correctly while running tests! Tests succeed without any issues when using 1.61.0 version (tested @ Ubuntu Bionic, Focal and Jammy).

@bitmeal this is a good bump on us. We should put out a new release and bump our rust version soon.

@kate-goldenring
Copy link
Contributor

@bitmeal looks like our tarpaulin workflow is running into the same issue you had and failing: #501
I dont think that blocks this merging. Lets get at least one more eye on this: @bfjelds @romoh @michaelzhang114

@adithyaj
Copy link
Collaborator

adithyaj commented Sep 2, 2022

This is a great catch and looks good to me! With @bfjelds fix (#501) checked in now we should be good to merge this now.

Copy link
Contributor

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

Thanks @bitmeal!

@kate-goldenring kate-goldenring merged commit 0e149f3 into project-akri:main Sep 2, 2022
@bitmeal
Copy link
Contributor Author

bitmeal commented Sep 5, 2022

Thanks @bitmeal!

Thanks to you for the good work!

leoluKL pushed a commit to leoluKL/akri that referenced this pull request Sep 23, 2022
… with Googles' C based gRPC (reference) implementation (closes project-akri#497) (project-akri#498)

* change uds grpc transport scheme to http:// from dummy://

Signed-off-by: Arne Wendt <[email protected]>

* bump patch version

Signed-off-by: Arne Wendt <[email protected]>

Signed-off-by: Arne Wendt <[email protected]>
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