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

Persist package installation across reboots #1185

Merged
merged 3 commits into from
Nov 14, 2016

Conversation

peterfeiner
Copy link
Contributor

Rather than installing stuff in /tmp/pkb, which gets wiped out on
reboot, use /opt/pkb. Keeping packages around after a reboot is
essential for re-using the perfkitbenchmarker.linux_packages.* code
for image preparation.

@peterfeiner
Copy link
Contributor Author

@tedsta another one for you - please take a look.


FLAGS = flags.FLAGS

GIT_REPO = 'https://github.com/aerospike/aerospike-server.git'
GIT_TAG = '3.7.5'
AEROSPIKE_DIR = '%s/aerospike-server' % vm_util.VM_TMP_DIR
AEROSPIKE_DIR = '%s/aerospike-server' % INSTALL_DIR
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate making the change look like the previous item but can we use the same posixpath.join(x,y) as used elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code seems to be split about 50/50. You'll notice that most of the places where I changed vm_util.VM_TMP_DIR to INSTALL_DIR use format strings.

I don't mind changing everything to posixpath.join, but I'd prefer to do it as a follow-on commit. I'll change everything, including the stuff that still uses VM_TMP_DIR. I'm also willing to change everything to use format strings -- that'd be my preference since posixpath.join() is just as portable as '%s/%s', but I'm not picky.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good

@@ -11,7 +11,7 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
"""Builds collectd from source, installs to VM_TMP_DIR.
"""Builds collectd from source, installs to /opt/pkb.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just say installs to INSTALL_DIR so that we won't have to change it again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@cwilkes
Copy link
Contributor

cwilkes commented Nov 4, 2016

Is this meant to change all the installation directories in all benchmarks? git grep vm_util.VM_TMP shows what could be a lot of other places where it could be an installation directory.

@tedsta
Copy link
Contributor

tedsta commented Nov 4, 2016

Unfortunately I don't think github allows me to comment on code that's not in the vicinity of changes, but:

In linux_virtual_machine.py in the PackageCleanup, SnapshotPackages, and RestorePackages methods of BaseLinuxMixin, vm_util.VM_TMP_DIR is still used. I don't think it breaks anything but it'd be nice to remain consistent.

I noticed PackageCleanup is only ever used for static VMs. Did you leave out deleting /opt/pkb in there for a reason?

@peterfeiner peterfeiner force-pushed the persist-installs branch 2 times, most recently from 0c6269e to e007c94 Compare November 4, 2016 23:07
@peterfeiner
Copy link
Contributor Author

@tedsta wrote:

In linux_virtual_machine.py in the CleanupPackages, SnapshotPackages, and RestorePackages methods of BaseLinuxMixin, vm_util.VM_TMP_DIR is still used. I don't think it breaks anything but it'd be nice to remain consistent.

Good catch. I've fixed these.

I noticed CleanupPackages is only used for static VMs. Did you leave out deleting /opt/pkb in there for a reason?

I'm not sure where you mean. There isn't any function called CleanupPackages anywhere.

@peterfeiner
Copy link
Contributor Author

@cwilkes wrote:

Is this meant to change all the installation directories in all benchmarks? git grep vm_util.VM_TMP shows what could be a lot of other places where it could be an installation directory.

I had looked through with git grep 'temp\|tmp\|TempDir\|temp_dir\|TMP_DIR' perfkitbenchmarker/linux_packages, but, as your command shows, there are quite a few uses of vm_util.VM_TMP outside of linux_packages. I'll go through all of the matches I get from git grep '\<temp\>\|\<tmp\>\|TempDir\|temp_dir\|TMP_DIR'

@tedsta
Copy link
Contributor

tedsta commented Nov 4, 2016

Sorry, meant PackageCleanup not CleanupPackages. Edited above comment.

@peterfeiner
Copy link
Contributor Author

@tedsta I did modify PackageCleanup to delete /opt/pkb instead of /tmp/pkb since none of the packages install stuff to /tmp/pkb anymore.

Rather than installing stuff in /tmp/pkb, which gets wiped out on
reboot, use /opt/pkb. Keeping packages around after a reboot is
essential for re-using the perfkitbenchmarker.linux_packages.* code
for image preparation.
@peterfeiner
Copy link
Contributor Author

@cwilkes There were two more places where vm_util.VM_TMP was being used as an installation directory: sysbench05plus and scimark2. I've fixed these. Please take another look.

@tedsta
Copy link
Contributor

tedsta commented Nov 12, 2016

This LGTM, but maybe we should wait for @cwilkes to give his approval too

@cwilkes
Copy link
Contributor

cwilkes commented Nov 14, 2016

Looks good to me

@tedsta tedsta merged commit cb4dde0 into GoogleCloudPlatform:master Nov 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants