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

Handle libzypp lock #6

Merged
merged 2 commits into from
May 9, 2017
Merged

Conversation

imobachgs
Copy link
Contributor

@imobachgs imobachgs commented May 8, 2017

During installation, we have two libzypp lockfiles: one in /var/run/zypp.pid and another on in /mnt/var/run/zypp.pid. So when the provisioner runs (Salt or Puppet), zypper is locked and you won't be able to install any package.

This code will be executed as part of the inst_finish client as the last save settings step, so I would assume that is safe to backup and restore the libzypp lock.

@imobachgs imobachgs changed the title ]WIP] Handle libzypp lock [WIP] Handle libzypp lock May 8, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 94.391% when pulling 81f5a12 on handle-libzypp-lock into 454b63f on merge-minion-config.

Copy link
Member

@lslezak lslezak left a comment

Choose a reason for hiding this comment

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

Small improvements

@@ -111,6 +113,21 @@ def run_cmd(*args)
rescue Cheetah::ExecutionFailed
false
end

# zypp lock file
ZYPP_PID = Pathname("/mnt/var/run/zypp.pid")
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather use Installation.destdir instead of the hard-coded /mnt.

# zypp lock backup file
ZYPP_PID_BACKUP = ZYPP_PID.sub_ext(".save")

# Run a block without the zypp lock
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment that this is really a nasty hack and should be used carefully, only in very specific cases.

# @param [Proc] Block to run
def without_zypp_lock(&block)
::FileUtils.mv(ZYPP_PID, ZYPP_PID_BACKUP) if ZYPP_PID.exist?
block.call
Copy link
Member

Choose a reason for hiding this comment

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

Please log some warning message, removing the lock is potentially dangerous operation so it should be logged for easier debugging.

::FileUtils.mv(ZYPP_PID, ZYPP_PID_BACKUP) if ZYPP_PID.exist?
block.call
ensure
::FileUtils.mv(ZYPP_PID_BACKUP, ZYPP_PID) if ZYPP_PID_BACKUP.exist?
Copy link
Member

Choose a reason for hiding this comment

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

Again, log also the lock restoration.

#
# @param [Proc] Block to run
def without_zypp_lock(&block)
::FileUtils.mv(ZYPP_PID, ZYPP_PID_BACKUP) if ZYPP_PID.exist?
Copy link
Member

Choose a reason for hiding this comment

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

Um, what if the backup already exists? Either use a unique temporary file name or raise an exception here.

It is very unlikely that the method will be called recursively, but it's better to be on the safe side...

Copy link
Member

Choose a reason for hiding this comment

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

Actually a temporary file does not help, raising an exception is the only way. Otherwise the lock might be restored too early:

def foo
  without_zypp_lock do
    bar
  end
end
  
def baz
  without_zypp_lock do
    foo
    do_something # here the lock is restored!!
  end
end

@imobachgs
Copy link
Contributor Author

Thanks for your comments! I've updated the PR.

@lslezak
Copy link
Member

lslezak commented May 9, 2017

LGTM. Is it still WIP? If not then update the version and add the changes.

@imobachgs imobachgs changed the title [WIP] Handle libzypp lock Handle libzypp lock May 9, 2017
@imobachgs imobachgs merged commit 3cdcccf into merge-minion-config May 9, 2017
@imobachgs imobachgs deleted the handle-libzypp-lock branch May 9, 2017 12:09
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 94.548% when pulling 95154fa on handle-libzypp-lock into 454b63f on merge-minion-config.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 94.548% when pulling 95154fa on handle-libzypp-lock into 454b63f on merge-minion-config.

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

Successfully merging this pull request may close these issues.

None yet

3 participants