Bug 2041183: Configure hostnames via ignition #35

sadasu · 2022-01-26T15:56:36Z

No description provided.

sadasu · 2022-01-26T16:00:42Z

/cc @andfasano

openshift-ci · 2022-01-26T16:02:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sadasu

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:

~~OWNERS~~ [sadasu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2022-01-26T16:02:25Z

@sadasu: An error was encountered querying GitHub for users with public email (yporagpa@redhat.com) for bug 2041183 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details.

Full error message.


non-200 OK status code: 403 Forbidden body: "{\n  \"documentation_url\": \"https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits\",\n  \"message\": \"You have exceeded a secondary rate limit. Please wait a few minutes before you try again.\"\n}\n"

Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

Bug 2041183: Configure hostnames via ignition

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.

sadasu · 2022-01-26T16:41:14Z

/cc @zaneb

zaneb · 2022-01-26T18:07:12Z

This looks plausible to me.

@cybertron I was thinking that we probably want this default hostname to take precedence over reverse DNS, to avoid problems with getting the hostname from the VIP. I suspect this change will have that effect, but we should check. WDYT?

Also need to check that this does not override DHCP if a hostname is provided that way. If it does we may need an additional NM dispatcher script.

cybertron · 2022-01-26T18:19:52Z

Agreed, I think we need this or something like it. I found the previous bug about this[0], but unfortunately that only helps with ipv6. The ipv4 vips don't show up as deprecated so there's no obvious way to differentiate them.

If I pull this down, will it work with the usual dev-scripts image override mechanism?

0: https://bugzilla.redhat.com/show_bug.cgi?id=1820770

cybertron · 2022-01-26T22:10:24Z

Testing locally with DHCP, I see in the logs:

policy: set-hostname: set hostname to 'master-0' (from DHCPv4)

Although it's worth noting that the hostname was already set at that point. Is that expected if I'm not passing nmstate config? I'm also not sure if DHCP would override the static hostname (assuming that's where it came from originally) since they match in this case. Maybe if I hack the names in install-config?

zaneb · 2022-01-26T22:13:26Z

Looks like 2 problems with this:

We're only setting the hostname in the IPA image. --copy-network doesn't result in /etc/hostname being copied to disk on installation.
The static hostname we're setting is taking precendence over DHCP.

Good news: If the solution to 2. were to set it in a NM dispatcher script, that would solve 1. as well.

zaneb · 2022-01-27T03:22:45Z

pkg/ignition/builder.go

+	config.Storage.Files = append(config.Storage.Files, ignitionFileEmbed(
+		"/etc/NetworkManager/dispatcher.d/02-hostname",
+		0744, false,
+		[]byte("hostnamectl set-hostname --static --transient b.hostname")))


I don't think we can set --static, that seems to take precedence over DHCP (it updates /etc/hostname).

From what I can gather we want to do set-hostname --transient whenever the current hostname is localhost.

I suggest we consolidate this into the 01-hostname script,

Updated the 01-hostname script. Not sure if this is what we want the order of the 2 operations to be.

sadasu · 2022-01-27T04:17:39Z

/test e2e-metal-ipi-serial-ipv6

openshift-ci · 2022-01-27T04:17:40Z

@sadasu: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test e2e-metal-ipi
/test e2e-metal-ipi-serial-ipv4
/test images
/test lint
/test unit

Use /test all to run all jobs.

In response to this:

/test e2e-metal-ipi-serial-ipv6

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.

dtantsur · 2022-01-27T11:09:45Z

/bugzilla refresh

openshift-ci · 2022-01-27T11:09:49Z

@dtantsur: This pull request references Bugzilla bug 2041183, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (yporagpa@redhat.com), skipping review request.

In response to this:

/bugzilla refresh

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.

dtantsur · 2022-01-27T11:11:44Z

pkg/ignition/builder.go

 		"/etc/NetworkManager/dispatcher.d/01-hostname",
 		0744, false,
-		[]byte("[[ \"$DHCP6_FQDN_FQDN\" =~ \".\" ]] && hostnamectl set-hostname --static --transient $DHCP6_FQDN_FQDN")))
+		[]byte("[[ \"$DHCP6_FQDN_FQDN\" =~ \".\" ]] && hostnamectl set-hostname --static --transient $DHCP6_FQDN_FQDN; [[ $HOSTNAME = localhost ]] && hostnamectl set-hostname --transient b.hostname")))


Just for my education: what is b.hostname here? How does it work?

During the installer bootstrap a bunch of files are created containing the nmstate configuration defined in the install-config.yaml file (per host) (see here) and then passed to the icc. These file are named as the hostname specified in the config. Here each filename is extracted and passed in this ignitionBuilder struct as a field, to be used as hostname).

So, it will execute hostnamectl set-hostname --transient b.hostname right? won't it set the hostnames of all nodes to literal string b.hostname?

Also I guess hostnamectl is different in coreos? mine has hostnamectl hostname, at least according to the man page.

I think we should be sprintfing the variable b.hostname in here, not setting the hostname to the literal string "b.hostname" though, right?

yeah, so

fmt.Sprintf("[[ \"$DHCP6_FQDN_FQDN\" =~ \".\" ]] && hostnamectl set-hostname --static --transient $DHCP6_FQDN_FQDN; [[ $HOSTNAME = localhost ]] && hostnamectl set-hostname --transient %s", b.hostname)

dtantsur · 2022-01-27T15:40:27Z

pkg/ignition/builder.go

 		"/etc/NetworkManager/dispatcher.d/01-hostname",
 		0744, false,
-		[]byte("[[ \"$DHCP6_FQDN_FQDN\" =~ \".\" ]] && hostnamectl set-hostname --static --transient $DHCP6_FQDN_FQDN")))
+		[]byte("[[ \"$DHCP6_FQDN_FQDN\" =~ \".\" ]] && hostnamectl set-hostname --static --transient $DHCP6_FQDN_FQDN; [[ $HOSTNAME = localhost ]] && hostnamectl set-hostname --transient $b.hostname")))


should be

[]byte(fmt.Sprintf("[[ \"$DHCP6_FQDN_FQDN\" =~ \".\" ]] && hostnamectl set-hostname --static --transient $DHCP6_FQDN_FQDN; [[ $HOSTNAME = localhost ]] && hostnamectl set-hostname --transient %s", b.hostname))

Fixed. I hope.

zaneb · 2022-01-27T16:06:46Z

pkg/ignition/builder.go

 		"/etc/NetworkManager/dispatcher.d/01-hostname",
 		0744, false,
-		[]byte("[[ \"$DHCP6_FQDN_FQDN\" =~ \".\" ]] && hostnamectl set-hostname --static --transient $DHCP6_FQDN_FQDN")))
+		[]byte(fmt.Sprintf("[[ \"$DHCP6_FQDN_FQDN\" =~ \".\" ]] && hostnamectl set-hostname --static --transient $DHCP6_FQDN_FQDN; [[ $HOSTNAME = localhost ]] && hostnamectl set-hostname --transient %s", b.hostname))))


I don't see any evidence that the HOSTNAME environment variable is defined for us?

I think we also need to take into account all of the possible variations of localhost that I linked earlier: https://github.com/systemd/systemd/blob/main/src/basic/hostname-util.c#L183-L192
I think [[ \"$(hostnamectl --transient)\" =~ (^|\\.)localhost(\\.localdomain)?\\.?$ ]] works.

Ordering I think doesn't really matter, since transient never overrides static. Separating commands with a newline rather than a ; might make the script itself easier to read - it might pay to find a better way to lay out this code as well (using ` strings instead of " maybe?).

node-valid-hostname checks the hostname this way: https://github.com/openshift/machine-config-operator/blob/47436bdb7b8c49425d6813abca594485171e1221/templates/common/_base/files/usr-local-bin-mco-hostname.yaml#L17 It probably makes sense to do it the same way here for consistency.

Thanks for both comments!

zaneb

/lgtm

zaneb · 2022-01-27T18:42:38Z

pkg/ignition/builder.go

 		[]byte("[connection]\nipv6.dhcp-duid=ll\nipv6.dhcp-iaid=mac")))
+
+	update_hostname := fmt.Sprintf(`
+	[[ \"$DHCP6_FQDN_FQDN\" =~ \".\" ]] && hostnamectl set-hostname --static --transient $DHCP6_FQDN_FQDN 


nit: I assume the escapes on the double quotes are still OK, but no longer necessary.

yeah, I didn't want to touch the existing DHCP6 FQDN based update to the hostname.

Right, but the \ was for golang because it was inside a double-quoted string, but now it's inside a backtick string there's no need for escaping afair.

sadasu · 2022-01-28T03:05:53Z

/retest

sadasu · 2022-01-28T14:00:56Z

/retest

dtantsur · 2022-01-28T14:12:20Z

pkg/ignition/builder.go

+
+	update_hostname := fmt.Sprintf(`
+	[[ "$DHCP6_FQDN_FQDN" =~ "." ]] && hostnamectl set-hostname --static --transient $DHCP6_FQDN_FQDN 
+	[[ "$(< /proc/sys/kernel/hostname)" =~ (localhost|localhost.localdomain) ]] && hostnamectl set-hostname --transient %s`, b.hostname)


nit: it's enough to do =~ localhost

True, but they both incorrectly match all kinds of rubbish (like test.definitely-not-a-localhost-domain.example.com (the correct regex is (^|\\.)localhost(\\.localdomain)?\\.?$), so there's no point doing it wrong and differently to https://github.com/openshift/machine-config-operator/blob/47436bdb7b8c49425d6813abca594485171e1221/templates/common/_base/files/usr-local-bin-mco-hostname.yaml#L17.

dtantsur · 2022-01-28T14:15:03Z

/lgtm

openshift-ci · 2022-01-28T17:12:57Z

@sadasu: all tests passed!

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.

openshift-ci · 2022-01-28T17:14:15Z

@sadasu: All pull requests linked via external trackers have merged:

openshift/image-customization-controller#35

Bugzilla bug 2041183 has been moved to the MODIFIED state.

In response to this:

Bug 2041183: Configure hostnames via ignition

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.

sadasu · 2022-01-31T15:36:15Z

/cherry-pick release-4.10

openshift-cherrypick-robot · 2022-01-31T15:36:54Z

@sadasu: new pull request created: #37

In response to this:

/cherry-pick release-4.10

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.

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 26, 2022

openshift-ci bot requested a review from andfasano January 26, 2022 16:00

sadasu marked this pull request as draft January 26, 2022 16:00

sadasu changed the title ~~WIP: Configure hostnames via ignition~~ Bug 2041183: Configure hostnames via ignition Jan 26, 2022

openshift-ci bot requested a review from hardys January 26, 2022 16:02

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2022

sadasu force-pushed the configure-hostnames branch from 24c57ff to b5d86d0 Compare January 26, 2022 16:32

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2022

openshift-ci bot requested a review from zaneb January 26, 2022 16:42

sadasu force-pushed the configure-hostnames branch from b5d86d0 to 2b96066 Compare January 26, 2022 17:40

sadasu marked this pull request as ready for review January 26, 2022 18:17

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 Jan 26, 2022

sadasu force-pushed the configure-hostnames branch from 2b96066 to 5f18c09 Compare January 27, 2022 03:15

zaneb reviewed Jan 27, 2022

View reviewed changes

sadasu force-pushed the configure-hostnames branch from 5f18c09 to ba72ac0 Compare January 27, 2022 04:15

openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jan 27, 2022

dtantsur reviewed Jan 27, 2022

View reviewed changes

sadasu force-pushed the configure-hostnames branch from ba72ac0 to 139dda1 Compare January 27, 2022 15:38

dtantsur reviewed Jan 27, 2022

View reviewed changes

sadasu force-pushed the configure-hostnames branch from 139dda1 to df54613 Compare January 27, 2022 15:44

zaneb reviewed Jan 27, 2022

View reviewed changes

sadasu force-pushed the configure-hostnames branch from df54613 to 5a0b71d Compare January 27, 2022 17:20

zaneb reviewed Jan 27, 2022

View reviewed changes

openshift-ci bot assigned zaneb Jan 27, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2022

Configure hostnames via ignition

00e7971

sadasu force-pushed the configure-hostnames branch from 5a0b71d to 00e7971 Compare January 27, 2022 22:50

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2022

dtantsur reviewed Jan 28, 2022

View reviewed changes

openshift-ci bot assigned dtantsur Jan 28, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2022

openshift-merge-robot merged commit 958472a into openshift:main Jan 28, 2022

openshift-cherrypick-robot mentioned this pull request Jan 31, 2022

[release-4.10] Bug 2048609: Configure hostnames via ignition #37

Closed

Bug 2041183: Configure hostnames via ignition #35

Bug 2041183: Configure hostnames via ignition #35

Uh oh!

Conversation

sadasu commented Jan 26, 2022

Uh oh!

sadasu commented Jan 26, 2022

Uh oh!

openshift-ci bot commented Jan 26, 2022

Uh oh!

openshift-ci bot commented Jan 26, 2022

Uh oh!

sadasu commented Jan 26, 2022

Uh oh!

zaneb commented Jan 26, 2022

Uh oh!

cybertron commented Jan 26, 2022

Uh oh!

cybertron commented Jan 26, 2022

Uh oh!

zaneb commented Jan 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadasu commented Jan 27, 2022

Uh oh!

openshift-ci bot commented Jan 27, 2022

Uh oh!

dtantsur commented Jan 27, 2022

Uh oh!

openshift-ci bot commented Jan 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadasu Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zaneb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadasu Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadasu commented Jan 28, 2022

Uh oh!

sadasu commented Jan 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtantsur commented Jan 28, 2022

Uh oh!

openshift-ci bot commented Jan 28, 2022

Uh oh!

openshift-ci bot commented Jan 28, 2022

sadasu Jan 27, 2022 •

edited

Loading

sadasu Jan 27, 2022 •

edited

Loading