Skip to content

Conversation

@andyasp
Copy link
Collaborator

@andyasp andyasp commented May 8, 2025

Addresses #125.

Relevant Kubernetes documentation here: https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#stable-network-id

A StatefulSet can use a Headless Service to control the domain of its Pods. The domain managed by this Service takes the form: $(service name).$(namespace).svc.cluster.local, where "cluster.local" is the cluster domain. As each Pod is created, it gets a matching DNS subdomain, taking the form: $(podname).$(governing service domain), where the governing service is defined by the serviceName field on the StatefulSet.

When constructing the endpoint to call for a pod's prepare-downscale, rollout-operator was assuming the .spec.serviceName of a StatefulSet was equal to the .metadata.name of the StatefulSet. This happens to be true for each ingester StatefulSet in Mimir's Jsonnet, but is not true for ingesters in Mimir's Helm since it has a single headless ingester service.

A community member kindly submitted a fix for this issue in #173, but it never got a response. From inspection I think that implementation missed support for statefulsets/scale. It also added another StatefulSet query. I implemented these changes from scratch to address these issues and avoid other unnecessary changes.

Marking as draft until I validate some more. Validated locally

@andyasp andyasp marked this pull request as ready for review May 9, 2025 15:30
@aknuds1 aknuds1 requested a review from Copilot May 9, 2025 15:49
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue with constructing the prepare-downscale endpoint by using a StatefulSet’s .spec.serviceName rather than its metadata name. Key changes include:

  • Updating the logic to obtain preparation info via getStatefulSetPrepareInfo.
  • Modifying createEndpoints calls to inject serviceName for correct DNS construction.
  • Renaming and updating tests to validate the new behavior.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
pkg/admission/zone_tracker.go Updated prepareDownscale logic to use serviceName and refactored label/annotation handling.
pkg/admission/prep_downscale_test.go Modified tests to align with the new statefulSetPrepareInfo structure and endpoint logic.
pkg/admission/prep_downscale.go Refactored endpoint construction and preparation info retrieval to support serviceName usage.
CHANGELOG.md Documented the bugfix regarding the use of .spec.serviceName when constructing endpoints.
Comments suppressed due to low confidence (1)

pkg/admission/zone_tracker.go:154

  • The use of 'for i := range int(diff)' is incorrect since 'range' requires a slice or array, not an integer. Please revert to the traditional loop syntax: 'for i := 0; i < int(diff); i++ {'.
for i := range int(diff) {

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM

@andyasp andyasp merged commit 97af6b1 into main May 9, 2025
10 checks passed
@andyasp andyasp deleted the aasp/service-name-prepare-downscale branch May 9, 2025 16:14
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.

2 participants