[Extensibility] Compilation errors #71

DazWilkin · 2020-10-26T20:21:37Z

What this PR does / why we need it:

I'm encountering some issues building Nessie and suspect the majority are my noob errors.

However, I suspect nessie_url should be (a) public field

Special notes for your reviewer:

error[E0616]: field `nessie_url` of struct `akri_shared::akri::configuration::NessieDiscoveryHandlerConfig` is private
  --> agent/src/protocols/nessie/discovery_handler.rs:24:14
   |
24 |             .nessie_url
   |              ^^^^^^^^^^ private field

error[E0061]: this function takes 1 argument but 0 arguments were supplied
  --> agent/src/protocols/nessie/discovery_handler.rs:27:28
   |
27 |         if let Ok(_body) = hyper::Client::new().get(url).compat().await {
   |                            ^^^^^^^^^^^^^^^^^^-- supplied 0 arguments
   |                            |
   |                            expected 1 argument

error[E0599]: no method named `compat` found for struct `hyper::client::FutureResponse` in the current scope
  --> agent/src/protocols/nessie/discovery_handler.rs:27:58
   |
27 |         if let Ok(_body) = hyper::Client::new().get(url).compat().await {
   |                                                          ^^^^^^ method not found in `hyper::client::FutureResponse`

error[E0616]: field `nessie_url` of struct `akri_shared::akri::configuration::NessieDiscoveryHandlerConfig` is private
  --> agent/src/protocols/nessie/discovery_handler.rs:31:47
   |
31 |                 self.discovery_handler_config.nessie_url.clone(),
   |                                               ^^^^^^^^^^ private field

error[E0616]: field `nessie_url` of struct `akri_shared::akri::configuration::NessieDiscoveryHandlerConfig` is private
  --> agent/src/protocols/nessie/discovery_handler.rs:34:48
   |
34 |                 &self.discovery_handler_config.nessie_url,
   |                                                ^^^^^^^^^^ private field

If applicable:

this PR contains documentation
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)

DazWilkin · 2020-10-26T20:32:26Z

And, when used here, DiscoveryResult::new parameter id_to_digest is &str:

  --> agent/src/protocols/nessie/discovery_handler.rs:35:17
   |
35 |                 self.discovery_handler_config.nessie_url,
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |                 |
   |                 expected `&str`, found struct `std::string::String`
   |                 help: consider borrowing here: `&self.discovery_handler_config.nessie_url`

Fixes? d7567b5

DazWilkin · 2020-10-26T20:43:20Z

HashMap::new() has a type of <&str, String> resulting in:

  --> agent/src/protocols/nessie/discovery_handler.rs:36:17
   |
36 |                 props,
   |                 ^^^^^ expected struct `std::string::String`, found `&str`
   |
   = note: expected struct `std::collections::HashMap<std::string::String, _>`
              found struct `std::collections::HashMap<&str, _>`

Fixes? 76ddab1

DazWilkin · 2020-10-26T21:03:09Z

samples/brokers/nessie should be added to the project's Cargo.toml so that protobuf generates nessie.rs via build.rs

Fixed: f818015

bfjelds · 2020-10-26T23:03:54Z

Cargo.toml

Do you think it would be helpful to include actual Nessie code? We have resisted and kept it as a doc sample up to now, but it would be interesting to hear the other side of that.

If we aren't including the Nessie source code, this probably shouldn't be included.

One advantage in having the example as code is that you can run it through the CI tool and ensure it builds.

The disadvantage is in needing to chop it up to form the documentation.

There must (!?) be something akin to a Jupyter notebook that can comprise build/run-able code and Markdown as comments that can be rendered on-the-fly as a tutorial.

In this case, the documentation omits this change and, as a result, nessie.rs does not get auto-generated from the proto.

Totally agree with everything you said. 100%.

If we are going to keep this as an md-based how-to, we need to find a way to validate the code changes (and keep them in sync).

Validating the changes easily would be a really nice to have.
Since this addition is needed to auto-generate the code, @DazWilkin, could you add a step to the extensibility doc to add nessie to the Cargo.toml?

@kate-goldenring I realized that your Dockerfile adds ./samples/brokers/nessie to members for the build to succeed.

So, the developer could either add the project to Cargo.toml?

Or they could cd ./samples/brokers/nessie && cargo build to generate it (while developing).

Do you have a preference? I'll then add.

Since anyone doing this will have to build a new agent as well, it seems like adding nessie to the /Cargo.toml's member list makes the most sense.

docs/extensibility.md

bfjelds · 2020-10-27T00:26:56Z

Another issue with extensibility.md: This line:

First, we need to leverage the generated gRPC code by creating `samples/brokers/nessie/src/util/nessie.rs`:

should read (tonic will generate nessie.rs based on the proto file and build.rs):

First, we need to leverage the generated gRPC code by creating `samples/brokers/nessie/src/util/nessie_service.rs`:

bfjelds · 2020-10-27T03:37:38Z

Another issue: nessie needs to be added to the list of oneOf items in deployment/helm/crds/akri-configuration-crd.yaml:

                   oneOf:
                     - required: ["nessie"]                # <--- add this line
                     - required: ["debugEcho"]
                     - required: ["onvif"]```

bfjelds · 2020-10-27T17:27:39Z

There is probably a more kuberentes-appropriate way to do this, but for the broker pod to have access to an external site, I had to add hostNetwork: true to the brokerPodSpec:

  brokerPodSpec:
    hostNetwork: true    # <---- 
    containers:
    - name: nessie-broker

DazWilkin · 2020-10-27T18:50:05Z

Assuming the developer creates ./samples/brokers/nessie and ./Dockerfile, they will also need to revise ./dockerignore to not ignore:

!samples/brokers/nessie
!shared/

Addressed: 011f3c8

DazWilkin · 2020-10-27T18:52:07Z

The Dockerfile has some issues:

An outdated version of Rust (1.41 vs. 1.47)
rustfmt required?
Corrected --from=build path /nessie/...
I think (!?) ENTRYPOINT >> CMD

Propose: b46aa11

bfjelds · 2020-10-27T19:13:38Z

I made some other changes to enable SSL from our agent and nessie in Dockerfile.nessie and Dockerfile.agent:

main...bfjelds:bfjelds/test-nessie

DazWilkin · 2020-10-28T17:51:26Z

There is probably a more kuberentes-appropriate way to do this, but for the broker pod to have access to an external site, I had to add hostNetwork: true to the brokerPodSpec:
  brokerPodSpec:
    hostNetwork: true    # <---- 
    containers:
    - name: nessie-broker

I did not have to do this (using MicroK8s)... This may be a constraint with K3s?

DazWilkin · 2020-10-28T17:58:19Z

With these changes from @bfjelds, I'm now working.

However, I was receiving image pull errors for private (!) nessie:latest from GHCR (curiously, I don't recall receiving these yesterday?).

The documentation Install akri with newly built containers considers the need to provide imagePullSecrets for agent and controller when deploying the Helm chart but this is not discussed for same-same deploying nessie.

I think the correct location for imagePullSecrets in Configuration (!) is as below and this likely should be added to the doc:

apiVersion: akri.sh/v0
kind: Configuration
metadata:
  name: nessie
spec:
  protocol:
    nessie:
      nessieUrl: https://www.lochness.co.uk/livecam/img/lochness.jpg
  capacity: 5
  brokerPodSpec:
    imagePullSecrets:
    - name: <your-secret-name>
    containers:
      - name: nessie-broker
        image: "ghcr.io/<your-github-alias>/nessie:latest"
        resources:
          limits:
            "{{PLACEHOLDER}}": "1"
  instanceServiceSpec:
    ports:
      - name: grpc
        port: 80
        targetPort: 8083
  configurationServiceSpec:
    ports:
      - name: grpc
        port: 80
        targetPort: 8083

bfjelds · 2020-10-28T18:55:43Z

There is probably a more kuberentes-appropriate way to do this, but for the broker pod to have access to an external site, I had to add hostNetwork: true to the brokerPodSpec:
  brokerPodSpec:
    hostNetwork: true    # <---- 
    containers:
    - name: nessie-broker
I did not have to do this (using MicroK8s)... This may be a constraint with K3s?

I still seem to need it for MicroK8s. Maybe I have something else going on ... will need to investigate.

DazWilkin · 2020-10-28T21:05:46Z

There is probably a more kuberentes-appropriate way to do this, but for the broker pod to have access to an external site, I had to add hostNetwork: true to the brokerPodSpec:
  brokerPodSpec:
    hostNetwork: true    # <---- 
    containers:
    - name: nessie-broker
I did not have to do this (using MicroK8s)... This may be a constraint with K3s?
I still seem to need it for MicroK8s. Maybe I have something else going on ... will need to investigate.

Perhaps this? If you're using a corporate host, it may be more secure than your average (e.g. my) installation.

bfjelds · 2020-10-28T22:34:47Z

There is probably a more kuberentes-appropriate way to do this, but for the broker pod to have access to an external site, I had to add hostNetwork: true to the brokerPodSpec:
  brokerPodSpec:
    hostNetwork: true    # <---- 
    containers:
    - name: nessie-broker
I did not have to do this (using MicroK8s)... This may be a constraint with K3s?
I still seem to need it for MicroK8s. Maybe I have something else going on ... will need to investigate.
Perhaps this? If you're using a corporate host, it may be more secure than your average (e.g. my) installation.

Seems a likely cuplrit. Let's get this PR polished off with what works for you, we can add comments later about corporate hosts.

It seems like this PR is almost there as far as getting Nessie as buttoned up as it can be, just need to update the code with what you have working and swap out the changes in .dockerignore and Cargo.toml for md descriptions of those changes, right?

DazWilkin · 2020-10-29T00:36:16Z

Yes, I think almost everything is represented in the PR 😁

While you're looking through it, containerizing the gRPC implementation could be improved; e.g. where should the Dockerfile be created? I put it in the repo root but I think it belongs in deployment?

I had to clone the Helm chart to test the changes on my GCE instance but that should work from your Helm chart once your changes are in.

I'm willing to clean slate and work through it again once you have everything in place.

bfjelds · 2020-10-29T14:56:58Z

if we were checking in code changes for nessie (rather than it being md-only), i'd say putting the Dockerfile in /build/containers makes sense (and updating akri-containers.mk).

but i think it is simpler to suggest creating the Dockerfile with the nessie broker code (/samples/brokers/nessie).

if you could remove the changes to .dockerignore and Cargo.toml, i think we can merge this PR.

once the PR is merged, i will start working on the next round of fixes for Nessie (which will include adding notes to extensibility.md about the changes needed in .dockerignore and Cargo.toml). sound good?

DazWilkin · 2020-10-29T16:04:19Z

if we were checking in code changes for nessie (rather than it being md-only), i'd say putting the Dockerfile in /build/containers makes sense (and updating akri-containers.mk).

but i think it is simpler to suggest creating the Dockerfile with the nessie broker code (/samples/brokers/nessie).

if you could remove the changes to .dockerignore and Cargo.toml, i think we can merge this PR.

once the PR is merged, i will start working on the next round of fixes for Nessie (which will include adding notes to extensibility.md about the changes needed in .dockerignore and Cargo.toml). sound good?

My naive approach to "remove changes" is to add more changes undoing them; hope OK? 49ce0a8

bfjelds · 2020-10-29T17:08:39Z

Perfect!

* public struct && public field? * Wants `&str` rather than `String` * Fix: mismatched types * Include "Nessie" in `Cargo.toml' for `build.rs` * Required for `HashMap` * Explain gRPC build and correct `nessie_service.rs` * Dockerfile * .dockerignore changes for Nessie * Fix links * Two writes (sic.) to undo a commit

public struct && public field?

32fbba2

DazWilkin requested review from bfjelds, Britel, jiria and kate-goldenring as code owners October 26, 2020 20:21

Wants &str rather than String

d7567b5

DazWilkin mentioned this pull request Oct 26, 2020

[Extensibility] Compilation errors around hyper::Client #72

Closed

DazWilkin changed the title ~~public struct && public field?~~ [Extensibility] Compilation errors Oct 26, 2020

Fix: mismatched types

76ddab1

Include "Nessie" in Cargo.toml' for build.rs`

f818015

bfjelds reviewed Oct 26, 2020

View reviewed changes

docs/extensibility.md Show resolved Hide resolved

bfjelds reviewed Oct 26, 2020

View reviewed changes

docs/extensibility.md Show resolved Hide resolved

bfjelds reviewed Oct 26, 2020

View reviewed changes

docs/extensibility.md Show resolved Hide resolved

bfjelds reviewed Oct 26, 2020

View reviewed changes

docs/extensibility.md Show resolved Hide resolved

DazWilkin added 2 commits October 27, 2020 09:43

Required for HashMap

c78ded1

Explain gRPC build and correct nessie_service.rs

1c2d9ca

DazWilkin added 2 commits October 27, 2020 11:47

Dockerfile

b46aa11

.dockerignore changes for Nessie

011f3c8

Fix links

d0c8c9e

DazWilkin mentioned this pull request Oct 27, 2020

[Extensibility] "invalid URL, scheme is not http" #77

Closed

Two writes (sic.) to undo a commit

49ce0a8

bfjelds approved these changes Oct 29, 2020

View reviewed changes

bfjelds merged commit 09dfcb5 into project-akri:main Oct 29, 2020

DazWilkin deleted the extensibility branch October 30, 2020 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Extensibility] Compilation errors #71

