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

Refactor the SyncTrigger extensions payload to make it generic #7394

Closed
TsuyoshiUshio opened this issue May 20, 2021 · 13 comments · Fixed by #8148 or #8229
Closed

Refactor the SyncTrigger extensions payload to make it generic #7394

TsuyoshiUshio opened this issue May 20, 2021 · 13 comments · Fixed by #8148 or #8229
Assignees

Comments

@TsuyoshiUshio
Copy link
Contributor

TsuyoshiUshio commented May 20, 2021

What problem would the feature you're requesting solve? Please describe.

Currently as a tactical solution for LIMA scenarios, FunctionsSyncManager is adding the extensions portion of host.json to the sync triggers payload only for Kubernetes environments. We want to remove that restriction so it's applied in other environments as well, which will allow us to address #5390 in a general manner.

For more details see the discussion in : #7386

CC: @pragnagopa @mathewc @cgillum

Describe the solution you'd like

Make the extensions payload to generic so it's always returned, not based on sku/environment. We'll need to update the minimal response that is returned when the full payload { triggers: [], functions: [], extensions: {}, secrets: {} } is over the size limit. The new minimal format will be { triggers: [], extensions: {} }. This way, consumers can rely on host level extensions config in addition to the functions config already included in the triggers data.

Describe alternatives you've considered

We can keep it only available for Kubernetes Environment, However, Container usage for the AppService will be the same.

Additional context

We considered the PR #7386 as hot fix. We need to refactor and add proper test for the generic usage.

@pragnagopa
Copy link
Member

@TsuyoshiUshio - would you like to address this sometime in future as you have most context?

@TsuyoshiUshio
Copy link
Contributor Author

@pragnagopa Yes. absolutely.

@pragnagopa
Copy link
Member

cc @mathewc

@TsuyoshiUshio - Please address comments - #7386 (comment) as well as part of this fix.

@fabiocav
Copy link
Member

@TsuyoshiUshio are you planning on working on this item as part of spring 103? Can we have a quick sync on this to talk about the proposed solution?

@mathewc
Copy link
Member

mathewc commented May 27, 2021

Yes - before anything is checked in for this, I need to review. The general goals for this are captured in #5390.

@TsuyoshiUshio
Copy link
Contributor Author

Not for todays deployment. I'd happy to discuss the design first! It should requires careful testing.

@TsuyoshiUshio
Copy link
Contributor Author

I'd like to know the counter microservices spec at first. I mean I'd like to research the pattern How the SyncTrigger Payload is used. I know the usage of Kubernetes environment with KuduLite, However, I don't know about the other use cases. Do you know the pointer?

@fabiocav fabiocav modified the milestones: Functions Sprint 103, Triaged Jun 9, 2021
@fabiocav
Copy link
Member

fabiocav commented Jun 9, 2021

@TsuyoshiUshio moving this to the triaged milestone until we have a chance to discuss.

@pragnagopa
Copy link
Member

Tagging @chiangvincent and @cachai2 - this needed for Scale Controller changes - Target Based Scaling feature

@pragnagopa pragnagopa modified the milestones: Triaged, Functions Sprint 113 Oct 29, 2021
@pragnagopa
Copy link
Member

Tagging @mathewc - as FYI for discussions and review

@fabiocav
Copy link
Member

Is there a proposed design for this? Can we sync on a design prior to having this assigned to a sprint?

@kashimiz kashimiz removed the design label Dec 15, 2021
@kashimiz
Copy link
Contributor

Marking this as in-progress as a PR by Tsuyoshi is under review: #7863

@TsuyoshiUshio
Copy link
Contributor Author

We decide to create a separate API admin/host/extensions/config for fetching the host.json with AppSertings/default value.
I'm going to share the Pull Request.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.