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

disable kernel multiversion setting (boo#1172229) #234

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lnussel
Copy link
Member

@lnussel lnussel commented Jul 16, 2020

No description provided.


MIL << "Parsing keep spec: " << _keepSpec << std::endl;

std::vector<std::string> words;
str::split( _keepSpec, std::back_inserter(words), ",", str::TRIM );
if ( words.empty() ) {
WAR << "Invalid keep spec: " << _keepSpec << " using default latest,running." << std::endl;
return;
WAR << "Invalid keep spec: " << _keepSpec << " using default latest,latest-1,running." << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we really just fall back to the default keep spec if the admin speficied a incorrect one? That might result in purging kernels and packages that should not be purged. IMO its better to error out in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe but that would be separate change. The previous code just skipped the parsing in error case so used the global defaults of those variables. Ie equivalent to "latest,running". That's even less than the config file default of "latest,latest-1,running". So this change is for consistency.

@bzeller bzeller requested a review from mlandres July 16, 2020 13:11
@mlandres
Copy link
Member

@lnussel boo#1172229 is about transactional systems. Your patch here would change everything down to SLE/LEAP 15.0. I don't think we want this.

I'll try provide a way to ship an empty zypp.conf in the future, keeping our current defaults, unless all TW and SLE15 stakeholders conclude that no-multiversion kernel shall be the new default for all those distros from now on.
Then transactional systems will be able to disable multiversion kernels by installing a file in multiversion.d (and the admin may overwrite this in zypp.conf again if he likes to).

@lnussel
Copy link
Member Author

lnussel commented Jul 17, 2020

@lnussel boo#1172229 is about transactional systems. Your patch here would change everything down to SLE/LEAP 15.0. I don't think we want this.

Eh, don't you have branches? :)

I'll try provide a way to ship an empty zypp.conf in the future, keeping our current defaults, unless all TW and SLE15 stakeholders conclude that no-multiversion kernel shall be the new default for all those distros from now on.
Then transactional systems will be able to disable multiversion kernels by installing a file in multiversion.d (and the admin may overwrite this in zypp.conf again if he likes to).

Yes, that's the idea. So far I was thinking of moving the config to the purge-kernel-service but the maintainer had concerns that the setting and the service might not necessarily related. So instead we could create a subpackge of libzypp or zypper for that.

@mlandres
Copy link
Member

We do have branches and master (libzypp-17.x) e.g. feeds TW and the SLE15 family.
But we also don't see the service being related to zypper or libzypp. The service happens to use zypper, that's all.
Let's focus on the config first....

Will be set by the purge-kernels-service package instead.
This avoids an explicit setting in the config
This reflects openSUSE defaults and avoids to ship an explicit
setting override via config.
@Conan-Kudo
Copy link
Member

This change should be done at image composition time, no? And for transactional-server role in YaST, can't we include an action there to set this properly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants