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.

Sign up for GitHub

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

Jump to bottom

Move to v1beta2 API and rewrite reconcilers #586

Merged
merged 63 commits into from
Feb 23, 2022
Merged

Move to v1beta2 API and rewrite reconcilers #586

merged 63 commits into from
Feb 23, 2022

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented Feb 18, 2022 •
edited
Loading

This PR contains the new v1beta2 API, with a series of changes mainly focused around improving observability and the internal workings of the reconcilers themselves.

Introduces

  • Introduction of pkg/runtime packages to work around common K8s difficulties (patch conflicts, working with Condition Types, etc.).
  • Compatability with the kstatus spec, more specifically the Stalled and Reconciling Condition Types to improve waiting for Source API objects.
  • More specific "abnormal-true" types for descriptive failure Condition Types, documented in: https://github.com/fluxcd/source-controller/tree/rewrite-spec/docs/spec/v1beta2
  • Introduction of a summarized Ready Condition based on other Types on the object.
  • Reworked Kubernetes and external Events to provide e.g. a proper Reason, and a better description for humans.
  • Restructure of reconciler code to provide a better flow, and testing possibilities.
  • Drop of Ginkgo to have Go native tests again, including their speed.
  • Increase of test coverage to >75%.

Fixes: #362
Fixes: #415
Fixes: #381
Fixes: #357
Fixes: #481
Fixes: #581

@hiddeco hiddeco mentioned this pull request Feb 21, 2022
Closed
@hiddeco hiddeco added area/git Git related issues and pull requests area/helm Helm related issues and pull requests enhancement New feature or request area/bucket Bucket related issues and pull requests labels Feb 21, 2022
@hiddeco hiddeco marked this pull request as ready for review February 21, 2022 15:12
@hiddeco hiddeco mentioned this pull request Feb 21, 2022
Closed
5 tasks
stefanprodan
stefanprodan reviewed Feb 23, 2022
View reviewed changes
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

We should set the copyright header for the v1beta2 API to 2022 (not a blocker for this PR).

go.mod Outdated Show resolved Hide resolved
darkowlzz and others added 13 commits February 23, 2022 12:34
@darkowlzz @hiddeco
Introduce v1beta2 API package
cc1e482 
This commit introduces a v1beta2 API package for the staged breaking
changes around conditions and general usage of the API objects.

Signed-off-by: Hidde Beydals <hello@hidde.co>
Signed-off-by: Sunny <darkowlzz@protonmail.com>
Co-authored-by: Hidde Beydals <hello@hidde.co>
@hiddeco
Switch to v1beta2 API package
7c3c149 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
Introduce more explicit Condition types
e42eedd 
This commit introduces new Condition types to the v1beta1 API,
facilitating easier observation of (potentially) problematic state for
end-users.

- `ArtifactUnavailableCondition`: indicates there is no artifact
  available for the resource. This Condition should be set by the
  reconciler as soon as it observes the absence of an artifact for a
  source.
- `CheckoutFailedCondition`: indicates a transient or persistent
  checkout failure. This Condition should be set by the reconciler as
  soon as it observes a Git checkout failure, including any
  prerequisites like the unavailability of the referenced Secret used
  for authentication. It should be deleted as soon as a successful
  checkout has been observed again.
- `SourceVerifiedCondition`: indicates the integrity of the source has
  been verified. The Condition should be set to True or False by the
  reconciler based on the result of the integrity check.
  If there is no verification mode and/or secret configured, the
  Condition should be removed.
- `IncludeUnavailableCondition`: indicates one of the referenced
  includes is not available. This Condition should for example be set
  by the reconciler when the include does not exist, or does not have
  an artifact. If the includes become available, it should be deleted.
- `ArtifactOutdatedCondition`: indicates the current artifact of the
  source is outdated. This Condition should for example be set by the
  reconciler when it notices there is a newer revision for an artifact,
  or the previously included artifacts differ from the current available
  ones. The Condition should be removed after writing a new artifact
  to the storage.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
Implement new runtime interfaces, prepare testenv
349739b 
This commit ensures all API objects implement the interfaces used by
the runtime package to work with conditions, etc., and prepares the
test suite to work with the `pkg/runtime/testenv` wrapper.

Changes are made in a backwards compatible way (that being: the
existing code can still be build and works as expected), but without
proper dependency boundaries. The result of this is that the API
package temporary depends on the runtime package, which is resolved
when all reconcilers have been refactored and the API package does
no longer contain condition modifying functions.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
Introduce artifactSet to replace hasArtifactUpdated
e190059 
NOTE: Remove `hasArtifactUpdated` in the future once it's no longer
used.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@darkowlzz @hiddeco
source: Add GetRequeueAfter
9eb6833 
The problem with `GetInterval()` was that the returned type was of
`metav1.Duration`, while almost anywhere it was used, a type of
`time.Duration` was requested. The result of this was that we had to
call `GetInterval().Duration` all the time, which would become a bit
cumbersome after awhile.

