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

Split write network and hostname configuration #4870

Closed

Conversation

temaps
Copy link

@temaps temaps commented Jun 29, 2023

When the parameter can_configure_network is disabled in the configuration, the file /etc/hostname is not written to the installed system.

@github-actions github-actions bot added the f39 label Jun 29, 2023
@mikhailnov
Copy link
Contributor

A bit of context why this change was made. A derivative of ROSA - MOS (ROSA-based distro for schools) - sets can_configure_network=False by default, because every teacher/student in Moscow schools has a personal password for the local WiFi network. Someone's password was made visible to every user by can_configure_network=True.

But we in ROSA generate random hostnames on start of the LiveCD (https://abf.io/import/anaconda/blob/rosa2023.1/anaconda-livecd-init.sh) and then inherit them into the installed system (see also: https://abf.io/import/anaconda/blob/6b6591efa3/0046-Copy-current-hostname-into-installed-system.patch , I think this could also be upstreamized, what do you think?). But without the changes in the PR both a randomly and manually (in the network spoke) set hostname was not copied into the installed system.

Setting random hostnames helps to identify machines inside network.

@VladimirSlavik
Copy link
Contributor

@rvykydal Radku, can you please take a look at this?

@rvykydal rvykydal added the manual testing required This issue can't be merged without manual testing label Jul 13, 2023
Copy link
Contributor

@rvykydal rvykydal left a comment

Choose a reason for hiding this comment

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

I think the split of hostname configuration makes sense.

In the PR can_configure_network still implies can_configure_hostname which seems reasonable and should prevent us from possible breaking of some current use cases.
But I'd prefer to split the hostname configuration from write_configuration completely and call it separately after "Network confgiration" task (if can_configure_network or can_configure_hostname) so we are more clear and explicit in the tasks and logs in what is happening.

)
hostname_config.append(Task(
"Hostname configuration",
network.network.write_configuration_hostname,
Copy link
Contributor

Choose a reason for hiding this comment

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

Anaconda crashes here, there should be network.write_configuration_hostname

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

That's exactly what was done. First, the operation is divided into two functions, and then they are called in turn depending on can_change. Moreover, the formulation of the name configuration task comes just after the network configuration task.

Copy link
Author

Choose a reason for hiding this comment

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

Anaconda crashes here, there should be network.write_configuration_hostname

fixed typo :-)

@mikhailnov
Copy link
Contributor

But I'd prefer to split the hostname configuration from write_configuration completely and call it separately after "Network confgiration" task (if can_configure_network or can_configure_hostname) so we are more clear and explicit in the tasks and logs in what is happening

+1 , code flow is not obvious otherwise

@rvykydal
Copy link
Contributor

But we in ROSA generate random hostnames on start of the LiveCD (https://abf.io/import/anaconda/blob/rosa2023.1/anaconda-livecd-init.sh) and then inherit them into the installed system (see also: https://abf.io/import/anaconda/blob/6b6591efa3/0046-Copy-current-hostname-into-installed-system.patch , I think this could also be upstreamized, what do you think?).

I am afraid making this default behavior would be in conflict with #643 where we stopped using the hostname from installer environment for target system.

@mikhailnov
Copy link
Contributor

But we in ROSA generate random hostnames on start of the LiveCD (https://abf.io/import/anaconda/blob/rosa2023.1/anaconda-livecd-init.sh) and then inherit them into the installed system (see also: https://abf.io/import/anaconda/blob/6b6591efa3/0046-Copy-current-hostname-into-installed-system.patch , I think this could also be upstreamized, what do you think?).

I am afraid making this default behavior would be in conflict with #643 where we stopped using the hostname from installer environment for target system.

Hm... Commit message there says: "Doing so, the installed system would not use new transient hostname if it boots in different environment". But the current behaviour is to set the same static hostname, but not skip setting it, isn't it?

@mikhailnov
Copy link
Contributor

The script generates a random hostname only if another hostname, including a transient one via DHCP, was not set:
https://abf.io/import/anaconda/blob/d94b36a3eb/anaconda-livecd-init.sh#lc-21

When the parameter can_configure_network is disabled in the configuration,
the file /etc/hostname is not written to the installed system.
@temaps temaps force-pushed the write-hostname-configuration branch from 7bc60ac to 432153c Compare August 23, 2023 13:54
@github-actions
Copy link

This PR is stale because it has been open 60 days with no activity.
Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the stale label Oct 23, 2023
@rvykydal rvykydal removed the stale label Nov 13, 2023
@temaps
Copy link
Author

temaps commented Dec 13, 2023

The tests are confused by the name "hostname" (write-hostname-configuration), there don't seem to be any more problems. Is this an obstacle?

Copy link

This PR is stale because it has been open 60 days with no activity.
Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the stale label Feb 12, 2024
Copy link

This PR was closed because it has been stalled for 30 days with no activity.

@github-actions github-actions bot closed this Mar 13, 2024
@temaps
Copy link
Author

temaps commented Mar 13, 2024

manual testing required...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f39 manual testing required This issue can't be merged without manual testing stale
Development

Successfully merging this pull request may close these issues.

4 participants