| Message ID | 1453207669-6677-1-git-send-email-ian.campbell@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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 --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");