docs(design): improve uninstall to support configuration, package support and label removal by ayuskauskas · Pull Request #189 · NVIDIA/nodewright

ayuskauskas · 2026-04-03T23:37:18Z

No description provided.

…port and label removal

docs/designs/uninstall-enhancement.md

lockwobr · 2026-04-04T00:23:45Z

docs/designs/uninstall-enhancement.md

+
+### Failure modes
+
+If all methods fail either because they do not return anything or package fetching fails for any reason set a `nodewright.nvidia.com/UninstallCapabilityUnknown` condition on the SCR. This should also be considered as an effective `false` for `uninstall.enabled`. To avoid loops it should set the persist annotation value to `unknown`


this seems like the wrong level. this is at the package level for supporting uninstall, but this is at the scr level.

Persistence is via an annotation that is specific to an image:

nodewright/repository@digest` or `nodewright/repository@SHA`

So while it is at the custom resource instance level it can still be targeted at a specific package.

docs/designs/uninstall-enhancement.md

lockwobr · 2026-04-04T00:32:53Z

docs/designs/uninstall-enhancement.md

+### `uninstall.apply`
+
+- When `true`, the reconciler should schedule uninstall for that package **while the package remains** in `spec.packages` with full `configMap`, `env`, etc.
+- After a **successful** uninstall on all relevant nodes, the controller **clears** `uninstall.apply` (e.g. back to `false` or omit). Exact defaulting is an implementation detail; the CRD should document one behavior.


The design covers the happy path for uninstall.apply well but is thin on failure recovery:

What happens if uninstall fails on some nodes but succeeds on others? Is uninstall.apply left true indefinitely?

Is there a backoff/retry strategy? A max-retry count?

How does a user cancel a failing uninstall (set apply: false? remove the field?)?

The status conditions table lists UninstallFailed for the finalizer path, but there is no equivalent condition for the uninstall.apply path.

Consider adding a "Failure and cancellation semantics" subsection for the uninstall.apply flow.

Added a section

docs/designs/uninstall-enhancement.md

…ve for later design

rayaank-afk · 2026-04-07T11:18:50Z

Hey @ayuskauskas, can you confirm if my understanding is correct?

A. Uninstall cases

Case 1: Package supports uninstall (has uninstall.sh and uninstall_check.sh)

The package in the SCR will have uninstall.enabled: true (users with existing SCRs will need to add this manually or through GitOps).
When the user sets uninstall.apply: true (manually or through GitOps), the operator creates the uninstall pod with the full configMap and env from the spec — since the package definition is still present in the spec.
The uninstall pod runs uninstall.sh successfully, cleaning up the host-level changes.
The controller removes the package's entry from the node state annotation.
The controller removes per-package labels/annotations from the targeted nodes.
The package is marked as uninstalled.

Case 2: Package does NOT support uninstall — two sub-cases:

Case 2.1: User sets uninstall.enabled: true but no uninstall.sh exists

When user sets uninstall.apply: true, the uninstall pod is created with the full configMap and env.
The agent runs shellscript_run.sh uninstall which looks for configmaps/uninstall.sh — the file won't be found (since it was never provided in the configMap), and shellscript_run.sh exits 0 silently.
The operator treats this as a successful uninstall — labels and annotations are removed from the node[Can @ayuskauskas confirm this removal of labels/annotations in this case] .
The package is marked as uninstalled.

Note: This is a user error — they declared uninstall.enabled: true but didn't provide the uninstall scripts. The operator trusts the declaration and proceeds as if uninstall succeeded, even though nothing was actually cleaned up on the host.

Case 2.2: Package has uninstall.enabled: false (or field is missing)

If uninstall.apply: true is set, the validating webhook rejects the update because uninstall.enabled is not true.
If the package is removed from the spec entirely:
- If there is no node state for the package → webhook allows removal.
- If node state exists → webhook allows removal (since the package never claimed to support uninstall, there's no cleanup path to enforce). Labels/annotations remain on the node.

Additional notes

Metrics corresponding to an uninstalled package are zeroed out and removed from reporting.
When all packages in the SCR are uninstalled or absent from node state, the controller also removes the SCR-level labels/annotations from the nodes (e.g. skyhook.nvidia.com/status_<name>, nodeState_<name>, version_<name>, and node conditions).

B. Downgrade workflow

First, set uninstall.apply: true on the current version and let it complete — this runs uninstall with the correct configMap/env for that version.
After successful uninstallation, change the version to the lower one and set uninstall.apply: false.
If you skip step 1 and just change the version directly, the downgrade uninstall of the old version would run with the new version's configMap/env, which may have different scripts or env vars — leading to incorrect or failed cleanup.

rayaank-afk · 2026-04-07T11:21:36Z

docs/designs/uninstall-enhancement.md

+| Value | Meaning |
+|-------|--------|
+| **`false`** (default) | Package **does not** support uninstall. The operator does **not** run uninstall pods, does **not** strip package-scoped metadata as if uninstall ran, and finalizer skips uninstall for this package. |
+| **`true`** | Package **supports** uninstall. `uninstall.apply` may schedule uninstall work and post-success cleanup applies as described below. |


Why does it say "may", is there any case when uninstall.enabled is true and pod is not scheduled?

I will change it to will. The only reasons it wouldn't is if the custom resource has the pause or skip annotation.

Ok, got it.

rayaank-afk · 2026-04-07T11:22:29Z

docs/designs/uninstall-enhancement.md

+
+#### Failure modes
+
+**Uninstall fails on some nodes**: This is the same as a failing install package and pods will continue to be scheduled until complete.


Shouldn't there be limit to retries if uninstall fails?

ayuskauskas · 2026-04-07T20:41:05Z

Hey @ayuskauskas, can you confirm if my understanding is correct?

Case 2.1: User sets uninstall.enabled: true but no uninstall.sh exists

When user sets uninstall.apply: true, the uninstall pod is created with the full configMap and env.

The agent runs shellscript_run.sh uninstall which looks for configmaps/uninstall.sh — the file won't be found (since it was never provided in the configMap), and shellscript_run.sh exits 0 silently.

The operator treats this as a successful uninstall — labels and annotations are removed from the node[Can @ayuskauskas confirm this removal of labels/annotations in this case] .

The package is marked as uninstalled.

Note: This is a user error — they declared uninstall.enabled: true but didn't provide the uninstall scripts. The operator trusts the declaration and proceeds as if uninstall succeeded, even though nothing was actually cleaned up on the host.

Yes the operator will respect the decisions by the user to set the uninstall.enabled=true even if they result in nothing happening.

Case 2.2: Package has uninstall.enabled: false (or field is missing)

If uninstall.apply: true is set, the validating webhook rejects the update because uninstall.enabled is not true.

If the package is removed from the spec entirely:

If there is no node state for the package → webhook allows removal.

If node state exists → webhook allows removal (since the package never claimed to support uninstall, there's no cleanup path to enforce). Labels/annotations remain on the node.

Yes. That is the case

Additional notes

Metrics corresponding to an uninstalled package are zeroed out and removed from reporting.

When all packages in the SCR are uninstalled or absent from node state, the controller also removes the SCR-level labels/annotations from the nodes (e.g. skyhook.nvidia.com/status_<name>, nodeState_<name>, version_<name>, and node conditions).

That is correct.

B. Downgrade workflow

First, set uninstall.apply: true on the current version and let it complete — this runs uninstall with the correct configMap/env for that version.

After successful uninstallation, change the version to the lower one and set uninstall.apply: false.

If you skip step 1 and just change the version directly, the downgrade uninstall of the old version would run with the new version's configMap/env, which may have different scripts or env vars — leading to incorrect or failed cleanup.

That is correct.

rayaank-afk · 2026-04-08T04:55:22Z

So when is the implementation of this change expected? @ayuskauskas

ayuskauskas · 2026-04-08T21:41:53Z

So when is the implementation of this change expected? @ayuskauskas

@rayaank-afk it will be the next thing worked on. ETA would be 1 to 2 weeks

docs(design): improve uninstall to support configuration, package sup…

0861f00

…port and label removal

ayuskauskas requested review from lockwobr, mskalka and rice-riley as code owners April 3, 2026 23:37

lockwobr reviewed Apr 4, 2026

View reviewed changes

docs/designs/uninstall-enhancement.md Outdated Show resolved Hide resolved

lockwobr reviewed Apr 4, 2026

View reviewed changes

docs/designs/uninstall-enhancement.md Outdated Show resolved Hide resolved

lockwobr reviewed Apr 4, 2026

View reviewed changes

docs/designs/uninstall-enhancement.md Outdated Show resolved Hide resolved

ayuskauskas added 2 commits April 6, 2026 10:16

docs(design/uninstall-enhancement): update for commenets

150739e

docs(design/uninstall-enhancement): remove capability discovery to sa…

5841458

…ve for later design

ayuskauskas mentioned this pull request Apr 6, 2026

Uninstall pod does not receive package ConfigMap or Env — user-provided scripts and environment variables are silently dropped #186

Open

rayaank-afk reviewed Apr 7, 2026

View reviewed changes

lockwobr approved these changes Apr 10, 2026

View reviewed changes


		### Failure modes

		If all methods fail either because they do not return anything or package fetching fails for any reason set a `nodewright.nvidia.com/UninstallCapabilityUnknown` condition on the SCR. This should also be considered as an effective `false` for `uninstall.enabled`. To avoid loops it should set the persist annotation value to `unknown`


		#### Failure modes

		Uninstall fails on some nodes: This is the same as a failing install package and pods will continue to be scheduled until complete.

Conversation

ayuskauskas commented Apr 3, 2026

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rayaank-afk commented Apr 7, 2026

A. Uninstall cases

Additional notes

B. Downgrade workflow

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!

ayuskauskas commented Apr 7, 2026

Additional notes

B. Downgrade workflow

Uh oh!

rayaank-afk commented Apr 8, 2026

Uh oh!

ayuskauskas commented Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants