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: enrich kubectl get output #878

Merged
merged 1 commit into from
Jan 27, 2023
Merged

Conversation

davidxia
Copy link
Contributor

@davidxia davidxia commented Jan 23, 2023

by adding additional printer columns for number of desired workers, number of available workers, status, age, head Pod IP, and head Service IP.

Before

kubectl get rayclusters
NAME         AGE
dxia-test    53m
keshi-test   2d23h

After

kubectl get rayclusters
NAME         DESIRED WORKERS   AVAILABLE WORKERS   STATUS   AGE
dxia-test    3                 4                   ready    60m
keshi-test   1                 2                   ready    2d23h

kubectl get rayclusters -o wide
NAME         DESIRED WORKERS   AVAILABLE WORKERS   STATUS   AGE     HEAD POD IP    HEAD SERVICE IP
dxia-test    3                 4                   ready    60m     10.169.4.130   10.160.217.214
keshi-test   1                 2                   ready    2d23h   10.169.6.102   10.160.218.91

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

by adding additional printer columns for number of desired workers, number of
available workers, status, age, head Pod IP, and head Service IP.

## Before

```
kubectl get rayclusters
NAME         AGE
dxia-test    53m
keshi-test   2d23h
```

## After

```
kubectl get rayclusters
NAME         DESIRED WORKERS   AVAILABLE WORKERS   STATUS   AGE
dxia-test    3                 4                   ready    60m
keshi-test   1                 2                   ready    2d23h

kubectl get rayclusters -o wide
NAME         DESIRED WORKERS   AVAILABLE WORKERS   STATUS   AGE     HEAD POD IP    HEAD SERVICE IP
dxia-test    3                 4                   ready    60m     10.169.4.130   10.160.217.214
keshi-test   1                 2                   ready    2d23h   10.169.6.102   10.160.218.91
```
@davidxia davidxia marked this pull request as ready for review January 23, 2023 14:00
@davidxia
Copy link
Contributor Author

There's a bug with .status.availableWorkerReplicas counting the head node as well. That's why the example output has that being one more than .status.desiredWorkerReplicas. I think this can be fixed in a later PR.

Comment on lines +150 to +151
//+kubebuilder:printcolumn:name="desired workers",type=integer,JSONPath=".status.desiredWorkerReplicas",priority=0
//+kubebuilder:printcolumn:name="available workers",type=integer,JSONPath=".status.availableWorkerReplicas",priority=0
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 wanted to show something like Deployments' READY column with an available/desired in one column. But only simple field access is supported. Lmk if there's some way.

kubectl get deployments
NAME             READY   UP-TO-DATE   AVAILABLE   AGE
my-deployment    1/1     1            1           515d

Copy link
Collaborator

Choose a reason for hiding this comment

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

Neither do I. separate columns sounds good.

@davidxia
Copy link
Contributor Author

cc @kevin85421 @DmitriGekhtman

Comment on lines +150 to +151
//+kubebuilder:printcolumn:name="desired workers",type=integer,JSONPath=".status.desiredWorkerReplicas",priority=0
//+kubebuilder:printcolumn:name="available workers",type=integer,JSONPath=".status.availableWorkerReplicas",priority=0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Neither do I. separate columns sounds good.

@Jeffwan
Copy link
Collaborator

Jeffwan commented Jan 25, 2023

kubectl get rayclusters -o wide
NAME DESIRED WORKERS AVAILABLE WORKERS STATUS AGE HEAD POD IP HEAD SERVICE IP
dxia-test 3 4 ready 60m 10.169.4.130 10.160.217.214

in this screenshot, you expect 3 but there're 4? What's the pod status?

Copy link
Collaborator

@DmitriGekhtman DmitriGekhtman left a comment

Choose a reason for hiding this comment

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

coool

@davidxia
Copy link
Contributor Author

kubectl get rayclusters -o wide
NAME DESIRED WORKERS AVAILABLE WORKERS STATUS AGE HEAD POD IP HEAD SERVICE IP
dxia-test 3 4 ready 60m 10.169.4.130 10.160.217.214

in this screenshot, you expect 3 but there're 4? What's the pod status?

Status is ready. It’s an existing bug. You can see it with kubectl get raycluster -o yaml. See comment above. Controller is counting head as a worker, I think.

Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

Thank @davidxia for the contribution! I tried to test this PR manually, and found that STATUS and HEAD POD IP do not print any message when all head Pod and worker Pod are READY and RUNNING. Is it an expected behavior? Thanks!

# Step 1: Create a Kind cluster
kind create cluster

# Step 2: Build a docker image
# (path: kuberay/ray-operator)
make docker-image
kind load docker-image controller:latest

# Step 3: Install a KubeRay operator with the docker image built in Step 2.
# (path: kuberay/helm-chart/kuberay-operator)
helm install kuberay-operator --set image.repository=controller,image.tag=latest .

# Step 4: Install a RayCluster
# (path: kuberay/helm-chart/ray-cluster)
helm install raycluster .

# Step 5: See the following figure.
kubectl get rayclusters.ray.io
kubectl get rayclusters.ray.io -o wide

Screen Shot 2023-01-25 at 6 47 30 AM

Screen Shot 2023-01-25 at 6 49 20 AM

@davidxia
Copy link
Contributor Author

STATUS and HEAD POD IP do not print any message when all head Pod and worker Pod are READY and RUNNING. Is it an expected behavior?

Yes this is expected in some cases. The printer columns simply access fields in the underlying RayCluster custom resource. Sometimes .status.state and/or .status.head.podIP are empty for various reasons like cluster isn't ready or head Pod doesn't have an IP yet.

I think it's OK to show an empty cell for a column if there isn't a value for it yet.

#875 marks clusters as pending if they aren't ready yet. Lmk what you think of that separate issue.

@kevin85421
Copy link
Member

kevin85421 commented Jan 26, 2023

Yes this is expected in some cases. The printer columns simply access fields in the underlying RayCluster custom resource. Sometimes .status.state and/or .status.head.podIP are empty for various reasons like cluster isn't ready or head Pod doesn't have an IP yet.

I think it's OK to show an empty cell for a column if there isn't a value for it yet.

#875 marks clusters as pending if they aren't ready yet. Lmk what you think of that separate issue.

if utils.CheckAllPodsRunnning(runtimePods) {
instance.Status.State = rayiov1alpha1.Ready
}

Based on the code snippet above, the State should be ready rather than empty because all Pods are running. Is it correct?

I check it again, and all Pods are running and ready as shown in the following figure.

Screen Shot 2023-01-26 at 4 28 15 AM

Is it related to #882?

@davidxia
Copy link
Contributor Author

Based on the code snippet above, the State should be ready rather than empty because all Pods are running. Is it correct?

yes

Is it related to #882?

yes, your RayClusters will more reliably be marked ready once this is merged

Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

LGTM

@kevin85421
Copy link
Member

The CI failure is due to #852. Merge it.

@kevin85421 kevin85421 merged commit db0ee96 into ray-project:master Jan 27, 2023
@davidxia davidxia deleted the print-cols branch January 27, 2023 17:31
@davidxia
Copy link
Contributor Author

amazing, thanks! Looking forward to some enriched output in the next release 🙌

lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
by adding additional printer columns for number of desired workers, number of
available workers, status, age, head Pod IP, and head Service IP.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants