Skip to content
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

refactor(controller): rework annotation mechanics #1856

Merged
merged 18 commits into from
Apr 18, 2024

Conversation

hiddeco
Copy link
Contributor

@hiddeco hiddeco commented Apr 16, 2024

Related to: #1479
Also addresses a bit of: #1231 (tracking who aborted a verification request)

This pull request refactors the approach to working with annotations that express desires, such as "refresh this resource" or "abort this verification." It eliminates the need to delete an annotation once a request has been fulfilled, by introducing carefully scoped predicates and comparisons against the Status of a resource to determine if a request is new.

There are multiple advantages to this approach:

  1. It reduces the number of API calls during reconciliation, as we no longer need to issue a second PATCH operation to remove an annotation.
  2. It removes the need to ignore update events from the PATCH operation mentioned in point 1.
  3. It makes the annotations compatible with GitOps, as they can now be safely declared in the desired state (e.g., due to a YAML export at an unfortunate moment) without retriggering the request on every sync.
  4. We avoid queuing for any annotation change.

Outline of changes

kargo.akuity.io/refresh

Instead of deleting the annotation at the end of the reconciliation, the token value is mirrored to the .status.lastHandledRefresh field of the resource. This allows the initiator of the refresh request to wait until the value in the Status field matches the token of their request or equals the current annotation value.

To enable a reconciler to detect new refresh requests, a RefreshRequested predicate is introduced. This predicate signals that a resource should be reconciled if the annotation is added or if the annotation value has changed compared to the previous annotation value.

kargo.akuity/reverify

Instead of having two separate annotations to put in a request to reverify the current Freight of a Stage and to keep track of the actor who put in the request. The values have been unified into the kargo.akuity/reverify annotation which is now expected to contain a JSON object with all information with the following structure:

type VerificationRequest struct {
	// ID is the identifier of the VerificationInfo for which the request is
	// being made.
	ID string `json:"id,omitempty"`
	// Actor is the user who initiated the request.
	Actor string `json:"actor,omitempty"`
	// ControlPlane is a flag to indicate if the request has been initiated by
	// a control plane.
	ControlPlane bool `json:"controlPlane,omitempty"`
}

To not create a burden for e.g. kubectl users, a Stage can still be annotated using a simple ID string value in which case the webhook will take care of transforming this into the expected JSON payload (including information about the actor).

During reconciliation, the data from the annotation value is compared to the .currentFreight.verficationHistory to determine if a reverification should be performed. In addition, when it's determined that a verification attempt is due to a reverify request, the actor of this request is mirrored to the .verificationInfo.actor field. When a Kubernetes Event is emitted for the verification process, the value of this field is used instead of annotation data to include metadata about the actor with the Event.

To enable a reconciler to detect new reverify requests, a ReverifyRequested predicate is introduced. This predicate signals that a resource should be reconciled if the annotation is added or if the ID in the annotation payload has changed compared to the previous ID.

kargo.akuity.io/abort

This annotation now relies on the same mechanics as introduced for kargo.akuity/reverify, and allows for tracking the actor which initiated the request.

To enable a reconciler to detect new abort requests, an AbortRequested predicate is introduced. This predicate signals that a resource should be reconciled if the annotation is added or if the ID in the annotation payload has changed compared to the previous ID.

Only small modifications were required to the reconciliation logic itself, as the abort request was already taken into account only when the verification process is still running and the value matches the ID of the current verification process. Since the former condition will be false as soon as the abort request is fulfilled, this minimized the changes to only mirroring the actor from the VerificationRequest to the VerificationInfo in the Status.


@devholic: specific commit of interest to you is 6cd5013.
@rbreeze, @rpelczar: I tried to make the frontend changes myself here... They are all related to the kargo.akuity.io/refresh annotation, and mainly involve waiting for the annotation value to equal the field in the Status – instead of the annotation being removed.

Copy link

netlify bot commented Apr 16, 2024

Deploy Preview for docs-kargo-akuity-io ready!

