-
Notifications
You must be signed in to change notification settings - Fork 10
feat(apisix-standalone): separate inline upstream #319
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
feat(apisix-standalone): separate inline upstream #319
Conversation
8c50260
to
4a26104
Compare
newConfig[upstreamResourceKey] = []; | ||
newConfig[upstreamResourceKey].push({ | ||
...this.fromADCUpstream( | ||
(event.newValue as ADCSDK.Service).upstream!, | ||
), | ||
id: event.resourceId, | ||
modifiedIndex: timestamp, | ||
name: event.resourceName, | ||
}); |
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.
newConfig[upstreamResourceKey] = []; | |
newConfig[upstreamResourceKey].push({ | |
...this.fromADCUpstream( | |
(event.newValue as ADCSDK.Service).upstream!, | |
), | |
id: event.resourceId, | |
modifiedIndex: timestamp, | |
name: event.resourceName, | |
}); | |
newConfig[upstreamResourceKey] = [{ | |
...this.fromADCUpstream( | |
(event.newValue as ADCSDK.Service).upstream!, | |
), | |
id: event.resourceId, | |
modifiedIndex: timestamp, | |
name: event.resourceName, | |
}]; |
If there are no other considerations, this change is more concise.
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.
The newConfig
is derived from the old configuration as a deep clone, not an empty object. Therefore, the key is not necessarily empty, and it is not feasible to override it directly.
const index = resources?.findIndex( | ||
(item) => item.id === eventGeneratedId, | ||
); | ||
if (!isNil(index) && index != -1) { |
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.
if (!isNil(index) && index != -1) { | |
if (index != -1) { |
If findIndex is the only assignment method, then the value of index can only be a number.
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.
The index
is calculated by resources?.findIndex
. Among ?
, it is decided that if resources are undefined, this value may be undefined.
const index = resources?.findIndex( | ||
(item) => item.id === eventGeneratedId, | ||
); | ||
if (!isNil(index) && index != -1) { |
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.
if (!isNil(index) && index != -1) { | |
if (!isNil(index) && index != -1) { |
ditto
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 reviewed the test cases and core changes and thought it would be nice to cover those cases:
- A service does not include inline upstream.
- An upstream referenced by multiple services.
But I think this can also be ensured by apisix itself?
LGTM
This should be possible, for example, when using some serverless plugins.
ADC does not allow this use case, and the use of YAML anchor syntax will be inlined during parsing. You may have some misunderstandings about the purpose of this PR. We have never allowed users to define upstream separately and reuse it with id, nor in this PR. For ADC input, upstream must be inlined, which is no different from the original. This will be quietly completed behind the scenes and specific optimizations will be executed, and users should not have any perception of it. |
Description
ADC uses the upstream that is inlined in service, which means it needs to be written that way in adc.yaml.
In the past, we did the same thing on each backend, but this caused some problems for APISIX. When the service resource was modified (because its internal upstream configuration was modified), it would cause the routing tree to be rebuilt.
According to APISIX's current routing tree reconstruction mechanism, this could cause sudden CPU spikes and result in additional traffic delays.
In Ingress Controller scenarios, changes to pod IP addresses are very common. It doesn't make sense for changes to pod IP addresses to further affect routing tree reconstruction and traffic proxying.
To avoid this issue, we have split the inline upstream in the backend implementation, replacing the upstream on the service with an
upstream_id
reference. This allows us to avoid the aforementioned issues.No need to modify adc.yaml, everything is transparent and quietly completed in the background during sync and dump.
This PR implements this optimization in the apisix-standalone backend, and I also plan to implement it in apisix backend, which will be completed in another PR. API7 backend does not require this, as its control plane has already implemented the necessary functionality.
Checklist