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.

Sign up for GitHub

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

Bug 1913536: update.go: add broken symlink check + removal during unit enable #2338

Merged
merged 2 commits into from
Jan 26, 2021
Merged

Bug 1913536: update.go: add broken symlink check + removal during unit enable #2338

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea1d58c
update.go: add broken symlink check + removal during unit enable
yuqi-zhang Jan 13, 2021
8e90105
enableUnits: only attempt to fix symlinks on RHEL nodes
yuqi-zhang Jan 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion pkg/daemon/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1432,7 +1432,40 @@ func (dn *Daemon) enableUnits(units []string) error {
args := append([]string{"enable"}, units...)
stdouterr, err := exec.Command("systemctl", args...).CombinedOutput()
Copy link
Contributor

@darkmuggle darkmuggle Jan 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The effect of this is that any failure to enable the units will trigger the check. Since you are changing on the on-disk state, you may have to do a systemctl daemon-reload.

Under the axiom that RHEL 7 can't handle null-target symlinks, I wonder if it would be simpler/safer to:

  • check for symlinks in /etc/systed/system (since you can't assume the unit is just for multi-user.target.wants)
  • check if the target is dangling, remove it
  • then systemctl-enable
  • guard for RHEL 7 only

Copy link
Contributor Author

@yuqi-zhang yuqi-zhang Jan 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The effect of this is that any failure to enable the units will trigger the check. Since you are changing on the on-disk state, you may have to do a systemctl daemon-reload.

Right, and I propagate the old error just in case. Put another way I am just performing one extra "cleanup" when I fail the first time, so normal operation specifically doesn't hit this.

check for symlinks in /etc/systed/system (since you can't assume the unit is just for multi-user.target.wants)

I don't want to do that because then we'd be potentially managing items not defined in a machineconfig or correct user error which I do not want to happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, if user error occurs, I still want to error as usual. I don't want this to actually fix incorrect unit definitions, for example, or if a user wrote a bad link with a files section config. I am only attempting to clean up bad links from our own code pre 4.7. I can try to make that clearer in the commit message if you think the approach makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you be ok if the logic was the same, but I only attempted this on RHEL7? Or if I parse the error specifically for "file exists" which is what the rhel 7 bug was? (somewhat hacky I feel)

if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we do a check for RHEL 7?

Copy link
Contributor Author

@yuqi-zhang yuqi-zhang Jan 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

that's another option, I could hard check for RHEL7 and remove broken symlinks that we wrote originally
edit: added a RHEL check

return fmt.Errorf("error enabling unit: %s", stdouterr)
if !dn.os.IsLikeTraditionalRHEL7() {
return fmt.Errorf("error enabling units: %s", stdouterr)
}
// In RHEL7, the systemd version is too low, so it is unable to handle broken
// symlinks during enable. Do a best-effort removal of potentially broken
// hard coded symlinks and try again.
// See: https://bugzilla.redhat.com/show_bug.cgi?id=1913536
wantsPathSystemd := "/etc/systemd/system/multi-user.target.wants/"
Copy link
Contributor

Choose a reason for hiding this comment

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

The enable code does not assume multi-user.target.wants; the unit could have a different target.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, this attempts to fix ONLY multi-user.target.wants, because that's where we wrote broken symlinks back when we had hardcoding. This ONLY attempts to fix those, and ONLY in the list of enables, and other user error is not handled.

for _, unit := range units {
unitLinkPath := filepath.Join(wantsPathSystemd, unit)
fi, fiErr := os.Lstat(unitLinkPath)
if fiErr != nil {
if !os.IsNotExist(fiErr) {
return fmt.Errorf("error trying to enable unit, fallback failed with %s (original error %s)",
fiErr, stdouterr)
}
continue
}
if fi.Mode()&os.ModeSymlink == 0 {
return fmt.Errorf("error trying to enable unit, a non-symlink file exists at %s (original error %s)",
unitLinkPath, stdouterr)
}
if _, evalErr := filepath.EvalSymlinks(unitLinkPath); evalErr != nil {
// this is a broken symlink, remove
if rmErr := os.Remove(unitLinkPath); rmErr != nil {
return fmt.Errorf("error trying to enable unit, cannot remove broken symlink: %s (original error %s)",
rmErr, stdouterr)
}
}
}
stdouterr, err := exec.Command("systemctl", args...).CombinedOutput()
if err != nil {
return fmt.Errorf("error enabling units: %s", stdouterr)
}
}
glog.Infof("Enabled systemd units: %v", units)
return nil
Expand Down