Skip to content

fix(controller): DescribeVersion on NotRegistered Versions to prevent pre-mature scaledown of said versions - #556

Merged
carlydf merged 6 commits into
mainfrom
shivam/issue-550-notregistered-notfound
Aug 30, 2026
Merged

fix(controller): DescribeVersion on NotRegistered Versions to prevent pre-mature scaledown of said versions #556
carlydf merged 6 commits into
mainfrom
shivam/issue-550-notregistered-notfound

Conversation

@Shivs11

@Shivs11 Shivs11 commented Aug 28, 2026

Copy link
Copy Markdown
Member

Summary

  • describe Kubernetes-backed Worker Deployment Versions that are absent from deployment summaries before treating them as NotRegistered
  • add an integration regression test using a real pinned workflow while filtering the version from deployment summaries

Behavioral impact

  • Versions present in VersionSummaries retain their existing status precedence and rollout behavior.
  • A Kubernetes build ID missing from VersionSummaries now triggers one DescribeVersion call:
    • success restores the version to controller state and prevents unsafe deletion
    • NotFound preserves the existing NotRegistered cleanup path
    • any other error stops reconciliation instead of continuing with incomplete state and potentially deleting the deployment
  • A successfully described inactive target version can use the existing progressive-rollout unversioned-poller check.
  • No planner, CRD, or public API behavior changes.

Testing

  • go test ./internal/temporal ./internal/controller ./internal/planner -count=1
  • go test -v -tags test_dep ./internal -run 'TestIntegration/missing-summary-version-is-described-before-deletion' -count=1
  • go vet ./internal/temporal ./internal/controller ./internal/planner
  • git diff --check

Closes #550

@CLAassistant

CLAassistant commented Aug 28, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Comment thread internal/tests/internal/not_registered_integration_test.go
@Shivs11 Shivs11 changed the title fix(controller): confirm versions missing from deployment summaries fix(controller): DescribeVersion on NotRegistered Versions to prevent pre-mature scaledown of said versions Aug 28, 2026
@Shivs11
Shivs11 marked this pull request as ready for review August 28, 2026 16:16
@Shivs11
Shivs11 requested review from a team, eniko-dif and jlegrone as code owners August 28, 2026 16:16
Comment thread internal/temporal/worker_deployment.go Outdated
Comment thread internal/temporal/worker_deployment.go Outdated
@Shivs11

Shivs11 commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

going to hold off on this one for now, and wait for @carlydf's PR to go through!

Comment thread internal/temporal/worker_deployment.go Outdated
@Shivs11
Shivs11 force-pushed the shivam/issue-550-notregistered-notfound branch from 7e1906d to 7b52420 Compare August 29, 2026 20:03
Comment thread internal/temporal/worker_deployment.go
Comment thread internal/temporal/worker_deployment.go Outdated
Comment thread internal/tests/internal/not_registered_integration_test.go
@carlydf
carlydf enabled auto-merge (squash) August 30, 2026 02:11
@carlydf
carlydf merged commit 6a222ee into main Aug 30, 2026
18 checks passed
@carlydf
carlydf deleted the shivam/issue-550-notregistered-notfound branch August 30, 2026 02:21
@jaypipes jaypipes added this to the vNext milestone Aug 31, 2026
for buildID := range k8sDeployments {
if _, exists := state.Versions[buildID]; exists {
continue
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

would be great to put a log line in here:

l.Info(
    "warning: Kubernetes Deployment not found in WorkerDeployment VersionSummaries. "+
        "Possible inconsistent Temporal server state.",
    "buildID", buildID,
)

},
)
if versionInfo == nil || versionInfo.Status == temporaliov1alpha1.VersionStatusNotRegistered {
return nil, fmt.Errorf("describe worker deployment version for buildID %q returned no registered status", buildID)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("describe worker deployment version for buildID %q returned no registered status", buildID)
return nil, fmt.Errorf("describe worker deployment version for buildID %q returned NotRegistered status", buildID)


var deployment appsv1.Deployment
if err := k8sClient.Get(ctx, types.NamespacedName{Name: deploymentNameV1, Namespace: namespace}, &deployment); err != nil {
return fmt.Errorf("v1 Deployment was deleted: %w", err)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Technically, this should check whether err was NotFound...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] scale down K8s deployments if the version is NotRegistered + NotFound

4 participants