Skip to content

feat(ws): Implement pause workspace functionality as backend API #328

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 27 additions & 19 deletions workspaces/backend/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,29 +27,30 @@ make run
If you want to use a different port:

```shell
make run PORT=8000
make run PORT=8000
```

### Endpoints

| URL Pattern | Handler | Action |
|----------------------------------------------|------------------------|-----------------------------------------|
| GET /api/v1/healthcheck | healthcheck_handler | Show application information |
| GET /api/v1/namespaces | namespaces_handler | Get all Namespaces |
| GET /api/v1/swagger/ | swagger_handler | Swagger API documentation |
| GET /api/v1/workspaces | workspaces_handler | Get all Workspaces |
| GET /api/v1/workspaces/{namespace} | workspaces_handler | Get all Workspaces from a namespace |
| POST /api/v1/workspaces/{namespace} | workspaces_handler | Create a Workspace in a given namespace |
| GET /api/v1/workspaces/{namespace}/{name} | workspaces_handler | Get a Workspace entity |
| PATCH /api/v1/workspaces/{namespace}/{name} | TBD | Patch a Workspace entity |
| PUT /api/v1/workspaces/{namespace}/{name} | TBD | Update a Workspace entity |
| DELETE /api/v1/workspaces/{namespace}/{name} | workspaces_handler | Delete a Workspace entity |
| GET /api/v1/workspacekinds | workspacekinds_handler | Get all WorkspaceKind |
| POST /api/v1/workspacekinds | TBD | Create a WorkspaceKind |
| GET /api/v1/workspacekinds/{name} | workspacekinds_handler | Get a WorkspaceKind entity |
| PATCH /api/v1/workspacekinds/{name} | TBD | Patch a WorkspaceKind entity |
| PUT /api/v1/workspacekinds/{name} | TBD | Update a WorkspaceKind entity |
| DELETE /api/v1/workspacekinds/{name} | TBD | Delete a WorkspaceKind entity |
| URL Pattern | Handler | Action |
|-----------------------------------------------------------|---------------------------|-----------------------------------------|
| GET /api/v1/healthcheck | healthcheck_handler | Show application information |
| GET /api/v1/namespaces | namespaces_handler | Get all Namespaces |
| GET /api/v1/swagger/ | swagger_handler | Swagger API documentation |
| GET /api/v1/workspaces | workspaces_handler | Get all Workspaces |
| GET /api/v1/workspaces/{namespace} | workspaces_handler | Get all Workspaces from a namespace |
| POST /api/v1/workspaces/{namespace} | workspaces_handler | Create a Workspace in a given namespace |
| GET /api/v1/workspaces/{namespace}/{name} | workspaces_handler | Get a Workspace entity |
| PATCH /api/v1/workspaces/{namespace}/{name} | TBD | Patch a Workspace entity |
| PUT /api/v1/workspaces/{namespace}/{name} | TBD | Update a Workspace entity |
| DELETE /api/v1/workspaces/{namespace}/{name} | workspaces_handler | Delete a Workspace entity |
| POST /api/v1/workspaces/{namespace}/{name}/actions/pause | workspace_actions_handler | Pause a running workspace |

Choose a reason for hiding this comment

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

nitpick: Seems like the table format is missing here.
can be neglected for next time

| GET /api/v1/workspacekinds | workspacekinds_handler | Get all WorkspaceKind |
| POST /api/v1/workspacekinds | TBD | Create a WorkspaceKind |
| GET /api/v1/workspacekinds/{name} | workspacekinds_handler | Get a WorkspaceKind entity |
| PATCH /api/v1/workspacekinds/{name} | TBD | Patch a WorkspaceKind entity |
| PUT /api/v1/workspacekinds/{name} | TBD | Update a WorkspaceKind entity |
| DELETE /api/v1/workspacekinds/{name} | TBD | Delete a WorkspaceKind entity |

### Sample local calls