Name Link
🔨 Latest commit e19b9dd
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/6621592f17b2ed0008ca3a35
😎 Deploy Preview https://deploy-preview-1856.kargo.akuity.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Apr 16, 2024

Codecov Report

Attention: Patch coverage is 84.98024% with 38 lines in your changes are missing coverage. Please review.

Project coverage is 46.43%. Comparing base (592ba2d) to head (e19b9dd).
Report is 2 commits behind head on main.

Files Patch % Lines
internal/cli/cmd/refresh/stage.go 0.00% 7 Missing ⚠️
internal/cli/cmd/refresh/warehouse.go 0.00% 7 Missing ⚠️
api/v1alpha1/annotations.go 79.31% 3 Missing and 3 partials ⚠️
internal/controller/promotions/promotions.go 0.00% 5 Missing and 1 partial ⚠️
api/v1alpha1/stage_helpers.go 80.76% 3 Missing and 2 partials ⚠️
internal/controller/stages/stages.go 44.44% 4 Missing and 1 partial ⚠️
internal/controller/warehouses/warehouses.go 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1856      +/-   ##
==========================================
+ Coverage   46.13%   46.43%   +0.30%     
==========================================
  Files         214      215       +1     
  Lines       13923    13965      +42     
==========================================
+ Hits         6423     6485      +62     
+ Misses       7214     7191      -23     
- Partials      286      289       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

hiddeco added 14 commits April 18, 2024 15:23
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
@hiddeco hiddeco force-pushed the refactor-annotations branch 2 times, most recently from 70df291 to 08a17c8 Compare April 18, 2024 13:31
@hiddeco hiddeco marked this pull request as ready for review April 18, 2024 13:31
@hiddeco hiddeco requested a review from a team as a code owner April 18, 2024 13:31
@hiddeco hiddeco changed the title refactor(controller): do not remove annotations for fulfilled requests refactor(controller): rework annotation mechanics Apr 18, 2024
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
@hiddeco hiddeco force-pushed the refactor-annotations branch from 08a17c8 to d5cd76e Compare April 18, 2024 13:35
@krancour krancour added this to the v0.6.0 milestone Apr 18, 2024
@krancour
Copy link
Member

@hiddeco despite the size, this looks very straightforward. Love this improvement.

LGTM, but I still want @rbreeze or @rpelczar to look at the UI parts.

Copy link
Contributor

@rbreeze rbreeze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UI LGTM! The logic for evaluating if the stage is refreshing should be refactored into a shared utility function, but I won't block the PR for it - I can follow up with it.

Signed-off-by: Hidde Beydals <hidde@hhh.computer>
@hiddeco hiddeco force-pushed the refactor-annotations branch from 651d80f to 6274e30 Compare April 18, 2024 17:03
@hiddeco
Copy link
Contributor Author

hiddeco commented Apr 18, 2024

Before I merge, it would be great to have an approval from @devholic as well. Given I (carefully) reconstructed bits he has been eagerly working on, and I want to be sure I do not introduce any unforeseen side-effects.

Copy link
Contributor

@devholic devholic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the awesome work! 🚀 LGTM

@hiddeco hiddeco added this pull request to the merge queue Apr 18, 2024
@hiddeco hiddeco removed this pull request from the merge queue due to a manual request Apr 18, 2024
hiddeco added 2 commits April 18, 2024 19:32
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
@hiddeco hiddeco force-pushed the refactor-annotations branch from 5ea95c7 to e19b9dd Compare April 18, 2024 17:32
@hiddeco hiddeco enabled auto-merge April 18, 2024 17:35
@hiddeco hiddeco added this pull request to the merge queue Apr 18, 2024
Copy link
Contributor

@rpelczar rpelczar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UI LGTM

Merged via the queue into akuity:main with commit a19fc14 Apr 18, 2024
16 checks passed
@hiddeco hiddeco deleted the refactor-annotations branch April 18, 2024 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants