carvel-dev / kapp

kapp is a simple deployment tool focused on the concept of "Kubernetes application" — a set of resources with the same label
https://carvel.dev/kapp
Apache License 2.0
938 stars 110 forks source link

remove conditions column from inspect/diff since it's misleadingly colored for resources like HPA which always have a false condition #235

Closed StarpTech closed 2 years ago

StarpTech commented 3 years ago

What steps did you take:

Namespace  test  
Name       test-71295  
Kind       HorizontalPodAutoscaler  
Status     conditions:  
           - lastTransitionTime: "2021-06-24T11:56:39Z"  
             message: recommended size matches current size  
             reason: ReadyForNewScale  
             status: "True"  
             type: AbleToScale  
           - lastTransitionTime: "2021-06-24T12:41:27Z"  
             message: the HPA was able to successfully calculate a replica count from cpu resource  
               utilization (percentage of request)  
             reason: ValidMetricFound  
             status: "True"  
             type: ScalingActive  
           - lastTransitionTime: "2021-06-24T11:58:24Z"  
             message: the desired count is within the acceptable range  
             reason: DesiredWithinRange  
             status: "False"  
             type: ScalingLimited  
           currentMetrics:  
           - resource:  
               current:  
                 averageUtilization: 3  
                 averageValue: 16m  
               name: cpu  
             type: Resource  
           currentReplicas: 1  
           desiredReplicas: 1  
           lastScaleTime: "2021-06-24T12:45:06Z"
Namespace           Name                              Kind                     Owner    Conds.  Rs  Ri  Age  
test                test-71295                        HorizontalPodAutoscaler  kapp     2/3 t   ok  -   52m  

What happened:

ScalingLimited: false is interpreted as an error.

What did you expect: ScalingLimited indicates that autoscaling is not allowed because a maximum or minimum replica count was reached.

A True condition indicates that you need to raise or lower the minimum or maximum replica count in order to scale.

A False condition indicates that the requested scaling is allowed.

Source: https://docs.openshift.com/container-platform/3.9/dev_guide/pod_autoscaling.html

Environment:


Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible" 👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

cppforlife commented 3 years ago

@StarpTech yeah, conditions coloring is a bit misleading. i wonder if we should remove coloring or may be even remove entire column -- ive not found myself using it at all. wdyt? (since conditions are very much resource specific there isnt a good pattern to follow here).

StarpTech commented 3 years ago

I mean if we can't determine if a condition is successful we should remove it. Otherwise, fix it. I think the information that all conditions are meet useful.

cppforlife commented 3 years ago

I mean if we can't determine if a condition is successful we should remove it.

unfortunately kubernetes doesnt provide any foundation for describing something being "successful" or not. conditions are more like facts. they are either True or False but that does not indicate whether something is succesful or not. so ultimately today in kubernetes, only human can decide whether something is "succesful".

ive added conds. column long time as more of an experiment in determining if it's going to be useful, but so far didnt have strong reaction from folks about it being actually useful.

rohitagg2020 commented 2 years ago

Task

Remove conditions column from kapp inspect and during diff stage since it's misleadingly coloured for some of the resources like HPA which always have a false condition. Currently this condition comes in the GUI output as mentioned in the description of the issue. So we need to remove it.

Code Changes

  1. https://github.com/vmware-tanzu/carvel-kapp/blob/6b191d2550397dc1007f5bcb2ef536d0358fdd3f/pkg/kapp/cmd/tools/inspect_tree_view.go#L25
  2. https://github.com/vmware-tanzu/carvel-kapp/blob/6b191d2550397dc1007f5bcb2ef536d0358fdd3f/pkg/kapp/cmd/tools/inspect_view.go#L22
  3. https://github.com/vmware-tanzu/carvel-kapp/blob/6b191d2550397dc1007f5bcb2ef536d0358fdd3f/pkg/kapp/clusterapply/changes_view.go#L35
renuy commented 2 years ago

Available as part of kapp v0.47.0