diff mbox

[OSSTEST,v2] cs-adjust-flight: Add jobs-rename command which applies a perlop to job names

Message ID 1453207669-6677-1-git-send-email-ian.campbell@citrix.com
State New, archived
Headers show

Commit Message

Ian Campbell Jan. 19, 2016, 12:47 p.m. UTC
My intention was to allow creation of adhoc jobs based on a template
but modified e.g. to enable/disable XSM with a sequence something
like:

./cs-adjust-flight $flight copy-jobs $template test-foo-xsm
./cs-adjust-flight $flight jobs-rename test-foo-xsm 's/-xsm$//'
./cs-adjust-flight $flight runvar-set $job enable_xsm false
./cs-adjust-flight ... update %buildjob

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
v2: s/job-rename/jobs-rename/
    I left job-status alone.
---
 cs-adjust-flight | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Ian Jackson Jan. 19, 2016, 2:25 p.m. UTC | #1
Ian Campbell writes ("[PATCH OSSTEST v2] cs-adjust-flight: Add jobs-rename command which applies a perlop to job names"):
> My intention was to allow creation of adhoc jobs based on a template
> but modified e.g. to enable/disable XSM with a sequence something
> like:
...
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

I know I acked this, and my next observations don't mean we shouldn't
apply it as-is, but:

> +    UPDATE jobs
> +       SET job = ?
> +     WHERE flight = ? AND job = ?
> +END

This can be applied to flights in intended blessing `play' in
blessings later than `constructing', or via `running:F' to running
flights.

And if that happens, there might be entries in the steps table which
ought also to be renamed.  If you don't rename them you will trip the
foreign key constraint.

I think a similar problem exists with jobs_del.

Note that this constraint is INITIALLY IMMEDIATE.  If we were to make
it INITIALLY DEFERRED then this code would become dangerous, because
   ./cs-adjust-flight F jobs-rename x 's/x/y/' copy-jobs G x
would leave F.x containing the steps from the old x but the
recipe and runvars from G.x.  But I don't think INITIALLY DEFERRED is
a good default for constraints so this is probably not relevant.

Ian.
Ian Campbell Jan. 19, 2016, 2:36 p.m. UTC | #2
On Tue, 2016-01-19 at 14:25 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH OSSTEST v2] cs-adjust-flight: Add jobs-
> rename command which applies a perlop to job names"):
> > My intention was to allow creation of adhoc jobs based on a template
> > but modified e.g. to enable/disable XSM with a sequence something
> > like:
> ...
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> I know I acked this, and my next observations don't mean we shouldn't
> apply it as-is, but:
> 
> > +    UPDATE jobs
> > +       SET job = ?
> > +     WHERE flight = ? AND job = ?
> > +END
> 
> This can be applied to flights in intended blessing `play' in
> blessings later than `constructing', or via `running:F' to running
> flights.
> 
> And if that happens, there might be entries in the steps table which
> ought also to be renamed.  If you don't rename them you will trip the
> foreign key constraint.

But fail immediately in this invocation of cs-adjust-flight with a
(hopefully) comprehensible SQL error spew?

Or is that the distinction between INNTIALLY IMMEDIATE vs DEFERRED you were
trying to get at?

Having stuffed the flight in this way would it have effects elsewhere (i.e.
on production flights) or do you just get to keep both pieces of this
particular flight?

> 
> I think a similar problem exists with jobs_del.
>
> Note that this constraint is INITIALLY IMMEDIATE.  If we were to make
> it INITIALLY DEFERRED then this code would become dangerous, because
>    ./cs-adjust-flight F jobs-rename x 's/x/y/' copy-jobs G x
> would leave F.x containing the steps from the old x but the
> recipe and runvars from G.x.  But I don't think INITIALLY DEFERRED is
> a good default for constraints so this is probably not relevant.
> 
> Ian.
Ian Campbell Jan. 19, 2016, 2:58 p.m. UTC | #3
On Tue, 2016-01-19 at 14:44 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH OSSTEST v2] cs-adjust-flight: Add jobs-
> rename command which applies a perlop to job names"):
> > On Tue, 2016-01-19 at 14:25 +0000, Ian Jackson wrote:
> > > And if that happens, there might be entries in the steps table which
> > > ought also to be renamed.  If you don't rename them you will trip the
> > > foreign key constraint.
> > 
> > But fail immediately in this invocation of cs-adjust-flight with a
> > (hopefully) comprehensible SQL error spew?
> 
> Yes.
> 
> > Or is that the distinction between INNTIALLY IMMEDIATE vs DEFERRED you
> > were
> > trying to get at?
> 
> Indeed.  With the current (and likely future) configuration of the
> constraint, the UPDATE statement will fail and the transaction will
> not commit.  So none of the updates in the invocation will take
> effect.
> 
> > Having stuffed the flight in this way would it have effects elsewhere
> > (i.e.
> > on production flights) or do you just get to keep both pieces of this
> > particular flight?
> 
> If the constraints were changed, you might manage to use
> cs-adjust-flight to produce strangeness in the flight you were
> adjusting.  If that flight were later exposed to archaeologists it
> might contain lies.  TBH this seems far-fetched.

Indeed, especially since:

 * Archaeologists typically don't look at play flights (or if you ask them
   to you should expect breakage).

 * If a real flight somehow got into this state (via running:N or being
   constructing) then I would expect it would trip over the constraint
   before it could complete, and therefore never get to its intended
   blessing in the db, and would again be ignored by archaeologists in
   general I think?

 * cs-adjust-flight already stops one from messing with a completed flight.

Ian.

Ian.
diff mbox

Patch

diff --git a/cs-adjust-flight b/cs-adjust-flight
index a72cd88..04f5a7e 100755
--- a/cs-adjust-flight
+++ b/cs-adjust-flight
@@ -18,6 +18,7 @@ 
 #   intended-blessing <intended-blessing>
 #   branch-set <new-branch>
 #   job-status <job-spec>
+#   jobs-rename <job-spec> <perl-expr>
 #
 # <foo-spec>:
 #   <foo-name>
@@ -371,6 +372,29 @@  END
     });
 }
 
+sub change__jobs_rename {
+    die unless @changes >= 2;
+    my $jobs = shift @changes;
+    my $perlop = shift @changes;
+
+    my $jobq = $dbh_tests->prepare(<<END);
+    UPDATE jobs
+       SET job = ?
+     WHERE flight = ? AND job = ?
+END
+    my $runvarq = $dbh_tests->prepare(<<END);
+    UPDATE runvars
+       SET job = ?
+     WHERE flight = ? AND job = ?
+END
+    for_jobs($dstflight, $jobs, sub {
+	my ($oldjob) = @_;
+	my $newjob = perlop_value ($oldjob, '', $perlop, $oldjob);
+	print "$oldjob renamed to $newjob\n";
+	$jobq->execute($newjob, $dstflight, $oldjob);
+	$runvarq->execute($newjob, $dstflight, $oldjob);
+    })
+}
 sub changes () {
     debug("CHANGES...\n");