-
Notifications
You must be signed in to change notification settings - Fork 60
fix: use K8s resource name for backup and restore after workspace rename #1565
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
Open
akurinnoy
wants to merge
3
commits into
main
Choose a base branch
from
CRW-10477
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
127 changes: 127 additions & 0 deletions
127
packages/dashboard-frontend/adr/adr-001-auto-generate-resource-name-on-restore.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| # ADR-001: Auto-generate K8s resource name when restoring from backup | ||
|
|
||
| - **Status:** Accepted | ||
| - **Date:** 2026-05-13 | ||
| - **PR:** [#1565](https://github.com/eclipse-che/che-dashboard/pull/1565) | ||
|
|
||
| ## Context | ||
|
|
||
| Eclipse Che workspaces have two names: | ||
|
|
||
| - **`metadata.name`** — the immutable Kubernetes resource identifier, set at creation time (e.g., `empty-1234`). Never shown to users in the dashboard UI. | ||
| - **Display name** — stored in the `kubernetes.io/metadata.name` label, changeable by the user (e.g., `not-empty-1234`). Shown everywhere in the UI: workspace list, sidebar, breadcrumbs, details page. | ||
|
|
||
| The restore-from-backup flow currently sets `metadata.name` directly from the user's text input. This creates a UX problem after workspace rename: | ||
|
|
||
| 1. User creates workspace `empty-1234` (this becomes `metadata.name`) | ||
| 2. User renames it to `not-empty-1234` (sets the display name label; `metadata.name` stays `empty-1234`) | ||
| 3. User sees `not-empty-1234` everywhere — the name `empty-1234` is invisible | ||
| 4. DWO backs up the workspace under `empty-1234` (uses `metadata.name`) | ||
| 5. User tries to restore a backup with name `empty-1234` | ||
| 6. Conflict error: "A workspace with this name already exists" — but the user cannot see any workspace named `empty-1234` | ||
|
|
||
| This looks like a bug to the user. | ||
|
|
||
| ## Options considered | ||
|
|
||
| ### 1. Improve the error message | ||
|
|
||
| Show the display name alongside the resource name in the conflict error: *"A workspace named 'not-empty-1234' (resource: empty-1234) already exists."* | ||
|
|
||
| - **Pros:** Minimal change, transparent, shippable immediately | ||
| - **Cons:** Leaks K8s internals (`metadata.name`) to users who have no mental model for it. Every other UI surface hides this identifier — the restore form would be the only place exposing it. Fixes the confusion but preserves the broken interaction model. | ||
|
|
||
| ### 2. Validate display names only, handle K8s 409 at API level | ||
|
|
||
| The frontend checks only display names for conflicts. If the K8s API rejects the create with a 409 (resource name collision), surface an error after submission. | ||
|
|
||
| - **Pros:** UX is intuitive from the user's perspective — if they can't see the name, no conflict is shown | ||
| - **Cons:** Error surfaces late (after clicking Restore), not in the form. The K8s 409 error message is cryptic and hard to translate into a user-friendly message. Split validation model (frontend checks one thing, backend checks another) is fragile. | ||
|
|
||
| ### 3. Auto-generate `metadata.name` at restore time | ||
|
|
||
| The user types a display name. The system generates a unique `metadata.name` using `generateWorkspaceName()` (appends a random 4-character suffix), and stores the user-typed name as the display name label. Conflict validation checks display names instead of resource names. | ||
|
|
||
| - **Pros:** Eliminates K8s resource name conflicts entirely. Consistent with the factory workspace creation flow, which already uses `generateWorkspaceName()`. Users never encounter errors referencing invisible identifiers. Proven pattern in production. | ||
| - **Cons:** `metadata.name` of a restored workspace differs from the original backup's workspace name (e.g., `my-ws-x7q2` instead of `my-ws`). Users who `kubectl get devworkspaces` see the suffixed name. Display name collisions are possible (two workspaces with the same display name but different resource names). | ||
|
|
||
| ### 4. Try exact name, append suffix on conflict | ||
|
|
||
| Use the user-typed name as `metadata.name`. If K8s rejects with a 409, retry with an auto-generated suffix. | ||
|
|
||
| - **Pros:** Clean names when no conflict exists | ||
| - **Cons:** Inconsistent behavior — sometimes the user gets what they typed, sometimes they don't. Requires two API calls in the conflict case. Factory creation always generates a suffix; restore should be consistent. | ||
|
|
||
| ### 5. Fix at DWO level | ||
|
|
||
| Make workspace rename update `metadata.name` by deleting and recreating the DevWorkspace resource with the new name. | ||
|
|
||
| - **Pros:** Eliminates the dual-name problem at the source | ||
| - **Cons:** Requires DWO changes (different repository, different release cycle). Recreating a DevWorkspace loses UID, events, and status history. Complex and risky. | ||
|
|
||
| ## Decision | ||
|
|
||
| **Option 3: Auto-generate `metadata.name` at restore time.** | ||
|
|
||
| ### Motivation | ||
|
|
||
| The restore flow creates a new DevWorkspace resource — it does not patch an existing one. The sequence is: | ||
|
|
||
| 1. Build a restore devfile with `RESTORE_WORKSPACE=true` and `RESTORE_SOURCE_IMAGE` attributes | ||
| 2. Call `fetchResources()` to generate DevWorkspace + DevWorkspaceTemplate resources | ||
| 3. `DwtApi.createTemplate()` — creates new DevWorkspaceTemplate | ||
| 4. `DwApi.createWorkspace()` — creates new DevWorkspace | ||
| 5. Set ownerReference linking the two | ||
|
|
||
| This is the same create-resource pattern that factory workspace creation uses. Factory creation already calls `generateWorkspaceName()` to produce suffixed names like `my-project-a1b2`. Applying the same pattern to restore is architecturally consistent. | ||
|
|
||
| Every "negative" consequence of this approach (suffixed `metadata.name`, display name collisions, kubectl showing a different name) already exists in factory workspace creation and is accepted as a trade-off. Restore should not behave differently from creation. | ||
|
|
||
| The other options either expose K8s internals to users (option 1), push errors to submission time (option 2), behave inconsistently (option 4), or require changes outside the dashboard (option 5). | ||
|
|
||
| ### What changes | ||
|
|
||
| In `restoreWorkspaceFromBackup.ts`, instead of: | ||
|
|
||
| ```typescript | ||
| devWorkspaceResource.metadata.name = workspaceName; | ||
| ``` | ||
|
|
||
| Do: | ||
|
|
||
| ```typescript | ||
| devWorkspaceResource.metadata.name = generateWorkspaceName(workspaceName); | ||
| if (!devWorkspaceResource.metadata.labels) { | ||
| devWorkspaceResource.metadata.labels = {}; | ||
| } | ||
| devWorkspaceResource.metadata.labels[DEVWORKSPACE_LABEL_METADATA_NAME] = workspaceName; | ||
| ``` | ||
|
|
||
| The conflict validation in `RestoreFromBackup/index.tsx` shifts from checking K8s resource names to checking display names: | ||
|
|
||
| ```typescript | ||
| // Before: checked metadata.name (invisible to user) | ||
| const existingWorkspaceNames = new Set(allWorkspaces.map(ws => ws.resourceName)); | ||
|
|
||
| // After: check display names (what the user sees) | ||
| const existingWorkspaceNames = new Set(allWorkspaces.map(ws => ws.name)); | ||
| ``` | ||
|
|
||
| ## Consequences | ||
|
|
||
| ### Positive | ||
|
|
||
| - K8s resource name conflicts become structurally impossible during restore | ||
| - Restore behavior is consistent with factory workspace creation | ||
| - Users never encounter errors referencing invisible identifiers | ||
| - The conflict check validates what the user actually sees | ||
|
|
||
| ### Negative | ||
|
|
||
| - `metadata.name` of a restored workspace will differ from the original backup's workspace name. This is the same trade-off factory creation makes. | ||
| - Users who `kubectl get devworkspaces` will see the suffixed name, not the display name. This is also consistent with factory-created workspaces. | ||
| - Display name collisions are possible (two workspaces with the same display name but different resource names). This is already possible with factory-created workspaces and is not a new risk. | ||
|
|
||
| ### Migration | ||
|
|
||
| No migration needed. This change only affects new restore operations. Existing workspaces are unaffected. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To double check, was this doc meant to be committed to the repo? (I mean, should this file be merged to the main branch with this PR?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I committed it intentionally. The restore flow change is a non-trivial decision with trade-offs, so I'd like to keep ADRs for such changes in the repo, so the reasoning is next to the code itself. I believe this will help both humans and AI agents providing better code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed the same matter with @rohanKanojia here: devfile/devworkspace-operator#1631 (review)
We agreed on having ADRs under the
adr/directory, in case of the dashboard - in the root of the package.@olexii4 @svor Are you OK with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akurinnoy do I understand correctly that ADR files should be maintained and updated whenever the logic changes?
Also, I’m not sure whether we should reference the downstream issue here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akurinnoy Placing ADRs under
packages/dashboard-frontend/adr/is OK for me.However, there is an important concern about treating ADRs as permanent, append-only artifacts.
ADR files like
adr-001-auto-generate-resource-name-on-restore.mddescribe a decision that was correct at a point in time. When the underlying logic changes, the ADR can become stale or misleading. Unlike code, outdated ADRs don't produce compiler errors — they silently rot.The risk: Without explicit rules for updating or retiring ADRs, the
adr/directory will accumulate documents that no longer reflect reality, which hurts rather than helps future contributors and AI agents.What's needed before committing to this pattern:
Without these rules, ADRs become archaeology rather than documentation.
Proposed First Step: Add a
/docsDirectory with a Project SnapshotBefore expanding the ADR convention further, a more immediately valuable step is to establish a top-level
docs/directory (oropenspec/docs/) containing a snapshot description of the project — what it does, its key architectural boundaries, package structure, and major design decisions in effect today.Why this matters:
The snapshot should be treated as a living document: updated when significant decisions change, not as a historical record. This is different from ADRs, which record the history of a decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@svor thanks for the feedback
The standard ADR practice is append-only: old ADRs are not edited, instead you write a new one that supersedes the previous (with
Status: Superseded by ADR-XXX). If the feature is removed entirely, you mark itStatus: Deprecated. The idea is that ADRs are a decision log, not a living document. Even a deprecated ADR still has value - it explains why the approach was tried and why it was abandoned, so future contributors don't repeat the same mistakes.Good point. I'll replace the Jira link with a reference to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olexii4 thanks for the thorough feedback
Regarding the ADR lifecycle - the standard ADR convention already covers this: ADRs are append-only, you don't edit old ones. If the logic changes, you write a new ADR that supersedes the previous one (
Status: Superseded by ADR-XXX). If a feature is removed, you mark itStatus: Deprecated. I can add a short README.md in theadr/directory explaining this convention.On the "silently rot" concern - ADRs don't describe current state, they describe why a decision was made at that point in time. A superseded or deprecated ADR is not stale, it's still doing its job: it explains why the approach was tried and why it was later changed or abandoned. This prevents future contributors from repeating the same mistakes.
Regarding the project snapshot in
docs/- interesting idea, but I thinkAGENTS.mdin the repo root already serves this purpose. Happy to discuss expanding it, but that's a separate effort and shouldn't block shipping ADRs.