Expand Down Expand Up @@ -128,6 +129,13 @@ Get a Workspace:
curl -i localhost:4000/api/v1/workspaces/default/dora
```

Pause a Workspace:

```shell
# POST /api/v1/workspaces/{namespace}/{name}/actions/pause
curl -X POST localhost:4000/api/v1/workspaces/default/dora/actions/pause
```

Delete a Workspace:

```shell
Expand Down
3 changes: 3 additions & 0 deletions workspaces/backend/api/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ const (
AllWorkspacesPath = PathPrefix + "/workspaces"
WorkspacesByNamespacePath = AllWorkspacesPath + "/:" + NamespacePathParam
WorkspacesByNamePath = AllWorkspacesPath + "/:" + NamespacePathParam + "/:" + ResourceNamePathParam
WorkspaceActionsPath = WorkspacesByNamePath + "/actions"
PauseWorkspacePath = WorkspaceActionsPath + "/pause"

// workspacekinds
AllWorkspaceKindsPath = PathPrefix + "/workspacekinds"
Expand Down Expand Up @@ -102,6 +104,7 @@ func (a *App) Routes() http.Handler {
router.GET(WorkspacesByNamePath, a.GetWorkspaceHandler)
router.POST(WorkspacesByNamespacePath, a.CreateWorkspaceHandler)
router.DELETE(WorkspacesByNamePath, a.DeleteWorkspaceHandler)
router.POST(PauseWorkspacePath, a.PauseWorkspaceHandler)

// workspacekinds
router.GET(AllWorkspaceKindsPath, a.GetWorkspaceKindsHandler)
Expand Down
3 changes: 3 additions & 0 deletions workspaces/backend/api/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ type Envelope[D any] struct {
Data D `json:"data"`
}

// EmptyResponse represents an empty JSON response
type EmptyResponse struct{}

// WriteJSON writes a JSON response with the given status code, data, and headers.
func (a *App) WriteJSON(w http.ResponseWriter, status int, data any, headers http.Header) error {

Expand Down
96 changes: 96 additions & 0 deletions workspaces/backend/api/workspace_actions_handler.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/*
Copyright 2024.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package api

import (
"errors"
"net/http"

"github.com/julienschmidt/httprouter"
kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/validation/field"

"github.com/kubeflow/notebooks/workspaces/backend/internal/auth"
"github.com/kubeflow/notebooks/workspaces/backend/internal/helper"
repository "github.com/kubeflow/notebooks/workspaces/backend/internal/repositories/workspaces"
)

// PauseWorkspaceHandler handles the pause workspace action
//
// @Summary Pause workspace
// @Description Pauses a workspace, stopping all associated pods.
// @Tags workspaces
// @Accept json
// @Produce json
// @Param namespace path string true "Namespace of the workspace" example(default)
// @Param workspaceName path string true "Name of the workspace" example(my-workspace)
// @Success 200 {object} EmptyResponse "Successful action. Returns an empty JSON object."
// @Failure 400 {object} ErrorEnvelope "Bad Request. Invalid workspace kind name format."
// @Failure 401 {object} ErrorEnvelope "Unauthorized. Authentication is required."
// @Failure 403 {object} ErrorEnvelope "Forbidden. User does not have permission to access the workspace."
// @Failure 404 {object} ErrorEnvelope "Not Found. Workspace does not exist."
// @Failure 500 {object} ErrorEnvelope "Internal server error. An unexpected error occurred on the server."
// @Router /workspaces/{namespace}/{workspaceName}/actions/pause [post]
// @Security ApiKeyAuth
func (a *App) PauseWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
namespace := ps.ByName(NamespacePathParam)
workspaceName := ps.ByName(ResourceNamePathParam)

// validate path parameters
var valErrs field.ErrorList
valErrs = append(valErrs, helper.ValidateFieldIsDNS1123Subdomain(field.NewPath(NamespacePathParam), namespace)...)
valErrs = append(valErrs, helper.ValidateFieldIsDNS1123Subdomain(field.NewPath(ResourceNamePathParam), workspaceName)...)
if len(valErrs) > 0 {
a.failedValidationResponse(w, r, errMsgPathParamsInvalid, valErrs, nil)
return
}

// =========================== AUTH ===========================
authPolicies := []*auth.ResourcePolicy{
auth.NewResourcePolicy(
auth.ResourceVerbUpdate,
&kubefloworgv1beta1.Workspace{
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
Name: workspaceName,
},
},
),
}
if success := a.requireAuth(w, r, authPolicies); !success {
return
}
// ============================================================

err := a.repositories.Workspace.PauseWorkspace(r.Context(), namespace, workspaceName)
if err != nil {
if errors.Is(err, repository.ErrWorkspaceNotFound) {
a.notFoundResponse(w, r)
return
}
a.serverErrorResponse(w, r, err)
return
}

// Return 200 OK with empty JSON object

Choose a reason for hiding this comment

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

instead of returning a empty, shall we return the workspace object , so frontend can use this information ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very valid feedback and happy to implement as such if folks agree that is preferred.

my rationale for structuring this as such (with an "empty object") was:

  • this endpoint is really focused and designed to only set the spec.paused attribute - and as such - there is nothing to be returned that the client wouldn't already know if they cared to (or already has rendered in the case of our frontend)... returning the entirety of the workspace implies the client should update/merge that with this existing state - and I didn't feel it necessary/warranted (purely my $0.02!)
    • if client receives 200 response - they can infer the value of spec.paused to be updated in their state without having to parse a payload
  • as this endpoint lives under this /actions "umbrella" - I didn't necessarily want to enforce the Envelope structure when other endpoints that live under /actions (like say, logs or something?) would themselves have totally different structures - such that trying to be consistent within /actions is probably impossible moving forward

Given those 2 points above - I leaned towards {}

But again, it was something I myself wondered during implementation - and willing to change if we get another +1 that the Envelope response would be preferred!

Choose a reason for hiding this comment

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

ack, thanks for the explanation behind the return type,
if frontend has the state, and can set the paused based on information, we should be good.

i guess, i need to fimilarize myself with frontend a little bit, how the state of the paused/start are being displayed.

Copy link
Member

Choose a reason for hiding this comment

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

Andy, for consistency and forward compatibility, returning an envelope object is preferred even for action endpoints like pause.

It will make it much easier on FE if the backend always returns an envelope (plus the right status code) and never an empty response.

This will help, for instance, in case of an error, and avoid a bunch of if and else statements on the FE.
The structure should be like this:

{
"data": {
"status": "paused",
"message": "Workspace successfully paused."
}
}

type PauseResponse struct {
Status string json:"status" // e.g., "paused"
Message string json:"message" // optional
}

If we standardize on envelope makes us easy to evolve etc.

err = a.WriteJSON(w, http.StatusOK, EmptyResponse{}, nil)
if err != nil {
a.serverErrorResponse(w, r, err)
return
}
}
166 changes: 166 additions & 0 deletions workspaces/backend/api/workspace_actions_handler_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
/*
Copyright 2024.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package api

import (
"fmt"
"io"
"net/http"
"net/http/httptest"
"strings"

"github.com/julienschmidt/httprouter"
kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/utils/ptr"
)

var _ = Describe("Workspace Actions Handler", func() {

// NOTE: the tests in this context work on the same resources, they must be run in order.
// also, they assume a specific state of the cluster, so cannot be run in parallel with other tests.
// therefore, we run them using the `Ordered` and `Serial` Ginkgo decorators.
Context("with existing Workspaces", Serial, Ordered, func() {

const namespaceName1 = "ws-ops-ns1"

var (
workspaceName1 string
workspaceKey1 types.NamespacedName
workspaceKindName string
)

BeforeAll(func() {
uniqueName := "ws-ops-test"
workspaceName1 = fmt.Sprintf("workspace-1-%s", uniqueName)
workspaceKey1 = types.NamespacedName{Name: workspaceName1, Namespace: namespaceName1}
workspaceKindName = fmt.Sprintf("workspacekind-%s", uniqueName)

By("creating Namespace 1")
namespace1 := &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: namespaceName1,
},
}
Expect(k8sClient.Create(ctx, namespace1)).To(Succeed())

By("creating a WorkspaceKind")
workspaceKind := NewExampleWorkspaceKind(workspaceKindName)
Expect(k8sClient.Create(ctx, workspaceKind)).To(Succeed())

By("creating Workspace 1 in Namespace 1")
workspace1 := NewExampleWorkspace(workspaceName1, namespaceName1, workspaceKindName)
Expect(k8sClient.Create(ctx, workspace1)).To(Succeed())
})

AfterAll(func() {
By("deleting Workspace 1 from Namespace 1")
workspace1 := &kubefloworgv1beta1.Workspace{
ObjectMeta: metav1.ObjectMeta{
Name: workspaceName1,
Namespace: namespaceName1,
},
}
Expect(k8sClient.Delete(ctx, workspace1)).To(Succeed())

By("deleting WorkspaceKind")
workspaceKind := &kubefloworgv1beta1.WorkspaceKind{
ObjectMeta: metav1.ObjectMeta{
Name: workspaceKindName,
},
}
Expect(k8sClient.Delete(ctx, workspaceKind)).To(Succeed())

By("deleting Namespace 1")
namespace1 := &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: namespaceName1,
},
}
Expect(k8sClient.Delete(ctx, namespace1)).To(Succeed())
})

It("should pause a workspace successfully", func() {
By("creating the HTTP request")
path := strings.Replace(PauseWorkspacePath, ":"+NamespacePathParam, namespaceName1, 1)
path = strings.Replace(path, ":"+ResourceNamePathParam, workspaceName1, 1)
req, err := http.NewRequest(http.MethodPost, path, http.NoBody)
Expect(err).NotTo(HaveOccurred())

By("setting the auth headers")
req.Header.Set(userIdHeader, adminUser)
req.Header.Set("Content-Type", "application/merge-patch+json")

By("executing PauseWorkspaceHandler")
ps := httprouter.Params{
httprouter.Param{Key: NamespacePathParam, Value: namespaceName1},
httprouter.Param{Key: ResourceNamePathParam, Value: workspaceName1},
}
rr := httptest.NewRecorder()
a.PauseWorkspaceHandler(rr, req, ps)
rs := rr.Result()
defer rs.Body.Close()

By("verifying the HTTP response status code")
Expect(rs.StatusCode).To(Equal(http.StatusOK), descUnexpectedHTTPStatus, rr.Body.String())

By("reading the HTTP response body")
body, err := io.ReadAll(rs.Body)
Expect(err).NotTo(HaveOccurred())

By("verifying the response is an empty JSON object")
Expect(string(body)).To(Equal("{}\n"))

By("getting the Workspace from the Kubernetes API")
workspace := &kubefloworgv1beta1.Workspace{}
Expect(k8sClient.Get(ctx, workspaceKey1, workspace)).To(Succeed())

By("ensuring the workspace is paused")
Expect(workspace.Spec.Paused).To(Equal(ptr.To(true)))
})

It("should return 404 for a non-existent workspace", func() {
missingWorkspaceName := "non-existent-workspace"

By("creating the HTTP request")
path := strings.Replace(PauseWorkspacePath, ":"+NamespacePathParam, namespaceName1, 1)
path = strings.Replace(path, ":"+ResourceNamePathParam, missingWorkspaceName, 1)
req, err := http.NewRequest(http.MethodPost, path, http.NoBody)
Expect(err).NotTo(HaveOccurred())

By("setting the auth headers")
req.Header.Set(userIdHeader, adminUser)

By("executing PauseWorkspaceHandler")
ps := httprouter.Params{
httprouter.Param{Key: NamespacePathParam, Value: namespaceName1},
httprouter.Param{Key: ResourceNamePathParam, Value: missingWorkspaceName},
}
rr := httptest.NewRecorder()
a.PauseWorkspaceHandler(rr, req, ps)
rs := rr.Result()
defer rs.Body.Close()

By("verifying the HTTP response status code")
Expect(rs.StatusCode).To(Equal(http.StatusNotFound), descUnexpectedHTTPStatus, rr.Body.String())
})
})
})
Loading