Bug 1044934

<Extracting the public info from Comment 0>

Description of problem:
Beaker doesn't check the uploaded task's version is bigger than the task which exists in beaker server. 
So if user uploads a old version, and run a job with it. The job will be aborted. 

Version-Release number of selected component (if applicable):
develop

How reproducible:
100%

Steps to Reproduce:
1. Create a task name: test-upload. The version is 1.0 . And upload this task
2. Modify Makefile: TESTVERSION=1.1 ,then upload this task
3. Modify Makefile: TESTVERSION=1.0 ,then upload this task
4. Write a job which will use this task.

Actual results:
It's aborted.  

Expected results:
Step3: don't allow to upload the old version task. 
or
Step4: The task rpm package can be installed on test machine.

We definitely want to allow reversion of tasks to old versions - if a new version is uploaded in error, people should be able to correct that.

However, it's very strange that the downgrade isn't working.

Dan pointed out that the relevant comment is this one in the TaskLibrary repo metadata generation code:

http://git.beaker-project.org/cgit/beaker/tree/Server/bkr/server/model.py?id=80f4711bc9977a3413b2062dca79642dbfaf3bc3#n7129

The TaskLibrary metadata generation currently runs when *both* RPMs are in the directory, on the assumption that the task versions always increase, so the new one will be the one yum looks for and it won't matter that there's a reference to the old file in the metadata as well.

However, that assumption is incorrect for downgrades - the "newer" version is the one that gets removed, yum picks it, and then the install attempt fails because it isn't actually there. It will remain in this broken state until a new task is uploaded, thus regenerating the task library metadata with proper acknowledgement of the downgrade.

To fix that, we need to change the way we regenerate the metadata (we're holding the Flock during this process, so other people can't mess with what we're doing):

- move the old RPM aside (so it is left out of the new metadata)
- generate the new metadata
- on success, unlink the renamed old RPM
- on failure, restore the old RPM to its original name and unlink the new one

There's a tiny chance of leaving the disk in an inconsistent state if renaming the old RPM back to its original name fails, but the risk of that is considered minimal given that it's the inverse of the original moving aside.

Alternatively, we could change the Task Library such that downgrades are actively disallowed. Dan's other point was that even if downgrades are "disallowed" from the Beaker point of view, users are still free to revert changes in source control, tag a "new" version of the task and upload that.

(Longer term, avoiding these kinds of problems is one of the main reasons for moving to git based tests, leaving the task library solely for utility tasks like reservesys, the inventory task and guest recipe provisioning)

Given the long term plan to move to git based tasks, and the intrusive nature of attempting to fix this properly, I think we should go with the option of simply disallowing task downgrades - the unwanted changes should instead be reverted externally and the task "updated".

So it turns out there *is* a workaround for this: downgrade to an even *older* version first, and then "upgrade" to the version you're trying to revert to.

We figured that out while trying to ensure that releasing a new version of reservesys for 1055815 wasn't going to trigger this bug :)

This will need a config setting - it should be off by default, but easy to turn back on if it causes problems for existing installations.

(In reply to Nick Coghlan from comment #9)
> This will need a config setting - it should be off by default, but easy to
> turn back on if it causes problems for existing installations.

Do you mean that we should "disallow" by default but allow installations to "allow" if needed?

Right, similar to the job permission changes. Those do the right thing by default (i.e. no implicit permissions for group members on individual jobs), but allow the legacy behaviour to be turned back on easily for existing installations if users object to the change.

In this case, a fresh Beaker installation should disallow downgrades, but there should be a setting to disable that check.

However, it occurs to me: if we know something is a downgrade, can't we just run createrepo again automatically before releasing the flock, *specifically* in the downgrade case?

So downgrades would get the two-calls-to-createrepo-with-the-flock held hit, but the bug itself still wouldn't recur, even if the "allow downgrades" setting was enabled.

Thinking about this some more, and I think we really do need to support task downgrades properly.

The specific case I am thinking of is when you have uploaded a new version of a task and it turns out to have problems - the natural thing to want to do is to just upload the old version again, not revert the changes in source control, tag and build a "new" version of the task and publish that.

So changing the issue title to reflect the idea of special casing downgrades to include the extra repo metadata rebuild after the old file is removed.

(In reply to Nick Coghlan from comment #12)
> Thinking about this some more, and I think we really do need to support task
> downgrades properly.
> 
> The specific case I am thinking of is when you have uploaded a new version
> of a task and it turns out to have problems - the natural thing to want to
> do is to just upload the old version again, not revert the changes in source
> control, tag and build a "new" version of the task and publish that.
> 

I think this is subjective. Whenever I have had this kind of problem with my Beaker I have fixed it in source, and upgraded to the new version. After all, if we have broken it in source, then it needs to be fixed in the source anyway.





> So changing the issue title to reflect the idea of special casing downgrades
> to include the extra repo metadata rebuild after the old file is removed.

Yes, but think about it from the perspective of a Beaker administrator updating to a new version of a standard task. For them, if the new version doesn't work, they have to file a bug with us to get it fixed - they can't just patch it and build a new version themselves.

So they need to be able to revert it back to the version they had before while their issue is resolved.

"Just patch it" works OK if you're uploading a task you wrote yourself - it doesn't work at all with the standard tasks we provide, or if there is a mandatory QA process involved before a new version of an existing task is uploaded to production.

From earlier comments, I think this sounds like a fair approach to take:

1. Have a server setting to allow/disallow task downgrades
2. If task downgrade is enabled, run createrepo twice with the lock held, if the task uploaded is a downgrade.

Something like this in code: http://fpaste.org/79721/

If we can make it work selectively (that is, only doing the second metadata rebuild on a downgrade), I don't think we need the config option.

http://gerrit.beaker-project.org/#/c/2843/1

Beaker 0.16.0 has been released.