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

reconcile: allow atomic reconciliation of release #532

Merged
merged 11 commits into from
Jul 14, 2023
Merged

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented Sep 16, 2022

This pull requests adds "atomic release" behavior to the internal reconcilers.

In other words, this means that it determines the current state of the Helm release and decides what action is best to take next, until it has reached the desired state or is not allowed to proceed any further. For example, because of it running a successful upgrade, or having run a remediation action.

In addition to this:

  • Bits have been added which deal with summarizing the API specific condition types into generic Ready, Stalled and/or Progressing types.
  • The API has been further improved to move methods to the HelmRelease instead of HelmRelease.Spec.
  • The emitting of events is now tested.
  • Release names exceeding the maximum limit are truncated.
  • Plus a variety of other tiny adjustments, see the individual commits.

With this added, we can move forward to an actual "main" reconciler implementation and further tidying of the UX.

@hiddeco hiddeco force-pushed the atomic-rls-reconcile branch 8 times, most recently from 6adade7 to 8aa3431 Compare September 23, 2022 11:54
@hiddeco hiddeco force-pushed the atomic-rls-reconcile branch from 927de64 to c4de611 Compare October 10, 2022 14:03
@palexster
Copy link

palexster commented Oct 24, 2022

Hi @hiddeco, is this PR related to solving #186?

@hiddeco
Copy link
Member Author

hiddeco commented Oct 24, 2022

It is in preparation of, but not solving it by itself.

@faganihajizada
Copy link

@hiddeco do you have any update or ETA for this PR? Thanks!

@kingdonb
Copy link
Member

@faganihajizada It is on this roadmap: https://fluxcd.io/roadmap/#flux-helm-ga-q2-2023

ETA for Q2 2023 - have a happy new year! 🍾 🥳

@faganihajizada
Copy link

Thanks, @kingdonb 🤝 happy new year! 🍾🥳

internal/action/verify.go Outdated Show resolved Hide resolved
internal/action/verify.go Show resolved Hide resolved
internal/action/verify.go Show resolved Hide resolved
internal/reconcile/action.go Show resolved Hide resolved
internal/reconcile/atomic_release_test.go Show resolved Hide resolved
internal/reconcile/install.go Outdated Show resolved Hide resolved
@hiddeco hiddeco marked this pull request as ready for review July 10, 2023 13:28
hiddeco added 2 commits July 11, 2023 09:23
- Change the map with Helm release test hooks to a pointer map. This
  allows (in combination with the constrains around JSON serialization)
  to distinguish a release _without_ a test run from a release _with_
  test run but no tests (an empty map).
- Add `GetTestHooks` and `SetTestHooks` methods to help circumvent some
  of the common problems around working with a pointer map in Go (e.g.
  not being capable of iterating over it using range).
- Add `HasBeenTested` and `HasTestInPhase` methods to help make
  observations on captured release information.
- Add `StorageNamespace` to Status to allow for observations of
  configuration changes which are mutating compared to the spec.
- Add `GetActiveRemediation` helper method to get the active
  remediation strategy based on the presence of Current and/or Previous
  release observations in the Status of the object.
- Add `ReleaseTargetChanged` helper method to determine if an immutable
  release target changed has occurred, in which case e.g. garbage
  collection needs to happen before performing any other action.
- Add `GetCurrent`, `HasCurrent`, `GetPrevious` and `HasPrevious`
  helper methods to ease access to their values nested in the Status.
- Add `FullReleaseName` and `VersionedChartName` helper methods to e.g.
  allow printing full name references in Condition and Event messages
  which can be placed in a point in time based on metadata more
  familiar to a user than for example the observed generation.
- Change `GetFailureCount` and `RetriesExhausted` signatures of
  `Remediation` interface to take a pointer. This eases use of the API,
  as generally speaking a (Kubernetes) API object is a pointer.
- Move methods from `HelmReleaseSpec` to `HelmRelease`, this is easier
  to access and matches `GetConditions`, etc.
- Remove `DeploymentAction` interface and `GetDescription` from
  `Remediation` interface as this is no longer of value.

Signed-off-by: Hidde Beydals <[email protected]>
hiddeco and others added 7 commits July 11, 2023 09:23
This allows for requesting the count of non-empty values in the ring
buffer, and thus the number of log lines.

Signed-off-by: Hidde Beydals <[email protected]>
This gives more fine-grain control over what release must be targeted,
as we do not always want to rely on the current spec but rather on e.g.
a release we have made ourselves with a previous configuration for
garbage collection purposes.

Signed-off-by: Hidde Beydals <[email protected]>
This solves the issue where a release name composed out of e.g.
the target namespace and name of the HelmRelease itself would exceed
the >=53 character length. By calculating the SHA256 checksum of the
release name, taking the first 12 characters of this checksum and
appending it to the release named trimmed to 40 characters separated
by a hyphen (`<long-release-name>-abcdef12345678`).

Signed-off-by: Hidde Beydals <[email protected]>
This commit adds an atomic release reconciler, capable of stepping
through a series of Helm actions. In addition, it adds the last bits
around eventing and summarizing the end state of the Condition types
into e.g. a Ready condition.

Signed-off-by: Hidde Beydals <[email protected]>
This provides more context to individual log entries (and the duration
between individual log lines) while e.g. printing them in an event.

Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
This includes the "token" in the emitted events which is used to rate
limit events received by the notification-controller.

Either by using the already calculated config (values) digest, or by
calculating it for the current reconciliation request in scenarios
where it isn't available from made observations.

Signed-off-by: Hidde Beydals <[email protected]>
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

Left a few minor comments in some test code.
Otherwise it LGTM!

internal/reconcile/release_test.go Outdated Show resolved Hide resolved
internal/reconcile/release_test.go Outdated Show resolved Hide resolved
hiddeco added 2 commits July 14, 2023 15:50
- Use `Unknown` status for the `TestSuccess` condition when tests
  have not been run yet.
- Update Ready summarization logic to incorportate conditions with an
  Unknown status. Within the context of readiness, this always caises
  Ready=False when the condition is included in the summarization.
- Variety of tiny fixes.
- Tiny nits in test mocks to prevent confusion.

Signed-off-by: Hidde Beydals <[email protected]>
Manual backport of the work done in #698, to keep things aligned.

Signed-off-by: Hidde Beydals <[email protected]>
@hiddeco hiddeco force-pushed the atomic-rls-reconcile branch from d0b508d to 6dcd1e7 Compare July 14, 2023 13:50
@hiddeco
Copy link
Member Author

hiddeco commented Jul 14, 2023

Merging this as the CI failure is something that will be addressed by rewriting the actual wiring of the HelmRelease reconcilers, and the Go tests pass.

@hiddeco hiddeco merged commit caf406c into dev Jul 14, 2023
@hiddeco hiddeco deleted the atomic-rls-reconcile branch July 14, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants