Bug 1078495 - skelcd-control-openSUSE: missing swap settings
skelcd-control-openSUSE: missing swap settings
Status: RESOLVED FIXED
: 1083769 (view as bug list)
Classification: openSUSE
Product: openSUSE Tumbleweed
Classification: openSUSE
Component: Installation
Current
Other Other
: P1 - Urgent : Normal (vote)
: ---
Assigned To: E-mail List
Jiri Srain
https://openqa.opensuse.org/tests/596...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2018-01-31 12:01 UTC by Ludwig Nussel
Modified: 2019-04-15 12:14 UTC (History)
7 users (show)

See Also:
Found By: ---
Services Priority:
Business Priority:
Blocker: ---
Marketing QA Status: ---
IT Deployment: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ludwig Nussel 2018-01-31 12:01:24 UTC
## Observation

openQA test in scenario opensuse-Tumbleweed-DVD-x86_64-ext4@64bit fails in
[partitioning_filesystem](https://openqa.opensuse.org/tests/596167/modules/partitioning_filesystem/steps/1)

Looks like swap settings are missing from control.xml
Comment 1 Richard Brown 2018-01-31 13:10:28 UTC
I hate to do this, I really do - but this the skelcd-controls are actually valid

https://github.com/yast/skelcd-control-openSUSE/blob/openSUSE-15_0/control/control.openSUSE.xml#L424-L439

There is one of two options

Option 1) 
The openQA tests are broken:

https://openqa.opensuse.org/tests/596167#step/partitioning_filesystem/4

partitioning_filesystem.pm needs to be selecting "Propose Separate Swap Partition"

This was not a separate option in the past (before libstorage-ng)

Or
Option 2)
YaST is broken, and "Propose Separate Swap Partition" should be ticked by default

AFAIK there is no way I can make YaST do option 2 via the control.xml

