-
Notifications
You must be signed in to change notification settings - Fork 55
feat(ws): Define k8s workload manifest for frontend component #404 #487
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
base: notebooks-v2
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
i'll ask for a couple quick edits that jump out now - but please be aware I'll do a much more comprehensive review on Monday where I will try to deploy these changes along with base Kubeflow manifests and backend
component to exercise end-to-end functionality.
For now, 2 easy "structure" changes I will ask for:
-
workspaces/frontend/manifests/kustomize
should be the "base" directory (no/config
folder) -
options/istio
should be the root folder in which you place the istio-specific resources
livenessProbe: | ||
httpGet: | ||
path: / | ||
port: 8080 |
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.
use the named port value of http-frontend
here
readinessProbe: | ||
httpGet: | ||
path: / | ||
port: 8080 |
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.
http: | ||
- match: | ||
- uri: | ||
prefix: /notebooks |
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.
I think we'll want to call this /workspaces
to be more future-proofed
but most importantly - it should be 'aligned' with what backend
is setting..
i.e.
- frontend:
/workspaces
- backend:
/workspaces/api
7edcef8
to
7c11794
Compare
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.
Found some minor discrepancy
selector: | ||
matchLabels: | ||
app.kubernetes.io/name: kubeflow-notebooks | ||
app.kubernetes.io/component: frontend |
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.
This doesnt match
app.kubernetes.io/component: workspace-frontend |
we should make them align
apiVersion: kustomize.config.k8s.io/v1beta1 | ||
kind: Kustomization | ||
|
||
namespace: kubeflow-system |
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.
all part in virtual service , look for service in workspace-controller-system
, however , we are explicitly deploying bits in kubeflow-system
@harshad16: changing LGTM is restricted to collaborators In response to this:
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. |
…w#404 Signed-off-by: Noa <[email protected]>
7c11794
to
a2a8b17
Compare
/lgtm verified these changes with help of a script I have been working on to deploy Central Dashboard in user requests to |
/ok-to-test |
closes: #404
In this PR I have deployed the frontend component to Kubernetes using Kustomize.