✨: Add guestinfo.hostname to config of a VM

[odvarkadaniel]

What this PR does / why we need it:
The VMs in the vCenter are currently without a DNSName (they don't have a config data at key guestinfo.hostname) while using an ignition to set the VM up. With setting this field up, we can also append an internal DNS address to the list of machine's addresses.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2013

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Set a DNSName for VMs to a corresponding machine's name.

[k8s-ci-robot]

Welcome @odvarkadaniel!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-vsphere 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-vsphere has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @odvarkadaniel. 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.

[zhanggbj]

/ok-to-test

[k8s-ci-robot]

@zhanggbj: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

[sbueringer]

/ok-to-test

[randomvariable]

I wouldn't mind some additional eyes from

/assign @akutz

[akutz]

I wouldn't mind some additional eyes from

/assign @akutz

Is this CAPV as a stand-alone management cluster?

Also, please see coreos/afterburn#888 as it will likely be useful in the future.

[akutz]

/hold

Please see #2071 (comment). Changes needed.

[odvarkadaniel]

I wouldn't mind some additional eyes from
/assign @akutz

Is this CAPV as a stand-alone management cluster?

Also, please see coreos/afterburn#888 as it will likely be useful in the future.

Do I get that right that I will need to get the necessary data from the newly introduced netplan config file which will be stored as guestinfo.metadata?

[randomvariable]

Do I get that right that I will need to get the necessary data from the newly introduced netplan config file which will be stored as guestinfo.metadata?

Yup that's correct. Given the complexity of networking on-prem environments, we decided that netplan had the required expressivity, and we'll be encouraging all bootstrap providers to adopt it. I would sync up with Flatcar counterparts in Ignition.

[odvarkadaniel]

Hi, I just came back from PTO.

I have chatted with folks from flatcar forum and I don't really understand what needs to be done here in order for this to get merged as this file exists and I suspect this is going to be plugged in as guestinfo.metadata and the field local-hostname is still going to be used to get the data necessary for this change.

As of right now we just take the metadata file from guestinfo.metadata and parse the local-hostname value out of it and assign it to guestinfo.hostname to see it on the VM in vCenter, which, as far as I understand, stays the same.

Thank you for clarifying this situation in case I am missing something.

[chrischdi]

/assign @randomvariable

for helping to clarify. Thanks!

[k8s-ci-robot]

PR needs rebase.

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.

[odvarkadaniel]

Hi, I just came back from PTO.

I have chatted with folks from flatcar forum and I don't really understand what needs to be done here in order for this to get merged as this file exists and I suspect this is going to be plugged in as guestinfo.metadata and the field local-hostname is still going to be used to get the data necessary for this change.

As of right now we just take the metadata file from guestinfo.metadata and parse the local-hostname value out of it and assign it to guestinfo.hostname to see it on the VM in vCenter, which, as far as I understand, stays the same.

Thank you for clarifying this situation in case I am missing something.

Can you please follow up on this, @randomvariable? Thank you!

[jepio]

Do I get that right that I will need to get the necessary data from the newly introduced netplan config file which will be stored as guestinfo.metadata?

Yup that's correct. Given the complexity of networking on-prem environments, we decided that netplan had the required expressivity, and we'll be encouraging all bootstrap providers to adopt it. I would sync up with Flatcar counterparts in Ignition.

So if I understand this PR and #2013 correctly - @odvarkadaniel issue is not about setting the hostname of the guest, but about exposing that value through vCenter.

Given that we are aligned on using guestinfo.metadata to pass cloud-init/netplan yaml (with the local-hostname field) on the CAPV side, will vCenter be able to handle that? As in: will vCenter parse out the hostname from that location and display it in the webui or does the guest have an interface to push it's hostname to vCenter?

[odvarkadaniel]

Do I get that right that I will need to get the necessary data from the newly introduced netplan config file which will be stored as guestinfo.metadata?

Yup that's correct. Given the complexity of networking on-prem environments, we decided that netplan had the required expressivity, and we'll be encouraging all bootstrap providers to adopt it. I would sync up with Flatcar counterparts in Ignition.

So if I understand this PR and #2013 correctly - @odvarkadaniel issue is not about setting the hostname of the guest, but about exposing that value through vCenter.

Given that we are aligned on using guestinfo.metadata to pass cloud-init/netplan yaml (with the local-hostname field) on the CAPV side, will vCenter be able to handle that? As in: will vCenter parse out the hostname from that location and display it in the webui or does the guest have an interface to push it's hostname to vCenter?

With the current config file stored in the field guestinfo.metadata, I am able to set the hostname value to vCenter's VM with the changes provided (I have used this fix multiple times to achieve the VM to have hostname set). What this PR should IMO introduce is the parsing of the config data at field local-hostname to set this value to guestinfo.hostname over at vCenter's VM.

What my question is, with all the other PRs mentioned in this one, what is actually getting changed? The netplan config will stay the same as it is now, as far as I understand, which is why I don't understand what direction this needs to be moved as the current change should fix the issue (set hostname on vCenter's VM).

[jepio]

With the current config file stored in the field guestinfo.metadata, I am able to set the hostname value to vCenter's VM with the changes provided (I have used this fix multiple times to achieve the VM to have hostname set). What this PR should IMO introduce is the parsing of the config data at field local-hostname to set this value to guestinfo.hostname over at vCenter's VM.

What my question is, with all the other PRs mentioned in this one, what is actually getting changed? The netplan config will stay the same as it is now, as far as I understand, which is why I don't understand what direction this needs to be moved as the current change should fix the issue (set hostname on vCenter's VM).

Sorry, but I can't follow. Let's forget guestinfo.hostname for a second. What usecase is currently broken for you? guestinfo.metadata.local_hostname is where CAPV places the hostname right now. Cloud-init processes it and sets the VM hostname to that value. The Flatcar template currently has a systemd service that performs that operation, and in future this will be handled by afterburn.

[odvarkadaniel]

Sorry, but I can't follow. Let's forget guestinfo.hostname for a second. What usecase is currently broken for you? guestinfo.metadata.local_hostname is where CAPV places the hostname right now. Cloud-init processes it and sets the VM hostname to that value. The Flatcar template currently has a systemd service that performs that operation, and in future this will be handled by afterburn.

Ah, totally forgot to mention that this is for Ignition, sorry. The ignition bootstrapping method currently does not set the hostname on the VM as cloud-init does. This just adds the same thing as cloud-init does, but for Ignition.

We are running into an issue, where MachineSet provisions a Machine and a new VM gets created in the vCenter, but never gets ignited due to a failure cause by empty hostname.

[jepio]

Do you have an error message/log that shows what's going on? As far as I know neither ignition, nor afterburn care about the value of `guestinfo.hostname`. Flatcar also uses ignition and in CAPV it has a systemd service defined in ignition data that sets the node hostname. Without this `kubeadm join` doesn't work, but ignition itself does. Do you use the same ignition template from this repo? Would that approach work for your case as well?

…

On Mon, Sep 25, 2023, 15:55 dodvarka ***@***.***> wrote: Sorry, but I can't follow. Let's forget guestinfo.hostname for a second. What usecase is currently broken for you? guestinfo.metadata.local_hostname is where CAPV places the hostname right now. Cloud-init processes it and sets the VM hostname to that value. The Flatcar template currently has a systemd service that performs that operation, and in future this will be handled by afterburn. Ah, totally forgot to mention that this is for Ignition, sorry. The ignition bootstrapping method currently does not set the hostname on the VM as cloud-init does. This just adds the same thing as cloud-init does, but for Ignition. We are running into an issue, where MachineSet provisions a Machine and a new VM gets created in the vCenter, but never gets ignited due to a failure cause by empty hostname. — Reply to this email directly, view it on GitHub <#2071 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABXINVXGCH46BEQAMQKKMLLX4GEN5ANCNFSM6AAAAAA2TZ4JEI> . You are receiving this because you commented.Message ID: ***@***.*** .com>

[odvarkadaniel]

Do you have an error message/log that shows what's going on? As far as I know neither ignition, nor afterburn care about the value of guestinfo.hostname. Flatcar also uses ignition and in CAPV it has a systemd service defined in ignition data that sets the node hostname. Without this kubeadm join doesn't work, but ignition itself does. Do you use the same ignition template from this repo? Would that approach work for your case as well?

I have been testing this with our openshift downstream version of CAPV (which should be identical to upstream) with our ignition config files. We do stuff a little bit different downstream, which comes to this issue, I thought it could also be useful adding this upstream (assign value to guestinfo.hostname for ignition from CAPV), so this is not a bug in here, just a feature that would help us downstream work with MachineSets.

[jepio]

Do you have a link to the downstream code that uses/depends on `guestinfo.hostname`?

…

On Mon, Sep 25, 2023, 17:36 dodvarka ***@***.***> wrote: Do you have an error message/log that shows what's going on? As far as I know neither ignition, nor afterburn care about the value of guestinfo.hostname. Flatcar also uses ignition and in CAPV it has a systemd service defined in ignition data that sets the node hostname. Without this kubeadm join doesn't work, but ignition itself does. Do you use the same ignition template from this repo? Would that approach work for your case as well? I have been testing this with our openshift downstream version of CAPV (which should be identical to upstream) with our ignition config files. We do stuff a little bit different downstream, which comes to this issue, I thought could also be useful adding upstream (assign value to guestinfo.hostname for ignition from CAPV), so this is not a bug in here, just a feature that would help us downstream work with MachineSets. — Reply to this email directly, view it on GitHub <#2071 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABXINVSMLICS7OSVUGMS2B3X4GQI3ANCNFSM6AAAAAA2TZ4JEI> . You are receiving this because you commented.Message ID: ***@***.*** .com>

[odvarkadaniel]

Sorry for confusion, it's been a long time since I worked on this.

I think that our machine-approver operator is preventing the machine from getting created as the newly created machine is currently missing an InternalDNS name (link). This piece of code is also enforcing us to have the InternalDNS name set to the machine's name, which is then also used in the ignition config file. CAPV is already setting the InternalDNS for cloud-init already, but not for ignition as far as I remember.

[jepio]

So it sounds like you really want this part of the PR merged:

+		machineAddresses = append(machineAddresses, clusterv1.MachineAddress{
+			Type:    clusterv1.MachineInternalDNS,
+			Address: ctx.Machine.Name,
+		})

[odvarkadaniel]

So it sounds like you really want this part of the PR merged:

+		machineAddresses = append(machineAddresses, clusterv1.MachineAddress{
+			Type:    clusterv1.MachineInternalDNS,
+			Address: ctx.Machine.Name,
+		})

Yes, that seems to fix this issue for us. Didn't realise at first that upstream's ignition template already has a service that sets the hostname on the VM.

[jepio]

I think that part should be OK to merge. Would you agree @akutz ?

@odvarkadaniel : in the meantime, are you able to help or find someone at Red Hat who could take care of the points blocking coreos/afterburn#888 (comment) from getting merged? Those are: reviews and packaging dependencies in Fedora. Then we can build on that PR to also handle the local-hostname field.

[odvarkadaniel]

I think that part should be OK to merge. Would you agree @akutz ?

@odvarkadaniel : in the meantime, are you able to help or find someone at Red Hat who could take care of the points blocking coreos/afterburn#888 (comment) from getting merged? Those are: reviews and packaging dependencies in Fedora. Then we can build on that PR to also handle the local-hostname field.

We are discussing this in the original issue linked to this PR.

Of course, I will ask about the coreos/afterburner PR.

[odvarkadaniel]

@jepio, Just to update you on the afterburner PR - I talked to folks from coreos team and they are currently not working on it nor on their radar as this is not a priority for them currently.

[jepio]

@jepio, Just to update you on the afterburner PR - I talked to folks from coreos team and they are currently not working on it nor on their radar as this is not a priority for them currently.

I see. That afterburn PR would help CAPV cover more scenarios, which would also benefit Openshift.

[k8s-ci-robot]

[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 ask for approval from randomvariable. For more information see the Kubernetes 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:

• OWNERS

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]

@odvarkadaniel: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-vsphere-e2e-main-oldpreset	f403d45	link	true	/test pull-cluster-api-provider-vsphere-e2e-main-oldpreset
pull-cluster-api-provider-vsphere-test-integration-main	fef5f54	link	true	/test pull-cluster-api-provider-vsphere-test-integration-main
pull-cluster-api-provider-vsphere-e2e-main	fef5f54	link	true	/test pull-cluster-api-provider-vsphere-e2e-main
pull-cluster-api-provider-vsphere-test-main	fef5f54	link	true	/test pull-cluster-api-provider-vsphere-test-main
pull-cluster-api-provider-vsphere-verify-main	fef5f54	link	true	/test pull-cluster-api-provider-vsphere-verify-main
pull-cluster-api-provider-vsphere-apidiff-main	fef5f54	link	false	/test pull-cluster-api-provider-vsphere-apidiff-main

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.