Oli, what do you think?
Comment 2 Ludwig Nussel 2018-01-31 13:33:04 UTC
IMO 2). Especially with so little ram
Comment 3 Richard Brown 2018-01-31 15:30:38 UTC
After discussing with the YaST team, they agree. It's a bug!
Comment 4 Ancor Gonzalez Sosa 2018-01-31 15:48:45 UTC
(In reply to Richard Brown from comment #1)
>
> [...]
> 
> This was not a separate option in the past (before libstorage-ng)

FYI, if you don't want swap to be optional, you can remove the corresponding proposed_configurable flag from control.xml (or make it false). Then a swap will be always proposed.

(In reply to Richard Brown from comment #3)
> After discussing with the YaST team, they agree. It's a bug!

Despite the former comment about control.xml, this is still true. With the current version of control.xml that should be ticked.
Comment 6 Ludwig Nussel 2018-02-12 08:47:45 UTC
any ETA on this? To help decide whether it's worth to work around with the proposed_configurable temporarily.
Comment 7 Ancor Gonzalez Sosa 2018-02-12 09:17:14 UTC
(In reply to Ludwig Nussel from comment #6)
> any ETA on this? To help decide whether it's worth to work around with the
> proposed_configurable temporarily.

All I can say is that this is not in the top-ten of our TODO list (aka "current backlog" at https://trello.com/b/OD6t7OJS/). So I wouldn't expect a fix this week. :(
Comment 8 Ludwig Nussel 2018-02-26 08:39:15 UTC
Turns out that setting the suggested proposed_configurable flag to false has the side effect of enforcing a minimum swap size also in a custom partition layout, not just in the proposal:
https://build.opensuse.org/request/show/579097

Can we please get a fix for this?
Comment 9 Ludwig Nussel 2018-03-05 15:18:28 UTC
*** Bug 1083769 has been marked as a duplicate of this bug. ***
Comment 12 Ancor Gonzalez Sosa 2018-04-09 12:40:26 UTC
I took a closer look to the logs at and I would say it's behaving as expected.

Apparently this is the specification for the three volumes:

root =>
  desired = 10 GiB
  min = 5 GiB
  snapshots_percentage=100
  snapshots_configurable = true
  proposed_configurable = false
  disable_order = nil

home =>
  desired = 10 GiB
  min = 7 GiB
  proposed_configurable = true
  disable_order = 1

swap =>
  desired = 2 GiB
  min = 1 GiB
  proposed_configurable = true
  disable_order = 2

The disk is 10 GiB big.
  
So the proposal tries this (extracted from the logs):

  Trying proposal with initial settings
  Trying proposal after disabling '/home'
  Trying proposal after disabling 'adjust_by_ram' for 'swap'
  Trying proposal after disabling 'swap'
  Trying proposal after disabling 'snapshots' for '/

Take into account the initial values of the form in "Guided Setup" are based on the current settings (that is, the ones that were used to produce the current proposal). So the checkbox for swap is not unchecked always, only if it was unchecked in the initial attempt. And it was disabled in that initial attempt because the proposal was told that is more important to keep the snapshots than to keep the swap.

So the bug, if any, is not in the form. It was in control.xml or, at most, in the proposal code (if it's supposed to work in a different way, which I don't see).
Comment 13 Ancor Gonzalez Sosa 2018-04-09 12:45:13 UTC
(In reply to Ancor Gonzalez Sosa from comment #12)
>
> So the bug, if any, is not in the form. It was in control.xml or, at most,
> in the proposal code (if it's supposed to work in a different way, which I
> don't see).

According to https://github.com/yast/yast-storage-ng/blob/master/doc/old_and_new_proposal.md#the-volumes-subsection there is indeed a bug in the proposal. Since disable_order is not specified for root, it should have never disabled the snapshots during the initial attempt. So the sequence should have been:


  Trying proposal with initial settings
  Trying proposal after disabling '/home'
  Trying proposal after disabling 'adjust_by_ram' for 'swap'
  Trying proposal after disabling 'swap'
  NO PROPOSAL POSSIBLE

But still, having "swap" unchecked by default would be correct. Because is still true that the proposal has been told (via control.xml) than swap is less important than snapshots and than snapshots require 20 GiB.
Comment 14 Ancor Gonzalez Sosa 2018-04-09 21:22:26 UTC
(In reply to Ancor Gonzalez Sosa from comment #13)
> (In reply to Ancor Gonzalez Sosa from comment #12)
> >
> > So the bug, if any, is not in the form. It was in control.xml or, at most,
> > in the proposal code (if it's supposed to work in a different way, which I
> > don't see).

So the options to fix skelcd are:

 - Set disable_order to nil for swap. That way, the initial proposal will never try to disable swap, although the user can still change it.
 - Set disable_order in root to some value lower than swap. That way, snapshots will be deactivated before killing swap.
 - Both.
 - None. Of course. :-)

I will leave that decision to Ludwig.

> According to
> https://github.com/yast/yast-storage-ng/blob/master/doc/old_and_new_proposal.
> md#the-volumes-subsection there is indeed a bug in the proposal. Since
> disable_order is not specified for root, it should have never disabled the
> snapshots during the initial attempt. So the sequence should have been:
> 
> 
>   Trying proposal with initial settings
>   Trying proposal after disabling '/home'
>   Trying proposal after disabling 'adjust_by_ram' for 'swap'
>   Trying proposal after disabling 'swap'
>   NO PROPOSAL POSSIBLE

This PR fixes the proposal so the behavior matches the documentation. That's in fact even worse regarding this bug report, but now is more correct.

https://github.com/yast/yast-storage-ng/pull/597
Comment 15 Ancor Gonzalez Sosa 2018-04-10 10:21:51 UTC
(In reply to Ancor Gonzalez Sosa from comment #14)
>
> This PR fixes the proposal so the behavior matches the documentation. That's
> in fact even worse regarding this bug report, but now is more correct.
> 
> https://github.com/yast/yast-storage-ng/pull/597

Already submitted as https://build.opensuse.org/request/show/595146

So now YaST indeed act as documented. Reassigning to Ludwig (feel free to reassign if the decision is not yours) for him to decide on the correct control.xml settings (adjusting disable_order for the three volumes).
Comment 16 Ludwig Nussel 2018-04-10 13:09:31 UTC
If RAM is enough I think I'd sacrifice swap before turning off snapshots. If the system is really too low on RAM though then it needs to keep swap on of course. Does that make sense?
Comment 17 Ancor Gonzalez Sosa 2018-04-11 08:19:50 UTC
(In reply to Ludwig Nussel from comment #16)
> If RAM is enough I think I'd sacrifice swap before turning off snapshots. If
> the system is really too low on RAM though then it needs to keep swap on of
> course. Does that make sense?

The amount of RAM is not a criterion right now when disabling volumes (or deactivating any of it's features like enlarging or snapshots).

The proposal tries to make everything fit with the default options and, if that's not possible, if disables volumes (first its features and then the volume itself[1]) one by one following the disable_order. Swap is just another generic volume in that regard[2], so it gets disabled when it's its turn, without any consideration about the RAM size.

We really need to make volumes generic and configurable via control.xml, ad-hoc logic for every volume was a dead end. It didn't fit many use cases like Kubic/CaaSP, SLE4SAAP, the KVM role, some product wanting to enlarge the root partition based on size (for kdump) and other half-dozen scenarios that were unhappy with the traditional desktop-centric logic.

One obvious option to make the mechanism more flexible (that was somehow expected from the very beginning, just not introduced to keep the format as simple as possible) would be to have (in addition to disable_order) more specific attributes proposed_disable_order, snapshots_disable_order and adjust_by_ram_disable_order. With that you could have something like:

  If initial settings fail, try disabling '/home'
  If that fails, try deactivating 'adjust_by_ram' for swap
  If that fails, try deactivating 'snapshots' for root
  If that fails, fail (i.e. don't automatically propose to disable root or swap)

While still having swap optional (i.e. possible to deactivate by the user in the UI).

Not exactly what you asked, I know.

What you asked would imply having a condition (the RAM size) to decide whether a particular volume (or one of its feature) must be disabled. Not sure how to make that while keeping the system understandable. Should that condition specified in the control file (ram_size_to_not_disable)? How long will it take for someone else to ask for another custom condition for another volume?

[1] It disables what is possible, based on the xxx_configurable settings.
Comment 18 Ludwig Nussel 2018-04-11 09:18:30 UTC
simple is usually better. IMO it's more honest to just fail if it gets too complicated rather than making some guesses. I'd probably even fail one earlier and not try to disable snapshots as that is a core feature and kind of the justification for btrfs in the first place.
Comment 19 Steffen Winterfeldt 2018-04-11 11:45:46 UTC
I believe we're at the brink of over-engineering a single minor aspect to a point
that has no value to anyone.

I would just accept that the proposal won't fit everybody and adjust the qa tests to reflect the present and not the past.

Is there anything the yast team still has to do?
Comment 20 Ancor Gonzalez Sosa 2018-04-12 08:59:01 UTC
(In reply to Ludwig Nussel from comment #18)
> simple is usually better. IMO it's more honest to just fail if it gets too
> complicated rather than making some guesses. I'd probably even fail one
> earlier and not try to disable snapshots as that is a core feature and kind
> of the justification for btrfs in the first place.

So that's exactly what you will get now with the settings that you used to have in that test case.

root =>
  proposed_configurable = false
  snapshots_configurable = true
  disable_order = NONE

home =>
  proposed_configurable = true
  disable_order = 1

swap =>
  proposed_configurable = true
  adjust_by_ram_configurable = true
  disable_order = 2

In a small disk it will try:  
  Initial settings (everything enabled)
  Disabled /home
  Disabled /home' + disabled 'adjust_by_ram' for swap
  Disabled /home' + disabled swap
  Failure (there is nothing else to disable since root has no disable_order)

So the code works as expected and does what Ludwig states to be the wanted behavior. The exact shape of control.xml (with all the corresponding disable_order) is to be defined by the release/product/whatever managers. Why is this reassigned to YaST maintainers?
Comment 21 Ludwig Nussel 2018-04-12 11:37:30 UTC
ok, close then. I've raised the disk size in openqa for most machine definitions to 20GB.
Comment 22 Steffen Winterfeldt 2018-04-12 12:07:50 UTC
closing