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.

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

Add _ONLYOBSOLETESAME parameter, obsolete only same BUILD jobs #1493

Merged
merged 1 commit into from
Nov 17, 2017

Conversation

AdamWill
Copy link
Contributor

@AdamWill AdamWill commented Nov 9, 2017

By default, whenever openQA receives an ISO POST request and
schedules a set of jobs for the posted image, any pending jobs
with the same DISTRI, VERSION, FLAVOR and ARCH are canceled.
This can be too aggressive, so a _NOOBSOLETEBUILD parameter is
available which disables this behaviour entirely. However, there
may be a need for a happy medium.

For instance, in Fedora, it's too aggressive to cancel any job
with the same DISTRI, VERSION, FLAVOR and ARCH as a newly-POSTed
ISO, especially at present, since we are currently producing two
streams of composes for the same release (VERSION), modular and
non-modular; we don't want pending jobs for a non-modular
compose to be canceled when a modular compose with the same
VERSION lands, and vice versa. However, there's also a problem
with _NOOBSOLETEBUILD. Sometimes it happens that a build lands,
100+ jobs are created, but we realize something's wrong - we
need to tweak something and re-run all the jobs. The normal way
to do this is just to re-POST the images from the same build and
rely on the obsoletion behaviour to cancel the existing jobs.
But if we do this with _NOOBSOLETEBUILD set, the existing jobs
for the same build will not be canceled.

So this commit adds _ONLYOBSOLETESAME, which restricts this
obsoletion (or, if _DEPRIORITIZEBUILD is used, deprioritization)
behaviour to only apply to jobs with the exact same BUILD. So
any pending jobs with the same DISTRI, VERSION, FLAVOR, ARCH and
BUILD get obsoleted or deprioritized, but no other jobs do.

@codecov
Copy link

codecov bot commented Nov 9, 2017

Codecov Report

Merging #1493 into master will decrease coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1493      +/-   ##
==========================================
- Coverage    88.6%   88.47%   -0.14%     
==========================================
  Files         106      106              
  Lines        8075     8078       +3     
==========================================
- Hits         7155     7147       -8     
- Misses        920      931      +11
Impacted Files Coverage Δ
lib/OpenQA/WebAPI/Controller/API/V1/Iso.pm 93.23% <100%> (+0.07%) ⬆️
lib/OpenQA/Worker/Common.pm 83.27% <0%> (-3.72%) ⬇️
lib/OpenQA/WebAPI.pm 92.03% <0%> (-0.32%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31ec90c...16af146. Read the comment docs.

@AdamWill
Copy link
Contributor Author

AdamWill commented Nov 9, 2017

I don't think the codecov result is correct. I don't think this actually decreased coverage.

@foursixnine
Copy link
Member

@AdamWill We're back into that day where unexpected coverage changes arise :(

@@ -217,6 +217,12 @@ builds but rather deprioritize them up to a configurable limit of priority.
_DEPRIORITIZE_LIMIT:: The configurable limit of priority up to which jobs
should be deprioritized. Needs `_DEPRIORITIZEBUILD`. Default 100.

_ONLYOBSOLETESAME:: Only obsolete (or deprioritize) jobs for the same BUILD.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I struggle with the word "SAME". Can we find better names than just "BUILD" and "SAME"? I guess the name "_NOOBSOLETEBUILD" should be adjusted accordingly already. I would be ok to change the old variable as well because it is kind of an "internal" one. Do you have good suggestions for the both variables?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IAGREETHATTHEWORD is hard to parse

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree they're not the best names, but I can't immediately think of anything better :/ gimme half an hour and I'll let it percolate a bit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random idea for names:_NO_JOB_BUILD_OBSOLENCE=1 and _EQUAL_JOB_BUILD_OBSOLENCE=1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OBSOLESCENCE . But yeah, not bad...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potato, potato :) :P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, I opt for just "_NO_OBSOLETE" and "_ONLY_OBSOLETE_SAME_BUILD".

  1. OBSOLESCENSE is hard to type right apparently :)
  2. underscore_separation_helps_reading
  3. I would not mention "job" when it's most likely about a whole lot of them, as in "a whole build"
  4. "no obsolete build" was ambiguous already because not a "build" was obsoleted but any "build" with matching other product parameters

By default, whenever openQA receives an ISO POST request and
schedules a set of jobs for the posted image, any pending jobs
with the same DISTRI, VERSION, FLAVOR and ARCH are canceled.
This can be too aggressive, so a _NOOBSOLETEBUILD parameter is
available which disables this behaviour entirely. However, there
may be a need for a happy medium.

For instance, in Fedora, it's too aggressive to cancel any job
with the same DISTRI, VERSION, FLAVOR and ARCH as a newly-POSTed
ISO, especially at present, since we are currently producing two
streams of composes for the same release (VERSION), modular and
non-modular; we don't want pending jobs for a non-modular
compose to be canceled when a modular compose with the same
VERSION lands, and vice versa. However, there's also a problem
with _NOOBSOLETEBUILD. Sometimes it happens that a build lands,
100+ jobs are created, but we realize something's wrong - we
need to tweak something and re-run all the jobs. The normal way
to do this is just to re-POST the images from the same build and
rely on the obsoletion behaviour to cancel the existing jobs.
But if we do this with _NOOBSOLETEBUILD set, the existing jobs
for the same build will not be canceled.

So this commit adds _ONLY_OBSOLETE_SAME_BUILD, restricting this
obsoletion (or, if _DEPRIORITIZEBUILD is used, deprioritization)
behaviour to only apply to jobs with the exact same BUILD. So
any pending jobs with the same DISTRI, VERSION, FLAVOR, ARCH and
BUILD get obsoleted or deprioritized, but no other jobs do.

This commit also renames _NOOBSOLETEBUILD to _NO_OBSOLETE, for
reasons discussed in the pull request:
os-autoinst#1493 (review)
For now both names are respected, but the documentation now only
refers to _NO_OBSOLETE. In future we could remove handling of
_NOOBSOLETEBUILD entirely.
@AdamWill
Copy link
Contributor Author

OK, I liked @okurz 's suggestion best, so I (tried to) implement that but with backward compatibility for _NOOBSOLETEBUILD. Both _NO_OBSOLETE and _NOOBSOLETEBUILD should be handled, but the docs only talk about _NO_OBSOLETE. I think this logic should work, but do double check it. Thanks folks.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! 👍

@AdamWill
Copy link
Contributor Author

BTW, I chickened out of making the tests cover both names, as it'd involve lots of trying to figure out what the hell the appropriate expected job counts and jobs are for the subsequent tests if I did. Man, this perl test framework stinks compared to py.test. :)

@coolo coolo merged commit af1fd2e into os-autoinst:master Nov 17, 2017
coolo pushed a commit that referenced this pull request Nov 17, 2017
commit af1fd2e
Merge: 31ec90c 16af146
Author:     Stephan Kulow <coolo@kde.org>
AuthorDate: Fri Nov 17 10:16:31 2017 +0100
Commit:     GitHub <noreply@github.com>
CommitDate: Fri Nov 17 10:16:31 2017 +0100

    Merge pull request #1493 from AdamWill/only-obsolete-build

    Add _ONLYOBSOLETESAME parameter, obsolete only same BUILD jobs
coolo pushed a commit that referenced this pull request Nov 18, 2017
commit af1fd2e
Merge: 31ec90c 16af146
Author:     Stephan Kulow <coolo@kde.org>
AuthorDate: Fri Nov 17 10:16:31 2017 +0100
Commit:     GitHub <noreply@github.com>
CommitDate: Fri Nov 17 10:16:31 2017 +0100

    Merge pull request #1493 from AdamWill/only-obsolete-build

    Add _ONLYOBSOLETESAME parameter, obsolete only same BUILD jobs
Martchus pushed a commit to Martchus/openQA that referenced this pull request Nov 28, 2017
By default, whenever openQA receives an ISO POST request and
schedules a set of jobs for the posted image, any pending jobs
with the same DISTRI, VERSION, FLAVOR and ARCH are canceled.
This can be too aggressive, so a _NOOBSOLETEBUILD parameter is
available which disables this behaviour entirely. However, there
may be a need for a happy medium.

For instance, in Fedora, it's too aggressive to cancel any job
with the same DISTRI, VERSION, FLAVOR and ARCH as a newly-POSTed
ISO, especially at present, since we are currently producing two
streams of composes for the same release (VERSION), modular and
non-modular; we don't want pending jobs for a non-modular
compose to be canceled when a modular compose with the same
VERSION lands, and vice versa. However, there's also a problem
with _NOOBSOLETEBUILD. Sometimes it happens that a build lands,
100+ jobs are created, but we realize something's wrong - we
need to tweak something and re-run all the jobs. The normal way
to do this is just to re-POST the images from the same build and
rely on the obsoletion behaviour to cancel the existing jobs.
But if we do this with _NOOBSOLETEBUILD set, the existing jobs
for the same build will not be canceled.

So this commit adds _ONLY_OBSOLETE_SAME_BUILD, restricting this
obsoletion (or, if _DEPRIORITIZEBUILD is used, deprioritization)
behaviour to only apply to jobs with the exact same BUILD. So
any pending jobs with the same DISTRI, VERSION, FLAVOR, ARCH and
BUILD get obsoleted or deprioritized, but no other jobs do.

This commit also renames _NOOBSOLETEBUILD to _NO_OBSOLETE, for
reasons discussed in the pull request:
os-autoinst#1493 (review)
For now both names are respected, but the documentation now only
refers to _NO_OBSOLETE. In future we could remove handling of
_NOOBSOLETEBUILD entirely.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants