fix(operator): keep a package's own interrupt after a config change

chart/templates/nodewright-crd.yaml

@@ -847,6 +847,13 @@ spec:
                       image:
                         description: Image for the package
                         type: string
+                      interruptPending:
+                        description: |-
+                          InterruptPending is set at apply or upgrade for a package that declares its own interrupt,
+                          and cleared once the package reaches post-interrupt. While set, that interrupt is still owed on this
+                          node, so a config change that matches no configInterrupts does not let it finish without it.
+                          Entries written before this field existed read as not pending.
+                        type: boolean
                       name:
                         description: Name is the name of the package
                         type: string

chart/templates/skyhook-crd.yaml

@@ -816,6 +816,10 @@ spec:
                       image:
                         description: Image for the package
                         type: string
+                      interruptPending:
+                        description: InterruptPending mirrors the nodewright.nvidia.com
+                          field of the same name.
+                        type: boolean
                       name:
                         description: Name is the name of the package
                         type: string

docs/architecture/lifecycle.md

@@ -377,7 +377,7 @@ the spec and it converges again:
 |---|---|
 | Package `version` increases | Package re-enters at `upgrade` |
 | Package `version` decreases | With `uninstall.enabled`, rejected unless the package was explicitly uninstalled first; without it, the new version applies |
-| A `configMap` key | Nothing happens until every node is complete or one is erroring at `config`, `interrupt` or `post-interrupt`. Then every node at `config`, `interrupt`, `post-interrupt` or complete re-enters at `config`, and its unfinished Jobs are cleared; `configInterrupts` decides whether that costs an interrupt. Nodes at `uninstall`, `apply` or `upgrade`, or without the package, are not reset and pick up the new ConfigMap when they reach `config` |
+| A `configMap` key | Nothing happens until every node is complete or one is erroring at `config`, `interrupt` or `post-interrupt`. Then every node at `config`, `interrupt`, `post-interrupt` or complete re-enters at `config`, and its unfinished Jobs are cleared; the matching `configInterrupts` entries, plus the package's own `interrupt` on any node that has not yet run it, decide whether that costs an interrupt. Nodes at `uninstall`, `apply` or `upgrade`, or without the package, are not reset and pick up the new ConfigMap when they reach `config` |
 | `nodeSelectors` | Newly matching nodes are enrolled; newly excluded nodes leave scope — their host changes remain |
 | `image` or `containerSHA` only | No stage change — `version` is the ordering key |
 

docs/user-guide/custom-resource.md

@@ -506,6 +506,9 @@ not cost you a node reboot, while the key that does need one still gets it.
   literal key that does not exist, or a glob that matches nothing, is rejected by
   the webhook — this catches the typo that would otherwise silently skip an
   interrupt.
+- A config change adds to the package's own [`interrupt`](#interrupt), it does not
+  replace it: on a node where the package's own interrupt has not yet run, that
+  interrupt still runs, merged with the matching `configInterrupts` entries.
 
 For secrets, do **not** put them in `configMap`. See
 [Providing Secrets to Packages](providing-secrets.md).
@@ -644,7 +647,7 @@ To end a failing interrupt sooner, or to run it again after the verdict:
 
 A failing `uninstall-interrupt` cannot be ended by a rerun or a reset. Both remove the package's entry, an absent entry reads as already uninstalled, and so the operator abandons the uninstall interrupt rather than running it again; `package rerun --stage` cannot target `uninstall-interrupt` either. Fix the cause and delete the failed Job instead.
 
-A configMap change does not retry the interrupt either. When the package's configMap changes while it is `erroring` at `config`, `interrupt` or `post-interrupt`, the operator deletes its interrupt Job and sends the package back through `config` with the changed keys recorded. From then on the package's own `interrupt` is not consulted: only its `configInterrupts` entries matching the changed keys decide whether an interrupt runs. With no match the package completes at `config` without its interrupt ever having succeeded, and the node is uncordoned; with a match, that entry runs instead. A configMap change is not applied at all while the package is mid-uninstall.
+A configMap change does not retry the interrupt either. When the package's configMap changes while it is `erroring` at `config`, `interrupt` or `post-interrupt`, the operator deletes its interrupt Job and sends the package back through `config` with the changed keys recorded. On a node where the package's own `interrupt` has not yet run (the node's `interruptPending` flag is still set), that interrupt still runs after `config`, merged with any `configInterrupts` entry matching the changed keys; the highest-priority one wins (a `reboot` over a service restart). On a node where it already ran, only the matching `configInterrupts` entries decide whether an interrupt runs, so a change that matches none completes at `config` without one. A package whose node entry predates the `interruptPending` flag reads as already done. A configMap change is not applied at all while the package is mid-uninstall.
 
 ### `uninstall`
 

operator/api/nodewright/v1alpha1/nodewright_types.go

@@ -548,8 +548,16 @@ type NodeWrightStatus struct {
 
 type NodeState map[string]PackageStatus
 
-// Upsert adds or updates specified state for package in the node state
+// Upsert adds or updates specified state for package in the node state, carrying the entry's
+// InterruptPending flag through unchanged apart from clearing it at post-interrupt.
 func (ns *NodeState) Upsert(_package PackageRef, image string, state State, stage Stage, restarts int32, containerSHA string) bool {
+	return ns.UpsertOwing(_package, image, state, stage, restarts, containerSHA, false)
+}
+
+// UpsertOwing is Upsert for a package that declares its own interrupt when ownInterrupt is set:
+// an entry written at apply or upgrade then owes that interrupt until the package reaches
+// post-interrupt.
+func (ns *NodeState) UpsertOwing(_package PackageRef, image string, state State, stage Stage, restarts int32, containerSHA string, ownInterrupt bool) bool {
 
 	if *ns == nil {
 		*ns = make(map[string]PackageStatus)
@@ -566,6 +574,15 @@ func (ns *NodeState) Upsert(_package PackageRef, image string, state State, stag
 	}
 
 	existing, ok := (*ns)[_package.GetUniqueName()]
+
+	status.InterruptPending = ok && existing.InterruptPending
+	switch {
+	case ownInterrupt && (stage == StageApply || stage == StageUpgrade):
+		status.InterruptPending = true
+	case stage == StagePostInterrupt:
+		status.InterruptPending = false
+	}
+
 	if ok && status.Equal(&existing) {
 		return false
 	}
@@ -575,6 +592,16 @@ func (ns *NodeState) Upsert(_package PackageRef, image string, state State, stag
 	return true
 }
 
+// OwesOwnInterrupt reports whether the package declares an interrupt that has not yet run on
+// this node: the entry is missing or still pending.
+func (ns *NodeState) OwesOwnInterrupt(_package Package) bool {
+	if !_package.HasInterrupt() {
+		return false
+	}
+	status, ok := (*ns)[_package.GetUniqueName()]
+	return !ok || status.InterruptPending
+}
+
 // RemoveState removes specified package from Node State
 func (ns *NodeState) RemoveState(_package PackageRef) bool {
 	if *ns == nil {
@@ -653,17 +680,15 @@ func (ns *NodeState) IsComplete(packages Packages, interrupt map[string][]*Inter
 	return false
 }
 
-// HasInterrupt determines whether package has an interrupt based off of config updates and interrupts passed to it
+// HasInterrupt determines whether package has an interrupt based off of config updates and interrupts passed to it.
+// With config updates recorded, the interrupts matching the changed keys apply, plus the package's own
+// while this node still owes it, so a config change cannot finish a package that never ran its interrupt.
 func (ns *NodeState) HasInterrupt(_package Package, interrupt map[string][]*Interrupt, config map[string][]string) bool {
-	var hasInterrupt bool
-
 	if len(config[_package.Name]) > 0 {
-		hasInterrupt = len(interrupt[_package.Name]) > 0
-	} else {
-		hasInterrupt = _package.HasInterrupt()
+		return len(interrupt[_package.Name]) > 0 || ns.OwesOwnInterrupt(_package)
 	}
 
-	return hasInterrupt
+	return _package.HasInterrupt()
 }
 
 func (ns *NodeState) NextStage(_package *Package, interrupt map[string][]*Interrupt, config map[string][]string) *Stage {
@@ -825,17 +850,25 @@ type PackageStatus struct {
 
 	// Restarts are the number of times a package restarted
 	Restarts int32 `json:"restarts,omitempty"`
+
+	// InterruptPending is set at apply or upgrade for a package that declares its own interrupt,
+	// and cleared once the package reaches post-interrupt. While set, that interrupt is still owed on this
+	// node, so a config change that matches no configInterrupts does not let it finish without it.
+	// Entries written before this field existed read as not pending.
+	//+optional
+	InterruptPending bool `json:"interruptPending,omitempty"`
 }
 
-// Equal checks name, version, image, containerSHA, stage, state, and restarts
+// Equal checks name, version, image, containerSHA, stage, state, restarts, and interruptPending
 func (left *PackageStatus) Equal(right *PackageStatus) bool {
 	return left.Name == right.Name &&
 		left.Version == right.Version &&
 		left.Image == right.Image &&
 		left.ContainerSHA == right.ContainerSHA &&
 		left.Stage == right.Stage &&
 		left.State == right.State &&
-		left.Restarts == right.Restarts
+		left.Restarts == right.Restarts &&
+		left.InterruptPending == right.InterruptPending
 }
 
 // IsInterruptStage reports whether the package is in one of the two interrupt-phase

operator/api/nodewright/v1alpha1/nodewright_types_test.go

@@ -218,6 +218,85 @@ var _ = Describe("NodeWrightSpec BuildGraph", func() {
 
 var _ = Describe("NodeState", func() {
 
+	Describe("owed own interrupt", func() {
+		ref := PackageRef{Name: "foo", Version: "1.0.0"}
+		withReboot := Package{PackageRef: ref, Interrupt: &Interrupt{Type: REBOOT}}
+		plain := Package{PackageRef: ref}
+		unmatched := map[string][]string{"foo": {"app.conf"}}
+		noMatch := map[string][]*Interrupt{}
+
+		pending := func(ns NodeState) bool { return ns["foo|1.0.0"].InterruptPending }
+
+		It("sets it on apply and upgrade, carries it through config and interrupt, clears it at post-interrupt", func() {
+			ns := NodeState{}
+			ns.UpsertOwing(ref, "img", StateInProgress, StageApply, 0, "", true)
+			Expect(pending(ns)).To(BeTrue())
+			ns.Upsert(ref, "img", StateComplete, StageConfig, 0, "")
+			Expect(pending(ns)).To(BeTrue())
+			ns.Upsert(ref, "img", StateInProgress, StageInterrupt, 0, "")
+			Expect(pending(ns)).To(BeTrue())
+			ns.Upsert(ref, "img", StateInProgress, StagePostInterrupt, 0, "")
+			Expect(pending(ns)).To(BeFalse())
+
+			// a config reset keeps it cleared
+			ns.Upsert(ref, "img", StateInProgress, StageConfig, 0, "")
+			Expect(pending(ns)).To(BeFalse())
+
+			// a new apply or upgrade owes it again
+			ns.UpsertOwing(ref, "img", StateInProgress, StageUpgrade, 0, "", true)
+			Expect(pending(ns)).To(BeTrue())
+		})
+
+		It("leaves a package without its own interrupt unflagged and keeps a carried flag on plain upserts", func() {
+			ns := NodeState{}
+			ns.UpsertOwing(ref, "img", StateInProgress, StageApply, 0, "", false)
+			Expect(pending(ns)).To(BeFalse())
+
+			ns = NodeState{}
+			ns.UpsertOwing(ref, "img", StateInProgress, StageApply, 0, "", true)
+			ns.Upsert(ref, "img", StateComplete, StageApply, 0, "")
+			Expect(pending(ns)).To(BeTrue())
+		})
+
+		It("reports a change when only the flag differs", func() {
+			ns := NodeState{"foo|1.0.0": PackageStatus{Name: "foo", Version: "1.0.0", Image: "img", Stage: StageApply, State: StateInProgress}}
+			Expect(ns.UpsertOwing(ref, "img", StateInProgress, StageApply, 0, "", true)).To(BeTrue())
+			Expect(ns.UpsertOwing(ref, "img", StateInProgress, StageApply, 0, "", true)).To(BeFalse())
+		})
+
+		It("owes it for a missing or pending entry and not for a settled one", func() {
+			Expect((&NodeState{}).OwesOwnInterrupt(withReboot)).To(BeTrue())
+			Expect((&NodeState{"foo|1.0.0": {InterruptPending: true}}).OwesOwnInterrupt(withReboot)).To(BeTrue())
+			Expect((&NodeState{"foo|1.0.0": {}}).OwesOwnInterrupt(withReboot)).To(BeFalse())
+			Expect((&NodeState{"foo|1.0.0": {InterruptPending: true}}).OwesOwnInterrupt(plain)).To(BeFalse())
+		})
+
+		It("keeps the own interrupt when a config change matches no configInterrupts", func() {
+			ns := NodeState{"foo|1.0.0": {Name: "foo", Version: "1.0.0", Stage: StageConfig, State: StateComplete, InterruptPending: true}}
+			Expect(ns.HasInterrupt(withReboot, noMatch, unmatched)).To(BeTrue())
+			Expect(ns.IsPackageComplete(withReboot, noMatch, unmatched)).To(BeFalse())
+			Expect(*ns.NextStage(&withReboot, noMatch, unmatched)).To(Equal(StageInterrupt))
+		})
+
+		It("finishes at config once the own interrupt already ran", func() {
+			ns := NodeState{"foo|1.0.0": {Name: "foo", Version: "1.0.0", Stage: StageConfig, State: StateComplete}}
+			Expect(ns.HasInterrupt(withReboot, noMatch, unmatched)).To(BeFalse())
+			Expect(ns.IsPackageComplete(withReboot, noMatch, unmatched)).To(BeTrue())
+		})
+
+		It("still runs a matching configInterrupt after the own interrupt ran", func() {
+			ns := NodeState{"foo|1.0.0": {Name: "foo", Version: "1.0.0", Stage: StageConfig, State: StateComplete}}
+			matched := map[string][]*Interrupt{"foo": {{Type: SERVICE}}}
+			Expect(ns.HasInterrupt(withReboot, matched, unmatched)).To(BeTrue())
+		})
+
+		It("ignores the flag when there are no config updates", func() {
+			ns := NodeState{"foo|1.0.0": {Name: "foo", Version: "1.0.0", Stage: StageConfig, State: StateComplete}}
+			Expect(ns.HasInterrupt(withReboot, noMatch, nil)).To(BeTrue())
+			Expect(ns.HasInterrupt(plain, noMatch, nil)).To(BeFalse())
+		})
+	})
+
 	Describe("RemoveState", func() {
 		It("should report no change when the state map is nil", func() {
 			var ns NodeState

operator/api/v1alpha1/nodewright_conversion.go

@@ -363,6 +363,8 @@ func convertPackageStatus(in PackageStatus) nwv1.PackageStatus {
 		Stage:        nwv1.Stage(in.Stage),
 		State:        nwv1.State(in.State),
 		Restarts:     in.Restarts,
+
+		InterruptPending: in.InterruptPending,
 	}
 }
 

operator/api/v1alpha1/nodewright_conversion_test.go

@@ -129,13 +129,14 @@ func fullSkyhook() *Skyhook {
 			NodeState: map[string]NodeState{
 				"node-1": {
 					"tuning|1.2.3": PackageStatus{
-						Name:         "tuning",
-						Version:      "1.2.3",
-						Image:        "alpine",
-						ContainerSHA: "sha256:deadbeef",
-						Stage:        StageConfig,
-						State:        StateComplete,
-						Restarts:     2,
+						Name:             "tuning",
+						Version:          "1.2.3",
+						Image:            "alpine",
+						ContainerSHA:     "sha256:deadbeef",
+						Stage:            StageConfig,
+						State:            StateComplete,
+						Restarts:         2,
+						InterruptPending: true,
 					},
 				},
 			},

operator/api/v1alpha1/skyhook_types.go

@@ -792,6 +792,10 @@ type PackageStatus struct {
 
 	// Restarts are the number of times a package restarted
 	Restarts int32 `json:"restarts,omitempty"`
+
+	// InterruptPending mirrors the nodewright.nvidia.com field of the same name.
+	//+optional
+	InterruptPending bool `json:"interruptPending,omitempty"`
 }
 
 // Equal checks name, version, image, containerSHA, stage, state, and restarts

operator/cmd/cli/app/package/package_rerun.go

@@ -309,6 +309,9 @@ func updateNodeAnnotations(
 			pkgStatus := nodeState[packageKey]
 			pkgStatus.Stage = v1alpha1.Stage(opts.stage)
 			pkgStatus.State = v1alpha1.StateInProgress
+			if pkgStatus.Stage == v1alpha1.StageApply || pkgStatus.Stage == v1alpha1.StageUpgrade {
+				pkgStatus.InterruptPending = true // a fresh apply owes the package's own interrupt again
+			}
 			nodeState[packageKey] = pkgStatus
 		} else {
 			delete(nodeState, packageKey)

operator/cmd/cli/app/update_state.go

@@ -196,6 +196,8 @@ func buildUpdateStateTarget(
 			Image:   pkg.Image,
 			Stage:   stage,
 			State:   state,
+
+			InterruptPending: stage == v1alpha1.StageApply || stage == v1alpha1.StageUpgrade,
 		}
 	} else {
 		if !hasEntry {
@@ -209,6 +211,12 @@ func buildUpdateStateTarget(
 		}
 		entry.Stage = stage
 		entry.State = state
+		switch stage {
+		case v1alpha1.StageApply, v1alpha1.StageUpgrade:
+			entry.InterruptPending = true // a fresh apply owes the package's own interrupt again
+		case v1alpha1.StagePostInterrupt:
+			entry.InterruptPending = false
+		}
 		ns[uniqueName] = entry
 	}
 

operator/config/crd/bases/nodewright.nvidia.com_nodewrights.yaml

@@ -852,6 +852,13 @@ spec:
                       image:
                         description: Image for the package
                         type: string
+                      interruptPending:
+                        description: |-
+                          InterruptPending is set at apply or upgrade for a package that declares its own interrupt,
+                          and cleared once the package reaches post-interrupt. While set, that interrupt is still owed on this
+                          node, so a config change that matches no configInterrupts does not let it finish without it.
+                          Entries written before this field existed read as not pending.
+                        type: boolean
                       name:
                         description: Name is the name of the package
                         type: string

operator/config/crd/bases/skyhook.nvidia.com_skyhooks.yaml

@@ -818,6 +818,10 @@ spec:
                       image:
                         description: Image for the package
                         type: string
+                      interruptPending:
+                        description: InterruptPending mirrors the nodewright.nvidia.com
+                          field of the same name.
+                        type: boolean
                       name:
                         description: Name is the name of the package
                         type: string

operator/internal/controller/skyhook_controller.go

@@ -1658,7 +1658,7 @@ func (r *SkyhookReconciler) RunSkyhookPackages(ctx context.Context, clusterState
 			return nil, fmt.Errorf("node %s: selecting packages to run: %w", node.GetNode().Name, err)
 		}
 
-		interrupt, pack := fudgeInterruptWithPriority(toRun, skyhook.GetSkyhook().GetConfigUpdates(), skyhook.GetSkyhook().GetConfigInterrupts())
+		interrupt, pack := fudgeInterruptWithPriority(toRun, skyhook.GetSkyhook().GetConfigUpdates(), skyhook.GetSkyhook().GetConfigInterrupts(), node.OwesOwnInterrupt)
 
 		for _, f := range toRun {
 
@@ -3115,7 +3115,7 @@ func (r *SkyhookReconciler) Interrupt(ctx context.Context, skyhookNode wrapper.S
 }
 
 // fudgeInterruptWithPriority takes a list of packages, interrupts, and configUpdates and returns the correct merged interrupt to run to handle all the packages
-func fudgeInterruptWithPriority(next []*v1alpha1.Package, configUpdates map[string][]string, interrupts map[string][]*v1alpha1.Interrupt) (*v1alpha1.Interrupt, string) {
+func fudgeInterruptWithPriority(next []*v1alpha1.Package, configUpdates map[string][]string, interrupts map[string][]*v1alpha1.Interrupt, owesOwn func(*v1alpha1.Package) bool) (*v1alpha1.Interrupt, string) {
 	var ret *v1alpha1.Interrupt
 	var pack string
 
@@ -3135,6 +3135,10 @@ func fudgeInterruptWithPriority(next []*v1alpha1.Package, configUpdates map[stri
 			if _package.HasInterrupt() {
 				interrupts[_package.Name] = append(interrupts[_package.Name], _package.Interrupt)
 			}
+		} else if owesOwn != nil && owesOwn(_package) {
+			// a config change only brings the interrupts its keys match; a package whose own
+			// interrupt never ran on this node still owes it
+			interrupts[_package.Name] = append(interrupts[_package.Name], _package.Interrupt)
 		}
 	}