perf(artifacts): use pageSize=1 when resolving prior artifacts #2955

mattnworb · 2019-06-03T14:56:08Z

This is a smaller-scoped attempt at
#2938, which relates to
spinnaker/spinnaker#4367.

ArtifactResolver.resolveArtifacts(Map) currently attempts to load
all executions for a pipeline in order to find the most recent
execution in the course of resolving expected artifacts. This causes a
lot of unnecessary data to be loaded from Redis/SQL, only to be
discarded a few operations later.

This change makes use of the fact that the ExecutionRepository
implementations will respect the executionCriteria.pageSize argument
when retrieving Execitions for a pipelineId.

In the Redis-based implementation, the executions are stored in a sorted
set scored on buildTime (or currentTimeMillis() if buildTime is
null), so retrieving all of the executions for the pipelineId with a
pageSize=1 should load the Execution with the most recent buildTime.

In the SQL-based implementation,
retrievePipelinesForPipelineConfigId(String, ExecutionCriteria) sorts
the query results based on the id field.

For both implementations, this is a small change from the existing
behavior of ArtifactResolver, which retrieves all executions and then
uses the one with the most recent startTime (or id). This change
seems like it will lead to the same result in most cases though, since
buildTime is set when the Execution is stored, and the id field is
ascending based on the timestamp of when it is generated.

The retrievePipelinesForPipelienConfigId method in both
implementations currently ignores the executionCriteria.sortType
field, but I've added this in the call from ArtifactResolver to at
least document ArtifactResolver's desire.

This is a smaller-scoped attempt at spinnaker#2938, which relates to spinnaker/spinnaker#4367. `ArtifactResolver.resolveArtifacts(Map)` currently attempts to load _all_ executions for a pipeline in order to find the most recent execution in the course of resolving expected artifacts. This causes a lot of unnecessary data to be loaded from Redis/SQL, only to be discarded a few operations later. This change makes use of the fact that the ExecutionRepository implementations will respect the `executionCriteria.pageSize` argument when retrieving Execitions for a pipelineId. In the Redis-based implementation, the executions are stored in a sorted set scored on `buildTime` (or `currentTimeMillis()` if `buildTime` is null), so retrieving all of the executions for the pipelineId with a pageSize=1 should load the Execution with the most recent `buildTime`. In the SQL-based implementation, `retrievePipelinesForPipelineConfigId(String, ExecutionCriteria)` sorts the query results based on the `id` field. For both implementations, this is a small change from the existing behavior of ArtifactResolver, which retrieves all executions and then uses the one with the most recent `startTime` (or `id`). This change seems like it will lead to the same result in most cases though, since buildTime is set when the Execution is stored, and the `id` field is ascending based on the timestamp of when it is generated. The `retrievePipelinesForPipelienConfigId` method in both implementations currently ignores the `executionCriteria.sortType` field, but I've added this in the call from ArtifactResolver to at least document ArtifactResolver's desire.

ezimanyi

Looks good, thanks for this!

mattnworb · 2019-06-12T18:40:34Z

@spinnakerbot cherry-pick 1.12

spinnakerbot · 2019-06-12T18:45:28Z

Cherry pick failed: Command failed (cherry pick commit 897ae9f) with exit code 1:

error: could not apply 897ae9fc0... perf(artifacts): use pageSize=1 when resolving prior artifacts (#2955)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

mattnworb · 2019-06-12T18:46:42Z

@spinnakerbot cherry-pick 1.14

This is a smaller-scoped attempt at #2938, which relates to spinnaker/spinnaker#4367. `ArtifactResolver.resolveArtifacts(Map)` currently attempts to load _all_ executions for a pipeline in order to find the most recent execution in the course of resolving expected artifacts. This causes a lot of unnecessary data to be loaded from Redis/SQL, only to be discarded a few operations later. This change makes use of the fact that the ExecutionRepository implementations will respect the `executionCriteria.pageSize` argument when retrieving Execitions for a pipelineId. In the Redis-based implementation, the executions are stored in a sorted set scored on `buildTime` (or `currentTimeMillis()` if `buildTime` is null), so retrieving all of the executions for the pipelineId with a pageSize=1 should load the Execution with the most recent `buildTime`. In the SQL-based implementation, `retrievePipelinesForPipelineConfigId(String, ExecutionCriteria)` sorts the query results based on the `id` field. For both implementations, this is a small change from the existing behavior of ArtifactResolver, which retrieves all executions and then uses the one with the most recent `startTime` (or `id`). This change seems like it will lead to the same result in most cases though, since buildTime is set when the Execution is stored, and the `id` field is ascending based on the timestamp of when it is generated. The `retrievePipelinesForPipelienConfigId` method in both implementations currently ignores the `executionCriteria.sortType` field, but I've added this in the call from ArtifactResolver to at least document ArtifactResolver's desire.

spinnakerbot · 2019-06-12T18:50:36Z

Cherry pick successful: #2983

This is a smaller-scoped attempt at #2938, which relates to spinnaker/spinnaker#4367. `ArtifactResolver.resolveArtifacts(Map)` currently attempts to load _all_ executions for a pipeline in order to find the most recent execution in the course of resolving expected artifacts. This causes a lot of unnecessary data to be loaded from Redis/SQL, only to be discarded a few operations later. This change makes use of the fact that the ExecutionRepository implementations will respect the `executionCriteria.pageSize` argument when retrieving Execitions for a pipelineId. In the Redis-based implementation, the executions are stored in a sorted set scored on `buildTime` (or `currentTimeMillis()` if `buildTime` is null), so retrieving all of the executions for the pipelineId with a pageSize=1 should load the Execution with the most recent `buildTime`. In the SQL-based implementation, `retrievePipelinesForPipelineConfigId(String, ExecutionCriteria)` sorts the query results based on the `id` field. For both implementations, this is a small change from the existing behavior of ArtifactResolver, which retrieves all executions and then uses the one with the most recent `startTime` (or `id`). This change seems like it will lead to the same result in most cases though, since buildTime is set when the Execution is stored, and the `id` field is ascending based on the timestamp of when it is generated. The `retrievePipelinesForPipelienConfigId` method in both implementations currently ignores the `executionCriteria.sortType` field, but I've added this in the call from ArtifactResolver to at least document ArtifactResolver's desire.

mattnworb · 2019-06-12T19:36:32Z

@spinnakerbot cherry-pick 1.13

spinnakerbot · 2019-06-12T19:40:30Z

Cherry pick failed: Command failed (cherry pick commit 897ae9f) with exit code 1:

error: could not apply 897ae9fc0... perf(artifacts): use pageSize=1 when resolving prior artifacts (#2955)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

ezimanyi · 2019-06-12T19:41:38Z

@mattnworb : You'll likely need to manually cherry-pick 1.12 and 1.13 as we ran a code formatter between 1.13 and 1.14 that touched a ton of lines of code, so cherry picks before that rarely work without manual intervention.

mattnworb · 2019-06-12T19:44:19Z

@ezimanyi thanks for the tip!

creating a new ObjectMapper instance for each Stage or other per-request contexts can be wasteful; ObjectMapper is safe to reuse across threads (after it is configured). See spinnaker/spinnaker#4367 for more details. fix(sql): optimize reading executions by pipeline id (spinnaker#2949) fix(sql): add precondition check to pipeline_config_id_idx migration (spinnaker#2953) chore(*): Bump kork (spinnaker#2954) fix(pipelines): save pipelines task shouldn’t reuse pipeline id (spinnaker#2951) When save pipelines task is used multiple times in a stage, it now reuses the pipeline ID. This is problematic because if new pipelines are created they will reuse a previous pipeline ID and overwrite it rather than creating a new pipeline. I think the real problem here is the use of “pipeline.id” in the stage context for multiple purposes (both of which make sense independently). 1) SaveServiceAccountTask generates a new pipeline.id if one doesn’t exist for use with regenerateCronTriggerIds. It effectively changes the contract from a new pipeline not having an id, to it being generated beforehand. 2) Later in this class, after a pipeline is updated or created, its id is stored in “pipeline.id” to aid with refresh logic. But the two of these together keeps the context “pipeline.id” from the last saved pipeline, and reuses it in the case that the pipeline.id is missing (when it should be created). It may be that these two usages should have different names in the stage context. In that case this loop wouldn’t happen. Since SavePipelinesFromArtifactStage is the only stage where multiple pipelines are saved, and it does not currently use managed service accounts, it seems easier to just avoid the new regenerateCronTriggers logic when isSavingMultiplePipelines is true. Changing the model can have unintended consequences. fix(kubernetes): force cache refresh after deploy stage artifact cleanup fix issue 4469: orca-core skipping most tests in build (spinnaker#2957) * fix(tests): rename fields in LockContextSpec works around the java.lang.VerifyError mentioned in spinnaker/spinnaker#4469 * fix(ExecutionSpec): fix test failure due to slf4j-simple With LockContextSpec fixed in the previous commit to no longer break the junit-vintage test runner, ExecutionSpec was also broken in commit a1de32d due to the change from using log4j's MDC to slf4j's MDC class. orca-core was setting `slf4j-simple` as `testRuntimeScope` - and `slf4j-simple` has a no-op MDC implementation which quietly discards any values set via `MDC.put()`. To fix this, allow the MDCAdapter provided by `slf4j-api` to be used instead by removing the dependency on slf4j-simple, and add an assertion to the test class to ensure that any MDCAdapter instance besides the no-op adapter is in use. chore(dependencies): Autobump korkVersion (spinnaker#2959) perf(artifacts): use pageSize=1 when resolving prior artifacts (spinnaker#2955) This is a smaller-scoped attempt at spinnaker#2938, which relates to spinnaker/spinnaker#4367. `ArtifactResolver.resolveArtifacts(Map)` currently attempts to load _all_ executions for a pipeline in order to find the most recent execution in the course of resolving expected artifacts. This causes a lot of unnecessary data to be loaded from Redis/SQL, only to be discarded a few operations later. This change makes use of the fact that the ExecutionRepository implementations will respect the `executionCriteria.pageSize` argument when retrieving Execitions for a pipelineId. In the Redis-based implementation, the executions are stored in a sorted set scored on `buildTime` (or `currentTimeMillis()` if `buildTime` is null), so retrieving all of the executions for the pipelineId with a pageSize=1 should load the Execution with the most recent `buildTime`. In the SQL-based implementation, `retrievePipelinesForPipelineConfigId(String, ExecutionCriteria)` sorts the query results based on the `id` field. For both implementations, this is a small change from the existing behavior of ArtifactResolver, which retrieves all executions and then uses the one with the most recent `startTime` (or `id`). This change seems like it will lead to the same result in most cases though, since buildTime is set when the Execution is stored, and the `id` field is ascending based on the timestamp of when it is generated. The `retrievePipelinesForPipelienConfigId` method in both implementations currently ignores the `executionCriteria.sortType` field, but I've added this in the call from ArtifactResolver to at least document ArtifactResolver's desire. fix(zombie-check): make zombie check run regardless of instance status (spinnaker#2962) Remove check for all activators being up before running a zombie check. It doesn't really buy us anything other than confusion. E.g. if there is a dedicated instance that runs zombie checks but has queue disabled the zombie check will not run. Even if down instances run a zombie check that should be ok since their view of the queue should still be accurate chore(dependencies): Autobump korkVersion (spinnaker#2964) chore(dependencies): kork 5.4.8, fiat 1.1.0 (spinnaker#2969) chore(dependencies): Autobump korkVersion (spinnaker#2970) fix(RRB): Add dedicated pin stage (spinnaker#2968) For RRB we start by pinning the source SG so that it doesn't scale down during deploy (and end with unpinning it). However, if for whatever reason some of the instances are taken out of discovery/targetgroup we will timeout waiting for capacity match on this operation. Since this pin/unpin operations are purely "cautionary" and don't require us to actually match the capacity, don't wait for capacity match so we don't fail for this non-critical reason fix(findImage) : Fix child deploy to use correct find image when clou… (spinnaker#2960) * fix(findImage) : Fix child deploy to use correct find image when cloud providers differ * fixup * adding tests fix(spel): Fix NPE in SpEL evaluation Evaluating SpEL on a null object throws an NPE; if the input object is null, just return null as the evaluated expression. fix(gcb): Bind artifacts produced from GCB stage Because we didn't include the BindProducedArtifacts task in the GCB stage, produced artifacts were not matched to expected artifacts, which prevented correctly using them downstream. fix(Orca): Feature: option to add a delay before polling starts in Webhook stage spinnaker#3450 (spinnaker#2974) feat(pipeline/expressions): support yaml parsing (spinnaker#2946) This patch adds two helper functions `readYaml` and `yamlFromUrl` to transfrom yaml strings (either embedded or from retrieved from an URL) to their object representation. DNMY: missing tests Revert "fix(Orca): Feature: option to add a delay before polling starts in Webhook stage spinnaker#3450 (spinnaker#2974)" (spinnaker#2976) This reverts commit 2c887f2. fix(core): Mark mptv2 items as inherited (spinnaker#2971) * fix(core): Mark mptv2 items as inherited spinnaker/spinnaker#4451 * fix(core): Refactor how items are marked inherited spinnaker/spinnaker#4451 fix(findImage) : Fix findImage bug introduced in spinnaker#2960 (spinnaker#2980) fix(fromUrl): Better error message for redirects (spinnaker#2978) When using SpEL functions like `*FromUrl` when the request requires auth and causes a redirect response, we get a confusing "Not retrying" error message. Make it a more descriptive error message that indicated where we are redirecting to (e.g. `login.asp`) and state that it's not supported fix(runJob): cancel underlying job on cancel stage (spinnaker#2966) fix(plugins): added dependency

…aker#2955) This is a smaller-scoped attempt at spinnaker#2938, which relates to spinnaker/spinnaker#4367. `ArtifactResolver.resolveArtifacts(Map)` currently attempts to load _all_ executions for a pipeline in order to find the most recent execution in the course of resolving expected artifacts. This causes a lot of unnecessary data to be loaded from Redis/SQL, only to be discarded a few operations later. This change makes use of the fact that the ExecutionRepository implementations will respect the `executionCriteria.pageSize` argument when retrieving Execitions for a pipelineId. In the Redis-based implementation, the executions are stored in a sorted set scored on `buildTime` (or `currentTimeMillis()` if `buildTime` is null), so retrieving all of the executions for the pipelineId with a pageSize=1 should load the Execution with the most recent `buildTime`. In the SQL-based implementation, `retrievePipelinesForPipelineConfigId(String, ExecutionCriteria)` sorts the query results based on the `id` field. For both implementations, this is a small change from the existing behavior of ArtifactResolver, which retrieves all executions and then uses the one with the most recent `startTime` (or `id`). This change seems like it will lead to the same result in most cases though, since buildTime is set when the Execution is stored, and the `id` field is ascending based on the timestamp of when it is generated. The `retrievePipelinesForPipelienConfigId` method in both implementations currently ignores the `executionCriteria.sortType` field, but I've added this in the call from ArtifactResolver to at least document ArtifactResolver's desire.

#2991) This is a smaller-scoped attempt at #2938, which relates to spinnaker/spinnaker#4367. `ArtifactResolver.resolveArtifacts(Map)` currently attempts to load _all_ executions for a pipeline in order to find the most recent execution in the course of resolving expected artifacts. This causes a lot of unnecessary data to be loaded from Redis/SQL, only to be discarded a few operations later. This change makes use of the fact that the ExecutionRepository implementations will respect the `executionCriteria.pageSize` argument when retrieving Execitions for a pipelineId. In the Redis-based implementation, the executions are stored in a sorted set scored on `buildTime` (or `currentTimeMillis()` if `buildTime` is null), so retrieving all of the executions for the pipelineId with a pageSize=1 should load the Execution with the most recent `buildTime`. In the SQL-based implementation, `retrievePipelinesForPipelineConfigId(String, ExecutionCriteria)` sorts the query results based on the `id` field. For both implementations, this is a small change from the existing behavior of ArtifactResolver, which retrieves all executions and then uses the one with the most recent `startTime` (or `id`). This change seems like it will lead to the same result in most cases though, since buildTime is set when the Execution is stored, and the `id` field is ascending based on the timestamp of when it is generated. The `retrievePipelinesForPipelienConfigId` method in both implementations currently ignores the `executionCriteria.sortType` field, but I've added this in the call from ArtifactResolver to at least document ArtifactResolver's desire.

#2990) This is a smaller-scoped attempt at #2938, which relates to spinnaker/spinnaker#4367. `ArtifactResolver.resolveArtifacts(Map)` currently attempts to load _all_ executions for a pipeline in order to find the most recent execution in the course of resolving expected artifacts. This causes a lot of unnecessary data to be loaded from Redis/SQL, only to be discarded a few operations later. This change makes use of the fact that the ExecutionRepository implementations will respect the `executionCriteria.pageSize` argument when retrieving Execitions for a pipelineId. In the Redis-based implementation, the executions are stored in a sorted set scored on `buildTime` (or `currentTimeMillis()` if `buildTime` is null), so retrieving all of the executions for the pipelineId with a pageSize=1 should load the Execution with the most recent `buildTime`. In the SQL-based implementation, `retrievePipelinesForPipelineConfigId(String, ExecutionCriteria)` sorts the query results based on the `id` field. For both implementations, this is a small change from the existing behavior of ArtifactResolver, which retrieves all executions and then uses the one with the most recent `startTime` (or `id`). This change seems like it will lead to the same result in most cases though, since buildTime is set when the Execution is stored, and the `id` field is ascending based on the timestamp of when it is generated. The `retrievePipelinesForPipelienConfigId` method in both implementations currently ignores the `executionCriteria.sortType` field, but I've added this in the call from ArtifactResolver to at least document ArtifactResolver's desire.

mattnworb mentioned this pull request Jun 3, 2019

perf(artifacts): only load prior artifacts when needed #2938

Closed

Merge branch 'master' into artifactresolver-pagesize

63ab11b

ezimanyi self-requested a review June 3, 2019 23:35

ezimanyi approved these changes Jun 4, 2019

View reviewed changes

Merge branch 'master' into artifactresolver-pagesize

12f2bfc

ezimanyi merged commit 897ae9f into spinnaker:master Jun 4, 2019

spinnakerbot added the target-release/1.15 label Jun 4, 2019

ezimanyi mentioned this pull request Jun 4, 2019

OutOfMemoryError for orca despite 80gb heap spinnaker/spinnaker#4367

Closed

mattnworb deleted the artifactresolver-pagesize branch June 12, 2019 18:40

spinnakerbot mentioned this pull request Jun 12, 2019

perf(artifacts): use pageSize=1 when resolving prior artifacts (#2955) #2983

Merged

mattnworb mentioned this pull request Jun 17, 2019

perf(artifacts): use pageSize=1 when resolving prior artifacts (#2955) #2990

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(artifacts): use pageSize=1 when resolving prior artifacts #2955

perf(artifacts): use pageSize=1 when resolving prior artifacts #2955

mattnworb commented Jun 3, 2019

ezimanyi left a comment

mattnworb commented Jun 12, 2019

spinnakerbot commented Jun 12, 2019

mattnworb commented Jun 12, 2019

spinnakerbot commented Jun 12, 2019

mattnworb commented Jun 12, 2019

spinnakerbot commented Jun 12, 2019

ezimanyi commented Jun 12, 2019

mattnworb commented Jun 12, 2019

perf(artifacts): use pageSize=1 when resolving prior artifacts #2955

perf(artifacts): use pageSize=1 when resolving prior artifacts #2955

Conversation

mattnworb commented Jun 3, 2019

ezimanyi left a comment

Choose a reason for hiding this comment

mattnworb commented Jun 12, 2019

spinnakerbot commented Jun 12, 2019

mattnworb commented Jun 12, 2019

spinnakerbot commented Jun 12, 2019

mattnworb commented Jun 12, 2019

spinnakerbot commented Jun 12, 2019

ezimanyi commented Jun 12, 2019

mattnworb commented Jun 12, 2019