[Extensibility] Compilation errors #71

DazWilkin commented Oct 26, 2020 •

edited

DazWilkin commented Oct 26, 2020 •

edited

DazWilkin commented Oct 26, 2020

DazWilkin commented Oct 26, 2020

bfjelds Oct 26, 2020

bfjelds Oct 26, 2020

DazWilkin Oct 27, 2020

bfjelds Oct 27, 2020

kate-goldenring Oct 27, 2020

DazWilkin Oct 27, 2020

bfjelds Oct 27, 2020

bfjelds commented Oct 27, 2020

bfjelds commented Oct 27, 2020

bfjelds commented Oct 27, 2020

DazWilkin commented Oct 27, 2020

DazWilkin commented Oct 27, 2020 •

edited

bfjelds commented Oct 27, 2020

DazWilkin commented Oct 28, 2020

DazWilkin commented Oct 28, 2020

bfjelds commented Oct 28, 2020

DazWilkin commented Oct 28, 2020

bfjelds commented Oct 28, 2020

DazWilkin commented Oct 29, 2020

bfjelds commented Oct 29, 2020

DazWilkin commented Oct 29, 2020

bfjelds commented Oct 29, 2020

[Extensibility] Compilation errors #71

[Extensibility] Compilation errors #71

Conversation

DazWilkin commented Oct 26, 2020 • edited

DazWilkin commented Oct 26, 2020 • edited

DazWilkin commented Oct 26, 2020

DazWilkin commented Oct 26, 2020

bfjelds Oct 26, 2020

Choose a reason for hiding this comment

bfjelds Oct 26, 2020

Choose a reason for hiding this comment

DazWilkin Oct 27, 2020

Choose a reason for hiding this comment

bfjelds Oct 27, 2020

Choose a reason for hiding this comment

kate-goldenring Oct 27, 2020

Choose a reason for hiding this comment

DazWilkin Oct 27, 2020

Choose a reason for hiding this comment

bfjelds Oct 27, 2020

Choose a reason for hiding this comment

bfjelds commented Oct 27, 2020

bfjelds commented Oct 27, 2020

bfjelds commented Oct 27, 2020

DazWilkin commented Oct 27, 2020

DazWilkin commented Oct 27, 2020 • edited

bfjelds commented Oct 27, 2020

DazWilkin commented Oct 28, 2020

DazWilkin commented Oct 28, 2020

bfjelds commented Oct 28, 2020

DazWilkin commented Oct 28, 2020

bfjelds commented Oct 28, 2020

DazWilkin commented Oct 29, 2020

bfjelds commented Oct 29, 2020

DazWilkin commented Oct 29, 2020

bfjelds commented Oct 29, 2020

DazWilkin commented Oct 26, 2020 •

edited

DazWilkin commented Oct 26, 2020 •

edited

DazWilkin commented Oct 27, 2020 •

edited