email policy sends email when package is REJECTED_TEMPORARILY

[Iain Lane]

Policies can reject excuses temporarily or permanently. In the temporary case, it is expected that sooner or later the excuse will either become a PASS or REJECTED_PERMANENTLY. For example, autopkgtest uses REJECTED_TEMPORARILY when tests are still running.

Consider the case where the autopkgtest infrastructure is down or has a large backlog. It could be that tests take more than 1 day to all run for a particular excuse but AFAICS we'll still email the uploaders in that case even though they can't fix it.

If an excuse has current verdict REJECTED_TEMPORARILY, we could avoid sending email even if it is > max_age old.

The problem is that the current verdict isn't available to policies. I can imagine that you could fix this by either changing the policy API to include the current verdict, or by adding the current verdict as an attribute to the excuse. Either of these I would imagine could be proposed / discussed / merged upstream first.

[Robert Bruce Park (robru)]

I thought this was the whole point of having a dynamic max_age. We can't inspect TEMPORARY vs PERMANENTLY directly, but if it's is_valid we wait until 5 days before sending a mail, that gives more time for long-running autopkgtests.

[Martin Pitt (pitti)]

Right, the idea was to replace excuse.is_valid with something like excuse.current_verdict. britney already has the policy_verdict local variable in should_upgrade_src() which contains what we need, it just needs to move into the Excuse object. Preferrably it should replace excuse.is_valid to avoid redundancy, or is_valid could maybe become a property which is true iff self.policy_verdict not in [PolicyVerdict.REJECTED_PERMANENTLY, PolicyVerdict.REJECTED_TEMPORARILY]

[Iain Lane (laney)]

Nah. The point of that is that valid excuses that are being held up at update_output stage often legitimately take a long time to resolve.

Things which are REJECTED_TEMOPORARILY are not valid excuses:

  if policy_verdict in [PolicyVerdict.REJECTED_PERMANENTLY, PolicyVerdict.REJECTED_TEMPORARILY]:
    excuse.is_valid = False

so they are subject to the 1 day limit. It's right for them to be invalid; they shouldn't migrate, but it's also my argument that the email policy shouldn't tell people about these until we have a definitive verdict.

[Iain Lane (laney)]

Hey pitti! Nice to see you on britney bugs still. :-)

On Thu, Mar 09, 2017 at 04:29:47PM -0000, Martin Pitt wrote:
> Preferrably it should replace excuse.is_valid to avoid redundancy, or
> is_valid could maybe become a property which is true iff
> self.policy_verdict not in [PolicyVerdict.REJECTED_PERMANENTLY,
> PolicyVerdict.REJECTED_TEMPORARILY]

I don't know if it's going to be easy to do this / how it would look -
policies aren't the only way that things can become invalid in the
current britney implemention. - `git grep is_valid` is scary. And
there's a bit of a mismatch with what we want to use it for: for
migration purposes you'd need to make REJECTED_TEMPORARILY set is_valid
= False, but for the email policy we want to send emails if is_valid =
False but *not* for REJECTED_TEMPORARILY. So we wouldn't be able to use
such an is_valid.

It's possible I'm not understanding something, but I'm not sure what the
right answer is, and I suspect the pragmatic thing to do right now is to
wire the current verdict into the excuse.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

[Martin Pitt (pitti)]

@Iain: We are talking about two different things. My comment #2 was about replacing the existing Excuse.is_valid variable with a property that looks at the new Excuse.current_verdict variable, and thus provides backwards compatibility (unless it's easier to just port the existing users of Excuse.is_valid directly to it -- can't be that many).

Of course is_valid is not the right field to look at for the email notification policy -- that should be Excuse.current_verdict, and of course it should only fire if the verdict is PolicyVerdict.REJECTED_PERMANENTLY.

[Iain Lane (laney)]

On Thu, Mar 09, 2017 at 05:04:10PM -0000, Martin Pitt wrote:
> @Iain: We are talking about two different things. My comment #2 was
> about replacing the existing Excuse.is_valid variable with a property
> that looks at the new Excuse.current_verdict variable, and thus provides
> backwards compatibility (unless it's easier to just port the existing
> users of Excuse.is_valid directly to it -- can't be that many).
>
> Of course is_valid is not the right field to look at for the email
> notification policy -- that should be Excuse.current_verdict, and of
> course it should only fire if the verdict is
> PolicyVerdict.REJECTED_PERMANENTLY.

Okey doke. Sorry for misunderstanding you then. I don't know about the
feasibility or implementation details of your suggestion.

Would you agree that doing this is not required to fix the issue
identified in this report? It feels like a case of "why not do this
refactoring while you're there?" that would put someone off implementing
the fix to the actual issue.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

[Martin Pitt (pitti)]

Oh right, I missed that uninstallability etc. also sets is_valid to False.

> It feels like a case of "why not do this refactoring while you're there?" that would put someone off implementing the fix to the actual issue.

Yes, that was it.

So indeed just adding the current verdict into Excuse in addition seems fine -- the email policy should then be able to check for something like

   if not excuse.is_valid and excuse.current_verdict == PolicyVerdict.REJECTED_PERMANENTLY:
      do_spam()

?

[Iain Lane (laney)]

I think we've fixed this already, going to close the bug. Thanks 2017-us!