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

feat: perform selected strategy for isbsvc fields to support PPND #317

Draft
wants to merge 26 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
e49cefb
feat: integrate USDE logic into isbsvc controller
dpadhiar Sep 30, 2024
398b6ac
chore: codegen
dpadhiar Sep 30, 2024
d4df605
Merge branch 'main' into usde-isb
dpadhiar Oct 2, 2024
146dd36
test: unit tests
dpadhiar Oct 2, 2024
df198ad
Merge branch 'main' into usde-isb
dpadhiar Oct 2, 2024
2e722d6
test: changes for checking pipeline paused cond
dpadhiar Oct 3, 2024
92dfa43
test: check inprogressstrategy of isbservice
dpadhiar Oct 3, 2024
075c649
test: add case for noop update on isbservice
dpadhiar Oct 3, 2024
2a6aeb5
fix: unset strategy
dpadhiar Oct 3, 2024
309e2cf
test: update unit tests
dpadhiar Oct 3, 2024
a1dcfe5
test: verifyISBServiceSpec compares pointer values
dpadhiar Oct 3, 2024
f69b7dd
fix: pipeline wasn't unpausing
dpadhiar Oct 3, 2024
721635c
feat: unset strategy
dpadhiar Oct 4, 2024
e0446c7
test: update unit test
dpadhiar Oct 4, 2024
baaed81
fix: invert processchildobjectwithPPND func
dpadhiar Oct 4, 2024
f601571
test: remove comments and add check that pl never pauses
dpadhiar Oct 4, 2024
b8c843b
fix: remove extra variable check in test case
dpadhiar Oct 4, 2024
a20a315
test: address review comments
dpadhiar Oct 4, 2024
d0ea107
fix: increase Consistently time
dpadhiar Oct 4, 2024
12491c8
fix: update func test for usde bug
dpadhiar Oct 7, 2024
1fa727c
Merge branch 'main' into usde-isb
dpadhiar Oct 7, 2024
c41e7d2
fix: merge main
dpadhiar Oct 7, 2024
9ed54b2
chore: comment out usde case
dpadhiar Oct 7, 2024
9c03871
chore: usde test
dpadhiar Oct 7, 2024
e7724f1
chore: remove comment
dpadhiar Oct 7, 2024
dcdd03e
test: remove eventually block
dpadhiar Oct 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,11 @@ spec:
- Deployed
- Failed
type: string
upgradeInProgress:
description: UpgradeInProgress indicates the upgrade strategy currently
being used and affecting the resource state or empty if no upgrade
is in progress
type: string
type: object
required:
- spec
Expand Down
5 changes: 5 additions & 0 deletions config/install.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,11 @@ spec:
- Deployed
- Failed
type: string
upgradeInProgress:
description: UpgradeInProgress indicates the upgrade strategy currently
being used and affecting the resource state or empty if no upgrade
is in progress
type: string
type: object
required:
- spec
Expand Down
93 changes: 77 additions & 16 deletions internal/controller/isbservicerollout_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,23 +66,47 @@ type ISBServiceRolloutReconciler struct {
customMetrics *metrics.CustomMetrics
// the recorder is used to record events
recorder record.EventRecorder

// maintain inProgressStrategies in memory and in ISBServiceRollout Status
inProgressStrategyMgr *inProgressStrategyMgr
}

func NewISBServiceRolloutReconciler(
client client.Client,
c client.Client,
s *runtime.Scheme,
restConfig *rest.Config,
customMetrics *metrics.CustomMetrics,
recorder record.EventRecorder,
) *ISBServiceRolloutReconciler {

return &ISBServiceRolloutReconciler{
client,
r := &ISBServiceRolloutReconciler{
c,
s,
restConfig,
customMetrics,
recorder,
nil,
}

r.inProgressStrategyMgr = newInProgressStrategyMgr(
// getRolloutStrategy function:
func(ctx context.Context, rollout client.Object) *apiv1.UpgradeStrategy {
isbServiceRollout := rollout.(*apiv1.ISBServiceRollout)

if isbServiceRollout.Status.UpgradeInProgress != "" {
return (*apiv1.UpgradeStrategy)(&isbServiceRollout.Status.UpgradeInProgress)
} else {
return nil
}
},
// setRolloutStrategy function:
func(ctx context.Context, rollout client.Object, strategy apiv1.UpgradeStrategy) {
isbServiceRollout := rollout.(*apiv1.ISBServiceRollout)
isbServiceRollout.Status.SetUpgradeInProgress(strategy)
},
)

return r
}

//+kubebuilder:rbac:groups=numaplane.numaproj.io,resources=isbservicerollouts,verbs=get;list;watch;create;update;patch;delete
Expand Down Expand Up @@ -277,33 +301,61 @@ func (r *ISBServiceRolloutReconciler) processExistingISBService(ctx context.Cont
// update our Status with the ISBService's Status
r.processISBServiceStatus(ctx, existingISBServiceDef, isbServiceRollout)

// if I need to update or am in the middle of an update of the ISBService, then I need to make sure all the Pipelines are pausing
isbServiceNeedsUpdating, isbServiceIsUpdating, err := r.isISBServiceUpdating(ctx, isbServiceRollout, existingISBServiceDef)
// if I am in the middle of an update of the ISBService, then I need to make sure all the Pipelines are pausing
_, isbServiceIsUpdating, err := r.isISBServiceUpdating(ctx, isbServiceRollout, existingISBServiceDef)
if err != nil {
return false, err
}

numaLogger.Debugf("isbServiceNeedsUpdating=%t, isbServiceIsUpdating=%t", isbServiceNeedsUpdating, isbServiceIsUpdating)
// determine if we're trying to update the ISBService spec
// if it's a simple change, direct apply
// if not, it will require PPND or Progressive
isbServiceNeedsToUpdate, upgradeStrategyType, err := usde.ResourceNeedsUpdating(ctx, newISBServiceDef, existingISBServiceDef)
if err != nil {
return false, err
}
numaLogger.
WithValues("isbserviceNeedsToUpdate", isbServiceNeedsToUpdate, "upgradeStrategyType", upgradeStrategyType).
Debug("Upgrade decision result")

// set the Status appropriately to "Pending" or "Deployed"
// if isbServiceNeedsUpdating - this means there's a mismatch between the desired ISBService spec and actual ISBService spec
// Note that this will be reset to "Deployed" later on if a deployment occurs
if isbServiceNeedsUpdating {
if isbServiceNeedsToUpdate {
isbServiceRollout.Status.MarkPending()
} else {
isbServiceRollout.Status.MarkDeployed(isbServiceRollout.Generation)
}

// determine the Upgrade Strategy user prefers
upgradeStrategy, err := usde.GetUserStrategy(ctx, isbServiceRollout.Namespace)
// what is the preferred strategy for this namespace?
userPreferredStrategy, err := usde.GetUserStrategy(ctx, newISBServiceDef.Namespace)
if err != nil {
return false, err
}

switch upgradeStrategy {
case config.PPNDStrategyID:
// is there currently an inProgressStrategy for the isbService? (This will override any new decision)
inProgressStrategy := r.inProgressStrategyMgr.getStrategy(ctx, isbServiceRollout)
inProgressStrategySet := (inProgressStrategy != apiv1.UpgradeStrategyNoOp)

return processChildObjectWithPPND(ctx, r.client, isbServiceRollout, r, isbServiceNeedsUpdating, isbServiceIsUpdating, func() error {
// if not, should we set one?
if !inProgressStrategySet {
if userPreferredStrategy == config.PPNDStrategyID {
if upgradeStrategyType == apiv1.UpgradeStrategyPPND {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I know you copied this from my redundant code in PipelineRolloutController, so my bad, but I believe the check for userPreferredStrategy here and below may be unnecessary as it's happening within ResourceNeedsUpdating(). We probably don't need to get the value for userPreferredStrategy above at all.

inProgressStrategy = apiv1.UpgradeStrategyPPND
r.inProgressStrategyMgr.setStrategy(ctx, isbServiceRollout, inProgressStrategy)
}
}
if userPreferredStrategy == config.ProgressiveStrategyID {
if upgradeStrategyType == apiv1.UpgradeStrategyProgressive {
inProgressStrategy = apiv1.UpgradeStrategyProgressive
r.inProgressStrategyMgr.setStrategy(ctx, isbServiceRollout, inProgressStrategy)
}
}
}

switch inProgressStrategy {
case apiv1.UpgradeStrategyPPND:
done, err := processChildObjectWithPPND(ctx, r.client, isbServiceRollout, r, isbServiceNeedsToUpdate, isbServiceIsUpdating, func() error {
r.recorder.Eventf(isbServiceRollout, corev1.EventTypeNormal, "PipelinesPaused", "All Pipelines have paused for ISBService update")
err = r.updateISBService(ctx, isbServiceRollout, newISBServiceDef)
if err != nil {
Expand All @@ -312,19 +364,28 @@ func (r *ISBServiceRolloutReconciler) processExistingISBService(ctx context.Cont
r.customMetrics.ReconciliationDuration.WithLabelValues(ControllerISBSVCRollout, "update").Observe(time.Since(syncStartTime).Seconds())
return nil
})
case config.NoStrategyID:
if isbServiceNeedsUpdating {
if err != nil {
return false, err
}
if done {
r.inProgressStrategyMgr.unsetStrategy(ctx, isbServiceRollout)
} else {
// requeue if done with PPND is false
return true, nil
}
case apiv1.UpgradeStrategyNoOp:
if isbServiceNeedsToUpdate {
// update ISBService
err = r.updateISBService(ctx, isbServiceRollout, newISBServiceDef)
if err != nil {
return false, err
}
r.customMetrics.ReconciliationDuration.WithLabelValues(ControllerISBSVCRollout, "update").Observe(time.Since(syncStartTime).Seconds())
}
case config.ProgressiveStrategyID:
case apiv1.UpgradeStrategyProgressive:
return false, errors.New("Progressive Strategy not supported yet")
default:
return false, fmt.Errorf("%v strategy not recognized", upgradeStrategy)
return false, fmt.Errorf("%v strategy not recognized", inProgressStrategy)
}

return false, nil
Expand Down
Loading
Loading