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.

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

Set Flatcar OS_TYPE as other4xLinux64Guest #1337

Merged
merged 2 commits into from
Nov 11, 2023

Conversation

erkanerol
Copy link
Contributor

@erkanerol erkanerol commented Nov 6, 2023

vSphere gives an error while importing OVA file because of the template. I started a thread in the Kubernetes' Slack about this issue https://kubernetes.slack.com/archives/C01E0Q35A8J/p1698754064351559

My understanding is the value should be other3xLinux64Guest. I changed the OVF file of the image manually and tested importing. It worked. The value in upstream Flatcar repo is also other3xLinux64Guest. See https://github.com/flatcar/scripts/blob/main/build_library/template_vmware.ovf#L138

See the discussions below.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 6, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @erkanerol. 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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 6, 2023
@AverageMarcus
Copy link
Member

/ok-to-test
/lgtm

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 6, 2023
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2023
@mboersma
Copy link
Contributor

mboersma commented Nov 6, 2023

Makes sense to me. @erkanerol this is still a draft PR; if you want to mark it "Ready for Review" I think we can merge it.

cc: @invidian @jepio @johananl

@jepio
Copy link
Contributor

jepio commented Nov 6, 2023 via email

@ohauer
Copy link

ohauer commented Nov 7, 2023

woudn't it be better to use other4xLinux64Guest instead other3xLinux64Guest in this case?

Enum - VirtualMachineGuestOsIdentifier

@jsturtevant
Copy link
Contributor

woudn't it be better to use other4xLinux64Guest instead other3xLinux64Guest in this case?

Enum - VirtualMachineGuestOsIdentifier

could you explain why?

@ohauer
Copy link

ohauer commented Nov 7, 2023

woudn't it be better to use other4xLinux64Guest instead other3xLinux64Guest in this case?
Enum - VirtualMachineGuestOsIdentifier

could you explain why?

Sure, as far as I remember other4xLinux64Guest was added to vSphere 6.5/6.7 and I suspect there are still some vSphere 6.7 in production (even if 6.7 is EoS) because the HCL for vSphere 7/8 excludes many common systems used in smaller shops

The 4 in other4xLinux64Guest stands for the kernel version used in the VM so for vSphere 7/8 it would be even more sense to change it to other5xLinux64Guest since flatcar stable and LTS using kernel version 5

I haven't found a more current table on vsphere documentation but this in govc
https://github.com/vmware/govmomi/blob/main/vim25/types/enum.go#L8607-L8610

@erkanerol
Copy link
Contributor Author

Now, we have a better understanding of this value. Thanks a lot @ohauer. As you said, the kernel version in the latest Flatcar is 5.x so the value should be other5xLinux64Guest.

There is a minimum version map in the referred code

minAPIVersionForEnumValue["VirtualMachineGuestOsIdentifier"] = map[string]string{
...
"other3xLinux64Guest":        "5.5",
"other4xLinux64Guest":        "6.7",
"other5xLinux64Guest":        "7.0.1.0",
"other6xLinux64Guest":        "8.0.0.1",
...

I tested other5xLinux64Guest in my vSphere environment (7.0.3) and it worked too.

I don't know if other5xLinux64Guest works for vSphere 6.7 and if we really need to care 6.7 even though it is not supported officially anymore.

For the sake of supporting 6.7, we can go with other4xLinux64Guest even though the kernel version is 5.x in the newest Flatcar images but I don't know what will be the negative side effect of this mismatch.

@ohauer
Copy link

ohauer commented Nov 8, 2023

The usage of the otherNxLinuxGuest can affect the data that is provided from the vmware-tools inside the VM to the host system.
I found an older interesting article on the VMware site KB71306 that shows a side effect of setting the otherNxLinuxGuest to a higher version without appropriate VMware tools installed.
So setting this to a lower version has less impact then setting it to a higher version and not havving the latest vmware-tools installed

@erkanerol
Copy link
Contributor Author

@ohauer Then I think the best value is other4xLinux64Guest. What do you think?

@ohauer
Copy link

ohauer commented Nov 8, 2023

@ohauer Then I think the best value is other4xLinux64Guest. What do you think?
yes, for the current time it looks like the best option / compromise

@akutz
Copy link
Contributor

akutz commented Nov 8, 2023

Hi @ravindravmw,

Can you possibly comment on the Tools compatibility (#1337 (comment)) regarding the guest OS ID used to deploy a VM?

@akutz
Copy link
Contributor

akutz commented Nov 8, 2023

cc @jsavanyo4vmw as well for any help you may be able to provide. John, FWIW, I'm pinging a few folks internally on the tools and VMX teams who may have more info as well.

@akutz
Copy link
Contributor

akutz commented Nov 8, 2023

@erkanerol / @ohauer,

The following is from one of the engineers on the Tools team:

Some background: there are two GuestOS identifiers: what the VM creator specifies, and what the guest reports. In a perfect world these match. The complexity is that new values traditionally need a newer vSphere to have the new guestOS constants in the API (that's the root issue with the KB linked in the thread). Sine these are API constants, that means if some new Linux distro comes out, unless its a Big Name its got to lie if it wants everything to look right in the API/UI. And even if its a Big Name, if the version came out after the vSphere, there won't be a constant for it.

There's all sorts of minor tweaks in the host side code based on these values. The VMX and UI will tune stuff (I'm vague on details, but things like default configs when creating the VM, devices, etc). We've also got some minor, subtle stuff in guestOps -- if it says its Windows but its not, weird problems happen in filenames (backslash conversion) and certain ops may not work.

We added some extra config options and details to handle the random distro scenario. William sums it up here: https://williamlam.com/2023/11/new-detailed-guestos-data-in-vsphere-8-0-update-2.html

I'm still digging into the specific question, but hopefully the above helps somewhat.

@akutz
Copy link
Contributor

akutz commented Nov 8, 2023

One more bit of info from the same engineer:

Here the tools.conf commentary on how to change what Tools reports.
https://github.com/vmware/open-vm-tools/blob/master/open-vm-tools/tools.conf#L357
Its a bit terse on what those values mean though.

This would enable the image to configure Tools to override what was injected when the VM was deployed.

@ohauer
Copy link

ohauer commented Nov 8, 2023

@akutz
Thanks for the research!
The ability to set the guest operating system in tools.conf opens up a new possibility to adapt this depending on the environment.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2023
@erkanerol erkanerol changed the title Set Flatcar OS_TYPE as other3xLinux64Guest Set Flatcar OS_TYPE as ~other3xLinux64Guest~ other4xLinux64Guest Nov 9, 2023
@erkanerol erkanerol changed the title Set Flatcar OS_TYPE as ~other3xLinux64Guest~ other4xLinux64Guest Set Flatcar OS_TYPE as other4xLinux64Guest Nov 9, 2023
@erkanerol
Copy link
Contributor Author

erkanerol commented Nov 9, 2023

In light of the latest information shared in the PR, I updated the value as other4xLinux64Guest to stay on the safe side.

I created another issue to improve the whole mechanism #1339

Thank you all!

@erkanerol erkanerol marked this pull request as ready for review November 9, 2023 09:49
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2023
Copy link
Member

@AverageMarcus AverageMarcus left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Thank you so much everyone for the detailed investigation and summary. I've certainly learnt something from it all! 🙂

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AverageMarcus

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 11, 2023
@k8s-ci-robot k8s-ci-robot merged commit 92a344d into kubernetes-sigs:main Nov 11, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants