-
Notifications
You must be signed in to change notification settings - Fork 401
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
kubevirt: Set transient hostname from metadata #4005
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qinqon 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 |
At KubeVirt the hostname is usually set using DHCP on the case of fcos/rhcos but at some use cases this is not the case and at least the transient hostname should be set from the metadata. This change runs the afterburn service that read metadata and dump and env file that can be used later on with hostnamectl to set the transient hostname. Signed-off-by: Enrique Llorente <ellorent@redhat.com>
b874791
to
05bd19b
Compare
I'm a bit confused by this. The VMI schema has an explicit I guess what I'm asking is:
|
I think it may be don on purpose since doing so whould not allow users with secondary networks to override the hostname.
At ocp hypershift kubevirt is the only want that uses the rhel image with kubevirt provider, and there the current behaviour is working for us, so doing the this change would "alter" that since we are not setting it using DHCP.
We can skip this PR and address it if we use there are problems u/s, kubevirt support at fcos/rhcos is quite new. |
Let's dig into this more. Why should DHCP from a secondary network have precedence over If a user wants the hostname set by DHCP, can't they just not set |
In the case of hypershift kubevirt, the VM is not created by user so there is no control over spec.hostname on that case the user can have their own DHCP server deliver the hostname per deliver IP. |
Thanks, this is making more sense now. :) So this is specific to HyperShift + KubeVirt. (Or I guess a cluster where users don't have ACLs to create VMIs directly.) What's the UX through which the user requests the VM to be created? |
So on default scenario user create a cluster and that's all, user don't really care of VM hostname normally. And that hostname is the vm generated name passed over DHCP by an internal dhcp server at each of them, that's how kubevirt works by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think it's a bug that Afterburn isn't currently setting the hostname on KubeVirt. So I think we do want coreos/afterburn#1005, but yes, it'll require some design work to ensure we still get the right behaviour in the HyperShift + KubeVirt + Multus case. So fine to defer for now.
enabled: true | ||
contents: | | ||
[Unit] | ||
Description=Set Transient Hostname from KubeVirt meta_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this have
ConditionPathExists=!/etc/hostname
?
We don't want to set a hostname if one has already been set (e.g. via Ignition).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well not sure, it depends on what the we want do at the end, maybe we want to convert the static into transient hostname.
|
||
[Service] | ||
# Mark afterburn environment file optional, due to it is possible that afterburn service was not executed | ||
EnvironmentFile=/run/metadata/afterburn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says to mark as optional, but this is missing the optional marker.
Also if it's truly missing, then we'd be setting the transient hostname to the empty string, which is not what we want. Likely instead we should add ConditionPathExists=/run/metadata/afterburn
to the unit?
@qinqon: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
/hold Looks like /etc/hostname is also created, maybe a byproduct of calling the afterburn service. System reports that hostname is transiently being set
Looks like mco is storing it as static so it cannot be overriden, so our scenario where user sets the hostname with DHCP server is not compatible with seting it transient from metadata.
|
"Closes: #xxxx"
- What I did
At KubeVirt the hostname is usually set using DHCP on the case of fcos/rhcos but at some use cases this is not the case and at least the transient hostname should be set from the metadata. This change runs the afterburn service that read metadata and dump and env file that can be used later on with hostnamectl to set the transient hostname.
Closes: https://issues.redhat.com/browse/CNV-33668
journal of unit running at an hypershift kubevirt node before and after restart
- Description for the changelog
kubevirt: Set transient hostname from metadata