fix: Revision history is not showing the timestamp of the revision

[armory-abedonik]

Observed Behavior

Expected Behavior

Issue reason

Desk uses this request to update pipeline.

POST http://{host}:8084/pipelines?staleCheck=true

payload {
  "id": string,
  "name": string,
  "application": string,
  "schema": string,
  "triggers": [],
  "index": number,
  "updateTs": "1659026349611",
  "lastModifiedBy": string,
  "stages": object[],
  "keepWaitingPipelines": bolean,
  "limitConcurrent": bolean,
  "spelEvaluator": string
}

As you can see, it has updateTs field inside the payload, so we need to have setter function to deserialize this DTO correctly.

[jasonmcintosh]

I'm not sure this is the "right" fix. the "intent" of using a Timestamped object was that updateTs isn't needed/should be replaced. Adding a getter/setter for the field I think is the wrong path. See:

front50/front50-sql/src/main/kotlin/com/netflix/spinnaker/front50/model/sql/TableDefinitions.kt

Line 106 in 4751d87

"last_modified_at" to item.lastModified,

So really I think instead of this PR, need to remove updateTs field entirely and REPLACE with the lastUpdated stuff and a wrapper on get for the lastUpdated data. The s3 storage though is one to evaluate and see how it handles this to confirm.

[armory-abedonik]

@jasonmcintosh , I don't see any problem in the code you mentioned because of secondary getter in this DTO is

  @Override
  public Long getLastModified() {
    String updateTs = this.updateTs;
    if (updateTs == null || updateTs == "") {
      return null;
    }
    return Long.valueOf(updateTs);
  }

the same for setter

  @Override
  public void setLastModified(Long lastModified) {
    if (lastModified != null) {
      this.updateTs = lastModified.toString();
    }
  }

I would like to avoid Desk modification because of the bug priority P1.

[armory-abedonik]

Also, I would like to note that timestamp data for existing pipeline history records was lost, and it seems like we will not be able to recover it.

[armory-abedonik]

Moreover, I guess they didn't remove updateTs field because of backward compatibility with existing S3.

[armory-abedonik]

@jasonmcintosh, ah, I see your point. They no longer store timestamp inside body field...

[armory-abedonik]

Update:

1. We didn't remove updateTs for 2 reasons:

• We need to ensure backward compatibility with history records created before getter and setter for lastModified field is introduced.
• We don't want to modify other services e.g. Desk now. It would be better to move this refactoring to a different task because of P1 priority for our customer.

2. SwiftStorageService, OracleStorageService don't support listObjectVersions operation, so no fixes required here.
3. AzureStorageService, GcsStorageService, S3StorageService already have fixes, so no fixes required here.
4. The changes tested locally for MySQL database.

[armory-abedonik]

We don't need to populate lastModified field from UI, we already do it here

[armory-abedonik]

Hi @cfieber, @jonsie, @mattgogerly Please take a look at this pull request and merge it if you don't have any objections. Thank you in advance!

[mattgogerly]

Are you sure this is necessary? We have a user reporting the same issue, and they say applying #1133 fixed it for them.

[armory-abedonik]

@mattgogerly, yes, these changes fix the bug for MySQL data source.

[mattgogerly]

@Mergifyio update

[mergify]

update

✅ Branch has been successfully updated

[mattgogerly]

@Mergifyio backport release-1.27.x release-1.28.x

[mergify]

backport release-1.27.x release-1.28.x

✅ Backports have been created

• #1146 fix: Revision history is not showing the timestamp of the revision (backport #1142) has been created for branch release-1.27.x
• #1147 fix: Revision history is not showing the timestamp of the revision (backport #1142) has been created for branch release-1.28.x

[JamieChe]

Hi @armory-abedonik ,
I am testing Spinnaker 1.29.0, found the code change in this commit would make getLastModified() of Pipeline.java return null during warming cache phase and further cause modifiedKeys list in StorageServiceSupport.java always gets and updates all pipelines in cache from DB.

Long modTime = existingItem.getLastModified();
                  if (modTime == null || entry.getValue() > modTime) {
                    numUpdated.getAndIncrement();
                    return true;
                  }
                  return false;

Here's the log when I debug the issue.

I think this bug should be related to loadObjects in SqlStorageService.kt, which didn't load the last_modified_at info from DB. I tried to add this field and seems the issue could be solved.

   val timeToLoadObjects = measureTimeMillis {
      objects.addAll(
        objectKeys.chunked(chunkSize).flatMap { keys ->
          val records = withPool(poolName) {
            jooq.withRetry(sqlRetryProperties.reads) { ctx ->
              ctx
                .select(
                  field("body", String::class.java),
                  field("created_at", Long::class.java),
                  field("last_modified_at", Long::class.java)   //======> fix 
                )
                .from(definitionsByType[objectType]!!.tableName)
                .where(
                  field("id", String::class.java).`in`(keys).and(
                    DSL.field("is_deleted", Boolean::class.java).eq(false)
                  )
                )
                .fetch()
            }
          }

          records.map {
            objectMapper.readValue(
              it.getValue(field("body", String::class.java)),
              objectType.clazz as Class<T>
            ).apply {
              this.createdAt = it.getValue(field("created_at", Long::class.java))
             
              this.lastModified = it.getValue(field("last_modified_at", Long::class.java))  // ======> fix
            }
          }

        }

Here's the cache metrics after I applying my code change.

Could you check whether it is a correct change and any other parts need to be updated as well? I don't have a dev environment for Spinnaker , not sure whether my change would breaking other thing. Hope we can get this issue fixed and backported in 1.29 ASAP.

Thanks