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

fix systemd_service.get_enabled, get_disabled and get_static on newer systemd version #59525

Merged
merged 7 commits into from Feb 24, 2021

Conversation

Vaarlion
Copy link
Contributor

@Vaarlion Vaarlion commented Feb 17, 2021

What does this PR do?

This PR fix systemd_service.get_enabled, get_disabled and get_static on newer systemd version

in older systemd version, the output of list-unit-files looked like this

boot-complete.target                                                    static                                                                                                                                                                
cryptsetup-pre.target                                                   static                                                                                                                                                                
cryptsetup.target                                                       static                                                                                                                                                                
ctrl-alt-del.target                                                     disabled                                                                                                                                                              
default.target                                                          static                                                                                                                                                                
emergency.target                                                        static                                                                                                                                                                
exit.target                                                             disabled                                                                                                                                                              
final.target                                                            static                                                                                                                                                                
getty-pre.target                                                        static                                                                                                                                                                
getty.target                                                            static                                                                                                                                                                
graphical.target                                                        static                                                                                                                                                                
halt.target                                                             disabled    

with only 2 fields, The unit name and the enabled state.
On newer version of systemd, they added a new field, the VENDOR_PRESET

boot-efi.mount                                       generated       -            
boot.mount                                           generated       -            
dev-hugepages.mount                                  static          -            
dev-mqueue.mount                                     static          -            
home-guillaumedk-.steam-library.mount                enabled         disabled     
home-guillaumedk-Nas.mount                           disabled        disabled     
home.mount                                           generated       -            
mnt-media.mount                                      disabled        disabled     
proc-fs-nfsd.mount                                   static          -            
proc-sys-fs-binfmt_misc.mount                        disabled        disabled     
run-vmblock\x2dfuse.mount                            disabled        disabled     
sys-fs-fuse-connections.mount                        static          -            
sys-kernel-config.mount                              static          -            
sys-kernel-debug.mount                               static          -            

Resulting in issue when parsed by this module.
unit_state would look like 'static -'

This fix allow any number of field to be split, but only keep the first 2.

What issues does this PR fix or reference?

Fixes: #59526

Previous Behavior

~$ sudo salt-call service.get_static
local:

New Behavior

~$ sudo salt-call service.get_static
local:
    - apt-daily
    - apt-daily-upgrade
    - auth-rpcgss-module
    - basic.target
    - blockdev@.target
    - bluetooth.target
    - boot-complete.target
    - container-getty@
    - cryptsetup-pre.target
    - cryptsetup.target
    - dbus
    - dbus.socket
    - dev-hugepages.mount
...

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@Vaarlion Vaarlion changed the title WIP: fix systemd_service.get_enabled, get_disabled and get_static on newer systemd version fix systemd_service.get_enabled, get_disabled and get_static on newer systemd version Feb 17, 2021
@Vaarlion Vaarlion marked this pull request as ready for review February 17, 2021 16:03
@Vaarlion Vaarlion requested a review from a team as a code owner February 17, 2021 16:03
@Vaarlion Vaarlion requested review from twangboy and removed request for a team February 17, 2021 16:03
@sagetherage
Copy link
Contributor

@Vaarlion we no longer use the develop branch please point this to master

@sagetherage sagetherage added the develop Pointing to develop branch; needs rebase label Feb 17, 2021
Copy link
Contributor

@twangboy twangboy left a comment

Choose a reason for hiding this comment

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

Please add some tests for the two different outputs to unit/modules/test_systemd_service.py

Also, please rebase this against the master branch. Develop is no longer used...

@sagetherage sagetherage added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Feb 17, 2021
@Vaarlion
Copy link
Contributor Author

Dammit ! i explicitly rebased from master to develop :(

in older systemd version, the output of list-unit-files looked like this
```
boot-complete.target                                                    static                                                                                                                                                                
cryptsetup-pre.target                                                   static                                                                                                                                                                
cryptsetup.target                                                       static                                                                                                                                                                
ctrl-alt-del.target                                                     disabled                                                                                                                                                              
default.target                                                          static                                                                                                                                                                
emergency.target                                                        static                                                                                                                                                                
exit.target                                                             disabled                                                                                                                                                              
final.target                                                            static                                                                                                                                                                
getty-pre.target                                                        static                                                                                                                                                                
getty.target                                                            static                                                                                                                                                                
graphical.target                                                        static                                                                                                                                                                
halt.target                                                             disabled    
```
with only 2 fields, The unit name and the enabled state.
On newer version of systemd, they added a new field, the VENDOR_PRESET
```
boot-efi.mount                                       generated       -            
boot.mount                                           generated       -            
dev-hugepages.mount                                  static          -            
dev-mqueue.mount                                     static          -            
home-guillaumedk-.steam-library.mount                enabled         disabled     
home-guillaumedk-Nas.mount                           disabled        disabled     
home.mount                                           generated       -            
mnt-media.mount                                      disabled        disabled     
proc-fs-nfsd.mount                                   static          -            
proc-sys-fs-binfmt_misc.mount                        disabled        disabled     
run-vmblock\x2dfuse.mount                            disabled        disabled     
sys-fs-fuse-connections.mount                        static          -            
sys-kernel-config.mount                              static          -            
sys-kernel-debug.mount                               static          -            
```
Resulting in issue when parsed by this script.
`unit_state` would look like `'static          -'`

This fix allow any number of field to be split, but only keep the first 2.
@Vaarlion Vaarlion changed the base branch from develop to master February 17, 2021 20:04
@Vaarlion
Copy link
Contributor Author

Vaarlion commented Feb 17, 2021

Please add some tests for the two different outputs to unit/modules/test_systemd_service.py

Also, please rebase this against the master branch. Develop is no longer used...

test have been added and the pre-commit did some cleanup.
EDIT: The test don't work, and right now i have no idea what those BAZ are supposed to be ...

@Vaarlion
Copy link
Contributor Author

sorry for all the noise.
I had to look how to run test locally
I don't get how, but somehow the fake service foo have to be ignored, bar disable et baz enable.
I've just matched the test to that, but i have no idea how it work so i couldn't add quxfor static.

I hop this is enough

@sagetherage sagetherage removed develop Pointing to develop branch; needs rebase Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases labels Feb 18, 2021
@sagetherage sagetherage added the Aluminium Release Post Mg and Pre Si label Feb 18, 2021
@sagetherage
Copy link
Contributor

looking good! I have updated the labels to reflect all your work and thank you!!

@sagetherage
Copy link
Contributor

it would be nice to get a changelog file since this fixes a bug @Vaarlion

@twangboy
Copy link
Contributor

twangboy commented Feb 18, 2021

@Vaarlion Does this change break those functions on older systemd versions? Or does it now handle both? Can you test the for the old system and for the new system?

@Vaarlion
Copy link
Contributor Author

it would be nice to get a changelog file since this fixes a bug @Vaarlion

I was confused about changelog, reading that those should only be done when in a release ? I'll look into it 👍

@Vaarlion Does this change break those functions on older systemd versions? Or does it now handle both? Can you test the for the old system and for the new system?

The change allow to work with both the old and new format, and the test are making sure both will work down the line.
I believe there is no reason to remove support anytime soon :D

twangboy
twangboy previously approved these changes Feb 19, 2021
@Vaarlion
Copy link
Contributor Author

Is that changelog good enough ?

@Ch3LL Ch3LL merged commit dbb3158 into saltstack:master Feb 24, 2021
@Vaarlion Vaarlion deleted the systemd_service_get-issue branch February 24, 2021 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aluminium Release Post Mg and Pre Si
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] module service.get_enabled/disabled/static nolonger work with latest systemd
5 participants