Record size of artifact #585
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hiddeco
reviewed
Feb 18, 2022
There was a problem hiding this comment.
Besides the Archive function, there are multiple other functions which also do writes (or copies). I think it would be useful if we would record the size there as well, at least in the ones actively used by _controller.go (as I suspect some may no longer be in issue).
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 <[email protected]> Signed-off-by: Sunny <[email protected]> Co-authored-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
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 <[email protected]>
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 <[email protected]>
NOTE: Remove `hasArtifactUpdated` in the future once it's no longer used. Signed-off-by: Hidde Beydals <[email protected]>
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 <[email protected]> Co-authored-by: Hidde Beydals <[email protected]>
Signed-off-by: Sunny <[email protected]>
Also, introduce FetchFailedCondition for generic fetch failures. Signed-off-by: Sunny <[email protected]> Co-authored-by: Hidde Beydals <[email protected]>
Signed-off-by: Sunny <[email protected]> Co-authored-by: Hidde Beydals <[email protected]>
Embedding runtime.Object in Source interface makes the Source type more useful to interact with k8s API machinery. Signed-off-by: Sunny <[email protected]>
- 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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
NOTE: This should be amended with the previous commit which has commented out tests. Update reconcileSource() to work with the test case where no secret is set. A minimal auth options is created and used for git checkout. Update TestGitRepositoryReconciler_verifyCommitSignature() to use the new git.Commit type. Update TestGitRepositoryReconciler_reconcileSource_checkoutStrategy to add skipForImplementation for branch commit test case. Signed-off-by: Sunny <[email protected]>
Fixes error returned from target path validation check and adds more test cases for TestGitRepositoryReconciler_reconcileArtifact. Signed-off-by: Sunny <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
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 introduced of this method deprecates `GetInterval()`, which should be removed in a future MINOR release. Signed-off-by: Hidde Beydals <[email protected]>
- Mention the current revision in the up-to-date log message. - Ensure any error that is "swallowed" (not returned) is logged to ensure they are visible within the logs, and not just by inspecting the object. Signed-off-by: Hidde Beydals <[email protected]>
Use the created artifact server test storage in reconcileInclude test's GitRepositoryReconciler and cleanup the created storage. Fix the test assertions to check the copied artifact directories in the correct path. Also, update the tests to expect artifacts in the include `toPath` to exist. Signed-off-by: Sunny <[email protected]>
This tests the status conditions update in the gitrepository reconciler. Given a mix of old status conditions, on a successful reconciliation, the status condition is set to Ready=True. Signed-off-by: Sunny <[email protected]>
Adds test cases for reconcileArtifact to check if old status conditions are removed after new artifact is created. Adds a test case to verify that the latest artifact symlink points to the created artifact. Signed-off-by: Sunny <[email protected]>
This commit consolidates the `DownloadFailed` and `CheckoutFailed` Condition types into a new more generic `FetchFailed` type to simplify the API and observations by consumers. Signed-off-by: Hidde Beydals <[email protected]>
This wraps the errors which are returned instead of logging them, as the returned error is logged at the end of the reconcile run. Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
SourceVerifiedCondition is a normal condition, remove it from negative polarity conditions. Add SourceVerifiedCondition in patch option WithOwnedConditions. Also, Update the signature of reconcileInclude() to remove include being passed and overwritten in the first line. Include is available as part of the passed source object. Signed-off-by: Sunny <[email protected]>
While deleting, patching an object with new status results in "not found" error because the object is already deleted. The patching operation first patches the status conditions, the rest of the object and, at the very end, the rest of the status. When an object is deleted, the garbage collection results in the artifact in the status to be updated, resulting in a diff that is attempted to be patched when the deferred patch runs. Since the status patching runs at the very end, the object gets deleted before it can be patched. Ignore "not found" error while patching when the delete timestamp is set. Signed-off-by: Sunny <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
darkowlzz
force-pushed
the
rewrite-dev
branch
from
7d1278a to
eff68c8
Compare
- 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 <[email protected]>
As it no longer contains any test cases. Signed-off-by: Hidde Beydals <[email protected]>
Remove the constants which are no longer in use from the API. Signed-off-by: Hidde Beydals <[email protected]>
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 <[email protected]>
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 <[email protected]>
All other errors returned by `build*` are already properly wrapped. Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
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 <[email protected]>
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 <[email protected]>
- 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 <[email protected]>
Use commit message in the NewArtifact event message to make it more user friendly. Signed-off-by: Sunny <[email protected]>
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 <[email protected]>
Signed-off-by: Hidde Beydals <[email protected]>
- Update summarize helper to have patch field owner. - Updated the controllers to set the patch field owner. Signed-off-by: Sunny <[email protected]>
Use kstatus to compute the status of the objects. Signed-off-by: Sunny <[email protected]>
Signed-off-by: Sunny <[email protected]>
Inform index size and repo instead of a revision. Signed-off-by: Sunny <[email protected]>
darkowlzz
force-pushed
the
rewrite-dev
branch
from
8011d09 to
6d037cb
Compare
5 tasks
This adds a Size field to Artifacts, which reflects the number of bytes written to the artifact when it's being archived. Signed-off-by: Kevin McDermott <[email protected]>
darkowlzz
force-pushed
the
rewrite-dev
branch
from
6d037cb to
1d988d5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This updates the repository archive mechanism to record the number of bytes written, and stores it on the Artifact.
For Artifacts that are not written out, this will be
nil.This also fixes a small bug in the way the tempdir calculation was being done which was causing tests to fail locally.