Surface Revision quota problems

[mattmoor]

Per the API spec, we should surface a condition like this when the Deployment underlying the Revision cannot be created (e.g. due to quota problems).

  conditions:
  - type: Ready
    status: False
    reason: NoDeployment
    message: "The controller could not create a deployment named ela-abc-e13ac."

This is somewhat contingent on the types of information the Deployment itself surfaces, that we can detect in our controller.

[johnugeorge]

Few questions:

1. Is this error raised only when deployment failed due to quota problem(lack of resources)?
2. Is this applicable to autoscaler-deployment as well as app deployment?
3. What is the error condition when quota problem is resolved(later in future ) but deployment fails again for a different reason?

[johnugeorge]

Eg: I see that revision failure condition is not not set to ready again in this endpoint failure case. eg: https://github.com/elafros/elafros/blob/master/pkg/controller/revision/revision.go#L525
Is this a bug?

[evankanderson]

This condition should be set whenever the controller is unable to materialize the underlying kubernetes resources for the Revision. IMO, I think the Failed condition should be split up into underlying conditions which better represent the type of error, which could be ContainerFailedToRun or UnableToProvision (the former would be a user error, while the latter is probably an operator/infrastructure problem).

[johnugeorge]

/assign @johnugeorge

[grantr]

Seems like there's some disagreement about the form this error reporting should take. @evankanderson @mattmoor @vaikas-google, is there time for you to get together and come to a consensus on this before the end of M4? If not, this should probably be punted to M5.

[mattmoor]

I think we want the guidance from #351 to land in M4, and this should be informed by that. I'm fine with moving this out of M4, if we don't think it will land.

[mattmoor]

Ok, #351 is now fixed.

Please see here for the source of truth as the discussion in the issue doesn't completely capture where we ended up.

[evankanderson]

In particular, interpreting the outcome of the discussion: If this is a temporary condition, it's probably reasonable to set `Ready` = `False`, and we should have the controller check (at a slower rate) items which have no other negative conditions, but have `Ready` = `False`. It would also be reasonable to make `Ready` = `False` a permanent condition. @steren may have some UX research at hand which could help guide this decision, though resource exhaustion feels like it should be a temporary problem which can be fixed later.

…

On Sun, Apr 29, 2018 at 7:53 AM Matt Moore ***@***.***> wrote: Ok, #351 <#351> is now fixed. Please see here <https://github.com/elafros/elafros/blob/master/docs/spec/errors.md> for the source of truth as the discussion in the issue doesn't completely capture where we ended up. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#496 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHlyN458anIM5cvZ2RqR4aY16LjXoPS-ks5ttdPigaJpZM4S55Pj> .

-- Evan Anderson <argent@google.com>