Hostname in installer #1000

mchf · 2019-11-04T12:37:11Z

Fate319639

Hotfix for network-ng to set hostname according to the fate above in the installer. It was only partly implemented in the code before network-ng. It basically means that:

if no other way is used the hostname is set to a default (currently install)
if a hostname is provided via dhcp then it is used (completely new functionality )
if a hostname is provided via linuxrc's hostname option then it is used (was not working in network-ng)

The hostname is set in order described above. Also, the hostname is written to the target only in third case (when set using hostname option).

Yast2-network cooperates with linuxrc here bcs linuxrc is e.g. the one who sets the default hostname and so on.

What should be done next:

in the old code the hostname and dns handling was mixed together. As this features are not fully related it would be nice to have it in separate classes in network-ng (will follow)

coveralls · 2019-11-04T12:58:57Z

Coverage increased (+0.08%) to 65.341% when pulling 9534643 on mchf:fate319639-hostname into 23db539 on yast:master.

jreidinger · 2019-11-04T14:50:49Z

src/lib/network/wicked.rb

+    # It parses both ipv4 and ipv6 lease files at once.
+    #
+    # @param iface [String] queried interface
+    # @param query [String] xpath query


maybe mention that it is just subset of xpath ( as specified in man wicked section xpath )

ok, I'll point to the wicked doc.

jreidinger · 2019-11-04T14:51:51Z

src/lib/y2network/hostname_reader.rb

-      if (Yast::Mode.installation || Yast::Mode.autoinst) &&
-          Yast::FileUtils.Exists("/etc/install.inf")
-        fqdn = hostname_from_install_inf
+      if Yast::Mode.installation || Yast::Mode.autoinst


will it work during update?

no, but network is not modified during update at all. I mean ... hostname will not be updated in the updated system, but it will be displayed correctly e.g. in bash prompt (bcs it is preset by linuxrc)

jreidinger · 2019-11-04T14:53:40Z

src/lib/y2network/hostname_reader.rb

+    #
+    # @return [String] Hostname
+    def hostname_for_installer
+      @install_inf_hostname = hostname_from_install_inf


why only this part is cached and others not?

see bellow comment about separating hostname class, this is another reason. In fact Hostname and DNS has nothing in common. It stayed together just because it was together in the old code.

jreidinger · 2019-11-04T14:54:22Z

src/lib/y2network/sysconfig/dns_reader.rb

@@ -31,9 +33,11 @@ class DNSReader
      #
      # @return [Y2Network::DnsConfig] DNS configuration
      def config
+        installer = Yast::Mode.installation || Yast::Mode.autoinst


what about autoinst mode in second stage?

network configuration runs completely in first stage. So in my POV when something invokes it in second stage then it should behave as on installed system.

jreidinger · 2019-11-04T14:55:41Z

src/lib/y2network/sysconfig/dns_reader.rb

+      #
+      # @return [Boolean]
+      def hostname_from_install_inf?
+        !@hostname_reader.install_inf_hostname.nil?


this is really nasty side effect that really depends on fact that hostname entry is constructed before save_hostname entry. What I really prefer is method hostname_reader that returns cached object and those two methods just call it.

I've follow up of this PR. I separate Hostname and DNS classes there exactly for this (and one above) reason. So, would you prefer to merge those two patches together?

I am fine with merging it together, so I can see final result.

teclator · 2019-11-05T08:41:30Z

I would like to see more documentation and description in general. Behavior, testing corner cases and so on.

The PR description only mentions a fate entry, which was for SLE-12-SP2, a link to the trello card would be nice to have or at least to the bug that reported it for SLE-15-SP2.

The fate is from 2016, there were some changes related to DHCLIENT_SET_HOSTNAME after it:

See https://github.com/yast/yast-network/pull/564/files

So, documenting different scenarios and the workflow with the different options knowing exactly how the static or transient hostname it is set in each case would be very nice to have.

Even linuxrc has it:

https://github.com/openSUSE/linuxrc/blob/master/linuxrc_hostname.md

teclator · 2019-11-20T21:36:58Z

@mchf I was asked about reviewing the PR but it still contains conflicts and the description basically says nothing. What the problem is, what it fixes, what is still missing..., of course there is an old fate number referenced but linking to it would be better for others to check it quickly and also as reference for the future as well as the trello card URL and any other useful link.

As we were discussing, we do not enforce a particular PR template but the one provided by @lslezak is a good one for using at least as an example.

I would like to also see some documentation about use cases, and different scenarios as it is not the first time we face a problem and different expectations affecting the hostname setup, but of course taking in account that is not part of the acceptance criteria probably we could omit it.

