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

Start autoupdate_agent_rollout controller in auth service #49101

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hugoShaka
Copy link
Contributor

@hugoShaka hugoShaka commented Nov 15, 2024

This PR adds the autoupdate_agent_rollout controller and starts it when we run as the auth service.

This also makes the Reconciler type private as there is currently no usage outside of the Controller.

@hugoShaka hugoShaka added the no-changelog Indicates that a PR does not require a changelog entry label Nov 15, 2024
@hugoShaka hugoShaka force-pushed the hugo/add-autoupdate-agent-rollout-controller branch from f611f35 to 1042ef7 Compare November 15, 2024 22:22
Comment on lines +1879 to +1880
require.NoError(t, process.Start())
t.Cleanup(func() { require.NoError(t, process.Close()) })
Copy link
Contributor

@rosstimothy rosstimothy Nov 15, 2024

Choose a reason for hiding this comment

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

FYI without waiting for all the services to complete this test could be susceptible to flakiness caused by clean up of the data directory while Teleport components may still be running. See #49050.

// See the reconciler godoc for more details about the reconciliation process.
// We currently wake up every minute, in the future we might decide to also watch for events
// (from autoupdate_config and autoupdate_version changefeed) to react faster.
type Controller struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Avoid repetition (rolloutcontroller.Controller), either by renaming this type or changing the package to rollout or something similar.

Jitter: retryutils.SeventhJitter,
Clock: c.clock,
}
ticker := interval.New(config)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ticker := interval.New(config)
ticker := interval.New(config)
defer ticker.Stop()

clt: client,
log: log,
}

// Test execution: run the reconciliation loop
require.ErrorContains(t, reconciler.Reconcile(cancelableCtx), "canceled")
require.ErrorContains(t, reconciler.reconcile(cancelableCtx), "canceled")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
require.ErrorContains(t, reconciler.reconcile(cancelableCtx), "canceled")
require.ErrorIs(t, reconciler.reconcile(cancelableCtx), context.Canceled)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants