SEP75 Automatic Workload Migration by dgn · Pull Request #850 · istio-ecosystem/sail-operator

dgn · 2025-05-26T13:11:27Z

First draft

codecov · 2025-05-26T13:19:41Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.48%. Comparing base (eb6389f) to head (df43c23).
⚠️ Report is 196 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #850      +/-   ##
==========================================
+ Coverage   76.70%   77.48%   +0.78%     
==========================================
  Files          44       44              
  Lines        2640     2834     +194     
==========================================
+ Hits         2025     2196     +171     
  Misses        529      529              
- Partials       86      109      +23

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

MaxBab · 2025-05-26T16:10:50Z

+    // The highest version that the Sail Operator will update to. Updates to versions later than MaximumVersion
+    // will not trigger workload migration.
+    // If unset, the operator will only trigger workload migration for new patch versions on the current minor version stream.
+    MaximumVersion *string `json:maximumVersion,omitempty"`


Q: Does it means that the maximum version set could be a version that is number of versions ahead of the current version running and not just the next one?
But from other side is to set a specific version I would not want to get over?

What would be an usage of this? To avoid automatic workload migration when upgrading to new minor version?

Yes- basically by default it will only update patch versions, unless you set a higher MaximumVersion (now that I'm reading it again, maybe MaxVersion is better)

I added a bunch of explanations for this field (and also renamed it maxVersion)

FilipB · 2025-05-26T18:44:13Z

+    // The highest version that the Sail Operator will update to. Updates to versions later than MaximumVersion
+    // will not trigger workload migration.
+    // If unset, the operator will only trigger workload migration for new patch versions on the current minor version stream.
+    MaximumVersion *string `json:maximumVersion,omitempty"`


What would be an usage of this? To avoid automatic workload migration when upgrading to new minor version?

dgn · 2025-05-27T22:00:58Z

Update the doc to now use an enum (strategy) instead of a bool (enabled) for enablement. Not sure I like the stuttering (updateStrategy.workloadMigration.strategy is not great) but I couldn't come up with a better term right now

nrfox · 2025-05-28T12:00:50Z

+    // The highest version that the Sail Operator will update to. Updates to versions later than MaximumVersion
+    // will not trigger workload migration.
+    // If unset, the operator will only trigger workload migration for new patch versions on the current minor version stream.
+    MaximumVersion *string `json:"maximumVersion,omitempty"`


If you want the operator to upgrade your workloads when going from say 1.25 --> 1.26, you'd need to set spec.version = 1.26 and spec.updateStrategy.maximumVersion = 1.26? For each minor upgrade you'd need to keep setting spec.updateStrategy.maximumVersion?

Yes, effectively. Which is a good thing I believe, as minor version upgrades could break your config. Or you set it to something like 1.999.0 to always upgrade

I actually did change this as I think it was confusing. Now, if you don't set maxVersion, there simply is no maximum, which I think is more what you would expect as a user.

skriss

Had a few thoughts mainly around the idea of supporting more of a canary upgrade/migration process. You could make the argument that that could be a separate strategy (canary instead of batched) with its own configuration. From my perspective it would probably be pretty appealing to users. In general though this seems like a pretty compelling feature and great differentiator for the operator!

skriss · 2025-10-22T19:15:51Z

+
+### Failure Policy
+
+The initial implementation will continue processing all batches even if individual workloads fail to migrate, reporting all failures at the end. Some users may want different behavior when migrations fail. We could implement a `failurePolicy` field that allows users to specify what the operator should do in case a workload does not become ready.


The initial implementation is somewhat at odds with a "canary upgrade" where you'd want to ensure a small subset of upgraded workloads are successful before proceeding, so IMO adding something like a failurePolicy will likely be pretty important for users, to be able to have a controlled upgrade / limit blast radius of any issues.

skriss · 2025-10-22T19:19:16Z

+Potential `failurePolicy` values:
+* `ContinueOnError`: Continue migrating all workloads, report failures at end
+* `FailFast`: Stop immediately on first failure


A configurable % of workloads that are allowed to fail before stopping the migration might be nice as it would give finer-grained control over success/failure conditions (i.e. "continue migration as long as no more than 5% of workloads failed upgrade").

skriss · 2025-10-22T19:24:54Z

+**Potential annotations:**
+- `sailoperator.io/readiness-timeout: 10s`: Override the readiness timeout for specific workloads
+- `sailoperator.io/skip-migration: true`: Exclude specific workloads from automatic migration
+- `sailoperator.io/migration-batch: 3`: Assign workload to a specific migration priority (integer, lower values migrate first)


Something like this would be very helpful for supporting canary-style upgrades as it would allow the user to identify the initial set of workloads (i.e. less critical) to try to migrate. Otherwise, it's possible that critical workloads could be ~randomly selected for migration first which would not be ideal.

This adds an enhancement proposal on Automatic Workload Migration, which helps users by automatically restarting their workloads when updating their mesh. This feature will only be available in sidecar mode. Signed-off-by: Daniel Grimm <dgrimm@redhat.com>

FilipB · 2025-10-24T10:00:56Z

+
+type WorkloadMigrationStatus struct {
+    // State represents the current state of the migration process.
+    // +kubebuilder:validation:Enum=Idle;InProgress;Completed;Failed


What would be a difference between Idle and Completed?

maybe NotStarted is a better name

FilipB · 2025-10-24T10:04:41Z

+    // Name of the failed workload.
+    Name string `json:"name"`
+
+    // Kind of the failed workload (e.g., "Deployment").


Is this needed if we only migrate Deployments and nothing else?

good call. we can leave it out for now and only add it once we support other kinds

nrfox

I had a few more questions/comments but overall this looks good

nrfox · 2025-12-02T19:43:40Z

+    // - MaxVersion: unset -> migrate to any version (default behavior)
+    // - MaxVersion: "1.24.999" -> migrate within 1.24.x only
+    // - MaxVersion: "1.26.0" -> migrate up to and including 1.26.0
+    // - MaxVersion: "1.999.0" -> migrate to any 1.x version, stop at 2.0


It's a little awkward having to specify 1.24.999 where 999 is just an arbitrarily large number.

The behavior could be that leaving off the patch version means "migrate within 1.24.x only"? e.g.

MaxVersion: "1.24.5" -> migrate up to and including 1.24.5 MaxVersion: "1.24" -> migrate within 1.24.x only MaxVersion: "1" -> migrate to any 1.x version, stop at 2.0

nrfox · 2025-12-02T19:46:16Z

+    MaxVersion *string `json:"maxVersion,omitempty"`
+
+    // Maximum time to wait for a deployment to become ready after restart.
+    // Defaults to 5m.
+    // +kubebuilder:default="5m"
+    ReadinessTimeout *metav1.Duration `json:"readinessTimeout,omitempty"`


Will these fields be relevant to other workload strategies as well? If we keep them here then we'll need to duplicate them on the other strategies. Should they be moved up into WorkloadMigrationConfig?

nrfox · 2025-12-02T20:01:30Z

+
+type WorkloadMigrationStatus struct {
+    // State represents the current state of the migration process.
+    // +kubebuilder:validation:Enum=Idle;InProgress;Completed;Failed


Does Failed mean "there was an error. won't retry" or "there was an error. will retry"?

nrfox · 2025-12-02T21:01:20Z

+### Performance Impact
+
+* **Discovery Overhead**: The operator lists all namespaces and deployments once per migration
+* **Batch Processing**: Migration impact is controlled through configurable batch sizes and delays
+* **Memory Usage**: Minimal additional memory for tracking migration state
+* **Network Traffic**: Standard Kubernetes API calls for object updates


If the whole migration is performed in a single reconcile loop, I'm more worried about the operator being tied up with the migration for a particular Istio resource and unable to respond to updates to that Istio resource until the migration is finished which could take awhile given a large amount of workloads and needing to wait until each pod has restarted.

dgn · 2025-12-04T12:51:35Z

as per our discussion regarding ZTunnel upgrades, this should be extended to be flexible enough to enable restarting ambient workloads when the ZTunnel instance is updated

nrfox · 2026-03-19T16:20:47Z

@dgn would having an implementation of this be helpful in moving this along?

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a first-draft SEP describing “Automatic Workload Migration” for Sail Operator’s revision-based Istio upgrades, including API shape, status reporting, migration flow, and a test plan.

Changes:

Specifies a new workloadMigration configuration under IstioUpdateStrategy with a “Batched” strategy and tunables (batching, delays, readiness timeout, max version boundary).
Proposes status reporting (WorkloadMigrationStatus) and describes discovery/migration phases.
Adds an initial implementation plan and detailed test scenarios.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+| Status | Authors | Created | 
+|---|---|---|
+| WIP | @dgn | 2025-05-22 |


+#### Deprecation / Removal of `updateStrategy.updateWorkloads` field
+
+We accidentally included this field in our v1 CRD even though it never had any functionality. It doesn't really follow our existing API standards so we won't use it for this functionality going forward, and will remove it instead. As it was never functional to begin with, removing it should cause no harm.
+


+The operator already has the required permission to update `Deployment` resources.
+


+2. **Discovery**: The operator discovers workloads using old revisions by:
+   - Listing all namespaces and checking their `istio.io/rev` or `istio-injection` labels
+   - Listing all deployments and checking their pod template labels
+   - Listing all pods and checking their pod annotations to detect injected revision


+   - Listing all pods and checking their pod annotations to detect injected revision
+   - Comparing current annotations against the active revision name


+* **Discovery Overhead**: The operator lists all namespaces and deployments once per migration
+* **Batch Processing**: Migration impact is controlled through configurable batch sizes and delays
+* **Memory Usage**: Minimal additional memory for tracking migration state
+* **Network Traffic**: Standard Kubernetes API calls for object updates


+   - Requires updating `maxVersion` when ready for 1.27
+   - Best for staged rollouts with validation between minor versions
+
+**Implementation Note:** Version comparison uses semantic versioning rules. When `maxVersion` is set, the operator compares `spec.version` against `maxVersion` using standard semver comparison (`<=`). When unset, all versions are accepted.


+metadata:
+  name: default
+spec:
+  version: v1.26.0


+    workloadMigration:
+      strategy: Batched
+      batched:
+        maxVersion: "1.26.999"  # Only migrate within 1.26.x, stop at 1.27


dgn requested a review from a team as a code owner May 26, 2025 13:11

istio-testing added the size/L label May 26, 2025

dgn force-pushed the sep-workload-migration branch from 6f842cc to 1f95a6b Compare May 26, 2025 13:13

dgn changed the title ~~SEP74 Automatic Workload Migration~~ SEP75 Automatic Workload Migration May 26, 2025

dgn force-pushed the sep-workload-migration branch from 1f95a6b to 7a66e00 Compare May 26, 2025 13:25

fjglira reviewed May 26, 2025

View reviewed changes

Comment thread enhancements/SEP75-automatic-workload-migration.md

MaxBab reviewed May 26, 2025

View reviewed changes

fjglira reviewed May 26, 2025

View reviewed changes

Comment thread enhancements/SEP75-automatic-workload-migration.md Outdated

FilipB reviewed May 26, 2025

View reviewed changes

dgn force-pushed the sep-workload-migration branch from 7a66e00 to 1e6c512 Compare May 28, 2025 08:41

nrfox reviewed May 28, 2025

View reviewed changes

Comment thread enhancements/SEP75-automatic-workload-migration.md

Comment thread enhancements/SEP75-automatic-workload-migration.md

Comment thread enhancements/SEP75-automatic-workload-migration.md

nrfox reviewed May 28, 2025

View reviewed changes

sridhargaddam reviewed Jun 4, 2025

View reviewed changes

Comment thread enhancements/SEP75-automatic-workload-migration.md

Comment thread enhancements/SEP75-automatic-workload-migration.md

FilipB mentioned this pull request Jul 10, 2025

Fully Automatic Upgrades #74

Open

dgn force-pushed the sep-workload-migration branch from 1e6c512 to 266f40e Compare October 22, 2025 11:58

istio-testing added size/XL and removed size/L labels Oct 22, 2025

dgn force-pushed the sep-workload-migration branch 4 times, most recently from e1e8399 to e029cc3 Compare October 22, 2025 12:12

skriss reviewed Oct 22, 2025

View reviewed changes

dgn force-pushed the sep-workload-migration branch from e029cc3 to df43c23 Compare October 23, 2025 09:26

FilipB reviewed Oct 24, 2025

View reviewed changes

nrfox reviewed Dec 2, 2025

View reviewed changes

dgn requested a review from Copilot April 16, 2026 14:40

Copilot AI reviewed Apr 16, 2026

View reviewed changes

Copilot started reviewing on behalf of dgn April 16, 2026 14:53 View session


		### Failure Policy

		The initial implementation will continue processing all batches even if individual workloads fail to migrate, reporting all failures at the end. Some users may want different behavior when migrations fail. We could implement a `failurePolicy` field that allows users to specify what the operator should do in case a workload does not become ready.

		#### Deprecation / Removal of `updateStrategy.updateWorkloads` field

		We accidentally included this field in our v1 CRD even though it never had any functionality. It doesn't really follow our existing API standards so we won't use it for this functionality going forward, and will remove it instead. As it was never functional to begin with, removing it should cause no harm.

		The operator already has the required permission to update `Deployment` resources.

		- Listing all pods and checking their pod annotations to detect injected revision
		- Comparing current annotations against the active revision name

Conversation

dgn commented May 26, 2025

Uh oh!

codecov Bot commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

MaxBab May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dgn commented May 27, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skriss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrfox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgn commented Dec 4, 2025

Uh oh!

nrfox commented Mar 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov Bot commented May 26, 2025 •

edited

Loading

MaxBab May 26, 2025 •

edited

Loading