Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add machineinfo command #35

Merged
merged 1 commit into from
Oct 4, 2021
Merged

add machineinfo command #35

merged 1 commit into from
Oct 4, 2021

Conversation

ffromani
Copy link
Member

@ffromani ffromani commented Jun 4, 2021

It's very useful to get the same picture as the kubelet will.

Signed-off-by: Francesco Romani [email protected]

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 4, 2021
@openshift-ci openshift-ci bot requested review from Tal-or and yanirq June 4, 2021 12:54
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 4, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fromanirh

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2021
@ffromani ffromani force-pushed the add-machineinfo-command branch 2 times, most recently from 799ab39 to e59a23a Compare June 8, 2021 14:14
@ffromani ffromani force-pushed the add-machineinfo-command branch from e59a23a to c0ced21 Compare August 12, 2021 10:11
@ffromani ffromani changed the title WIP: add machineinfo command add machineinfo command Aug 12, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 12, 2021
@ffromani ffromani force-pushed the add-machineinfo-command branch from c0ced21 to 75aafec Compare August 12, 2021 11:12
MarSik
MarSik reviewed Oct 1, 2021
View reviewed changes
Copy link
Member

@MarSik MarSik left a comment

Choose a reason for hiding this comment

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

I am generally ok with the tool, I would just like to see a bit more verbose description of the relocatablesysfs purpose or even examples

@ffromani ffromani force-pushed the add-machineinfo-command branch from 75aafec to fef3c8a Compare October 1, 2021 15:18
yanirq
yanirq reviewed Oct 3, 2021
View reviewed changes
pkg/knit/cmd/machineinfo/machineinfo.go
},
Args: cobra.MaximumNArgs(1),
}
mInfo.Flags().BoolVarP(&opts.handle.RawOutput, "raw-output", "X", false, "include machine-identifiable data")
Copy link
Member

Choose a reason for hiding this comment

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

what is BoolVarP with the P ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://pkg.go.dev/github.com/spf13/pflag#BoolVarP

@yanirq
Copy link
Member

yanirq commented Oct 4, 2021

how do we get the list of all the available commands ? only via CLI ? or are we going to expose some sort of documentation for that as well ?

@ffromani
Copy link
Member Author

ffromani commented Oct 4, 2021

how do we get the list of all the available commands ? only via CLI ? or are we going to expose some sort of documentation for that as well ?

Yep, via CLI. The idea is to keep the CLI as living documentation

$ ./_output/knit help
knit allows to check system settings for low-latency workload

Usage:
  knit [flags]
  knit [command]

Available Commands:
  cpuaff      show cpu thread affinities
  help        Help about any command
  irqaff      show IRQ/softirq thread affinities
  irqwatch    watch IRQ counters
  lscpu       show the system CPU details
  lspci       show the system PCI details
  lstopo      show the system topology
  machineinfo show cadvisor's machine info
  podres      show currently allocated pod resources
  wait        wait forever, or until a UNIX signal (SIGINT, SIGTERM) arrives

Flags:
  -C, --cpulist string   isolated cpu set to check (see man (7) cpuset - List format (default "0-16383")
  -D, --debug            enable debug log
  -h, --help             help for knit
  -J, --json             output as JSON
  -P, --procfs string    procfs root (default "/proc")
  -S, --sysfs string     sysfs root (default "/sys")

Use "knit [command] --help" for more information about a command.

MarSik
MarSik reviewed Oct 4, 2021
View reviewed changes
pkg/machineinformer/relocatablesysfs.go Outdated
// derived from https://github.com/google/cadvisor/blob/master/utils/sysfs/sysfs.go @ ef7e64f9
// as Apache 2.0 license allows.

// relocatablesysfs allows to consume a sysfs tree whose root is not `/sys`.
Copy link
Member

Choose a reason for hiding this comment

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

Would moving this close to the struct definition allow proper golang docs to be generated (eventually or locally?)

Copy link
Member Author

Choose a reason for hiding this comment

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

we can, but we don't have a godoc reference yet. Docstrings are pretty scattered. Anyway, let me start with this.

@ffromani
command: machineinfo: add support
bae6c18 
It's very useful to get the same picture as the kubelet will.

Signed-off-by: Francesco Romani <[email protected]>
@ffromani ffromani force-pushed the add-machineinfo-command branch from fef3c8a to bae6c18 Compare October 4, 2021 08:30
@ffromani ffromani merged commit d68a039 into openshift-kni:main Oct 4, 2021
@ffromani ffromani deleted the add-machineinfo-command branch October 4, 2021 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yanirq yanirq yanirq left review comments

@MarSik MarSik MarSik left review comments

@Tal-or Tal-or Awaiting requested review from Tal-or

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ffromani @yanirq @MarSik @Tal-or