Bug 998741

Created attachment 788225 [details]
File: anaconda-tb

Created attachment 788226 [details]
File: anaconda.log

Created attachment 788227 [details]
File: environ

Created attachment 788228 [details]
File: ks.cfg

Created attachment 788229 [details]
File: lsblk_output

Created attachment 788230 [details]
File: nmcli_dev_list

Created attachment 788231 [details]
File: os_info

Created attachment 788232 [details]
File: program.log

Created attachment 788233 [details]
File: storage.log

Created attachment 788234 [details]
File: syslog

Created attachment 788235 [details]
File: ifcfg.log

Created attachment 788236 [details]
File: packaging.log

This is consistent i can reproduce it all the time. 

  Try #1 - installer crashes
   Try #2 - installer successfully installs stuff
   Try #3 - installer crashes
   Try #4 - installer works and so on

This "in newLV raise ValueError("name already in use")" seems to be a bit of a greatest hit. We have at least five other bugs with it: one closed as fixed, the others mostly as insufficient information:

https://bugzilla.redhat.com/show_bug.cgi?id=875477 ('fixed')
https://bugzilla.redhat.com/show_bug.cgi?id=907467
https://bugzilla.redhat.com/show_bug.cgi?id=882699
https://bugzilla.redhat.com/show_bug.cgi?id=894544
https://bugzilla.redhat.com/show_bug.cgi?id=875497

dlehman, clumens, is it possible to check through them and see if there's some kind of pattern? Or are they likely to be different issues that happen to raise the same failure? Thanks!

Mini-code review:

The ValueError exception strings would be more informative if they:
1. Actually reported the "name already in use".
2. Said whether the name was a VG name or an LV name.

$ cat -n blivet-0.17-1/blivet/__init__.py | gedit -
...
  1050	    def newVG(self, *args, **kwargs):
  1051	        """ Return a new LVMVolumeGroupDevice instance. """
...
  1071	        if name in self.names:
  1072	            raise ValueError("name already in use")
  1073	
  1074	        return LVMVolumeGroupDevice(name, pvs, *args, **kwargs)
  1075	
  1076	    def newLV(self, *args, **kwargs):
  1077	        """ Return a new LVMLogicalVolumeDevice instance. """
...
  1105	        if "%s-%s" % (vg.name, name) in self.names:
  1106	            raise ValueError("name already in use")
  1107	
  1108	        return LVMLogicalVolumeDevice(name, *args, **kwargs)
...

From ks.cfg %pre:

dd if=/dev/zero of=/dev/sda bs=512 count=1
parted -s /dev/sda mklabel msdos
echo "#partitioning scheme generated in %pre for 1 drive" > /tmp/part-include
echo "clearpart --all --drives=$drive1" >> /tmp/part-include

Which is a good way to make sure there is plenty of stale metadata all over your disk. It's a real shame everyone thinks you can initialize a disk by overwriting only 512 bytes of it. In this case, the disk was corrupted to the point that udev saw disk sda (with no disklabel) and partition sda3. I'm not overly concerned with how gracefully we handle wreckage of this magnitude.

dlehman: steve's points about making the logging a bit clearer seem to have validity beyond this particular case, though - see all the 'duplicate' reports I linked, if the error was a bit more detailed, they would be more separable, presumably.

