How to resolve a broken deploy --verify

[johnrichardrinehart]

I ran into the following case when using sqitch. I had a valid deploy (so it completed successfully), but I ran sqitch deploy --verify foo and foo's verification raised an exception, so sqitch tried to apply the relevant revert. But, the revert was written poorly (bad order of operations) so it failed to execute. I fixed the revert order of operations and tried to run sqitch revert foo but was informed that

$ sqitch revert foo
Change not deployed: "foo"

I ended up resolving this by running psql postgresql://postgres@localhost/gypsy -f sqitch/revert/foo.sql but I didn't like that solution.

How should I handle this inside of sqitch?

[theory]

Good question. This is part of the distinction that Sqitch draws between development and release. I myself has made this sort of mistake with revert scripts many times in the past --- but never in a released (tagged) version of a project, only when doing development and I'm constantly deploying and reverting (or rebasing) to find exactly issues like this. more than once I've dropped a database and started over from scratch when the state got screwed up.

That said, it should not have logged foo as reverted when the revert script failed. Can you make a test case for this?

[johnrichardrinehart]

Though I realized that if your revert script succeeded (didn't die), but didn't do what you needed it to do, then this is to be expected, and the recourse is to manually apply it.

Okay, but in this case the revert script isn't executed. I run deploy --verify and the deploy script is executing successfully but the verify step fails. After this, sqitch is in a state where nothing is deployed (because verify failed), even though the deploy script finished successfully. And, so, in this case, sqitch revert can't be used to undo the applied DB deploy changes since sqitch doesn't see anything as deployed (since verify failed).

It seems that sqitch should exist in a deployed state if deploy finished successfully. Then, I can run sqitch verify until everything looks good or revert, if I want.

[theory]

Yeah, it should absolutely be running the revert script after the verify fails, IIRC. Will have to do some code spelunking.

[theory]

If you run sqitch status, does it show the change as deployed?

[theory]

The deploy script succeeds before the verify runs. Though you may be right that Sqitch doesn't log it as successful if verify fails.

[johnrichardrinehart]

Running sqitch status immediately after sqitch deploy --verify in the above (so, deploy succeeds and verify fails) yields

# On database db:pg://postgres@localhost/sqitch
No changes deployed

[theory]

Okay, taking myself back 10 years to when we added verification, I think the thinking was that verification failure was considered the same as deploy failure, so we never logged the deploy or ran the revert. The logic is here:

sqitch/lib/App/Sqitch/Engine.pm

Lines 972 to 974 in ef7767d

	$self->verify_change( $change ) if $self->with_verify;
	$self->log_deploy_change($change);
	$sqitch->info(__ 'ok');

The verify_change method throws an exception on verify failure, so log_deploy_change is never called.

Thinking on it now, I think it is probably reasonable not to log the deploy, but perhaps the revert script should be run when verify fails. I'm going to post to the developer mail list and see what people think. [Update: see next comment, this is already how it works]

In the meantime, manually applying the revert script on verify failure is the workaround.

[johnrichardrinehart]

Responding to the next comment.

[theory]

Actually I lied: It already works that way. Here's an example where both the verify and revert scripts fail for change baz:

❯ sqitch deploy -d try --verify
Deploying changes to db:pg:try
  + foo .. ok
  + bar .. ok
  + baz .. psql:verify/baz.sql:6: ERROR:  division by zero
Verify script "verify/baz.sql" failed.
psql:revert/baz.sql:6: ERROR:  division by zero
"psql" unexpectedly returned exit value 3

not ok
Reverting all changes
  - bar .. ok
  - foo .. ok
Deploy failed

Note that after it reports the verify script failing on line 6, it then also reports that the revert failed on line 7. In short, if verify fails, it rolls back logging the deploy then runs the revert, ignoring it its failure.

IOW, I think this already works how you'd like. Trouble is, if your revert doesn't do what you want, you will need to modify it and run it manually.

[johnrichardrinehart]

Thanks for running through this. But, I'm still concerned.

In my humble opinion, I believe that this behavior should be changed such that all DB state can be managed exclusively by sqitch (i.e., not using external tools, like psql, even though it isn't awful to use 2 tools).

Fine print: What follows are opinions phrased as "oughts". I'm generally a happy (novice) user of sqitch and greatly appreciate your contributions to the open-source community.

If deploy is successful sqitch shouldn't try to roll back if verify was unsuccessful. I can think of 2 good reasons why this shouldn't be the case:

1. Some may assume (rightly, I'd argue) that the behavior of sqitch deploy && sqitch verify would/should be the same as sqitch deploy --verify. But, my sqitch environment may "break" if I sqitch deploy --verify, like we have in our test cases.
2. verify scripts are used to ensure that the state of the deployment was successful. I didn't create them with the expectation that, if they fail, the DB should revert state. Actually, I created the revert scripts to trigger revert operations which I would hope wouldn't implicitly be executed by any forward-failing action.

Now, there's a solution/path to creating deploy,verify and revert scripts that wouldn't seem to have this problem, but it discourages authoring verify scripts (which is bad, I think). The solution/path would involve writing deploy and revert scripts but omitting anything in the verify script (if verify scripts can't be empty then a BEGIN; ROLLBACK; should be enough). Then I can be assured that I won't enter this "untracked" state which occurs both 1) only on deploy --verify, but not deploy && verify and 2) only when verify fails. If I remember or if I care, then I can fill out my verify script with some sanity-checking, but it's only more likely to introduce problems in the sqitch deploy --verify flow (which I've seen recommended in different sqitch fora, but which I can't find now) so I'd be more likely to abstain from checking my deployments.

Basically, if deploy is successful then my argument is that sqitch should be in a deployed state, regardless if verify failed. I think it makes sense to make state a 2-tuple of (state, valid) where deploy --verify failing on verify would be in (deploy, false) preventing future deploy operations but not automatically attempting to revert (which has the potential to leave you in an invalid sqitch state).

[theory]

In my humble opinion, I believe that this behavior should be changed such that all DB state can be managed exclusively by sqitch (i.e., not using external tools, like psql, even though it isn't awful to use 2 tools).

Part of it is mindset, I think. You're in developer mindset, actively workin on a change. From that POV it would be nicer not to require the use of psql. But as you're doing development, unless you're doing something dead simple, there are going to be times you'll screw things up and have to fix them manually. Sqitch only knows if a script ran without error; it has no way of knowing any other state.

Fine print: What follows are opinions phrased as "oughts". I'm generally a happy (novice) user of sqitch and greatly appreciate your contributions to the open-source community.

💯

If deploy is successful sqitch shouldn't try to roll back if verify was unsuccessful. I can think of 2 good reasons why this shouldn't be the case:

1. Some may assume (rightly, I'd argue) that the behavior of sqitch deploy && sqitch verify would/should be the same as sqitch deploy --verify. But, my sqitch environment may "break" if I sqitch deploy --verify, like we have in our test cases.

I'd like to deprecate sqitch verify TBH. There are times when you run it on a mature project and some verifications fail because the changes they check have changed since they were deployed. It's far more intended as a check for successful deployment, not the state of the database.

2. verify scripts are used to ensure that the state of the deployment was successful. I didn't create them with the expectation that, if they fail, the DB should revert state. Actually, I created the revert scripts to trigger revert operations which I would hope wouldn't implicitly be executed by any forward-failing action.

I think, then, you probably don't want to use --verify, but run sqitch verify separately. It better matches your mindset. But to be clear the intent of verification scripts are to indicate that a deploy was successful, and the implication is that, if a verify fails, the deploy failed and should be reverted.

Now, there's a solution/path to creating deploy,verify and revert scripts that wouldn't seem to have this problem, but it discourages authoring verify scripts (which is bad, I think). The solution/path would involve writing deploy and revert scripts but omitting anything in the verify script (if verify scripts can't be empty then a BEGIN; ROLLBACK; should be enough). Then I can be assured that I won't enter this "untracked" state which occurs both 1) only on deploy --verify, but not deploy && verify and 2) only when verify fails. If I remember or if I care, then I can fill out my verify script with some sanity-checking, but it's only more likely to introduce problems in the sqitch deploy --verify flow (which I've seen recommended in different sqitch fora, but which I can't find now) so I'd be more likely to abstain from checking my deployments.

Right, sqitch assumes a verify failure is a deploy failure that needs to be fixed. (psql scripts can be empty, yes.)

Basically, if deploy is successful then my argument is that sqitch should be in a deployed state, regardless if verify failed. I think it makes sense to make state a 2-tuple of (state, valid) where deploy --verify failing on verify would be in (deploy, false) preventing future deploy operations but not automatically attempting to revert (which has the potential to leave you in an invalid sqitch state).

That just starts to sound complicated to me.