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

Allow deleting suspended objects #937

Merged
merged 2 commits into from
Oct 20, 2022
Merged

Allow deleting suspended objects #937

merged 2 commits into from
Oct 20, 2022

Conversation

darkowlzz
Copy link
Contributor

  • Reorders the object suspended check in all the reconcilers to allow
    deletion of objects when they are suspended. Objects used to get stuck
    on delete because the finalizers were not getting removed due to the
    suspended state.

  • Add setters and getters for spec.suspend and status.artifact in internal/object package.
    This is needed for writing generic tests for any source kind.

  • Adds a generic test for all the reconcilers to check if a suspended
    source object can be delete.

Some observations related to the metrics:

When a suspended object is created, finalizer is set and status.observedGeneration remains in -1.

apiVersion: source.toolkit.fluxcd.io/v1beta2
kind: GitRepository
metadata:
  ...
  finalizers:
  - finalizers.fluxcd.io
  generation: 1
  name: podinfo3
  namespace: default
  ...
status:
  observedGeneration: -1

With prometheus query gotk_reconcile_condition{kind=~"GitRepository",name="podinfo3"}==1, following are the observed metrics:

  • status=Unknown when suspended on creation

gotk_reconcile_condition{container="manager", endpoint="http-prom", exported_namespace="default", instance="10.244.0.21:8080", job="monitoring/flux-system", kind="GitRepository", name="podinfo3", namespace="flux-system", pod="source-controller-8bb478cb9-knmg4", status="Unknown", type="Ready"}

  • status=Ready on resuming

gotk_reconcile_condition{container="manager", endpoint="http-prom", exported_namespace="default", instance="10.244.0.21:8080", job="monitoring/flux-system", kind="GitRepository", name="podinfo3", namespace="flux-system", pod="source-controller-8bb478cb9-knmg4", status="True", type="Ready"}

  • status=Ready when the object is suspended again, same as above.

  • status=Deleted when the suspended object is deleted

gotk_reconcile_condition{container="manager", endpoint="http-prom", exported_namespace="default", instance="10.244.0.21:8080", job="monitoring/flux-system", kind="GitRepository", name="podinfo3", namespace="flux-system", pod="source-controller-8bb478cb9-knmg4", status="Deleted", type="Ready"}


Fixes #934

@darkowlzz darkowlzz added the bug Something isn't working label Oct 18, 2022
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 @darkowlzz

@stefanprodan
Copy link
Member

Looks like main brach is broken as well:

--- FAIL: TestGitRepositoryReconciler_reconcileSource_authStrategy (29.15s)
    --- FAIL: TestGitRepositoryReconciler_reconcileSource_authStrategy/SSH_with_password_protected_private_key_secret_makes_ArtifactOutdated=True (25.77s)
        --- FAIL: TestGitRepositoryReconciler_reconcileSource_authStrategy/SSH_with_password_protected_private_key_secret_makes_ArtifactOutdated=True/libgit2 (19.21s)
            gitrepository_controller_test.go:543: 
                expected
                	[]v1.Condition{v1.Condition{Type:"FetchFailed", Status:"True", ObservedGeneration:0, LastTransitionTime:time.Date(2022, time.October, 11, 15, 7, 2, 0, time.UTC), Reason:"GitOperationFailed", Message:"failed to checkout and determine revision: recovered from git2go panic: runtime error: invalid memory address or nil pointer dereference"}}
                to match
                	[]v1.Condition{v1.Condition{Type:"ArtifactOutdated", Status:"True", ObservedGeneration:0, LastTransitionTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), Reason:"NewRevision", Message:"new upstream revision 'master/9a6150b9f28091197cb5c0e6f52a50f2fa07b6bd'"}, v1.Condition{Type:"Reconciling", Status:"True", ObservedGeneration:0, LastTransitionTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), Reason:"NewRevision", Message:"new upstream revision 'master/9a6150b9f28091197cb5c0e6f52a50f2fa07b6bd'"}}

@darkowlzz
Copy link
Contributor Author

Looks like main brach is broken as well

failed to checkout and determine revision: recovered from git2go panic: runtime error: invalid memory address or nil pointer dereference

Seems to be because of git2go panic, which is resulting in a different status condition. Also, only panicking on macOS 11.

@stefanprodan
Copy link
Member

Ok we can ignore the panic then, it's not something we can fix in this PR.

Add setters and getters for spec.suspend and status.artifact.
This is needed for writing generic tests for any source kind.

Signed-off-by: Sunny <[email protected]>
Reorders the object suspended check in all the reconcilers to allow
deletion of objects when they are suspended. Objects used to get stuck
on delete because the finalizers were not getting removed due to the
suspended state.

Adds a generic test for all the reconcilers to check if a suspended
source object can be delete.

Signed-off-by: Sunny <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Suspended sources can't be deleted
4 participants