Skip to content

extend vmware-tools/tools.conf #1738

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ohauer
Copy link
Contributor

@ohauer ohauer commented May 10, 2025

Change description

  • disable CoreDump
  • set maxOldLogFiles
  • set maxLogSize
  • add primary-nics filter
  • extend exclude-nics man (7) systemd.net-naming-scheme
  • set max-ipv4-routes, max-ipv6-routes

Additional context

tested with open-vm-tools 12.3.5
check before and after with

govc vm.info -r -json <vmName> | jq '(.virtualMachines[].guest.ipStack[].ipRouteConfig)'

- disable CoreDump
- set maxOldLogFiles
- set maxLogSize
- add primary-nics filter
- extend exclude-nics
  man (7) systemd.net-naming-scheme
- set max-ipv4-routes, max-ipv6-routes
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 10, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign averagemarcus for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@k8s-ci-robot
Copy link
Contributor

Hi @ohauer. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 10, 2025
@AverageMarcus
Copy link
Member

@kkeshavamurthy are you able to take a look at this PR for me? 🙏 I don't have enough VMWare knowledge to be any use here.

Comment on lines +1 to +20
[logging]
## Disables core dumps on fatal errors; they're enabled by default.
enableCoreDump = false
## Setup file rotation - keep 3 files
vmsvc.maxOldLogFiles = 3
## Max log file size kept: 1 MB
vmsvc.maxLogSize = 1

[guestinfo]
exclude-nics=antrea-*,cali*,cilium*,lxc*,ovs-system,br*,flannel*,veth*,vxlan_sys_*,genev_sys_*,gre_sys_*,stt_sys_*,????????-??????
## This configuration ensures that any address in the interfaces matching en* is sorted on top of the list of IP addresses.
primary-nics=en*

## The configuration will exclude all interfaces with the names matching the patterns specified from GuestInfo.
exclude-nics=antrea-*,br*,cali*,cbr*,cilium*,cni*,docker*,dummy*,flannel*,genev_sys_*,gre_sys_*,kube-ipvs*,lo,lxc*,nodelocaldns*,ovs-system,stt_sys_*,tunl*,veth*,virbr*,vxlan_sys_*,wg*,????????-??????

## https://github.com/vmware/open-vm-tools/commit/065f09b94e09f1127901db29e73cc9b9f36df4fc
# max-ipv4-routes: Max IPv4 routes to gather. Set 0 to disable gathering.
# max-ipv6-routes: Max IPv6 routes to gather. Set 0 to disable gathering.
max-ipv4-routes=0
max-ipv6-routes=0
Copy link
Member

Choose a reason for hiding this comment

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

Do you know what the default values are for these variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as i can see in the source: (https://github.com/vmware/open-vm-tools.git)

  • enableCoreDump = true (-> lib/include/vmware/tools/log.h)

  • maxOldLogFiles = 10 ( -> lib/include/vmware/tools/log.h, scripts/linux/network for each of vmware-network, vmware-vmsvc, and vmware-vmtoolsd logs)

  • primary-nics = (empty)

    • from man (7) systemd.net-naming-scheme
      Prefix | Description
      en | Ethernet
      ib | InfiniBand
      sl | Serial line IP (slip)
      wl | Wireless local area network (WLAN)

Perhaps InfiniBand is also used with pci passthrough but I can't imagine something like this is practical on vSphere with many k8s nodes

  • exclude-nics = empty (I extended it with nics I found on our systems)

  • max-ipv4-routes = no restriction

  • max-ipv6-routes = no restriction

    • however only the first 100 IPv4 + IPv6 will be placed into the VM metadata, We had some issued with high CPU without this setting in the past

Maybe someone from tthe capv devs (VMware / Broadcom) has better insights to tune this settings ...

Copy link
Member

Choose a reason for hiding this comment

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

It feels like
enableCoreDump = true (-> lib/include/vmware/tools/log.h)
could be set to default and you can toggle it in your builds.

The ones regarding max-ipv4/6-routes, I'm not sure what the implications of this would be. I do not know if NSX-T is looking for this information in some way. I understand that when large number of routes are present it causes high cpu issues. But what about the other cases when it can handle it.

After reading this

Added two switches "max-ipv4-routes" and "max-ipv6-routes"
(NICINFO_MAX_ROUTES by default) in tools.conf and let SlashProcNet_GetRoute*()
only read the first max ipv4/6 routes lines of /proc/net/[ipv6_]route to avoid
performance problem. 

I'm understanding that if it was set to say 100, then the cpu problem since they are now only reading the first 100 lines. Maybe that can be a solution instead of turning it off
What do you think?

Regarding other variable, I think the changes look good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It feels like
enableCoreDump = true (-> lib/include/vmware/tools/log.h)
could be set to default and you can toggle it in your builds.

do you think here about a jinja2 template, with an additional var?

The ones regarding max-ipv4/6-routes, I'm not sure what the implications of this would be. I do not know if NSX-T is looking for this information in some way. I understand that when large number of routes are present it causes high cpu issues. But what about the other cases when it can handle it.

Hm, sure I haven't thought about NSX-T

Copy link
Member

Choose a reason for hiding this comment

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

do you think here about a jinja2 template, with an additional var?
Yeah that would be the way.

@k8s-ci-robot
Copy link
Contributor

Adding label do-not-merge/contains-merge-commits because PR contains merge commits, which are not allowed in this repository.
Use git rebase to reapply your commits on top of the target branch. Detailed instructions for doing so can be found here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@sriramandev
Copy link
Contributor

Can you please provide a little more information about what exactly we are trying to achieve here by setting these values. Basically why we need this?

@@ -1,3 +1,21 @@
[logging]
## Disables core dumps on fatal errors; they're enabled by default.
enableCoreDump = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Would suggest to keep the default rather than override this. If you need customizations, would suggest to do that in local CI environments rather than setting here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/contains-merge-commits needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants