Commit fe8cc493 authored by Leigh Stoller's avatar Leigh Stoller

Image handling changes:

1. The primary change is to the Create Image modal; we now allow users
   to optionally specify a description for the image. This needed to be
   plumbed through all the way to the GeniCM CreateImage() API. Since
   the modal is getting kinda overloaded, I rearranged things a bit and
   changed the argument checking and error handling. I think this is the
   limit of what we want to do on this modal, need a better UI in the
   future.

2. Of course, if we let users set descriptions, lets show them on the
   image listing page. While I was there, I made the list look more like
   the classic image list; show the image name and project, and put the
   URN in a tooltip, since in general the URN is noisy to look at.

3. And while I was messing with the image listing, I noticed that we
   were not deleting profiles like we said we would. The problem is that
   when we form the image list, we know the profile versions that can be
   deleted, but when the user actually clicks to delete, I was trying to
   regen that decision, but without asking the cluster for the info
   again. So instead, just pass through the version list from the web
   UI.
parent ef7f6eef
......@@ -2776,10 +2776,11 @@ sub ConsoleURL($$)
#
# Create an Image,
#
sub CreateImage($$$$;$$$$$)
sub CreateImage($$$$;$$$$$$)
{
my ($self, $sliver_urn, $imagename, $update_prepare,
$copyback_uuid, $bsname, $nosnapshot, $mustnotexist, $wholedisk) = @_;
$copyback_uuid, $bsname, $nosnapshot,
$mustnotexist, $wholedisk, $description) = @_;
my $authority = $self->GetGeniAuthority();
my $geniuser = $self->instance()->GetGeniUser();
my $slice = $self->instance()->GetGeniSlice();
......@@ -2818,6 +2819,8 @@ sub CreateImage($$$$;$$$$$)
if ($mustnotexist);
$args->{'wholedisk'} = 1
if ($wholedisk);
$args->{'description'} = $description
if (defined($description));
my $cmurl = $authority->url();
$cmurl = devurl($cmurl) if ($usemydevtree);
......
......@@ -221,6 +221,8 @@ sub DoListImages()
if (!defined($authority));
# URN without the version.
$urn = GeniHRN::GenerateImage($auth,$ospid,$os,undef);
# Put it into the object so that PHP/JS code can find it easy.
$image->{'imagename'} = $os;
# Default to version zero, for old sites not reporting version.
my $vers = (defined($osvers) ? $osvers : 0);
......@@ -258,9 +260,16 @@ sub DoListImages()
#
if (exists($image0->{'project_urn'})) {
my $projhrn = GeniHRN->new($image0->{'project_urn'});
if ($projhrn->domain() eq $OURDOMAIN &&
defined($projhrn->project())) {
my $project = Project->Lookup($projhrn->project());
if ($projhrn->domain() eq $OURDOMAIN) {
my $project;
if (defined($projhrn->project())) {
$project = Project->Lookup($projhrn->project());
}
else {
# Backwards compat; we did not always send project urns.
$project = Project->Lookup($image0->{'pid'});
}
if (defined($project)) {
$ref->{'pid'} = $project->pid();
$ref->{'pid_idx'} = $project->pid_idx();
......@@ -271,6 +280,7 @@ sub DoListImages()
# Remote pid, set above
$ref->{'pid'} = $image0->{'pid'};
}
$ref->{'imagename'} = $image0->{'imagename'};
#
# Find profiles using the named image
......@@ -322,11 +332,18 @@ sub DoListImages()
while (@versions) {
my $image = shift(@versions);
my $urn = $image->{'urn'};
my $hrn = GeniHRN->new($urn);
my @using = ();
$image->{'using'} = [];
$image->{'candelete'} = 0;
$image->{'deleted'} = 0;
my (undef, undef, undef, $osvers) = $hrn->ParseImage();
# Default to version zero, for old sites not reporting version.
my $vers = (defined($osvers) ? $osvers : 0);
# Put it into the object so that PHP/JS code can find it easy.
$image->{'version'} = int($vers);
next
if (APT_Profile::ImageInfo::FindProfilesUsing($urn, \@using));
......@@ -395,7 +412,7 @@ sub DoDeleteImage()
print STDERR "Usage: manage_images delete [-a am_urn] <image_urn>\n";
exit(-1);
};
my $optlist = "a:d:n";
my $optlist = "a:d:v:n";
my $aggregate_urn = $MYURN;
my $impotent = 0;
my $profile;
......@@ -422,64 +439,24 @@ sub DoDeleteImage()
if ($profile->isLocked()) {
fatal("Profile is locked down, cannot be deleted");
}
#
# This argument says; delete any version of the specified
# profile, that reference the image being deleted. So we
# have to go through every version of the profile and check
# to see if its using this image. For any of those versions,
# we try to delete it.
# The caller tells us what versions of the image to delete,
#
if (!exists($options{"v"})) {
fatal("Missing version number list");
}
if ($options{"v"} !~ /^[\d,]+$/) {
fatal("Version number list should be comma separated integers");
}
my @todelete = ();
foreach my $version ($profile->AllVersions()) {
my $usingimage = 0;
my $conflict;
#
# Check image references for this version. We want to
# know if there are any other images associated with this
# version beside the one we are trying to delete. If so,
# we cannot delete the profile version since that will
# result in another image getting deleted.
#
my %irefs = %{ $version->images() };
foreach my $client_id (keys(%irefs)) {
my $imageinfo = $irefs{$client_id};
# We do not ever care about system images.
next
if ($imageinfo->ospid() eq "emulab-ops");
# The image we are trying to delete is okay
if ($imageinfo->image() eq $image_urn) {
$usingimage = 1;
next;
}
my $snapname = $profile->name() . "." . $client_id;
if ($imageinfo->os() eq $profile->name() ||
$imageinfo->os() eq $snapname) {
$conflict = $imageinfo;
}
}
if ($usingimage && $conflict) {
my $mesg =
"Version " . $version->version() . " of the " .
$version->name() . " profile has another ".
"image that would be deleted as well: ".
$conflict->image() . ". ".
"You will need to go to the profile page and delete ".
"that profile version before you can delete this image.";
foreach my $versnum (split(",", $options{"v"})) {
my $version = APT_Profile->Lookup($profile->profileid(), $versnum);
next
if (!defined($version));
if ($webtask) {
$webtask->reason("conflict");
$webtask->profile($version->uuid());
$webtask->image($conflict->image());
}
UserError($mesg);
}
if ($usingimage && $version->isLocked()) {
if ($version->isLocked()) {
my $mesg =
"Version " . $version->version() . " of the " .
$version->name() . " profile is locked down, ".
......@@ -491,11 +468,42 @@ sub DoDeleteImage()
}
UserError($mesg);
}
if ($usingimage) {
push(@todelete, $version);
print "Would delete version " . $version->version() .
" of profile " . $profile->name() . "\n";
#
# Check image references for this version. We want to
# know if there are any other images associated with this
# version beside the one we are trying to delete. If so,
# we cannot delete the profile version since that will
# result in another image getting deleted.
#
my %irefs = %{ $version->images() };
if (keys(%irefs) > 1) {
foreach my $client_id (keys(%irefs)) {
my $imageinfo = $irefs{$client_id};
# We do not ever care about system images.
next
if ($imageinfo->ospid() eq "emulab-ops");
my $mesg =
"Version " . $version->version() . " of the " .
$version->name() . " profile is using multiple ".
"images. As a safety measure, we require that you ".
"delete or edit that profile before you can delete ".
"this image.";
if ($webtask) {
$webtask->reason("conflict");
$webtask->profile($version->uuid());
$webtask->image($imageinfo->image());
}
UserError($mesg);
}
}
print "Would delete version " . $version->version() .
" of profile " . $profile->name() . "\n";
push(@todelete, $version);
}
foreach my $version (@todelete) {
my $vers = $version->version();
......
......@@ -299,6 +299,7 @@ sub DoSnapshot()
my $old_status = $instance->status();
my $node_id;
my $imagename;
my $description;
my $cloneprofile;
my $update_profile;
my $copyback_uuid;
......@@ -312,7 +313,7 @@ sub DoSnapshot()
my $usetracker = 0;
my $operation = "image-only"; # Default to just snapshot.
my $optlist = "n:i:u:Uc:O:Sse";
my $optlist = "n:i:u:Uc:O:SseD:";
my %options = ();
if (! getopts($optlist, \%options)) {
usage();
......@@ -335,6 +336,9 @@ sub DoSnapshot()
if (defined($options{"U"})) {
$update_prepare = 1;
}
if (defined($options{"D"})) {
$description = ReadFile($options{"D"});
}
if (defined($options{"s"})) {
$nosnapshot = 1;
}
......@@ -604,12 +608,14 @@ sub DoSnapshot()
my $response =
$aggregate->CreateImage($sliver_urn, $imagename,
$update_prepare, $copyback_uuid,
undef, $nosnapshot, $mustnotexist, $wholedisk);
undef, $nosnapshot, $mustnotexist, $wholedisk,
$description);
if ($response->code() != GENIRESPONSE_SUCCESS) {
$errcode = $response->code();
($exitcode,$errmsg) = ResponseErrorMessage($aggregate, $response);
# Important to tell web user about these.
if ($response->code() == GENIRESPONSE_NOSPACE ||
$response->code() == GENIRESPONSE_FORBIDDEN ||
$response->code() == GENIRESPONSE_ALREADYEXISTS) {
$exitcode = 1;
}
......@@ -933,7 +939,7 @@ sub DoSnapshot()
$slice->UnLock()
if ($needunlock);
exit($errcode);
exit($exitcode);
}
sub DoImageTrackerStuff($$$$$$$)
......
......@@ -156,6 +156,18 @@ function Do_DeleteImage()
return;
}
$pdarg = "-d " . escapeshellarg($ajax_args["profile-delete"]);
if (!isset($ajax_args["profile-delete-versions"])) {
SPITAJAX_ERROR(-1, "Missing profile version list for deletion");
return;
}
foreach ($ajax_args["profile-delete-versions"] as $vers) {
if (!preg_match("/^\d+$/", $vers)) {
SPITAJAX_ERROR(1, "Illegal characters in version number");
return;
}
}
$pdarg .= " -v " . implode(",", $ajax_args["profile-delete-versions"]);
}
$uid = $target_user->uid();
......@@ -167,6 +179,7 @@ function Do_DeleteImage()
"webmanage_images -t $webtask_id ".
" delete -a '$aggurn' $pdarg $image_urn",
SUEXEC_ACTION_IGNORE);
if ($retval) {
$webtask->Refresh();
if (!$webtask->exited() || $retval < 0) {
......
......@@ -19,6 +19,7 @@ $(function ()
{
window.APT_OPTIONS.initialize(sup);
amlist = decodejson('#amlist-json');
window.IMLIST = imagelist;
$('#oops_div').html(oopsString);
$('#waitwait_div').html(waitwaitString);
......@@ -297,22 +298,33 @@ $(function ()
* should be deleted along with the image. Pass that along,
* the backend is going to check anyway.
*/
var profiles = null;
if ($(row).find("td.delete-profile").length) {
var uuid = $(row).find("td.delete-profile").attr('data-uuid');
args["profile-delete"] = uuid;
}
args["profile-delete"] = uuid;
args["profile-delete-versions"] = [];
/*
* The confirm modal is a template in case we need to warn
* about profiles that will be deleted. Need to find that
* list in the saved data structure.
*/
var profiles = null;
if ($(row).find("td.delete-profile").length) {
/*
* The confirm modal is a template in case we need to warn
* about profiles that will be deleted. Need to find that
* list in the saved data structure.
*/
_.each(imagelist[cluster], function(image, index) {
_.each(image.versions, function(version, index) {
if (version.urn == urn) {
profiles = version.using;
/*
* Add the version list to the args.
*/
_.each(profiles, function(profile, i) {
_.each(profile.versions, function(version, j) {
args["profile-delete-versions"]
.push(version.version);
});
});
// Just one profile can be deleted.
return;
}
});
});
......@@ -364,6 +376,7 @@ $(function ()
$('#confirm-delete-image-modal #confirm-delete-image')
.click(function () {
sup.HideModal('#confirm-delete-image-modal');
sup.ShowWaitWait('It takes a moment to delete an image; ' +
'patience please');
......
This diff is collapsed.
......@@ -164,7 +164,7 @@ function Do_Create()
(!$project->IsMember($this_user, $isapproved) || !$isapproved)) {
$errors["profile_pid"] = "Illegal project";
}
elseif ($action == "create" &&
elseif (($action == "create" || $action == "clone") &&
Profile::LookupByName($project, $formfields{"profile_name"})) {
$errors["profile_name"] = "Already in use";
}
......
......@@ -659,6 +659,7 @@ function Do_Snapshot()
{
global $this_user, $instance, $suexec_output;
global $ajax_args;
$errors = array();
if (StatusSetupAjax(0)) {
return;
......@@ -669,6 +670,8 @@ function Do_Snapshot()
}
$this_idx = $this_user->uid_idx();
$uuid = $ajax_args["uuid"];
$checkonly = (isset($ajax_args["checkonly"]) &&
$ajax_args["checkonly"] == 1 ? 1 : 0);
#
# As per Rob, if an experiment is locked down, then only the creator,
......@@ -678,7 +681,7 @@ function Do_Snapshot()
if ($this_idx != $instance->creator_idx() && !ISADMIN() &&
!$instance->Project()->IsLeader($this_user)) {
SPITAJAX_ERROR(1, "Not enough permission, ".
"experiment is locked down. Maybe Clone instead?");
"experiment is locked down.");
return;
}
}
......@@ -711,17 +714,18 @@ function Do_Snapshot()
}
if ($operation == "update-profile" &&
($this_idx != $profile->creator_idx() && !ISADMIN())) {
SPITAJAX_ERROR(1, "Not your profile to change. Clone first!");
SPITAJAX_ERROR(1, "Not your profile to change. Make a copy first!");
return;
}
$optargs = "";
if (isset($ajax_args["node_id"]) && $ajax_args["node_id"] != "") {
$node_id = $ajax_args["node_id"];
if (!TBvalid_vnode_id($node_id)) {
SPITAJAX_ERROR(1, "Bad node id");
return;
$errors["node_id"] = "Invalid node id";
}
else {
$optargs .= " -n $node_id ";
}
$optargs .= " -n $node_id ";
}
if (isset($ajax_args["update_prepare"]) &&
$ajax_args["update_prepare"]) {
......@@ -735,10 +739,40 @@ function Do_Snapshot()
}
if (isset($ajax_args["imagename"]) && $ajax_args["imagename"] != "") {
if (!TBvalid_imagename($ajax_args["imagename"])) {
SPITAJAX_ERROR(1, "Not a valid imagename, alphanumeric only");
return;
$errors["imagename"] = "Invalid imagename, alphanumeric only";
}
else {
$optargs .= " -i " . escapeshellarg($ajax_args["imagename"]);
}
$optargs .= " -i " . escapeshellarg($ajax_args["imagename"]);
}
if (isset($ajax_args["description"]) && $ajax_args["description"] != "") {
$desc = $ajax_args["description"];
if (!TBvalid_fulltext($desc)) {
$errors["description"] = "Illegal characters";
}
elseif (strlen($desc) > 225) {
$errors["description"] = "Too long, must be < 225 chars";
}
elseif (strlen($desc) < 10) {
$errors["description"] = "Too short, must be >= 10 chars";
}
elseif (!$checkonly) {
$filename = tempnam("/tmp", "description");
$fp = fopen($filename, "w");
fwrite($fp, $desc);
fclose($fp);
chmod($filename, 0666);
$optargs .= " -D $filename";
}
}
if (count($errors)) {
SPITAJAX_ERROR(2, $errors);
return;
}
if ($checkonly) {
SPITAJAX_RESPONSE("Success");
return;
}
$optargs .= " -O $operation ";
......@@ -751,7 +785,9 @@ function Do_Snapshot()
" snapshot $uuid $optargs",
SUEXEC_ACTION_IGNORE);
$webtask->Refresh();
if (isset($filename)) {
unlink($filename);
}
if ($retval != 0) {
if (!$webtask->exited() || $retval < 0) {
SUEXECERROR(SUEXEC_ACTION_CONTINUE);
......
......@@ -22,7 +22,7 @@
padding: 2px;
}
</style>
<div class='col-lg-10 col-lg-offset-1
<div class='col-lg-12 col-lg-offset-0
col-md-12 col-md-offset-0
col-sm-12 col-sm-offset-0
col-xs-12 col-xs-offset-0'>
......@@ -45,10 +45,10 @@
<th>Name</th>
<th>Project</th>
<th>Created</th>
<th class="sorter-false">Description</th>
<% if (showformat) { %>
<th>Format</th>
<% } %>
<th class="sorter-false">Description</th>
<th class="sorter-false">URN</th>
</tr>
</thead>
......@@ -60,10 +60,10 @@
<td><a href='show-project.php?pid=<%- value.pid_idx %>'>
<%= value.pid %></a></td>
<td class="format-date"><%- value.created %></td>
<td><%- value.description %></td>
<% if (showformat) { %>
<td><%- value.format %></td>
<% } %>
<td><%- value.description %></td>
<td align="center">
<a href="#"
data-toggle='popover'
......
......@@ -22,7 +22,7 @@
padding: 2px;
}
</style>
<div class='col-lg-10 col-lg-offset-1
<div class='col-lg-12 col-lg-offset-0
col-md-12 col-md-offset-0
col-sm-12 col-sm-offset-0
col-xs-12 col-xs-offset-0'>
......@@ -63,12 +63,15 @@
</caption>
<thead>
<tr>
<th class="col-md-5">Image URN</th>
<th class="col-md-2">Image</th>
<th class="col-md-2">Project</th>
<th class="col-md-2">Created</th>
<th class="col-md-3 sorter-false">Description</th>
<% if (showformat) { %>
<th>Format</th>
<% } %>
<th>File Size</th>
<th class="sorter-false">URN</th>
</tr>
</thead>
<tbody>
......@@ -79,18 +82,42 @@
<td>
<a href="" class="toggle-image">
<span class="glyphicon glyphicon-chevron-right"></span></a>
<%- image.urn %>
<%- image.imagename %>
<% if (image.candelete) { %>
<a href="" class="delete-button pull-right">
<span class='glyphicon glyphicon-remove'
style='color: red; margin-left: 3px;'></span></a>
<% } %>
</td>
<% if (_.has(image, "pid_idx")) { %>
<td>
<a target="_blank"
href=show-project.php?pid=<%- image.pid_idx %>>
<%- image.pid %>
</a>
</td>
<% } else { %>
<td><%- image.pid %></td>
<% } %>
<td class="format-date"><%- image.versions[0].created %></td>
<td><%- image.versions[0].description %></td>
<% if (showformat) { %>
<td><%- image.versions[0].format %></td>
<% } %>
<td class="image-filesize"></td>
<td align="center">
<a href="#"
data-toggle='popover'
data-html='true'
data-trigger='click'
data-title="URN for your geni-lib script or RSpec"
data-content="<input type=text readonly=readonly
class=form-control
onClick='this.select();'
value='<%- image.urn %>'>">
<span class="glyphicon glyphicon-link"></span>
</a>
</td>
</tr>
<% _.each(image.versions, function(version, versionindex) { %>
<% if (! version.candelete) { return; } %>
......@@ -100,18 +127,34 @@
data-index="<%- versionindex %>"
data-imageindex="<%- imageindex %>">
<td>
<span style="padding-left: 15px"><%- version.urn %></span>
<span style="padding-left: 15px">
<%- image.imagename %>:<%- version.version %></span>
<% if (version.candelete) { %>
<a href="" class="delete-button pull-right">
<span class='glyphicon glyphicon-remove'
style='color: red; margin-left: 3px;'></span></a>
<% } %>
</td>
<td><%- image.pid %></td>
<td class="format-date"><%- version.created %></td>
<td><%- version.description %></td>
<% if (showformat) { %>
<td><%- version.format %></td>
<% } %>
<td class="version-filesize"><%- version.filesize %></td>
<td align="center">
<a href="#"
data-toggle='popover'
data-html='true'
data-trigger='click'
data-title="URN for your geni-lib script or RSpec"
data-content="<input type=text readonly=readonly
class=form-control
onClick='this.select();'
value='<%- version.urn %>'>">
<span class="glyphicon glyphicon-link"></span>
</a>
</td>
</tr>
<% }); %>
<% }); %>
......@@ -152,12 +195,15 @@
</caption>
<thead>
<tr>
<th class="col-md-5">Image URN</th>
<th class="col-md-2">Image</th>
<th class="col-md-2">Project</th>
<th class="col-md-2">Created</th>
<th>File Size</th>
<th class="col-md-3 sorter-false">Description</th>
<% if (showformat) { %>
<th>Format</th>
<% } %>
<th>File Size</th>
<th class="sorter-false">URN</th>
<th class="sorter-false">Profile</th>
</tr>
</thead>
......@@ -176,13 +222,37 @@
<td>
<a href="" class="toggle-image">
<span class="glyphicon glyphicon-chevron-right"></span></a>
<%- image.urn %>
<%- image.imagename %>
</td>
<% if (_.has(image, "pid_idx")) { %>
<td>
<a target="_blank"
href=show-project.php?pid=<%- image.pid_idx %>>
<%- image.pid %>
</a>
</td>
<% } else { %>
<td><%- image.pid %></td>
<% } %>
<td class="format-date"><%- image.versions[0].created %></td>
<td><%- image.versions[0].description %></td>
<% if (showformat) { %>
<td><%- image.versions[0].format %></td>
<% } %>
<td class="image-filesize"></td>
<td align="center">
<a href="#"
data-toggle='popover'
data-html='true'
data-trigger='click'
data-title="URN for your geni-lib script or RSpec"
data-content="<input type=text readonly=readonly
class=form-control
onClick='this.select();'
value='<%- image.urn %>'>">
<span class="glyphicon glyphicon-link"></span>
</a>
</td>
<td></td>
</tr>
<% _.each(image.versions, function(version, versionindex) { %>
......@@ -194,16 +264,32 @@
data-index="<%- versionindex %>"
data-imageindex="<%- imageindex %>">
<td>
<span style="padding-left: 15px"><%- version.urn %></span>
<span style="padding-left: 15px">
<%- image.imagename %>:<%- version.version %></span>
<a href="" class="delete-button pull-right">
<span class='glyphicon glyphicon-remove'
style='color: red; margin-left: 3px;'></span></a>
</td>
<td><%- image.pid %></td>
<td class="format-date"><%- version.created %></td>
<td><%- version.description %></td>
<% if (showformat) { %>
<td><%- version.format %></td>
<% } %>
<td class="version-filesize"><%- version.filesize %></td>
<td align="center">
<a href="#"
data-toggle='popover'
data-html='true'
data-trigger='click'
data-title="URN for your geni-lib script or RSpec"
data-content="<input type=text readonly=readonly
class=form-control
onClick='this.select();'
value='<%- version.urn %>'>">
<span class="glyphicon glyphicon-link"></span>
</a>
</td>
<td class="delete-profile"
data-uuid="<%- version.using[0].uuid %>">
<a target="_blank"
......@@ -214,7 +300,7 @@
(Version
<a target="_blank"
href="show-profile.php?uuid=<%- version.using[0].versions[0].uuid %>">
<%- version.using[0].versions[0].version %>
<%- version.using[0].versions[0].version %>
</a>)
<% } else { %>
(Versions
......@@ -228,6 +314,7 @@
print(vlist.join());
%>)
<% } %>
</td>
</tr>
<% }); %>
<% }); %>
......@@ -251,12 +338,15 @@
</caption>
<thead>
<tr>
<th class="col-md-5">Image URN</th>
<th class="col-md-2">Image</th>
<th class="col-md-2">Project</th>
<th class="col-md-2">Created</th>
<th class="col-md-3 sorter-false">Description</th>
<% if (showformat) { %>
<th>Format</th>
<% } %>
<th>File Size</th>
<th class="sorter-false">URN</th>
</tr>
</thead>
<tbody>
......@@ -273,13 +363,37 @@
<td>
<a href="" class="toggle-image">
<span class="glyphicon glyphicon-chevron-right"></span></a>
<%- image.urn %>
<%- image.imagename %>
</td>
<% if (_.has(image, "pid_idx")) { %>
<td>
<a target="_blank"
href=show-project.php?pid=<%- image.pid_idx %>>
<%- image.pid %>