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

Error is logged even if result of status.Preflight doesn't have one #374

Open
tomasaschan opened this issue Jan 25, 2024 · 2 comments · May be fixed by #386
Open

Error is logged even if result of status.Preflight doesn't have one #374

tomasaschan opened this issue Jan 25, 2024 · 2 comments · May be fixed by #386
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@tomasaschan
Copy link
Member

What happened:

I implemented a custom declarative.Status like this:

type myStatus {}

func (myStatus) Preflight(ctx context.Context, o declarative.DeclarativeObject) error {
    // omitted: some precondition is not fulfilled
    // want to requeue with backoff and try again after some other controller has had a chance to make the precondition true
    return declarative.ErrorResult{
        Result: result.Result{Requeue: true},
        // note: Err: nil implied here
    }
}

func NewMyStatus() declarative.Status {
    return declarative.StatusBuilder{
        Preflight: myStatus{},
    }
}

When my controller reconciles with this, it logs "preflight check failed, not reconciling" with a stack trace that doesn't point to anywhere in my own code. That's annoying and a little confusing.

What you expected to happen: reconciliation is requeued without any errors logged.

How to reproduce it (as minimally and precisely as possible):

Create a declarative controller with a status implementation like the above, reconcile an instance and observe logs.

Anything else we need to know?:

This seems related to, but is different from #232. I imagine if my status also had a Reconcile function, it would be confusing that it gets called even though the preflight check failed (signalled by returning a non-nil result, even if that result has a nil Err). That, in turn, happens because this deferred function doesn't check for the preflight result. (It seems it once did check, but it's unclear to me why that was removed...)

@tomasaschan tomasaschan added the kind/bug Categorizes issue or PR as related to a bug. label Jan 25, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 24, 2024
@tomasaschan
Copy link
Member Author

/remove-lifecycle stale

I've encountered this again recently, and have a mind on hacking on a better implementation here.

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants