NotificationsYou must be signed in to change notification settings
Fork911
Star10k

refactor: skip reconciliation for some presets#17595

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

evgeniy-scherbina merged 1 commit intomainfromyevhenii/510-reconciliation-loop

Apr 29, 2025

Merged

refactor: skip reconciliation for some presets#17595

evgeniy-scherbina merged 1 commit intomainfromyevhenii/510-reconciliation-loop

Apr 29, 2025

Conversation

Copy link

Contributor

evgeniy-scherbina commentedApr 28, 2025

No description provided.

refactor: skipping reconciliation for some of the presets

9dd36a2

github-actionsbot assignedevgeniy-scherbina

Apr 28, 2025

evgeniy-scherbina commented

Apr 28, 2025

View reviewed changes

enterprise/coderd/prebuilds/reconcile.go

		@@ -310,6 +310,15 @@ func (c *StoreReconciler) ReconcilePreset(ctx context.Context, ps prebuilds.Pres
		return nil
		}

		// Nothing has to be done.
		if !ps.Preset.UsingActiveVersion && actions.IsNoop() {

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

@dannykopping Do we need!ps.Preset.UsingActiveVersion? I think we can skip reconciliation for all presets ifactions.IsNoop() == true?

Copy link

Contributor

Choose a reason for hiding this comment

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

I guess, yeah, but if the preset is part of an inactive version then we shouldn't be logging anything for that; for active presets which have no operations to perform, that's at least diagnostically useful.

Consider an installation that's been using prebuilds for a while and they have >100 inactive preset versions; that'll log a tonne for no value.

Copy link

ContributorAuthor

evgeniy-scherbinaApr 29, 2025

Choose a reason for hiding this comment

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

okay, let's keep it as it is

Copy link

ContributorAuthor

evgeniy-scherbinaApr 29, 2025•
edited
Loading

Choose a reason for hiding this comment

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

I mean here we log withdebug level which is probably okay?

// Nothing has to be done.if!ps.Preset.UsingActiveVersion&&actions.IsNoop() {logger.Debug(ctx,"skipping reconciliation for preset - nothing has to be done",slog.F("template_id",ps.Preset.TemplateID.String()),slog.F("template_name",ps.Preset.TemplateName),slog.F("template_version_id",ps.Preset.TemplateVersionID.String()),slog.F("template_version_name",ps.Preset.TemplateVersionName),slog.F("preset_id",ps.Preset.ID.String()),slog.F("preset_name",ps.Preset.Name))returnnil}