Skip to content
This repository has been archived by the owner on Jan 19, 2023. It is now read-only.

Add a node pods view #1773

Merged
merged 5 commits into from
Feb 3, 2021
Merged

Add a node pods view #1773

merged 5 commits into from
Feb 3, 2021

Conversation

zparnold
Copy link
Contributor

Signed-off-by: Zach Arnold me@zacharnold.org

What this PR does / why we need it:
Adds a "Pods" table to the Node overview so that people can see which pods are scheduled onto the node

Which issue(s) this PR fixes

Special notes for your reviewer:
I'm not quite sure how to mock the Kubernetes Client to create a test for this, I'd need some help on that.

Release note:

Adds a "Pods" table to the Node overview so that people can see which pods are scheduled onto the node

Signed-off-by: Zach Arnold <me@zacharnold.org>
Signed-off-by: Zach Arnold <me@zacharnold.org>
Signed-off-by: Zach Arnold <me@zacharnold.org>
Signed-off-by: Zach Arnold <me@zacharnold.org>
@zparnold
Copy link
Contributor Author

The test timeout on MacOS is not related to my code I believe. Please let me know what I need to do to get this merged.

Copy link
Contributor

@GuessWhoSamFoo GuessWhoSamFoo left a comment

Choose a reason for hiding this comment

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

There is an unrelated test flake causing CI failures at the moment.

For testing, I could see two options:

  1. Create a pod via testutil.CreatePod, adding a fake node name to the pod spec, using options.DashConfig.Objectstore to list all known pods with store.Key{Kind: "Pod", APIVersion: "v1"}, use gomock objectStore.EXPECT().List..., then check if the table generated this way filters for the node name correctly.

  2. Import testClient "k8s.io/client-go/kubernetes/fake" and populate that with the pods. Then filter the mock data for node name as it doesn't support field selectors (see Fake client doesn't filter Pods on FieldSelector kubernetes/client-go#326)

Currently inclined to say go with the first option since it can actually test if octant is filtering for the correct pods and could potentially avoid a call to the API server (although trading off performance if there are lots of pods).

@@ -325,8 +330,10 @@ func createNodeConditionsView(node *corev1.Node) (*component.Table, error) {
return table, nil
}

const GB = float64(1073741824)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will likely see usage in other places in printer especially around volumes/storage. Perhaps util is a better place for this.

@wwitzel3
Copy link
Contributor

@zparnold were there any other questions you had about this change besides the testing suggestions?

@zparnold
Copy link
Contributor Author

zparnold commented Jan 19, 2021

HI there, sorry I've really been struggling with the testing angle of this. I may need some hand holding here unfortunately. It certainly works internal to my company (we just use my copy for our clusters) but that's obviously not a test case. I don't suppose I can push the broken code I have now and have you tell me how to go in the right direction? Or even let's meet so I can see what I'm doing wrong? This type of a complex testing scenario I just have very little experience with and all my exploration of the codebase has not revealed which direction I should go.

@wwitzel3
Copy link
Contributor

@zparnold sure, we'd love to help out, are you able to join our slack channel https://kubernetes.slack.com/archives/CM37M9FCG we can coordinate there, if not, you can use https://calendly.com/wwitzel3/octant-office-hours to setup a block of time for us to sync via Zoom.

@GuessWhoSamFoo
Copy link
Contributor

GuessWhoSamFoo commented Jan 29, 2021

@zparnold Ping again on this as 0.17 is likely to be released next week and I'd to see this included. I'd also be okay with writing tests on this if needed.

@wwitzel3
Copy link
Contributor

wwitzel3 commented Feb 3, 2021

Let's go ahead with writing some tests for this. We'll make sure to keep the contribution history for Zach and add who ever does the tests as a co-author.

@GuessWhoSamFoo GuessWhoSamFoo merged commit 71adc45 into vmware-archive:master Feb 3, 2021
@zparnold
Copy link
Contributor Author

zparnold commented Feb 9, 2021

Thank you guys for being willing to do this for me. I’m sorry I kind of dropped the ball here. Feel free to attribute the code to whomever no worries. I hope to improve my Go skillz at some point, but open source contribution is sadly not yet something I can do a ton of at this time. Really appreciate your supportiveness! High five for community!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pods running on each node
3 participants