(In reply to David Lehman from comment #16)
...
> It's a real shame everyone thinks you can initialize a disk by
> overwriting only 512 bytes of it.
...

Arun: Do you have special requirements that prevent you from letting the installer handle disk initialization?

The installer runs wipefs[1] repeatedly during storage setup, but wipefs needs to have a partition table, so zeroing the partition table is counter-productive:
dd if=/dev/zero of=/dev/sda bs=512 count=1

Dave: Can you suggest a better way to do this?
#Write out partition scheme based on whether there are 1 or 2 hard drives
[Following lines write kickstart options to /tmp/part-include.]
%include /tmp/part-include

[1] wipefs is in the util-linux package:
$ rpm -qf `which wipefs`
util-linux-2.23.2-2.fc19.x86_64
$ man wipefs

echo "part swap --size=$RAM_IN_MB --ondisk $drive1" >> /tmp/part-include
                       ^^^^^^^^^^

Here is one problem: Kickstart does not support parameterization, AFAIK.

Steve, that's a bash script which is going to write out that line with the value of $RAM_IN_MB in that place instead of the variable name.

As far as generic exception text goes, it's a generic error. There are many ways to pass through that particular code path. Putting the name in the exception text is going to help what, exactly? Even with the name in there you're still going to have to look at the traceback in order to tell apart different occurrences of this generic failure.

(In reply to David Lehman from comment #16)
> From ks.cfg %pre:
> 
> dd if=/dev/zero of=/dev/sda bs=512 count=1
> parted -s /dev/sda mklabel msdos
> echo "#partitioning scheme generated in %pre for 1 drive" > /tmp/part-include
> echo "clearpart --all --drives=$drive1" >> /tmp/part-include
> 
> Which is a good way to make sure there is plenty of stale metadata all over
> your disk. It's a real shame everyone thinks you can initialize a disk by
> overwriting only 512 bytes of it. In this case, the disk was corrupted to
> the point that udev saw disk sda (with no disklabel) and partition sda3. I'm
> not overly concerned with how gracefully we handle wreckage of this
> magnitude.

Sorry about that, i already removed that those lines from my kickstart file. Things are working!

(In reply to Steve Tyler from comment #18)
> 
> Arun: Do you have special requirements that prevent you from letting the
> installer handle disk initialization?
> 

No i don't. I am fine removing that dd line off the kickstart file

(In reply to David Lehman from comment #20)
> Steve, that's a bash script which is going to write out that line with the
> value of $RAM_IN_MB in that place instead of the variable name.

Exactly. And why does that have to be done with a shell-script hack?
Because kickstart is so primitive, that its idea of modularity is include files.

> As far as generic exception text goes, it's a generic error. There are many
> ways to pass through that particular code path. Putting the name in the
> exception text is going to help what, exactly? Even with the name in there
> you're still going to have to look at the traceback in order to tell apart
> different occurrences of this generic failure.

The specific name is not generic. There is a specific name that is already in use. The message does not say what that specific name is. What am I supposed to do next, read the code? Because I can't figure out what it is from your variable names in the attached anaconda-tb:

...
Local variables in innermost frame:
name: lv_root
vg: non-existent 936320MB lvmvg vg_root (21)
self: <blivet.Blivet object at 0x7f1b39fa1350>
args: ()
safe_name: vg_root-lv_root
safe_vg_name: vg_root
full_name: vg_root-lv_root
...

(In reply to Arun S A G from comment #22)
> (In reply to Steve Tyler from comment #18)
> > 
> > Arun: Do you have special requirements that prevent you from letting the
> > installer handle disk initialization?
> > 
> 
> No i don't. I am fine removing that dd line off the kickstart file

Thanks for your followup replies, Arun.

AFAICT, the shell script you are writing out is intended to allocate an amount of swap space equal to the amount of RAM. Is that a requirement, or could you get by using a standard fixed amount of swap space? Have you ever had a problem due to insufficient swap space? Or due to over-allocation of swap space?

Basically, I'm wondering if you could eliminate the shell script altogether ...

An alternative approach would be to use the LVM tools post-install to configure swap space. I was hoping Dave would have some ideas, because he usually does ... (Comment 20)

(In reply to Steve Tyler from comment #24)
> 
> Thanks for your followup replies, Arun.
> 
> AFAICT, the shell script you are writing out is intended to allocate an
> amount of swap space equal to the amount of RAM. Is that a requirement, or
> could you get by using a standard fixed amount of swap space? Have you ever
> had a problem due to insufficient swap space? Or due to over-allocation of
> swap space?
> 
> Basically, I'm wondering if you could eliminate the shell script altogether
> ...
> 
> An alternative approach would be to use the LVM tools post-install to
> configure swap space. I was hoping Dave would have some ideas, because he
> usually does ... (Comment 20)


The shell script allocates swap space about the amount of RAM + additional 1GB. Yes, i could remove the swap size computation all together and just use a standard swap size. Is there a recommended swap size? 

we used to do some complex stuff with that %pre shell script,  i reduced the complexity one by one. Yes now it looks like i can completely get rid of this shell script and just use standard kickstart commands/directives .

Thanks for all your replies.

Thanks for your clarifications.

The simplest way to determine how much swap you need is to run your system using realistic applications and monitor swap usage over time. System-monitor[1] gives a nice graphical display. You could run "free" periodically and save the results to a file for later analysis. A more elaborate method is to run "top" periodically in batch mode to capture memory usage and top memory users.[2]

From the package selection in your kickstart file, it looks like you are installing desktop environments, development tools, and virtualization technology. Is there a particular application that you know to be a heavy memory user?

VMs may be big memory users, since the general rule is you need enough memory on the host as each VM requires plus memory to run anything else.

The bottom line on swap is simple: If you are using swap, you need to buy more memory.

[1] gnome-system-monitor-3.8.2.1-1.fc19.x86_64
[2] Bug 633807, Comment 6
    Bug 633807, Comment 7

The alpha release criteria have a sentence that I have long been interested in, because it is not clear whether the requirement is implementable. This bug comes close to showing that it is not implementable:

"[The installer] must work whether the disk is formatted or not and whether or not it contains any existing data ..."[1]

Here, the partition table was zeroed out part way into the install, and the install failed. Does this scenario fall under the release criteria anywhere?[2]

[1] Quoted out of context from here:
Disk layouts
https://fedoraproject.org/wiki/Fedora_20_Alpha_Release_Criteria#Disk_layouts
(The internal link doesn't work, but the section is in the TOC at the top.)

[2] See also, Comment 16.

We generally defer to the anaconda team in defining what's 'sane'. The intent of that criterion is really that you can do a fresh install - _using the installer's own disk formatting / re-partitioning stuff_ - to a disk whatever it contained when you went in. It's not really meant to cover a scenario where you inject a kickstart that does crazy stuff to the disk. We could try and clarify it, but it's a bit hard to come up with precise wording. We've generally not taken stuff like 'stale RAID metadata' as blockers, though, for instance.

Thanks for clarifying that, Adam.

Arguably, a shell-script in a kickstart file is part of the installer, but it could be interpreted as a user-supplied installer *extension*. This puts such shell-scripts in the same class as non-Fedora packages and drivers. The wording could be: "The install is not required to succeed when a user-supplied shell-script is executed as part of a kickstart file." (That's a negative criterion ...)

FWIW, I tried installing to a disc image that had a default, minimal install on it with a zeroed boot sector. The installer saw the disc image as empty, as expected, and handled the test-case perfectly.

Tested with:
$ qemu-kvm -m 4096 -hda f19-test-3.img -cdrom ~/xfr/fedora/F19/Fedora-19-x86_64-DVD.iso -vga std -boot menu=on

(In reply to Steve Tyler from comment #23)
> (In reply to David Lehman from comment #20)
> > Steve, that's a bash script which is going to write out that line with the
> > value of $RAM_IN_MB in that place instead of the variable name.
> 
> Exactly. And why does that have to be done with a shell-script hack?
> Because kickstart is so primitive, that its idea of modularity is include
> files.
...

That "shell-script hack" happens to be based on an example in the Anaconda/Kickstart wiki:
https://fedoraproject.org/w/index.php?title=Anaconda/Kickstart&oldid=350435#Chapter_4._Pre-installation_Script

The example certainly highlights the deficiencies of the kickstart syntax ...

(In reply to Steve Tyler from comment #26)

> From the package selection in your kickstart file, it looks like you are
> installing desktop environments, development tools, and virtualization
> technology. Is there a particular application that you know to be a heavy
> memory user?
> 
> VMs may be big memory users, since the general rule is you need enough
> memory on the host as each VM requires plus memory to run anything else.

Most people at work run VM's on their fedora machines. So yes VM's are the big memory users. A typical desktop system at my work place has 8 - 16 GB of RAM

(In reply to Arun S A G from comment #31)
...
> Most people at work run VM's on their fedora machines. So yes VM's are the
> big memory users. A typical desktop system at my work place has 8 - 16 GB of
> RAM

OK, then memory-usage in the VMs needs to be monitored too. I sometimes run "top" simultaneously on the host and the guest. The worst-case scenario is having the host and the guest both swapping ...

(In reply to Adam Williamson from comment #14)
...
> https://bugzilla.redhat.com/show_bug.cgi?id=875477 ('fixed')
> https://bugzilla.redhat.com/show_bug.cgi?id=907467
> https://bugzilla.redhat.com/show_bug.cgi?id=882699
> https://bugzilla.redhat.com/show_bug.cgi?id=894544
> https://bugzilla.redhat.com/show_bug.cgi?id=875497
...

We'll need to look at why the abrt_hash is different in each case:
(4 are F18 and 2 are F19, so that might explain some of the differences.)

$ bugzilla query -b 875477,907467,882699,894544,875497,998741 --outputformat='Bug %{id} %{version} %{whiteboard}'
Bug 875477 18 abrt_hash:3b240e77666effa70d72974441b7c9147f215a0b7125cb32d95b6cf92b7b592a AcceptedBlocker
Bug 875497 18 abrt_hash:38222283d6e1a5a93deb47cc6365118d87e648241a95a45c8a79641a61eccad8
Bug 882699 18 abrt_hash:349f8ba41b1be9898eff14db3de32fe96d6c49f1c43f4da5c0b0d67268ebfa19
Bug 894544 18 abrt_hash:2e49622f57b8e3e71ad83dfbd9590afdc051971db85316c3433dce476b0908bb
Bug 907467 19 abrt_hash:907198d38ef012bf3df3c64a7c0565f2eff22740b1b4736bcafb66ca67f46462
Bug 998741 19 abrt_hash:882a83ee0ca365b2f3d81f95111b47bbf3e87eac233c9ef9f995a28184daa40d

Created attachment 789885 [details]
six different tracebacks for "ValueError: name already in use" exceptions

(In reply to Steve Tyler from comment #33)
...
> We'll need to look at why the abrt_hash is different in each case:
...

The short answer is that each has a distinct traceback:

$ cat ValueError-name-already-in-use-tracebacks-1.txt | egrep 'Bug |File ' | sed 's/ in /@/' | cut -d'@' -f2
=== Bug 998741 ===
newLV
execute
execute
doKickstartStorage
_doExecute
run
run
=== Bug 907467 ===
newLV
new_device
newDevice
_replace_device
_save_right_side
on_apply_clicked
=== Bug 894544 ===
newLV
new_device
newDevice
_replace_device
_save_right_side
on_selector_clicked
_onSelectorClicked
=== Bug 882699 ===
newLV
new_device
newDevice
on_add_clicked
=== Bug 875497 ===
newLV
_scheduleVolumes
doAutoPartition
execute
doKickstartStorage
_doExecute
run
run
=== Bug 875477 ===
newLV
_scheduleVolumes
doAutoPartition
_do_autopart
on_create_clicked

(In reply to Adam Williamson from comment #17)
> dlehman: steve's points about making the logging a bit clearer seem to have
> validity beyond this particular case, though - see all the 'duplicate'
> reports I linked, if the error was a bit more detailed, they would be more
> separable, presumably.

This highlights the problem with having "generic" exception strings:

$ bugzilla query -b 875477,907467,882699,894544,875497,998741 --outputformat='Bug %{id} "%{summary}"'
Bug 875477 "ValueError: name already in use"
Bug 875497 "ValueError: name already in use"
Bug 882699 "ValueError: name already in use"
Bug 894544 "ValueError: name already in use"
Bug 907467 "ValueError: name already in use"
Bug 998741 "ValueError: name already in use"

steve: dlehman already explained that any additional information we can put in the summary still wouldn't be much intrinsic use; it'd be device names that would still be meaningless without reading the trace. so there's not a lot of point.

Well, those exception strings end up in bug summaries, and bug summaries are read by humans ...

...who *still wouldn't get any actual meaningful information* from an arbitrary device ID.

The names that are not being reported are, IIUC, LV and VG names. Those are exposed to the user, so they are as meaningful as any such names could be. Comment 15 has the code snippet where those exceptions are raised.

Since the strings are not, IIUC, intended for end-users, it's hard to argue from User Interface Design principles, but ISTM that having SIX bugs with identical bug summaries is confusing. At least that is what I thought was the reason you started this in the first place ...

As I showed in Comment 33 and Comment 34, the six bugs are rightly classified as distinct bugs, because they have distinct tracebacks and hence distinct abrt_hash strings ... yet they have identical bug summaries.

(In reply to Steve Tyler from comment #39)
> The names that are not being reported are, IIUC, LV and VG names. Those are
> exposed to the user, so they are as meaningful as any such names could be.
> Comment 15 has the code snippet where those exceptions are raised.
...

The attached anaconda-tb has the actual value. In Comment 15, line 1071, the variable "name" is tested for being already in use.

"name" has the value "lv_root", which is certainly meaningful:

...
Local variables in innermost frame:
name: lv_root
...

BTW, the conditions tested in Comment 15, lines 1071 and 1105, are effectively assertions, so the "name already in use" exceptions are, in fact, *assertion failures* in a low-level function.

(In reply to Steve Tyler from comment #40)
> (In reply to Steve Tyler from comment #39)
> > The names that are not being reported are, IIUC, LV and VG names. Those are
> > exposed to the user, so they are as meaningful as any such names could be.
> > Comment 15 has the code snippet where those exceptions are raised.
> ...
> 
> The attached anaconda-tb has the actual value. In Comment 15, line 1071, the
> variable "name" is tested for being already in use.
> 
> "name" has the value "lv_root", which is certainly meaningful:
...

Here is the complete list of names from the anaconda-tb attachments:

Bug 875477  name: root
Bug 875497  name: home
Bug 882699  name: fedora
Bug 894544  name: root
Bug 907467  name: home
Bug 998741  name: lv_root

Putting those in the exception strings wouldn't achieve unique bug summaries, so I concede ... :-)

Actually, there are eight bugs with the same summary. Bug 892255 is still open.

$ bugzilla query -s 'ValueError: name already in use' --outputformat='Bug %{id} "%{summary}" %{version} %{status}(%{resolution})'
Bug 691817 "ValueError: name already in use" 6.1 CLOSED(ERRATA)
Bug 875477 "ValueError: name already in use" 18 CLOSED(CURRENTRELEASE)
Bug 875497 "ValueError: name already in use" 18 CLOSED(DUPLICATE)
Bug 882699 "ValueError: name already in use" 18 CLOSED(INSUFFICIENT_DATA)
Bug 892255 "ValueError: name already in use" 18 NEW()
Bug 894544 "ValueError: name already in use" 18 CLOSED(INSUFFICIENT_DATA)
Bug 907467 "ValueError: name already in use" 19 CLOSED(WORKSFORME)
Bug 998741 "ValueError: name already in use" 19 CLOSED(WONTFIX)