Feat: System Info & Diagnose #4379
Conversation
Signed-off-by: foursevenlove <foursevenlove@gmail.com>
foursevenlove
requested review from
Somefive,
zzxwill and
StevenLeiZhang
as code owners
Codecov Report
@@ Coverage Diff @@
## master #4379 +/- ##
==========================================
+ Coverage 60.59% 61.51% +0.91%
==========================================
Files 343 348 +5
Lines 33670 34512 +842
==========================================
+ Hits 20402 21229 +827
+ Misses 10589 10512 -77
- Partials 2679 2771 +92
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
references/cli/system.go
Outdated
| return cmd | ||
| } | ||
|
|
||
| // NewSystemDiagnoseCommand create command to help user to diagonse system's health |
There was a problem hiding this comment.
misspell: diagonse is a misspelling of diagnose
Reply with "@sonatype-lift help" for info about LiftBot commands.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.
When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.
Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]
references/cli/system.go
Outdated
| "strings" | ||
|
|
||
| "github.com/oam-dev/cluster-gateway/pkg/generated/clientset/versioned" | ||
| "github.com/oam-dev/kubevela/apis/types" |
There was a problem hiding this comment.
goimports: File is not goimports-ed with -local github.com/oam-dev/kubevela
Reply with "@sonatype-lift help" for info about LiftBot commands.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.
When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.
Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]
Signed-off-by: foursevenlove <foursevenlove@gmail.com>
Signed-off-by: foursevenlove <foursevenlove@gmail.com>
Signed-off-by: foursevenlove <foursevenlove@gmail.com>
references/cli/system.go
Outdated
| if err != nil { | ||
| panic(err) | ||
| } | ||
| table.AddRow(deployment.Name, deployment.Namespace, deployment.Spec.Template.Spec.Containers[0].Image, strings.Join(deployment.Spec.Template.Spec.Containers[0].Args, " ")) |
There was a problem hiding this comment.
If we are getting a single deployment, which means the user want some detailed info, is fitting that deployment in a single row a good choice?
references/cli/system.go
Outdated
| func NewSystemInfoCommand(c common.Args) *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "info", | ||
| Short: "Print the system deployment detail information in vela-system namespace.", |
There was a problem hiding this comment.
Notice that while dealing with SystemInfo, the install namespace of KubeVela could be something other than "vela-system", so it would be better if we can have namespace-agnostic detection logics. This can be reserved as a future enhancement, not necessary for this PR.
There was a problem hiding this comment.
Except for the comments by @charlie0129 (which are great advices), this PR, as an initial bootstrap for the vela system command generally LGTM. There are more details to dig for these two commands, which can be addressed in future PRs. Thanks for the contribution!
Details:
- The logic for
vela system infois looking for the deployments invela-system. But there are potential problems for that. Users can install KubeVela controller in other namespaces instead ofvela-systemand it is also possible to install vela-irrelevant deployments in thevela-system. Furthermore, even deployments can have zero replicas, which indicates they are not working. Deployments could be upgrading as well, which means the current working pods might not use the latest configuration for deployments while upgrading. So it might be better to add checks for the pods and look for KubeVela controllers by labels. - We could add simple resource check for KubeVela controller & ClusterGateway, for example, showing the current CPU/Memory usage.
- Adding information for environment variables is also helpful, not only args.
- Args can be grouped, according to their usage. You can try to make some groupings according to your understanding and let's make discussion in the future.
- The discussion of diagnose command can be reserved in the future as well.
|
Would you like to make a small video / gif / snapshots for the command and attach it in the introduction of the PR? It would be nice and straightforward for other reviewers to understand it. :) |
… of by namespace 3.when getting a single deployment, the result is displayed in multi rows. Feat: 1.the system info command displays the cpu and memory metrics 2.the system info command displays the numbers of ready pods and desired pods.
|
Thank you @Somefive @charlie0129 for reviewing my code and making valuable suggestions. Based on the original, I made the following modifications:
Feat:
Finally I made a gif to show how to use the system command： |
Looks great! I notice that the DCO CI has some problem. You could follow the guide to fix your commit signature. |
There was a problem hiding this comment.
As a first initial PR, looks good to me. Following possible enhancements in later PR:
- For system info, the list of arguments in the demo gif looks to be truncated due to the width of the terminal. Multi-line display might be more helpful.
- For demo gif, it is clear that when everything is right, the command works pretty well. Let's make some fake broken scenarios, for example, let controller down, and show how the system-info will show.
- For demo gif, another interesting thing is the upgrade process. What if the kubevela-controller is upgrading and there are two different pods running at the same time. Could the system info command show their differences?
- For system info, showing CPU/Memory usage is cool, let's add the percentage usage if the resource limit is set. For example, memory: 456Mi (45.6%).
- For system info, there are some logs could be potentially useful. But I haven't come up with the idea of how to handle the log information. Feel free to make discussions if you have any ideas on it.
As for the diagnose, similar to [2] above, let's make some bad situation and see how this command performs.
wonderflow
added
the
backport release-1.5
|
Successfully created backport PR #4499 for |
Signed-off-by: foursevenlove foursevenlove@gmail.com
Description of your changes
Feat #3924
I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
vela system infoto check all detail system informationvela system diagnoseto diagnose system's healthSpecial notes for your reviewer
@Somefive