To prevent this, we introduce a new `GetRequeueAfter() time.Duration`
method, which both results the right type, and bears a name that is
easier to remember where the value is used most; while setting the
`Result.RequeueAfter` during reconcile operations.

The introduction of this method deprecates `GetInterval()`, which
should be removed in a future MINOR release.

Signed-off-by: Sunny <darkowlzz@protonmail.com>

Co-authored-by: Hidde Beydals <hello@hidde.co>
@darkowlzz @hiddeco
Use new events and metrics helpers in main.go
a1efbad 
Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz @hiddeco
Move Artifact conditions to conditions
b1233dc 
Also, introduce FetchFailedCondition for generic fetch failures.

Signed-off-by: Sunny <darkowlzz@protonmail.com>

Co-authored-by: Hidde Beydals <hello@hidde.co>
@darkowlzz @hiddeco
Add gomega matcher for artifact
dc9d8b7 
Signed-off-by: Sunny <darkowlzz@protonmail.com>

Co-authored-by: Hidde Beydals <hello@hidde.co>
@darkowlzz @hiddeco
api: Embed runtime.Object in Source interface
6789aaf 
Embedding runtime.Object in Source interface makes the Source type more
useful to interact with k8s API machinery.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz @hiddeco
Add internal packages error and reconcile
5df4acb 
- internal/error - Contains internal error type used across the
  source-controller reconcilers.
- internal/reconcile - Contains helper abstractions for the
  controller-runtime reconcile Result type and functions to
  interact with the abstractions.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@hiddeco
internal/util: introduce temp dir/path helpers
5ab2f62 
In most of the reconcilers we have a repetative pattern of using part of
the object metadata to construct a temporary file path.

This commit introduces helpers as an abstraction, for both the creation
of a temporary directory based on `client.Object` type and object
metadata, and the generation of an arbitrary random temporary path
string.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
Rewrite GitRepositoryReconciler to new standards
31d2e6d 
This commit rewrites the `GitRepositoryReconciler` to new standards,
while implementing the newly introduced Condition types, and trying
to adhere better to Kubernetes API conventions.

More specifically it introduces:

- Implementation of more explicit Condition types to highlight
  abnormalities.
- Extensive usage of the `conditions` subpackage from `runtime`.
- Better and more conflict-resilient (status)patching of reconciled
  objects using the `patch` subpackage from runtime.
- Proper implementation of kstatus' `Reconciling` and `Stalled`
  conditions.
- First (integration) tests that solely rely on `testenv` and do not
  use Ginkgo.

There are a couple of TODOs marked in-code, these are suggestions for
the future and should be non-blocking.
In addition to the TODOs, more complex and/or edge-case test scenarios
may be added as well.

Signed-off-by: Hidde Beydals <hello@hidde.co>
darkowlzz and others added 24 commits February 23, 2022 12:35
@darkowlzz @hiddeco
helmrepo: Add more reconciler design improvements
f14a053 
- Remove ArtifactUnavailable condition and use Reconciling condition to
  convey the same.
- Make Reconciling condition affect the ready condition.
- Introduce summarizeAndPatch() to calculate the final status conditions
  and patch them.
- Introduce reconcile() to iterate through the sub-reconcilers and
  execute them.
- Simplify logging and event recording
- Introduce controller-check/status checks to assert that the status
  conditions are valid at the end of the tests.
- Create variables for various condition groups: owned conditions, ready
  dependencies and ready dependencies negative.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz @hiddeco
helmchart: Replace GetInterval() with GetRequeueAfter()
e0e048a 
Signed-off-by: Sunny <darkowlzz@protonmail.com>
@hiddeco
HelmChartReconciler refactor
8e107ea 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
Rewrite HelmChartReconciler tests
527fce0 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: tweak events and logging
032ffb4 
- Ensure all logged messages start with a lowercase.
- Make some pushed (and logged) events of type `EventTypeTrace` to
  prevent them from being sinked to the external event recorder, to
  prevent spam.
- Only log if artifact is up-to-date with upstream (instead of pushing
  an event).

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: remove legacy test suite
ceb61a3 
As it no longer contains any test cases.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
api: remove obsolete constants
474658a 
Remove the constants which are no longer in use from the API.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@darkowlzz @hiddeco
Consolidate result conversion and computation
78882b3 
Consolidate BuildRuntimeResult() into summarizeAndPatch() to simplify
where the results are computed, summarized and patched.

Move the event recording and logging of context specific errors into
RecordContextualError() and call it in summarizeAndPatch().

Introduce Waiting error for wait and requeue scenarios. Update
ComputeReconcileResult() and RecordContextualError() to consider Waiting
error.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@hiddeco
internal/helm: ErrChartReference on local load err
eb0a554 
If a local reference does not contain a path to a valid file, returning
`ErrChartReference` is more correct to signal the reference is invalid.

This also indirectly causes the reconciler to signal a Suspend, as the
source or resource requires a change before a reattempt might be
successful.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: only handle BuildError
fb45032 
All other errors returned by `build*` are already properly wrapped.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: HelmChart Reconcile test
21a7dfe 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: cleanup TestStorageCopyFromPath files
028a85d 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: truncate temporary cached Helm index
ad0993e 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: replace logr.NewContext shims
849f7ee 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@darkowlzz @hiddeco
storage: Return details about the deleted items
9b56137 
Update Storage.RemoveAll() and Storage.RemoveAllButCurrent() to return
the details about the deleted items. This helps emit useful information
about garbage collection in the controllers and ignore no-op garbage
collections.

RemoveAll() returns the path of the deleted directory if any.
RemoveAllButCurrent() returns a slice of path of all the deleted items
from a resource's artifact dir.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz @hiddeco
Make generic SummarizeAndPatch()
d997876 
summarizeAndPatch() was used by all the reconcilers with their own
object type. This creates a generic SummarizeAndPatch helper that takes
a conditions.Setter object and performs the same operations. All the
reconcilers are updated to use SummarizeAndPatch(). The process of
summarize and patch can be configured using the HelperOptions.

Introduce ResultProcessor to allow injecting middlewares in the
SummarizeAndPatch process.

Introduce RuntimeResultBuilder to allow defining how the reconciliation
result is computed for specific reconciler. This enabled different
reconcilers to have different meanings of the reconciliation results.

Introduce Conditions in summary package to store all the status
conditions related information of a reconciler. This is passed to
SummarizeAndPatch() to be used for summary and patch calculation.

Remove all the redundant summarizeAndPatch() tests per reconciler.

Add package internal/object containing helpers for interacting with
runtime.Object needed by the generic SummarizeAndPatch().

Add tests for ComputeReconcileResult().

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz @hiddeco
Update API descriptions and messages to be consistent
45df2d7 
- Update v1beta2 API descriptions and reconciling messages to be
  consistent.
- Replace 'download' with 'fetch'. Since the status condition for
  download failure is called FetchFailed, using the term 'fetch' makes
  the messaging more consistent.
- Replace `BucketOperationSucceed` with `BucketOperationSucceeded` and
  generate api docs.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz @hiddeco
gitrepo: Use commit msg in NewArtifact message
acda998 
Use commit message in the NewArtifact event message to make it more user
friendly.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz @hiddeco
bucket: Make NewArtifact event more informative
47d0958 
Use the etagIndex to provide more information about the artifact in
NewArtifact events and remove the revision from the event message. The
revision is still kept in the event annotations.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@hiddeco
build: update meta API and pkg/runtime to non RC
07a539e 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@darkowlzz @hiddeco
Use field owner in the patch helper
f72a28a 
- Update summarize helper to have patch field owner.
- Updated the controllers to set the patch field owner.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz @hiddeco
Add kstatus client conformance tests
e9ae0c2 
Use kstatus to compute the status of the objects.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz @hiddeco
fuzz: Update to use v1beta2 APIs
84bf8c8 
Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz @hiddeco
helmrepo: Make NewArtifact event human friendly
9c7661d 
Inform index size and repo instead of a revision.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
stefanprodan
stefanprodan approved these changes Feb 23, 2022
View reviewed changes
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @hiddeco @darkowlzz and everyone else who helped with this tremendous task

@hiddeco hiddeco merged commit ff94d41 into main Feb 23, 2022
@hiddeco hiddeco deleted the rewrite-dev branch February 23, 2022 12:30
@hiddeco hiddeco mentioned this pull request Feb 23, 2022
Closed
@chanwit chanwit mentioned this pull request Feb 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darkowlzz darkowlzz darkowlzz left review comments

@stefanprodan stefanprodan stefanprodan approved these changes

@relu relu Awaiting requested review from relu

@pjbgf pjbgf Awaiting requested review from pjbgf

@makkes makkes Awaiting requested review from makkes

Assignees
No one assigned
Labels
area/bucket Bucket related issues and pull requests area/git Git related issues and pull requests area/helm Helm related issues and pull requests enhancement New feature or request
Projects
Maintainers' Focus
Status: Done
Milestone
No milestone
3 participants
@hiddeco @darkowlzz @stefanprodan