-
Notifications
You must be signed in to change notification settings - Fork 30
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 podTemplatePod
to Workspace status, for backend logs
#210
feat(ws): add podTemplatePod
to Workspace status, for backend logs
#210
Conversation
Signed-off-by: Mathew Wicks <[email protected]>
@ederign this should be ready to review/lgtm |
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'm learning how controller works, so just left a few comments to clarify. @etirelli @harshad16 @andyatmiami maybe one of you can take a look on it?
@@ -4,5 +4,5 @@ resources: | |||
- manager.yaml | |||
images: | |||
- name: controller | |||
newName: ghcr.io/kubeflow/notebooks/workspace-controller | |||
newName: controller |
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.
@thesuperzapper, just to double-check that this was intentional!
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.
Do we need to also change https://github.com/kubeflow/notebooks/blob/notebooks-v2/workspaces/controller/test/e2e/e2e_test.go#L38 ?
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.
It was just an automated thing, but it was a mistake, yes!
It happens if you run make docker-build
without IMG
set.
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.
Fixed in 689c828
log.Error(err, "unable to update Workspace status") | ||
return ctrl.Result{}, err | ||
} | ||
|
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'm new for controller, but why we don't need workspace.Status.PendingRestart = false anymore?
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.
@ederign a few lines up I realized we were not always correctly setting it.
Now I build the status by setting it to false at the beginning and then only setting it to true when I detect an option has a redirect.
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.
Thank you for the clarification!
Signed-off-by: Mathew Wicks <[email protected]>
/lgtm |
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.
still getting upto speed with codeblock
lgtm in general
just one question
return status, nil | ||
state = kubefloworgv1beta1.WorkspaceStateRunning | ||
stateMessage = stateMsgRunning | ||
return state, stateMessage, nil | ||
} | ||
|
||
// get container status | ||
// https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#container-states | ||
var containerStatus corev1.ContainerStatus |
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.
Is there initcontainerstatus maintained some other place ?
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.
@harshad16 you are correct that the initContainer status is stored under status.InitContainerStatuses
, but in this case, we are looking for the status of the "main"
container, so we can get info about it.
But you are correct that we might have a problem if the Pod is failing because an init-container (or in fact some other container is in an error state), as we would fall through to the Unknown State.
Do you want to pick this up as another task after we merge?
I am merging this for now, but as @harshad16 said in #210 (comment), there is still some work to do on the state extraction. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ederign, 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 |
Hmm, it seems like we now always need to have the /ok-to-test |
related: #208
related: #172
Currently, it would have been impossible for the backend to know which pod to read logs from as part of #208, so this PR adds
podTemplatePod
to the status of Workspaces.This new status field will look something like (when the Pod exists):
This new status field will look something like (when the Pod does not exist):
This PR also:
generateWorkspaceStatus
method by factoring out agenerateWorkspaceState
to generate the state/stateMessageworkspacePodTemplateContainerName
variable, rather than using"main"
everywhere