-
Notifications
You must be signed in to change notification settings - Fork 801
feat(pipelineRef): Replace PipelineTrigger with PipelineRef for Spinnaker UI #4842
feat(pipelineRef): Replace PipelineTrigger with PipelineRef for Spinnaker UI #4842
Conversation
…ionRepository with pipelineRef
… SpelAutoComplete stage
…deNestedExecutions
|
I will add more tests around TaskController and endpoints. |
|
I added all tests I have in mind for this feature. This is ready now. |
|
@dbyron-sf Since the 1.37.x has been released do you think we can start the review process for this? |
| default Observable<PipelineExecution> retrievePipelinesForPipelineConfigId( | ||
| @Nonnull String pipelineConfigId, | ||
| @Nonnull ExecutionCriteria criteria, | ||
| Boolean includeNestedExecutions) { |
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.
Possible boolean instead of Boolean?
| } | ||
|
|
||
| fun map(rs: ResultSet, context: DSLContext): Collection<PipelineExecution> { | ||
| fun map(rs: ResultSet, context: DSLContext, includeNestedExecutions: Boolean): Collection<PipelineExecution> { |
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.
Same here: boolean since Boolean doesnt seem to be needed? Unless there is a specific reason im missing
christosarvanitis
left a comment
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.
Thanks @edgarulg I have tested this PR and it looks good functionality wise! This will boost the heavy nested pipelines performance by a lot 🚀
What is your view on adding an additional configuration/feature flag on the pipelineRef feature to control the includeNestedPipelines flag?
Im thinking that a user might want to keep the reconstruction of the PipelineRef in Orca as it was first implemented (for whatever reason). Others might want to have the Ref which is done via this PR.
Or maybe we discover a bug in the feature and instead of rolling back the release we just disable this added functionality
|
@christosarvanitis Thanks for the review. I tried to put this improvement behind a feature flag to allow for easy disabling or rollback in case of issues. However, since my changes update functionality in orca-core, fully encapsulating them is challenging but not impossible. Doing so would require additional changes to my PR that are not directly related to this improvement. Additionally, my changes are already substantial, making the review process more complex. :( However, I remember I hear there is a initiative to make more easy to write experimental code behind a feature flag but not sure if we can use that for this or what is the state for that work |
|
@dbyron-sf @jasonmcintosh Can I get a review in my PR please? |
|
I'm still struggling to get 1.37.x working... |
mattgogerly
left a comment
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.
Might just be me, but I'm finding it hard to follow the passing of true/false everywhere. I wonder if it would be easier to read if we used dedicated methods for includingNestedExecutions.
| private fun retrieve(type: ExecutionType, criteria: ExecutionCriteria, partition: String?): Observable<PipelineExecution> { | ||
| withPool(readPoolName) { | ||
| private fun retrieve(type: ExecutionType, criteria: ExecutionCriteria, partition: String?, includeNestedExecutions: Boolean = false): Observable<PipelineExecution> { | ||
| withPool(poolName) { |
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.
What's the rationale for this change? I would have thought we would want to be using the read pool here?
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.
Yeah I SUSPECT this was a confusion on this. There's a risk possibly on chained executions on concurrency but... still think should be readPoolName on this one...
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.
This is a mistake I did in the code, the right pool should be readPoolName.
| pipelineConfigId: String, | ||
| criteria: ExecutionCriteria | ||
| ): Observable<PipelineExecution> { | ||
| return retrievePipelinesForPipelineConfigId(pipelineConfigId, criteria, false) |
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.
nit: indentation
Feature: Performance Optimization for Nested Pipelines in Spinnaker Orca
In Spinnaker, pipelines with many nested child pipelines (e.g., Pipeline → Pipeline → Pipeline → Pipeline) can cause significant delays in loading times, particularly for large instances. This is because the Spinnaker UI often needs to load parent executions for any pipeline that was triggered by another, which unnecessarily increases the load on the Orca execution repository.
Solution:
To address this issue, I’ve introduced an optimization using the
PipelineReffeature that was previously delivered. The idea is to propagate thePipelineRef triggerinstead of the fullPipelineTriggerfor nested pipelines. This reduces the load on Orca and improves UI performance.How It Works:
ExecutionRepository Update:
I added a new flag,
includeNestedExecutions, to the retrieve method of the ExecutionRepository interface.Default Behavior:
Existing execution repository implementations don’t need to change.
SqlExecutionRepository Logic:
The only repository that requires an update is the
SqlExecutionRepository, where the business logic forincludeNestedExecutionsis implemented.If includeNestedExecutions is true, Orca will convert any
PipelineRefTriggerinto aPipelineTriggerto ensure nested executions are returned.If the flag is false, Orca will return executions with the
PipelineRefTrigger.SPeL Compatibility:
To maintain backward compatibility with Spinnaker’s SPeL, the
OrcaMessageHandlerensures that nested executions are included when evaluating any expressions. This guarantees that SPeL expressions still resolve correctly.SPeLAutoComplete Compatibility:
The
SPeLAutoCompletefeature continues to work as expected. The endpoint that retrieves previous executions to feed the autocomplete is still retrieving executions with the full execution context, including nested executions. This ensures that the autocomplete functionality is unaffected by the performance improvement.Summary:
The main idea is to minimize unnecessary load on Orca by returning a PipelineRefTrigger for external requests, while keeping full execution context for internal modules that need it. This change significantly reduces the pressure on the Orca execution repository and improves UI performance, especially in large Spinnaker instances.
This performance improvement is backward compatible, and SPeLAutoComplete continues to function as expected, with no disruption to existing Spinnaker functionality.
How to enable it
Here is an example on how the Spinnaker UI looks like when a pipeline is triggered by another pipeline:

Here is another example on how the execution looks like if we inspect it in the Spinnaker UI:

Finally here is another example that probes the SPeLAutoComplete still works by converting PipelineRefTrigger to PipelineTrigger and existing SPeLs still works as expected:
