Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

batched regression: updates are not present in any repo if karma is supplied quick enough #1930

Closed
kparal opened this issue Oct 24, 2017 · 3 comments · Fixed by #1979
Closed

batched regression: updates are not present in any repo if karma is supplied quick enough #1930

kparal opened this issue Oct 24, 2017 · 3 comments · Fixed by #1979
Assignees
@bowlofeggs
Labels
Composer Issues related to the composer Critical We can't go on living in this sqalor, drop everything and fix it! reliability Issues pertaining to Bodhi's reliability

Comments

@kparal
Copy link
Contributor

kparal commented Oct 24, 2017

Before batched: If karma was supplied quick enough, the package went stable immediately.
With batched: If karma is supplied quick enough, the package goes to batched, and waits for 2 weeks before going stable. During this time, it's not available in any repository. Instead, it should be pushed to updates-testing repo for the duration of the batched tag.

Example:
https://bodhi.fedoraproject.org/updates/FEDORA-2017-6db5c0829b
@bowlofeggs bowlofeggs added Critical We can't go on living in this sqalor, drop everything and fix it! Composer Issues related to the composer reliability Issues pertaining to Bodhi's reliability labels Oct 24, 2017
@bowlofeggs bowlofeggs self-assigned this Nov 6, 2017
@bowlofeggs
Copy link
Contributor

This bug is surprisingly tricky to fix. Bodhi's state machine code is in real bad shape, and is essentially spread all over the codebase. That's how this bug happened in the first place, as its exceedingly easy to make a mistake or overlook something when altering the state machine since it is not clearly expressed in one place as it should be.

I've been considering three different ideas for fixing this bug:

  1. I could make bodhi-push and the masher also select pending:batched updates when mashing testing updates. This seems like the natural thing to do, but I think it might actually be the trickiest solution because so many if statements in the code need to be touched to say something like "or update.status is pending and update.request is batched".
  2. It occurred to me that the state:pending should always imply request:testing, and that I could adjust the code that looks to mash request:testing mash state:pending instead, which would pick up these updates too. However, this solution suffers from a similar problem to the first solution - a whole lot of places in the code would need to be touched and so it's not a simple fix. (The more places I have to touch to fix this, the higher chance I have of introducing new bugs ☺).
  3. I could make it so pending updates don't get request:batched with karma, but stay at request:testing. Then after the mash runs, I could have the masher check the mashed updates to see if any of them are eligible to become batched and go ahead and do it. This seems simpler than Use tagger2 API for sqlitebuildtags. #1 and Fix import of sqlite3 exceptions #2, except for non-autokarma updates - we need to make sure that developers can't manually mark their updates to be batched when they are state:pending or those will end up in this same situation. That last caveat makes this potential solution a little less simple than it would otherwise be.

I would really like to take the time to refactor Bodhi to use a proper state machine implementation so bugs like this happen less frequently, and so that fixing bugs like this is much easier when they do happen. I'm currently debating whether now is the right time to go ahead and just do that refactor or not. I think it would take a long time to do that, and of course, also introduces the risk of new bugs.

@bowlofeggs
Copy link
Contributor

I talked with @jeremycline today and he preferred the refactoring approach, but agreed that due to the potential severity of the bug that solution 3 above is a reasonable approach to solve it.

I also talked with @puiterwijk who also concurred that solution 3 would work.

Thus, I am going for solution 3 for now, until I finally refactor all this crazy code one day.

bowlofeggs added a commit to bowlofeggs/bodhi that referenced this issue Nov 6, 2017
@bowlofeggs
Do not allow pending updates to get request: batched.
23f657c 
Previously, if pending updates received enough karma they would get
their request set to batched. This was bad because the update would
then not be mashed into the testing repository and would sit in
Koji only until the batched updates were set to stable, which means
that further testing oppotunities were missed.

Four different ideas were considered for fixing this bug:

1. We could make bodhi-push and the masher also select
   pending:batched updates when mashing testing updates. This
   seemed like the natural thing to do, but it might actually be
   the trickiest solution because so many if statements in the code
   need to be touched to say something like "or update.status is
   pending and update.request is batched".
2. The state:pending should always imply request:testing, and so we
   could adjust the code that looks to mash request:testing mash
   state:pending instead, which would pick up these updates too.
   However, this solution suffers from a similar problem to the
   first solution - a whole lot of places in the code would need to
   be touched and so it's not a simple fix. (The more places we
   have to touch to fix this, the higher chance we have of
   introducing new bugs ☺).
3. We could make it so pending updates don't get request:batched
   with karma, but stay at request:testing. Then after the mash
   runs, it checks the mashed updates to see if any of them are
   eligible to become batched and go ahead and do it. This seems
   simpler than #1 and #2, except for non-autokarma updates - we
   need to make sure that developers can't manually mark their
   updates to be batched when they are state:pending or those will
   end up in this same situation. That last caveat makes this
   solution a little less simple than it would otherwise be.
4. Refactor the state machine so that it is expressed in one place
   and is therefore much easier to modify and verify. This is a
   large undertaking.

After some deliberation, solution fedora-infra#3 was chosen, though it was
tempting to go with fedora-infra#4.

fixes fedora-infra#1930

Signed-off-by: Randy Barlow <randy@electronsweatshop.com>
bowlofeggs added a commit to bowlofeggs/bodhi that referenced this issue Nov 7, 2017
@bowlofeggs
Do not allow pending updates to get request: batched.
69a0c53 
Previously, if pending updates received enough karma they would get
their request set to batched. This was bad because the update would
then not be mashed into the testing repository and would sit in
Koji only until the batched updates were set to stable, which means
that further testing oppotunities were missed.

Four different ideas were considered for fixing this bug:

1. We could make bodhi-push and the masher also select
   pending:batched updates when mashing testing updates. This
   seemed like the natural thing to do, but it might actually be
   the trickiest solution because so many if statements in the code
   need to be touched to say something like "or update.status is
   pending and update.request is batched".
2. The state:pending should always imply request:testing, and so we
   could adjust the code that looks to mash request:testing mash
   state:pending instead, which would pick up these updates too.
   However, this solution suffers from a similar problem to the
   first solution - a whole lot of places in the code would need to
   be touched and so it's not a simple fix. (The more places we
   have to touch to fix this, the higher chance we have of
   introducing new bugs ☺).
3. We could make it so pending updates don't get request:batched
   with karma, but stay at request:testing. Then after the mash
   runs, it checks the mashed updates to see if any of them are
   eligible to become batched and go ahead and do it. This seems
   simpler than #1 and #2, except for non-autokarma updates - we
   need to make sure that developers can't manually mark their
   updates to be batched when they are state:pending or those will
   end up in this same situation. That last caveat makes this
   solution a little less simple than it would otherwise be.
4. Refactor the state machine so that it is expressed in one place
   and is therefore much easier to modify and verify. This is a
   large undertaking.

After some deliberation solution fedora-infra#3 was chosen, though it was
tempting to go with fedora-infra#4.

fixes fedora-infra#1930

Signed-off-by: Randy Barlow <randy@electronsweatshop.com>
@bowlofeggs bowlofeggs mentioned this issue Nov 7, 2017
Merged
bowlofeggs added a commit that referenced this issue Nov 16, 2017
@bowlofeggs
Do not allow pending updates to get request: batched.
b786fbd 
Previously, if pending updates received enough karma they would get
their request set to batched. This was bad because the update would
then not be mashed into the testing repository and would sit in
Koji only until the batched updates were set to stable, which means
that further testing oppotunities were missed.

Four different ideas were considered for fixing this bug:

1. We could make bodhi-push and the masher also select
   pending:batched updates when mashing testing updates. This
   seemed like the natural thing to do, but it might actually be
   the trickiest solution because so many if statements in the code
   need to be touched to say something like "or update.status is
   pending and update.request is batched".
2. The state:pending should always imply request:testing, and so we
   could adjust the code that looks to mash request:testing mash
   state:pending instead, which would pick up these updates too.
   However, this solution suffers from a similar problem to the
   first solution - a whole lot of places in the code would need to
   be touched and so it's not a simple fix. (The more places we
   have to touch to fix this, the higher chance we have of
   introducing new bugs ☺).
3. We could make it so pending updates don't get request:batched
   with karma, but stay at request:testing. Then after the mash
   runs, it checks the mashed updates to see if any of them are
   eligible to become batched and go ahead and do it. This seems
   simpler than #1 and #2, except for non-autokarma updates - we
   need to make sure that developers can't manually mark their
   updates to be batched when they are state:pending or those will
   end up in this same situation. That last caveat makes this
   solution a little less simple than it would otherwise be.
4. Refactor the state machine so that it is expressed in one place
   and is therefore much easier to modify and verify. This is a
   large undertaking.

After some deliberation solution #3 was chosen, though it was
tempting to go with #4.

fixes #1930

Signed-off-by: Randy Barlow <randy@electronsweatshop.com>
bowlofeggs added a commit to bowlofeggs/bodhi that referenced this issue Nov 16, 2017
@bowlofeggs
Do not allow pending updates to get request: batched.
c3beab4 
Previously, if pending updates received enough karma they would get
their request set to batched. This was bad because the update would
then not be mashed into the testing repository and would sit in
Koji only until the batched updates were set to stable, which means
that further testing oppotunities were missed.

Four different ideas were considered for fixing this bug:

1. We could make bodhi-push and the masher also select
   pending:batched updates when mashing testing updates. This
   seemed like the natural thing to do, but it might actually be
   the trickiest solution because so many if statements in the code
   need to be touched to say something like "or update.status is
   pending and update.request is batched".
2. The state:pending should always imply request:testing, and so we
   could adjust the code that looks to mash request:testing mash
   state:pending instead, which would pick up these updates too.
   However, this solution suffers from a similar problem to the
   first solution - a whole lot of places in the code would need to
   be touched and so it's not a simple fix. (The more places we
   have to touch to fix this, the higher chance we have of
   introducing new bugs ☺).
3. We could make it so pending updates don't get request:batched
   with karma, but stay at request:testing. Then after the mash
   runs, it checks the mashed updates to see if any of them are
   eligible to become batched and go ahead and do it. This seems
   simpler than #1 and #2, except for non-autokarma updates - we
   need to make sure that developers can't manually mark their
   updates to be batched when they are state:pending or those will
   end up in this same situation. That last caveat makes this
   solution a little less simple than it would otherwise be.
4. Refactor the state machine so that it is expressed in one place
   and is therefore much easier to modify and verify. This is a
   large undertaking.

After some deliberation solution fedora-infra#3 was chosen, though it was
tempting to go with fedora-infra#4.

fixes fedora-infra#1930

Signed-off-by: Randy Barlow <randy@electronsweatshop.com>
@bowlofeggs bowlofeggs mentioned this issue Nov 16, 2017
Merged
bowlofeggs added a commit that referenced this issue Nov 16, 2017
@bowlofeggs
Do not allow pending updates to get request: batched.
22d80db 
Previously, if pending updates received enough karma they would get
their request set to batched. This was bad because the update would
then not be mashed into the testing repository and would sit in
Koji only until the batched updates were set to stable, which means
that further testing oppotunities were missed.

Four different ideas were considered for fixing this bug:

1. We could make bodhi-push and the masher also select
   pending:batched updates when mashing testing updates. This
   seemed like the natural thing to do, but it might actually be
   the trickiest solution because so many if statements in the code
   need to be touched to say something like "or update.status is
   pending and update.request is batched".
2. The state:pending should always imply request:testing, and so we
   could adjust the code that looks to mash request:testing mash
   state:pending instead, which would pick up these updates too.
   However, this solution suffers from a similar problem to the
   first solution - a whole lot of places in the code would need to
   be touched and so it's not a simple fix. (The more places we
   have to touch to fix this, the higher chance we have of
   introducing new bugs ☺).
3. We could make it so pending updates don't get request:batched
   with karma, but stay at request:testing. Then after the mash
   runs, it checks the mashed updates to see if any of them are
   eligible to become batched and go ahead and do it. This seems
   simpler than #1 and #2, except for non-autokarma updates - we
   need to make sure that developers can't manually mark their
   updates to be batched when they are state:pending or those will
   end up in this same situation. That last caveat makes this
   solution a little less simple than it would otherwise be.
4. Refactor the state machine so that it is expressed in one place
   and is therefore much easier to modify and verify. This is a
   large undertaking.

After some deliberation solution #3 was chosen, though it was
tempting to go with #4.

fixes #1930

Signed-off-by: Randy Barlow <randy@electronsweatshop.com>
@bowlofeggs
Copy link
Contributor

The patch for this will be included in the upcoming 3.2.0 release:

#2092

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bowlofeggs bowlofeggs

Labels
Composer Issues related to the composer Critical We can't go on living in this sqalor, drop everything and fix it! reliability Issues pertaining to Bodhi's reliability
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Do not allow pending updates to get request: batched. bowlofeggs/bodhi
2 participants
@bowlofeggs @kparal