-
Notifications
You must be signed in to change notification settings - Fork 144
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
fix(events): Fix paused events for resources due to pausing application #842
Conversation
2c4ec99
to
3d45d73
Compare
keel-sql/src/main/resources/db/changelog/20200228-multi-scope-event-table.yml
Show resolved
Hide resolved
184a000
to
a491c54
Compare
@lorin If you want to review... (I think someone with admin permissions needs to add you to the org -- @robzienert @ajordens?) |
import java.time.Clock | ||
import java.time.Instant | ||
|
||
abstract class PersistentEvent { |
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 the new base class for all persistent events. It defines the common properties I expect to see in all such events regardless of the sub-class.
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.
Again I find the name to be a bit confusing... maybe it's just me :) but can't it be just event
? curios to hear other opinions.
resource.apiVersion, | ||
resource.kind, | ||
resource.id, | ||
resource.application, | ||
reason, | ||
clock.instant() | ||
) | ||
|
||
constructor(resource: Resource<*>, reason: String? = null, timestamp: Instant) : this( |
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.
Added to allow the synthetic events of this type to be added with a specific timestamp in EventController
.
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 I understand my kotlin correctly, I think you can avoid defining a second constructor by using default arguments.
constructor(resource: Resource<*>, reason: String? = null, timestamp: Instant = clock.instant()) : this(
(I haven't figured out how to suggest a change as a diff yet).
@@ -31,7 +34,7 @@ import org.springframework.context.ApplicationEventPublisher | |||
import org.springframework.stereotype.Component | |||
|
|||
@Component | |||
class ResourcePauser( | |||
class ActuationPauser( |
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.
Renamed this since it handles pausing of things other than resources.
* @param application the name of the application. | ||
* @param limit the maximum number of events to return. | ||
*/ | ||
fun applicationEventHistory(application: String, limit: Int = DEFAULT_MAX_EVENTS): List<ApplicationEvent> |
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.
We don't have an ApplicationRepository
since we don't store application metadata, so I decided to add these here for now. Let me know if there's a better place.
@@ -4,6 +4,7 @@ plugins { | |||
`java-library` | |||
id("kotlin-spring") | |||
id("ch.ayedo.jooqmodelator") version "3.6.0" | |||
id("org.liquibase.gradle") version "2.0.2" |
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 added this so we can run liquibase migration stuff from the command line.
keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlResourceRepository.kt
Outdated
Show resolved
Hide resolved
keel-web/src/main/kotlin/com/netflix/spinnaker/keel/rest/EventController.kt
Outdated
Show resolved
Hide resolved
@@ -201,28 +202,27 @@ abstract class ResourceRepositoryTests<T : ResourceRepository> : JUnit5Minutests | |||
context("updating the state again") { | |||
before { | |||
tick() | |||
// TODO: ensure persisting a map with actual data | |||
subject.appendHistory(ResourceDeltaDetected(resource, emptyMap(), clock)) | |||
subject.appendHistory(ResourceValid(resource, clock)) |
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 was unrelated, but ResourceDeltaDetected
has ignoreRepeatedInHistory
set to false
, so I changed this to an event that had it set to true
.
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 think this looks good. Couple of minor comments/questions
keel-core/src/main/kotlin/com/netflix/spinnaker/keel/events/ApplicationEvent.kt
Show resolved
Hide resolved
events.remove(it) | ||
resourceEvents.remove(it) |
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.
Should we remove application level events too?
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 thought about that, but wasn't 100% sure we weren't using application references elsewhere (meaning, those events might still be relevant even if all the resources were removed). Also, the only place this method is called is from deleteByApplication
, which in turn is only called by CombinedRepository.deleteResourcesByApplication
, which, in turn... is not called from anywhere.
I think this may have been a side-effect of the change Emily made from delete-by-application to be delete-by-delivery-config. We might be able to remove this method altogether...
keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlResourceRepository.kt
Outdated
Show resolved
Hide resolved
keel-sql/src/main/resources/db/changelog/20200228-multi-scope-event-table.yml
Show resolved
Hide resolved
keel-web/src/main/kotlin/com/netflix/spinnaker/keel/rest/EventController.kt
Outdated
Show resolved
Hide resolved
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 had a few comments and questions, but nothing that would block a merge.
keel-core/src/main/kotlin/com/netflix/spinnaker/keel/events/PersistentEvent.kt
Show resolved
Hide resolved
resource.apiVersion, | ||
resource.kind, | ||
resource.id, | ||
resource.application, | ||
reason, | ||
clock.instant() | ||
) | ||
|
||
constructor(resource: Resource<*>, reason: String? = null, timestamp: Instant) : this( |
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 I understand my kotlin correctly, I think you can avoid defining a second constructor by using default arguments.
constructor(resource: Resource<*>, reason: String? = null, timestamp: Instant = clock.instant()) : this(
(I haven't figured out how to suggest a change as a diff yet).
* @param application the name of the application. | ||
* @param downTo the time of the oldest event to return. | ||
*/ | ||
fun applicationEventHistory(application: String, downTo: Instant): List<ApplicationEvent> |
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.
My instinct is to add a limit
parameter to all API calls that return a list of objects, just like there is for the previous call.
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 decided not to add this now because the whole point of this variant of the method was not to be bound by an arbitrary limit and instead make sure we look into the application event history in the exact same time window as the resource events we're returning. Hopefully, since those are subject to a limit, the number of application events returned will not be a problem.
* Records an event associated with an application. | ||
* TODO: adding this here as there's no ApplicationRepository or EventRepository, but might want to move it. | ||
*/ | ||
fun appendHistory(event: ApplicationEvent) |
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.
Is there an advantage to having separate appendHistory
calls for the different subclasses of PersistentEvent
versus just having one method for the base class:
fun appendHistory(event: PersistentEvent)
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 actually tried to merge them, but the issue I ran into is that we are dynamically determining the value to insert in the uid
column for ResourceEvents
by doing a select into the resources
table, which prevents adding events for resources that don't exist in the database (which is a good thing since we don't have FKs anywhere). We could add the uid
to insert as a parameter, but I think this would complicate life unnecessarily for callers.
Maybe worth a refactor in a separate PR?
.../src/main/kotlin/com/netflix/spinnaker/keel/persistence/memory/InMemoryResourceRepository.kt
Outdated
Show resolved
Hide resolved
keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlResourceRepository.kt
Outdated
Show resolved
Hide resolved
2471d58
to
9e05ea0
Compare
private val lastCheckTimes = mutableMapOf<String, Instant>() | ||
|
||
override fun deleteByApplication(application: String): Int { |
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 was not used anywhere...
@@ -35,37 +40,6 @@ open class SqlResourceRepository( | |||
private val sqlRetry: SqlRetry | |||
) : ResourceRepository { | |||
|
|||
override fun deleteByApplication(application: String): Int { |
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 was not used anywhere...
@robfletcher Hopefully this is in decent shape now, if you want to take a final look. |
btw, here's the issue for this: #634 |
keel-core/src/main/kotlin/com/netflix/spinnaker/keel/events/KeelApplicationEvent.kt
Outdated
Show resolved
Hide resolved
keel-core/src/main/kotlin/com/netflix/spinnaker/keel/events/KeelApplicationEventListener.kt
Outdated
Show resolved
Hide resolved
keel-core/src/main/kotlin/com/netflix/spinnaker/keel/persistence/CombinedRepository.kt
Outdated
Show resolved
Hide resolved
Look good to me and makes total sense. Small comments about naming :) |
3f5e9e8
to
24bd007
Compare
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 for addressing my feedback!
Introduces the following changes:
resource_event
table into a genericevent
table as per the aboveApplicationEvent
base class for application-level eventsApplicationActuationPaused
andApplicationActuationResumed
eventsEventController
to add a matching and "immutable"ResourceActuationPaused
event for everyApplicationActuationPaused
in the event history in the right position of the timestamp-ordered listSample table records:
Matching API response:
Closes #837
Closes #634