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(ws): add Workspace and WorkspaceKind CRD scaffolds #6

Merged
merged 4 commits into from
Jun 19, 2024

Conversation

jiridanek
Copy link
Member

@jiridanek jiridanek commented May 15, 2024

Followup on top of my:

Compared to the previous PR, WorkspaceKind must be created as cluster-wide resource, so

kubebuilder create api --version v1beta1 --kind WorkspaceKind --namespaced=false

(it has to be --namespaced=false; --namespaced false is ignored, see kubernetes-sigs/kubebuilder#322 (comment))

Current CRD spec:

Todo's from meeting https://docs.google.com/document/d/1SiWLah-U07hAc47sSsoI8-NkbkLnHl1YVzxw193QUKE/edit#heading=h.naudedl8te2q

  • Make image .... httpPort into a list of ports (to allow multiple services per image)
  • Add a spec.spawner.deprecated: true/false
  • Add a spec.spawner.deprecatedMessage: “xxxxx”

@thesuperzapper thesuperzapper changed the title feat(nb): Create Workspace and WorkspaceKind CRD definitions feat(ws): add Workspace and WorkspaceKind CRD scaffolds May 23, 2024
Copy link
Member

@thesuperzapper thesuperzapper left a comment

Choose a reason for hiding this comment

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

@jiridanek I have made a large number of suggestions.

But the most important thing you need to do is regenerate the WorkspaceKind resource as a Cluster-Scoped resource, its currently Namespace-scoped.

workspaces/controller/api/v1beta1/workspacekind_types.go Outdated Show resolved Hide resolved
workspaces/controller/api/v1beta1/workspacekind_types.go Outdated Show resolved Hide resolved
workspaces/controller/api/v1beta1/workspacekind_types.go Outdated Show resolved Hide resolved
workspaces/controller/api/v1beta1/workspacekind_types.go Outdated Show resolved Hide resolved
workspaces/controller/api/v1beta1/workspacekind_types.go Outdated Show resolved Hide resolved
workspaces/controller/api/v1beta1/workspacekind_types.go Outdated Show resolved Hide resolved
workspaces/controller/api/v1beta1/workspacekind_types.go Outdated Show resolved Hide resolved
workspaces/controller/api/v1beta1/workspacekind_types.go Outdated Show resolved Hide resolved
workspaces/controller/api/v1beta1/workspacekind_types.go Outdated Show resolved Hide resolved
workspaces/controller/api/v1beta1/workspacekind_types.go Outdated Show resolved Hide resolved
@jiridanek jiridanek force-pushed the vtranslate-branch branch 2 times, most recently from 3ebcb5e to b019d0c Compare June 17, 2024 09:25
Co-authored-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>

Signed-off-by: Jiri Daněk <jdanek@redhat.com>
Signed-off-by: Jiri Daněk <jdanek@redhat.com>
@jiridanek jiridanek marked this pull request as ready for review June 17, 2024 09:53
Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>
Copy link
Member

@thesuperzapper thesuperzapper left a comment

Choose a reason for hiding this comment

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

@jiridanek I pushed a commit with some changes, I would appreciate you taking a look to see if I messed anything up: 4dc0c16

Also, I just want to clarify, in what situations are you using pointers (*) in your type definitions, e.g. why are we using pointers for (I am forgetting when they are needed):

type WorkspaceKindActivityProbe struct {
	// a "shell" command to run in the Workspace, if the Workspace had activity in the last 60 seconds, this command
	//  should return status 0, otherwise it should return status 1
	Exec *WorkspaceKindActivityProbeExec `json:"exec,omitempty"`

	// a Jupyter-specific probe will poll the /api/status endpoint of the Jupyter API, and use the last_activity field
	//  Users need to be careful that their other probes don't trigger a "last_activity" update,
	//	e.g. they should only check the health of Jupyter using the /api/status endpoint
	Jupyter *WorkspaceKindActivityProbeJupyter `json:"jupyter,omitempty"`
}

Comment on lines +196 to +206
// +kubebuilder:validation:XValidation:message="must specify exactly one of 'exec' or 'jupyter'",rule="!(has(self.exec) && has(self.jupyter)) && (has(self.exec) || has(self.jupyter))"
type WorkspaceKindActivityProbe struct {
// a "shell" command to run in the Workspace, if the Workspace had activity in the last 60 seconds, this command
// should return status 0, otherwise it should return status 1
Exec *WorkspaceKindActivityProbeExec `json:"exec,omitempty"`

// a Jupyter-specific probe will poll the /api/status endpoint of the Jupyter API, and use the last_activity field
// Users need to be careful that their other probes don't trigger a "last_activity" update,
// e.g. they should only check the health of Jupyter using the /api/status endpoint
Jupyter *WorkspaceKindActivityProbeJupyter `json:"jupyter,omitempty"`
}
Copy link
Member Author

@jiridanek jiridanek Jun 18, 2024

Choose a reason for hiding this comment

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

Also, I just want to clarify, in what situations are you using pointers (*) in your type definitions, e.g. why are we using pointers for (I am forgetting when they are needed):

That's for the has(self.exec). Pointers have the meaning of optionals in Go, and nil means that value is not present. I understand it might be helpful not to need it (1B $$$ mistake and so on), but I did not manage to set up the XValidation without it.

The other occurrence of pointers in the PR is the same case, also XValidation.

@jiridanek
Copy link
Member Author

@jiridanek I pushed a commit with some changes, I would appreciate you taking a look to see if I messed anything up: 4dc0c16

looks good to me, thanks!

Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>
@thesuperzapper
Copy link
Member

@jiridanek I did some small updates in bb2ca34 (mostly adding a few fields to the Workspace status that are needed for the UI), but I also added some configs to make the kubectl get workspace and kubectl get workspacekind more useful.

I am going to merge this now, so we can start the reconciliation loop PR (we can make any further changes in that PR).

/lgtm
/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thesuperzapper

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit 0ff4d03 into kubeflow:notebooks-v2 Jun 19, 2024
3 checks passed
@jiridanek jiridanek deleted the vtranslate-branch branch June 19, 2024 05:19
Adembc pushed a commit to Adembc/notebooks that referenced this pull request Jun 20, 2024
* feat(ws): add `Workspace` and `WorkspaceKind` CRD scaffolds

Co-authored-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>

Signed-off-by: Jiri Daněk <jdanek@redhat.com>

* fixup, regenerate WOrkspaceKind to be cluster scope

Signed-off-by: Jiri Daněk <jdanek@redhat.com>

* fixes to crd spec

Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>

* updates to CRD spec

Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>

---------

Signed-off-by: Jiri Daněk <jdanek@redhat.com>
Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>
Co-authored-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>
Adembc pushed a commit to Adembc/notebooks that referenced this pull request Jun 23, 2024
* feat(ws): add `Workspace` and `WorkspaceKind` CRD scaffolds

Co-authored-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>

Signed-off-by: Jiri Daněk <jdanek@redhat.com>

* fixup, regenerate WOrkspaceKind to be cluster scope

Signed-off-by: Jiri Daněk <jdanek@redhat.com>

* fixes to crd spec

Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>

* updates to CRD spec

Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>

---------

Signed-off-by: Jiri Daněk <jdanek@redhat.com>
Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>
Co-authored-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants