-
Notifications
You must be signed in to change notification settings - Fork 141
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
[Extensibility] Zeroconf #163
Conversation
per feedback (#162 (comment))
#[derive(Serialize, Deserialize, Clone, Debug)] | ||
#[serde(rename_all = "camelCase")] | ||
pub struct ZeroconfDiscoveryHandlerConfig { | ||
pub filter: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like the zeroconf crate supports several filters and this is leveraging that ... is the crate's filter syntax well known? if not, we might consider using the filter constructs that onvif and udev use (which allow inclusion and exclusion).
could be something like:
pub struct ZeroconfDiscoveryHandlerConfig {
#[serde(default, skip_serializing_if = "Option::is_none")]
pub names: Option<FilterList>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub domains: Option<FilterList>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub kinds: Option<FilterList>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub ports: Option<FilterList>,
}
the crate search code could run without filter, and the discovery handler could filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I like this. I wish I'd thought of it. It's cleaner and clearer than what I have. I'll incorporate. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think much better, thank you!
Fixed: ea19332
@@ -0,0 +1,18 @@ | |||
apiVersion: akri.sh/v0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should move this to deployment/helm/templates/zeroconf.yaml and parameterize it like the onvif/udev
@DazWilkin, that explanation is super helpful. Can you quantify the bloat a little? Maybe let us know what the agent footprint is before the change and after the change (maybe for both debug and release, but most importantly for release). I think if the bloat isn't obscene, we are happy to take it on for the moment. In the near future, we hope to implement a solution to our monolithic problem :) |
Before the change is what you have now. With I think perhaps, more importantly, agent users would get e.g. And so they get any increased size but they also get its security profile and risk its CVEs. To be clear, this is a consequence of the monolith not zeroconf but it raises the reasonable (enterprise) customer concern "Why do we need to patch the agent for a zeroconf exploit that we're not using?" |
I have an old |
I just built locally and see that without your changes: debug (cargo build --bin=agent): 352M can you build with |
I get 21M (!?) but isn't the issue the image size? |
You are totally right. Sorry, the old app programmer in me hasn't fully given way to this new world yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get 21M (!?) but isn't the issue the image size?
You are totally right. Sorry, the old app programmer in me hasn't fully given way to this new world yet.
Nothing to apologize for :-)
With a bit more time thinking, my gut says that we should hit the pause button on this until we get a different extensibility model that would allow this provider to be more on-demand. But I'll pull more people into this discussion to see if we can get more heads involved. |
I agree that this might expand the size of the Agent too much. Maybe we can do the following:
|
Comment threading would be useful, nah? The
The
Reduces the image size to 476MB For a multi-stage build test, the image size was 206MB but this may be unrealistic. I've received no response to my issue on The issue is likely to arise with any extensions that are added to the Agent while it's a monolith. Zeroconf is likely just the first. |
No doubt. Getting a working gRPC interface into the Agent to allow out-of-proc discovery would be a valuable addition ASAP!! |
Hi @DazWilkin ! The new |
Awesome work! As you know I've had a look at your HTTP example and the gRPC-based I think I/we should close this PR. I'm working on another project at the moment and it's unclear when I'll get time to rewrite the Zeroconf protocol. If I do get an opportunity, this will be top of my Akri list. |
Sounds good to me! I'll let you hit the close button @DazWilkin |
Implements: #132
What this PR does / why we need it:
Provides a Linux-only Zeroconf protocol implementation for Akri
Special notes for your reviewer:
I've tried (!) to avoid repeating some of the commitment feedback from
http-extensibility
.If applicable:
cargo fmt
)cargo build
)cargo clippy
)cargo test
)cargo doc
)./version.sh
)