-
Notifications
You must be signed in to change notification settings - Fork 39.4k
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
Make the output of kubectl describe service
more informative
#125117
Conversation
/sig network |
From sig-cli POV, this looks good to me. But I'd prefer sig-network reviews first. |
@ardaguclu thanks for the review. @danwinship @aojea could you please take a look? |
@@ -3030,7 +3030,7 @@ func describeService(service *corev1.Service, endpointSlices []discoveryv1.Endpo | |||
w.Write(LEVEL_0, "External IPs:\t%v\n", strings.Join(service.Spec.ExternalIPs, ",")) | |||
} | |||
if service.Spec.LoadBalancerIP != "" { | |||
w.Write(LEVEL_0, "IP:\t%s\n", service.Spec.LoadBalancerIP) | |||
w.Write(LEVEL_0, "LoadBalancer IP:\t%s\n", service.Spec.LoadBalancerIP) |
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.
should be something like "Requested LoadBalancer IP" to distinguish it from the "LoadBalancer Ingress" (which is the assigned IP/hostname). Though that would be the new longest field name and it seems like we probably don't want the field names to be too long (since it pushes the field values further away, making it harder to read) so if we could abbreviate that somehow...
Alternatively, this field is deprecated anyway and we discourage people from using it, so maybe we should just remove it from "kubectl describe"? (Is describe supposed to show everything or just the most useful stuff?)
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.
Thanks for the suggestion. How about "Desired LoadBalancer IP"? It has the same length as "External Traffic Policy", so it won't push the values further away.
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 prefer Requested over Desired honestly, better an english native to judge here, at least in spanish desire has some connotations that will not ideal for this context :)
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 see desired is already used in some objects' describers :)
w.Write(LEVEL_0, "Replicas:\t%d current / %d desired\n", controller.Status.Replicas, *controller.Spec.Replicas) |
w.Write(LEVEL_0, "Desired Number of Nodes Scheduled: %d\n", daemon.Status.DesiredNumberScheduled) |
w.Write(LEVEL_0, "Replicas:\t%d desired | %d updated | %d total | %d available | %d unavailable\n", *(d.Spec.Replicas), d.Status.UpdatedReplicas, d.Status.Replicas, d.Status.AvailableReplicas, d.Status.UnavailableReplicas) |
Please let me know if I should still update it to Requested. It will push values two spaces further when the field is not empty, but it's not the longest field, "LoadBalancer Source Ranges" is the one, but none of the tests set it.
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.
@danwinship @aojea as "desired" is already used in several describers, do you think it could be used in Service describer, or you prefer using "Requested" (which would push the values two spaces unless "LoadBalancer Source Ranges" is also 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.
either way is fine, if there is precedence better to use the convention
lgtm |
/approve |
/triage accepted |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, tnqn 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 |
For a LoadBalancer Service, there were two "IP" fields in the output of `kubectl describe service` if its loadBalancerIP is not empty, which looks ambiguous.
@@ -3027,7 +3030,7 @@ func describeService(service *corev1.Service, endpointSlices []discoveryv1.Endpo | |||
w.Write(LEVEL_0, "External IPs:\t%v\n", strings.Join(service.Spec.ExternalIPs, ",")) | |||
} | |||
if service.Spec.LoadBalancerIP != "" { | |||
w.Write(LEVEL_0, "IP:\t%s\n", service.Spec.LoadBalancerIP) | |||
w.Write(LEVEL_0, "Desired LoadBalancer IP:\t%s\n", service.Spec.LoadBalancerIP) |
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.
Comments is addressed.
Do you have time to take a look again @danwinship @aojea ?
/lgtm |
LGTM label has been added. Git tree hash: 8b203af44d07a73758b1a48d6bc083f95a79eb11
|
/label tide/merge-method-squash |
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
…netes#125117) * kubectl: add internalTrafficPolicy to Service describer * kubectl: add loadBalancer ipMode to Service describer * kubectl: fix duplicate IP fields in Service describer For a LoadBalancer Service, there were two "IP" fields in the output of `kubectl describe service` if its loadBalancerIP is not empty, which looks ambiguous.
…netes#125117) * kubectl: add internalTrafficPolicy to Service describer * kubectl: add loadBalancer ipMode to Service describer * kubectl: fix duplicate IP fields in Service describer For a LoadBalancer Service, there were two "IP" fields in the output of `kubectl describe service` if its loadBalancerIP is not empty, which looks ambiguous.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
InternalTrafficPolicy has been GA and LoadBalancerIPMode has been beta and enabled by default. Adding them to the output of
kubectl describe service
to make it more informative.In addition, for a LoadBalancer Service, there were two "IP" fields in the output of
kubectl describe service
if its loadBalancerIP is not empty, which looks confusing. The second "IP" is the loadBalancer IP in spec.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: