Skip to content

✨ Promote mounts to spec #3282

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

✨ Promote mounts to spec #3282

wants to merge 11 commits into from

Conversation

mjudeikis
Copy link
Contributor

@mjudeikis mjudeikis commented Feb 2, 2025 •
edited
Loading

Summary

Related issue(s)

Fixes #

Release Notes

Deprecate experimental mount annotations
Promote mounts to workspace.spec.mount

@kcp-ci-bot kcp-ci-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has signed the DCO. labels Feb 2, 2025
@kcp-ci-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kcp-ci-bot kcp-ci-bot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 2, 2025
@mjudeikis mjudeikis force-pushed the mjudeikis/mount.promote branch from 27700c4 to 2fb34bc Compare February 2, 2025 13:12
@mjudeikis
Copy link
Contributor Author

/test all

@mjudeikis mjudeikis force-pushed the mjudeikis/mount.promote branch from 2fb34bc to 8503144 Compare February 2, 2025 19:25
@kcp-ci-bot kcp-ci-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 2, 2025
@mjudeikis mjudeikis force-pushed the mjudeikis/mount.promote branch from 8503144 to d230cb9 Compare February 2, 2025 19:27
@mjudeikis
Copy link
Contributor Author

/test all

@mjudeikis mjudeikis marked this pull request as ready for review February 2, 2025 19:29
@kcp-ci-bot kcp-ci-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Feb 2, 2025
@mjudeikis
Copy link
Contributor Author

/retest

sttts
sttts reviewed Feb 4, 2025
View reviewed changes
sdk/apis/tenancy/v1alpha1/types_workspace.go Outdated
// using reference mount object.
//
// +optional
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="mount is immutable"
Copy link
Member

Choose a reason for hiding this comment

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

is this enough to stop you from removing the field? I think we need something like line 153 (on the higher level) with has.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. Somehow, it fails to start when I add a filter to namespace Do you have any idea why the omit empty optional field fails to start?

@sttts
Copy link
Member

sttts commented Feb 4, 2025

Have not read all the controller changes. Have you changed it to not have the shadow logical cluster? I.e. a mount is a mount, always. There is not this hybrid state depending on readiness.

@mjudeikis
Copy link
Contributor Author

mjudeikis commented Feb 4, 2025 •
edited
Loading

Have not read all the controller changes. Have you changed it to not have the shadow logical cluster? I.e. a mount is a mount, always. There is not this hybrid state depending on readiness.

I think I did, need to check I pushed it out, but basically goal is "shallow workspace object" with no logicalcluster backing it.

Yes, basically "mounted" workspaces don't even create LogicalCluster object

@mjudeikis mjudeikis force-pushed the mjudeikis/mount.promote branch 2 times, most recently from 0e549a3 to 846b84c Compare February 4, 2025 19:21
@kcp-ci-bot kcp-ci-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 4, 2025
@mjudeikis
Copy link
Contributor Author

/retest

mjudeikis
mjudeikis commented Apr 8, 2025
View reviewed changes
pkg/reconciler/tenancy/workspacemounts/workspacemounts_reconcile_updater.go
wsTypePath, found, err := unstructured.NestedString(obj.Object, "status", "type", "path")
if !found || err != nil {
return fmt.Errorf("unable to read .status.type.path, found %v, err: %w", found, err)
if len(wss) != 1 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we never should have more than 1 here. But need to double check.

@mjudeikis mjudeikis force-pushed the mjudeikis/mount.promote branch from 73994f7 to df4c082 Compare April 8, 2025 15:55
@mjudeikis
Copy link
Contributor Author

/test all

@mjudeikis mjudeikis force-pushed the mjudeikis/mount.promote branch 4 times, most recently from 632d1f2 to 85f32bb Compare April 8, 2025 18:16
@mjudeikis mjudeikis requested a review from Copilot April 9, 2025 07:56
Copilot
Copilot AI reviewed Apr 9, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 44 out of 44 changed files in this pull request and generated 1 comment.

@kcp-ci-bot kcp-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2025
mjudeikis and others added 10 commits April 9, 2025 18:29
@mjudeikis
Promote workspaces to spec
f66c2e1
@mjudeikis
add create admission
fcabf26
@yastij @mjudeikis
extend the mount contract to .spec.cluster and .spec.type
1d2560e
@palnabarun @mjudeikis
fix issue on deletion flow
fae732e 
Signed-off-by: Nabarun Pal <[email protected]>
@palnabarun @mjudeikis
address the admission changes for the mounting a workspace
b526c75 
Signed-off-by: Nabarun Pal <[email protected]>
@palnabarun @sanjimoh @mjudeikis
refactor the workspacemounts controller to handle both spec and statu…
4765635 
…s changes

Co-authored-by: Sanjit Mohanty <[email protected]>
Signed-off-by: Nabarun Pal <[email protected]>
@palnabarun @mjudeikis
fixup! Promote workspaces to spec
ed902e9 
Signed-off-by: Nabarun Pal <[email protected]>
@mjudeikis
update nits
7ce5371
@palnabarun @mjudeikis
make the type and cluster reflection optional
b91acd6 
Signed-off-by: Nabarun Pal <[email protected]>
@palnabarun @mjudeikis
fix lint issues
4b908e1 
Signed-off-by: Nabarun Pal <[email protected]>
@mjudeikis mjudeikis force-pushed the mjudeikis/mount.promote branch from 53b8302 to 15d8125 Compare April 9, 2025 15:31
@kcp-ci-bot kcp-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2025
@mjudeikis mjudeikis force-pushed the mjudeikis/mount.promote branch from e4de5e3 to d472ac1 Compare April 9, 2025 18:38
@mjudeikis mjudeikis force-pushed the mjudeikis/mount.promote branch from d472ac1 to 95ac113 Compare April 9, 2025 18:39
@mjudeikis
Copy link
Contributor Author

/retest

@embik
Copy link
Member

embik commented Apr 15, 2025

/retest

flake (#3371)

@mjudeikis mjudeikis mentioned this pull request Apr 15, 2025
Merged
@kcp-ci-bot kcp-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 23, 2025
@kcp-ci-bot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@yastij yastij yastij left review comments

@palnabarun palnabarun palnabarun left review comments

@embik embik embik requested changes

@dakraus dakraus dakraus left review comments

Copilot code review Copilot Copilot left review comments

Assignees

@embik embik

Labels
dco-signoff: yes Indicates the PR's author has signed the DCO. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mjudeikis @kcp-ci-bot @sttts @embik @palnabarun @yastij @dakraus