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

Crossplane top command - pods resources utilization implementation #5245

Merged
merged 1 commit into from Jan 24, 2024

Conversation

Piotr1215
Copy link
Contributor

@Piotr1215 Piotr1215 commented Jan 15, 2024

Description of your changes

Initial implementation of the crossplane top command.

This implementation covers running the top command to show resources utilization of crossplane pods:

  • crossplane
  • functions
  • providers

Prerequisites

In order to display metrics, the metrics-server needs to be enabled. Prometheus is not required. Enabling metrics server like so, for example:

 kubectl apply -f https://github.com/kubernetes-sigs/metrics-server/releases/latest/download/components.yaml
 kubectl patch deployment metrics-server -n kube-system \
  --type='json' \
  -p='[{"op": "add", "path": "/spec/template/spec/containers/0/args/-", "value": "--kubelet-insecure-tls=true"}]'

Running the command

The top command behaves very similarly to the kubectl top pods. It displays CPU and Memory utilization for Crossplane pods. By default the command will search for crossplane pods in the crossplane-system namespace. It's possible to use -n or --namespace flag to look for pods in another namespace.

Running command without flags will default to a table view, sorted by pod type and pod name.

image

From here, it's easy to use command line utilities like sort awk or sed to manipulate the output.

Running command with -s or --summary flag will add pods summary on top of the table view:

image

Metrics Server

Before pods can emit metrics it can take a moment for new pods to register, so transient errors like this are possible, just run the command again.

crossplane: error: error adding metrics to pod, check if metrics-server is running or wait until metrics are available for the pod: pods "provider
-aws-052e7b5a5276-96b9888bc-gff49" not found
exit status 1

Partially Implements #5036

I have:

Need help with this checklist? See the cheat sheet.

cmd/crank/beta/top/top.go Outdated Show resolved Hide resolved
Copy link
Contributor

@lsviben lsviben left a comment

Choose a reason for hiding this comment

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

Did a quick pass, maybe some comments are uneccesary as I know its a WIP, but added them just in case.

cmd/crank/beta/top/top.go Outdated Show resolved Hide resolved
cmd/crank/beta/top/top.go Outdated Show resolved Hide resolved
cmd/crank/beta/top/top.go Outdated Show resolved Hide resolved
cmd/crank/beta/top/top.go Outdated Show resolved Hide resolved
@Piotr1215 Piotr1215 force-pushed the crossplane-top branch 4 times, most recently from 124ab38 to bfd4be7 Compare January 18, 2024 15:12
@Piotr1215 Piotr1215 changed the title [WIP] Crossplane top command Crossplane top command - pods resources utilization implementation Jan 18, 2024
@Piotr1215 Piotr1215 marked this pull request as ready for review January 18, 2024 15:17
@Piotr1215 Piotr1215 requested a review from a team as a code owner January 18, 2024 15:17
@negz
Copy link
Member

negz commented Jan 18, 2024

@Piotr1215 Could you please update the PR description with an example or two, showing what it's like to use the command?

cmd/crank/beta/top/top.go Outdated Show resolved Hide resolved
cmd/crank/beta/top/top_test.go Outdated Show resolved Hide resolved
cmd/crank/beta/top/top.go Outdated Show resolved Hide resolved
cmd/crank/beta/top/top.go Outdated Show resolved Hide resolved
cmd/crank/beta/top/top.go Outdated Show resolved Hide resolved
@jbw976
Copy link
Member

jbw976 commented Jan 19, 2024

@Piotr1215 Could you please update the PR description with an example or two, showing what it's like to use the command?

+1 to this as I'm actually having trouble testing this PR myself. I built from your branch/fork, installed Crossplane with --set metrics.enabled=true, but running beta top gives me:

~/dev/go/src/github.com/crossplane/crossplane/_output/bin/darwin_arm64 @c865430c
❯ ./crank beta top -s
crossplane: error: error adding metrics to pod: the server could not find the requested resource (get pods.metrics.k8s.io crossplane-5b8df59d5-9cqt8)

Is there more config I need to do to enable the metrics and get the top command working? 🤔

@Piotr1215
Copy link
Contributor Author

@negz @jbw976 thank you for the reviews. I have clarified the command usage in PR description and updated command help with examples. In order for the command to work, the metrics server must be enabled.

The error you have seen @jbw976 is a bit unfortunate because it can mean that the metrics server is not enabled (prerequisite not met) or that there are pods that are still registering with the server (transient error).

I've marked as resolved the suggestions that are implemented/fixed. The ones still open have some open questions/comments.

I think adding more tests would also be a good idea :)

cmd/crank/beta/top/top.go Show resolved Hide resolved
cmd/crank/beta/beta.go Outdated Show resolved Hide resolved
cmd/crank/beta/top/top.go Outdated Show resolved Hide resolved
cmd/crank/beta/top/top.go Outdated Show resolved Hide resolved
cmd/crank/beta/top/top.go Outdated Show resolved Hide resolved
cmd/crank/beta/top/top.go Outdated Show resolved Hide resolved
cmd/crank/beta/top/top.go Outdated Show resolved Hide resolved
@Piotr1215 Piotr1215 force-pushed the crossplane-top branch 7 times, most recently from 6984151 to 13c6b15 Compare January 24, 2024 15:27
cmd/crank/beta/top/top.go Outdated Show resolved Hide resolved
The command filters all crossplane pods from a given namespace and
displays CPU/Memory metrics similar to the kubectl top pods.

Results printing comes in two variations:
- default tabular view with pod type
- ability to add header summary for all crossplane pods

The default printer implementation uses tabwriter package to create equal
spacing between the results.

Signed-off-by: Piotr Zaniewski <piotr@upbound.io>
Copy link
Member

@jbw976 jbw976 left a comment

Choose a reason for hiding this comment

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

awesome this looks like it got to a clean place to ship initial functionality!! 💪

@phisco phisco merged commit fc625c3 into crossplane:master Jan 24, 2024
15 of 17 checks passed
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.

None yet

5 participants