I already mentioned that in previous comment, but have not seen an answer or movement from there in advance, so not sure what more I could door what is expected from my side at all :(

jsrain · 2019-11-21T06:23:29Z

@teclator @mchf I fully understand that this PR's description says almost nothing (and this concern has not been addressed since more than two weeks now). Please, adjust the PR and get it merged ASAP. Currently, yast2-network fails to build in Staging (one test case failing) and effectively blocks merging any further changes of YaST. Thanks!

It somehow worked before, but due to changes caused by re-implementation in network-ng it got somehow broken.

teclator · 2019-11-21T07:49:04Z

Fate319639

Hotfix for network-ng to set hostname according to the fate above in the installer. It was only partly implemented in the code before network-ng. It basically means that:
* if no other way is used the hostname is set to a default (currently install)

At the end of the installation, if not hostname was set and no DHCP hostname what will be set? linux-(random), install or nothing?

* if a hostname is provided via dhcp then it is used (completely new functionality )

Well, it was already the case since: #564 but probably lost in network-ng.

* if a hostname is provided via linuxrc's hostname option then it is used (was not working in network-ng)
The hostname is set in order described above. Also, the hostname is written to the target only in third case (when set using hostname option).

Yast2-network cooperates with linuxrc here bcs linuxrc is e.g. the one who sets the default hostname and so on.

What should be done next:
* in the old code the hostname and dns handling was mixed together. As this features are not fully related it would be nice  to have it in separate classes in network-ng (will follow)

In the past there was even a separate dialog for setting the hostname but then it was removed and mixed with dns. Agree about moving the responsibility elsewhere.

teclator

So, LGTM as it is as we also have plans to do some more improvements in short, so lets move forward and fix some of the current issues.

teclator · 2019-11-21T07:55:23Z

package/yast2-network.changes

+-------------------------------------------------------------------
+Thu Nov 21 07:13:28 UTC 2019 - Michal Filka <mfilka@suse.com>
+
+- fate#319639


I would prefer to use the bug number instead of the fate, so, if you wanted to mention it then add both.

mchf · 2019-11-21T08:00:09Z

Fate319639
Hotfix for network-ng to set hostname according to the fate above in the installer. It was only partly implemented in the code before network-ng. It basically means that:
* if no other way is used the hostname is set to a default (currently install)
At the end of the installation, if not hostname was set and no DHCP hostname what will be set? linux-(random), install or nothing?

not specified in the feature - so a default. As the feature says that hostname should be updated only if set via the hostname option.

* if a hostname is provided via dhcp then it is used (completely new functionality )
Well, it was already the case since: #564 but probably lost in network-ng.

Yes, partly ... however stopped to work after transition to the network-ng

* if a hostname is provided via linuxrc's hostname option then it is used (was not working in network-ng)
The hostname is set in order described above. Also, the hostname is written to the target only in third case (when set using hostname option).
Yast2-network cooperates with linuxrc here bcs linuxrc is e.g. the one who sets the default hostname and so on.
What should be done next:
* in the old code the hostname and dns handling was mixed together. As this features are not fully related it would be nice  to have it in separate classes in network-ng (will follow)
In the past there was even a separate dialog for setting the hostname but then it was removed and mixed with dns. Agree about moving the responsibility elsewhere.

sure, by the old code I mean state before switching to network-ng.

teclator

LGTM for the time being

yast-bot · 2019-11-21T08:44:21Z

✔️ Public Jenkins job #99 successfully finished
✔️ Created OBS submit request #750053

yast-bot · 2019-11-21T08:46:46Z

✔️ Internal Jenkins job #29 successfully finished
✔️ Created IBS submit request #205809

mchf requested a review from jreidinger November 4, 2019 12:59

jreidinger reviewed Nov 4, 2019

View reviewed changes

mchf added 6 commits November 21, 2019 08:12

Implemented hostname recognition workflows for network-ng. fate#319639

f3504a0

It somehow worked before, but due to changes caused by re-implementation in network-ng it got somehow broken.

Parsing lease files for hostname. fate#319639

7eec795

During installation save hostname only if set via hostname= option

929c435

Updated testsuite

f3d02cb

Updated changelog

9428904

Happy rubocop

ed6b6fa

mchf force-pushed the fate319639-hostname branch 2 times, most recently from b5f9a6b to ed6b6fa Compare November 21, 2019 07:32

teclator reviewed Nov 21, 2019

View reviewed changes

Updated changelog

9534643

teclator approved these changes Nov 21, 2019

View reviewed changes

mchf merged commit dbb39c0 into yast:master Nov 21, 2019

mchf deleted the fate319639-hostname branch November 21, 2019 08:41

This was referenced Nov 21, 2019

network-ng: test cases document (draft) #947

Merged

Fix mocking in hostname_reader_test #1008

Merged

mchf restored the fate319639-hostname branch November 29, 2019 09:38

This was referenced Nov 29, 2019

Separated Hostname class from DNS one #1010

Merged

Fixed internal error in AY's second stage #1011

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hostname in installer #1000

Hostname in installer #1000

mchf commented Nov 4, 2019 •

edited

coveralls commented Nov 4, 2019 •

edited

jreidinger Nov 4, 2019

mchf Nov 5, 2019

jreidinger Nov 4, 2019

mchf Nov 5, 2019

jreidinger Nov 4, 2019 •

edited

mchf Nov 5, 2019

jreidinger Nov 4, 2019

mchf Nov 5, 2019

jreidinger Nov 4, 2019

mchf Nov 5, 2019

jreidinger Nov 5, 2019

teclator commented Nov 5, 2019

teclator commented Nov 20, 2019 •

edited

jsrain commented Nov 21, 2019

teclator commented Nov 21, 2019

teclator left a comment

teclator Nov 21, 2019

mchf commented Nov 21, 2019

teclator left a comment

yast-bot commented Nov 21, 2019

yast-bot commented Nov 21, 2019

Hostname in installer #1000

Hostname in installer #1000

Conversation

mchf commented Nov 4, 2019 • edited

coveralls commented Nov 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreidinger Nov 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teclator commented Nov 5, 2019

teclator commented Nov 20, 2019 • edited

jsrain commented Nov 21, 2019

teclator commented Nov 21, 2019

teclator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchf commented Nov 21, 2019

teclator left a comment

Choose a reason for hiding this comment

yast-bot commented Nov 21, 2019

yast-bot commented Nov 21, 2019

mchf commented Nov 4, 2019 •

edited

coveralls commented Nov 4, 2019 •

edited

jreidinger Nov 4, 2019 •

edited

teclator commented Nov 20, 2019 •

edited