search

devfile / devworkspace-operator

Apache License 2.0
59 stars 50 forks source link

Fail DevWorkspaces when DevWorkspaceRouting controller sees error #1199

Closed amisevsk closed 8 months ago

amisevsk commented 8 months ago

What does this PR do?

Mark DevWorkspaceRoutings as failed if any of the sync operations returns an UnrecoverableSyncError rather than ignoring it and treating it as a transient error (i.e. retrying).

This allows workspaces to fail with a meaningful message more quickly, rather than waiting for timeout and failing then.

What issues does this PR fix or reference?

Closes https://github.com/devfile/devworkspace-operator/issues/1198

Is it tested? How?

To test, create a resource quota, then create workspaces that would violate that quota and ensure they fail.

For example, for services,

  1. Create a resource quota specifying e.g. a maximum number of services in a namespace: 
    apiVersion: v1
kind: ResourceQuota
metadata:
  name: resource-quota-services
spec:
  hard:
    count/services: "1"
  2. Create two workspaces that have at least one endpoint (resulting in DWO attempting to create two services)
  3. Workspace should fail with message 
    Failed to set up networking for workspace: services "workspace82736d3c797e4150-service" is forbidden: exceeded quota: resource-quota-services, requested: count/services=1, used: count/services=1, limited: count/services=1

We should verify that such quotas are respected for routes and ingresses as well, as applicable.

PR Checklist

AObuchow commented 8 months ago

The Check sources GitHub action is failing due to a DWR test-case I wrote that was abusing the bug that this PR addresses.

IMO we should just remove the Gets Preparing status test case & the associated createsPreparingDWR() function.

@amisevsk let me know if you'd like me to add an extra commit to the PR for this.

openshift-ci[bot] commented 8 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, AObuchow

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/devfile/devworkspace-operator/blob/main/OWNERS)~~ [amisevsk] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
codecov[bot] commented 8 months ago

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (800f386) 53.01% compared to head (c73d089) 52.89%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1199 +/- ## ========================================== - Coverage 53.01% 52.89% -0.13% ========================================== Files 84 84 Lines 7583 7595 +12 ========================================== - Hits 4020 4017 -3 - Misses 3275 3290 +15 Partials 288 288 ``` | [Files](https://app.codecov.io/gh/devfile/devworkspace-operator/pull/1199?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=devfile) | Coverage Δ | | |---|---|---| | [...s/controller/devworkspacerouting/sync\_ingresses.go](https://app.codecov.io/gh/devfile/devworkspace-operator/pull/1199?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=devfile#diff-Y29udHJvbGxlcnMvY29udHJvbGxlci9kZXZ3b3Jrc3BhY2Vyb3V0aW5nL3N5bmNfaW5ncmVzc2VzLmdv) | `64.06% <0.00%> (ø)` | | | [...lers/controller/devworkspacerouting/sync\_routes.go](https://app.codecov.io/gh/devfile/devworkspace-operator/pull/1199?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=devfile#diff-Y29udHJvbGxlcnMvY29udHJvbGxlci9kZXZ3b3Jrc3BhY2Vyb3V0aW5nL3N5bmNfcm91dGVzLmdv) | `64.78% <0.00%> (ø)` | | | [...rs/controller/devworkspacerouting/sync\_services.go](https://app.codecov.io/gh/devfile/devworkspace-operator/pull/1199?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=devfile#diff-Y29udHJvbGxlcnMvY29udHJvbGxlci9kZXZ3b3Jrc3BhY2Vyb3V0aW5nL3N5bmNfc2VydmljZXMuZ28=) | `64.06% <0.00%> (-3.13%)` | :arrow_down: | | [...workspacerouting/devworkspacerouting\_controller.go](https://app.codecov.io/gh/devfile/devworkspace-operator/pull/1199?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=devfile#diff-Y29udHJvbGxlcnMvY29udHJvbGxlci9kZXZ3b3Jrc3BhY2Vyb3V0aW5nL2RldndvcmtzcGFjZXJvdXRpbmdfY29udHJvbGxlci5nbw==) | `48.90% <0.00%> (-3.17%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/devfile/devworkspace-operator/pull/1199/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=devfile)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.