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

Add automatically some important kernel parameters to KERNEL_CMDLINE #1495

Merged
merged 3 commits into from Sep 18, 2017

Conversation

schabrolles
Copy link
Member

@schabrolles schabrolles commented Sep 15, 2017

I propose to check the current kernel parameters used (in /proc/cmdline) to detect some important parameter we should use when booting in rescue mode (means add them in KERNEL_CMDLINE variable).

Based on the discussion with @gdha in #1400, I started to detect net.ifnames or biosdevname parameter in order to preserve interface naming between rescue mode and real system after migration.

Copy link
Member

@jsmeix jsmeix left a comment

Choose a reason for hiding this comment

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

@schabrolles
I think this is a very good enhancement.

Could you perhaps implement it via
generically working code in the script
(i.e. avoid hardcoded values in the scripts)
plus a config array in default.conf that
contains those kernel commandline options
that are checked if present on the original system
and if yes also used for the recovery system?

@jsmeix jsmeix added the enhancement Adaptions and new features label Sep 15, 2017
@jsmeix jsmeix added this to the ReaR v2.3 milestone Sep 15, 2017
Copy link
Member

@gdha gdha left a comment

Choose a reason for hiding this comment

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

@schabrolles @jsmeix @schlomo Woudln't it not be better to make an array of KERNEL_CMDLINE?

@schabrolles
Copy link
Member Author

schabrolles commented Sep 15, 2017

@gdha It was my thought.... I was starting this scipt by considering KERNEL_CMDLINE was an array ... but it was not ;-) May be @jsmeix or @schlomo could tell us why it is a variable and not an array.

@jsmeix, I've just made some adjustments (using a Array CHECK_KERNEL_PARAMETER in default.conf). Does it fit your expectation ? (not sure I understand what you mean by "implement it via
generically working code"
)

@jsmeix
Copy link
Member

jsmeix commented Sep 18, 2017

FWIW:
Bash arrays are in the end only needed when the elements
could be strings (i.e. consist of several words) so that

# strings=( first 'second thing' last )

# for string in "${strings[@]}" ; do echo "'$string'" ; done
'first'
'second thing'
'last'

works.
When things are only words, a simple string is sufficient
because then simple

# words=" first second   third "

# for word in $words ; do echo "'$word'" ; done
'first'
'second'
'third'

already works.
Careful note quoting versus non-quoting!

Copy link
Member

@jsmeix jsmeix left a comment

Choose a reason for hiding this comment

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

Looks perfect!

With generically working code in the actual scripts
(regardless that default.conf and local.conf
are sourced and executed as scripts
I do not count them as actual scripts)
I mean to avoid hardcoded values in the actual scripts.
Now the user has final power because
he can - if needed - specify the actual behaiour
via config varaiables - e.g with
CHECK_KERNEL_PARAMETER=()
he can disable this functionality if
he wants that.

@schabrolles
is perhaps CHECK_KERNEL_PARAMETERS
(with a plural 'S' at the end) a better name?
Or even COPY_KERNEL_PARAMETERS
because in ReaR we often use the
word 'copy' when we mean to
"have the same in the ReaR recovery system".

@schabrolles
Copy link
Member Author

schabrolles commented Sep 18, 2017

@jsmeix variable name changed. (and tested an ubuntu with net.ifnames)

I'm waiting for last feedback/approval from @gdha and @schlomo before merging this one.

Copy link
Member

@gdha gdha left a comment

Choose a reason for hiding this comment

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

@schabrolles looks good albeit I have seen persistence naming being used without these kernel parameters being set.
For sure, it is a nice enhancement, but it is not full proof, but I don't know how to make it full proof either.
OK to commit.

@schlomo
Copy link
Member

schlomo commented Sep 18, 2017

Looks good to me. Did you check it with kernel options that are bare words without a =? Like gpt?

@schabrolles
Copy link
Member Author

@gdha, Yes I have some system (in PowerVM) where even if you put net.ifnames=0, the inet are name with biosdevname. But it is a first step ... (some other modification will come).
Without this one, you could face the following problem:

  1. During a migration of a system that have "net.ifnames=0" (source machine) interface is named "eth0"
  2. Migrate to a new system... So boot on the ReaR rescue media, interface could be named "enpos1" if running ubuntu.... So ReaR start the network migration and rename interfaces from eth0 to enpos1 (ubuntu is not using udev rules file by default).
  3. After migration, reboot the the "Real OS" .... but this one will restart with "net.ifnames=0" ... so network won't be setup correctly ... :(

That's why we need to copy this option into the rescue image.

@schabrolles
Copy link
Member Author

@schlomo, Yes it also works with parameters without = like quiet (just tested)

@schlomo
Copy link
Member

schlomo commented Sep 18, 2017

Thanks, go ahead and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adaptions and new features fixed / solved / done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants