group podLists from different nodes into single list #11216
Conversation
why? this made-up format is not a valid API list type... and will not work with any of the other |
|
if you want pods for a particular node, run if you want to know what node a particular pod is on, just look at the |
|
@liggitt Thanks for the feedback, I'll close this PR EDIT: reopened PR, rather than changing the format of the output, the new commit groups the list of pods of every node into a single list to be printed by a json or yaml printer. |
juanvallejo
force-pushed
the
jvallejo/group-podlists-w-corresponding-node
branch
from
299fadf to
978eaf0
Compare
| // determine if printer kind is json or yaml and modify output | ||
| // to combine all pod lists into a single list | ||
| switch printer.(type) { | ||
| case *kubectl.JSONPrinter: |
There was a problem hiding this comment.
type checking these smells bad... why not flatten the list for all custom output types?
|
[test] |
juanvallejo
changed the title
|
@liggitt PTAL |
| } | ||
|
|
||
| if unifiedPodList == nil { | ||
| unifiedPodList = pods |
There was a problem hiding this comment.
careful about using the first list as the aggregate... not sure the selfLink or resourceVersion will be right
There was a problem hiding this comment.
@liggitt Hm, I am not sure how to handle cases where either selfLink or resourceVersion fields are different between PodLists. Should I use the last node's PodList as the aggregate, or maybe the list with the most recent resourceVersion?
There was a problem hiding this comment.
or aggregate them into a newly constructed PodList without a selfLink or resourceVersion
|
@liggitt PTAL |
| // objects for every node, into a single list. This allows output containing multiple nodes to be | ||
| // printed to a single writer, and be easily parsed as a single data format. | ||
| func (l *ListPodsOptions) handleRESTOutput(nodes []*kapi.Node, printer kubectl.ResourcePrinter) []error { | ||
| var unifiedPodList *kapi.PodList |
There was a problem hiding this comment.
why not always initialize it? I'd expect an empty list if there were no pods
| errList = append(errList, err) | ||
| } | ||
|
|
||
| if unifiedPodList == nil { |
There was a problem hiding this comment.
this simplifies to a straight append, then
|
@liggitt Thanks for the feedback! Review comments addressed |
|
|
||
| pods, err := l.Options.Kclient.Pods(kapi.NamespaceAll).List(kapi.ListOptions{LabelSelector: labelSelector, FieldSelector: fieldSelector}) | ||
| if err != nil { | ||
| errList = append(errList, err) |
There was a problem hiding this comment.
continue here, don't go on to use pods, which is likely nil in this case
|
one last comment, @fabianofranz has final LGTM |
juanvallejo
force-pushed
the
jvallejo/group-podlists-w-corresponding-node
branch
from
1d71d7a to
d15311e
Compare
|
@liggitt Thanks for the review |
|
integration, conformance flake on yum, #8571, re[test] |
|
squash then LGTM |
juanvallejo
force-pushed
the
jvallejo/group-podlists-w-corresponding-node
branch
from
d15311e to
101b8de
Compare
|
@fabianofranz Done! |
|
[merge] |
|
conformance flaked on #11094 |
|
flaked on #11094 re[merge] |
|
Related Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1381621 When listing pods as JSON or YAML for one or more nodes with `oadm manage-node`, the node name is printed to stderr, while the PodList object is printed to stdout. This patch updates the output of `oadm manage-node --list-pods` to join the list of pods of every node into a single PodList, allowing the output to consist of a single api.PodList object. **Before** ``` $ oadm manage-node --selector="type=infra" --list-pods -o json Listing matched pods on node: ip-172-31-53-15.ec2.internal { "metadata": { "selfLink": "/api/v1/pods", "resourceVersion": "350009" }, "items": [ { .... } Listing matched pods on node: ip-172-31-53-16.ec2.internal { "metadata": { "selfLink": "/api/v1/pods", "resourceVersion": "350009" }, "items": [ { ... } ``` **After** `"items" will contain the pods of every node` ``` $ oadm manage-node --selector="type=infra" --list-pods -o json { "metadata": { "selfLink": "/api/v1/pods", "resourceVersion": "350009" }, "items": [ { ... } ```
juanvallejo
force-pushed
the
jvallejo/group-podlists-w-corresponding-node
branch
from
101b8de to
048b9bd
Compare
|
@fabianofranz hm, looks like it was renamed to re[test] |
|
Evaluated for origin test up to 048b9bd |
|
[merge] |
|
Evaluated for origin merge up to 048b9bd |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10320/) (Base Commit: 9ef2b7f) |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10462/) (Base Commit: 8a79d34) (Image: devenv-rhel7_5222) |
Related Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1381621
When listing pods as JSON or YAML for one or more nodes with
oadm manage-node, thenode name is printed to stderr, while the PodList object is printed to stdout.
This patch updates the output of
oadm manage-node --list-podsto join the list ofpods of every node into a single PodList, allowing the output to consist of a single
api.PodList object.
Before
After
"items" will contain the pods of every node@openshift/cli-review