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

feat: configure workspace start timeout from Che Cluster CR #1576

Merged
merged 5 commits into from
Dec 28, 2022

Conversation

AObuchow
Copy link
Contributor

@AObuchow AObuchow commented Dec 7, 2022

What does this PR do?

This PR allows the configuration of the DWOC's progressTimeout field from the Che Cluster CR's spec.devEnvironments.startTimeoutSeconds field.

The spec.devEnvironments.startTimeoutSeconds field is a int32 pointer so that we can differentiate between when the field is unset (nil, allowing the default value of 5 minutes to be used) and when a timeout of 0 seconds is desired.

Note: This PR also includes the changes from #1549 for convenience. I can remove the commits from that PR if desired.

Screenshot/screencast of this PR

n/a

What issues does this PR fix or reference?

eclipse-che/che#21820

How to test this PR?

Tests were added for this PR.

To manually test:

  1. Checkout this PR
  2. Deploy Che: build/scripts/olm/test-catalog-from-sources.sh (for OpenShift)
  3. Edit the Che Cluster CR and add the devEnvironments.startTimeout field: kubectl edit checluster eclipse-che -n eclipse-che
Spec:
(...)
  Dev Environments:
    Default Components:
      Container:
        Image:           quay.io/devfile/universal-developer-image:ubi8-38da5c2
        Source Mapping:  /projects
      Name:              universal-developer-image
    Default Editor:      che-incubator/che-code/insiders
    Default Namespace:
      Auto Provision:                      true
      Template:                            <username>-che
    Disable Container Build Capabilities:  true
    Seconds Of Inactivity Before Idling:   1800
    Seconds Of Run Before Idling:          -1
+    startTimeoutSeconds:                         420 #7 minutes
    Storage:
      Pvc Strategy:  per-user
  Git Services:
  Networking:
    Auth:
      Gateway:
        Config Labels:
          App:        che
          Component:  che-gateway-config
  1. Verify that config.workspace.progressTimeout in the DWOC has the same value as the devEnvironments.startTimeout field from the Che Cluster CR: kubectl describe dwoc -n eclipse-che
Name:         devworkspace-config
Namespace:    eclipse-che
Labels:       <none>
Annotations:  <none>
API Version:  controller.devfile.io/v1alpha1
Config:
  Workspace:
+    Progress Timeout:  420s
(...)
  1. Remove the devEnvironments.startTimeout field from the Che Cluster CR: kubectl edit checluster eclipse-che -n eclipse-che
Spec:
(...)
  Dev Environments:
    Default Components:
      Container:
        Image:           quay.io/devfile/universal-developer-image:ubi8-38da5c2
        Source Mapping:  /projects
      Name:              universal-developer-image
    Default Editor:      che-incubator/che-code/insiders
    Default Namespace:
      Auto Provision:                      true
      Template:                            <username>-che
    Disable Container Build Capabilities:  true
    Seconds Of Inactivity Before Idling:   1800
    Seconds Of Run Before Idling:          -1
-    startTimeoutSeconds:                         420s
    Storage:
      Pvc Strategy:  per-user
  Git Services:
  Networking:
    Auth:
      Gateway:
        Config Labels:
          App:        che
          Component:  che-gateway-config
  1. Verify that the config.workspace.progressTimeout from the DWOC is now gone: kubectl describe dwoc -n eclipse-che
Name:         devworkspace-config
Namespace:    eclipse-che
Labels:       <none>
Annotations:  <none>
API Version:  controller.devfile.io/v1alpha1
Config:
  Workspace:
Kind:  DevWorkspaceOperatorConfig
Metadata:
(...)

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

@openshift-ci
Copy link

openshift-ci bot commented Dec 7, 2022

Hi @AObuchow. Thanks for your PR.

I'm waiting for a eclipse-che member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@AObuchow
Copy link
Contributor Author

AObuchow commented Dec 7, 2022

This PR is set to be a draft until DWO 0.18.0 is released.

@tolusha if you would like to merge this PR (and the 2 other PR's included in it), it might be efficient to follow the testing steps from this PR, as well as #1549 and #1565 all at once.

If you would like to merge the PR's one-by-one, just ping me and I'll rebase my earlier PR :)

@codecov
Copy link

codecov bot commented Dec 7, 2022

Codecov Report

Merging #1576 (dfdcbaf) into main (149a39f) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head dfdcbaf differs from pull request most recent head 9b02186. Consider uploading reports for the commit 9b02186 to get more accurate results

@@            Coverage Diff             @@
##             main    #1576      +/-   ##
==========================================
- Coverage   60.62%   60.61%   -0.02%     
==========================================
  Files          73       73              
  Lines        6316     6314       -2     
==========================================
- Hits         3829     3827       -2     
  Misses       2146     2146              
  Partials      341      341              
Impacted Files Coverage Δ
api/v2/checluster_types.go 30.43% <ø> (ø)
...eploy/dev-workspace-config/dev_workspace_config.go 91.37% <100.00%> (+0.81%) ⬆️
pkg/deploy/gateway/traefik_config_util.go 91.80% <0.00%> (-0.74%) ⬇️
controllers/devworkspace/solver/che_routing.go 79.36% <0.00%> (-0.06%) ⬇️
api/v2/zz_generated.deepcopy.go 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@tolusha
Copy link
Contributor

tolusha commented Dec 8, 2022

@AObuchow
Let's wait for DWO v0.18.0 and merge a single PR

@tolusha
Copy link
Contributor

tolusha commented Dec 22, 2022

DWO updated to v0.18.0

@openshift-ci
Copy link

openshift-ci bot commented Dec 22, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tolusha
Copy link
Contributor

tolusha commented Dec 23, 2022

/test v11-operator-test

// "15m", "20s", "1h30m", etc.
// +optional
// +kubebuilder:default:="5m"
StartTimeout string `json:"startTimeout,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering why not put this into seconds, like this?
StartTimeoutSeconds *int32 json:"startTimeoutSeconds,omitempty"``
The reasons are the following:

  • simplify validation on checluster level
  • dashboard shouldn't parse the value but always uses timeout in seconds

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 chose string to match the expected input that the DevWorkspaceOperatorConfig expects.

However, since the progressTimeout field of the DWOC supports seconds, I could simply convert the int32 to string and append an s. The maximum int32 value seems to be 2147483647 which is ~596523 hours, so there doesn't seem to be any issue about not being able to set the start timeout high enough.

I'll make your suggested modification 👍

@openshift-ci
Copy link

openshift-ci bot commented Dec 23, 2022

New changes are detected. LGTM label has been removed.

@AObuchow
Copy link
Contributor Author

@tolusha I updated the PR to use startTimeoutSeconds as well as the PR description (note that this PR still includes the commits from #1549, which can be removed if desired.

#1587 (comment) might also be worth addressing in this PR as an extra commit.

@devstudio-release
Copy link

Build 3.5 :: dsc_3.x/531: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.5 :: dsc_3.x/531: SUCCESS

3.5.0 CI

@devstudio-release
Copy link

Build 3.5 :: operator-bundle_3.x/738: Console, Changes, Git Data

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.5 :: copyIIBsToQuay/581: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.5 :: push-latest-container-to-quay_3.x/1450: SUCCESS

Copied: devspaces-operator-bundle; bundle-generated updated;
/job/DS_CI/job/Releng/job/copyIIBsToQuay triggered for OCP v4.12 v4.11 v4.10

@devstudio-release
Copy link

Build 3.5 :: sync-to-downstream_3.x/1885: SUCCESS

Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/1829 triggered; /job/DS_CI/job/dsc_3.x triggered;

@devstudio-release
Copy link

Build 3.5 :: operator-bundle_3.x/738: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.x/1885 triggered

@devstudio-release
Copy link

Build 3.5 :: dsc_3.x/532: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.5 :: dsc_3.x/532: SUCCESS

3.5.0 CI

@devstudio-release
Copy link

Build 3.5 :: operator-bundle_3.x/739: Console, Changes, Git Data

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.5 :: copyIIBsToQuay/583: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.5 :: push-latest-container-to-quay_3.x/1452: SUCCESS

Copied: devspaces-operator-bundle; bundle-generated updated;
/job/DS_CI/job/Releng/job/copyIIBsToQuay triggered for OCP v4.12 v4.11 v4.10

@devstudio-release
Copy link

Build 3.5 :: sync-to-downstream_3.x/1888: SUCCESS

Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/1831 triggered; /job/DS_CI/job/dsc_3.x triggered;

@devstudio-release
Copy link

Build 3.5 :: operator-bundle_3.x/739: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.x/1888 triggered

@devstudio-release
Copy link

Build 3.5 :: dsc_3.x/534: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.5 :: dsc_3.x/534: SUCCESS

3.5.0 CI

@devstudio-release
Copy link

Build 3.5 :: copyIIBsToQuay/583: SUCCESS

arches = x86_64, s390x, ppc64le;
  * LATEST DS OPERATOR BUNDLE = <a href=https://quay.io/repository/devspaces/devspaces-operator-bundle?tab=tags>registry-proxy.engineering.redhat.com/rh-osbs/devspaces-operator-bundle:3.5-33
  * LATEST DWO OPERATOR BUNDLE = <a href=https://quay.io/repository/devworkspace/devworkspace-operator-bundle?tab=tags>registry-proxy.engineering.redhat.com/rh-osbs/devworkspace-operator-bundle:0.18-1
+ x86_64-rhel8 IIB(s) copied:
  + quay.io/devspaces/iib:3.5-v4.12-405744-402447-x86_64
  + quay.io/devspaces/iib:3.5-v4.11-405742-402443-x86_64
  + quay.io/devspaces/iib:3.5-v4.10-405933-402439-x86_64
  + quay.io/devspaces/iib:3.5-v4.10-x86_64
  + quay.io/devspaces/iib:next-v4.10-x86_64
+ ppc64le-rhel8 IIB(s) copied:
  + quay.io/devspaces/iib:3.5-v4.12-405744-402447-ppc64le
  + quay.io/devspaces/iib:3.5-v4.11-405936-402443-ppc64le
  + quay.io/devspaces/iib:3.5-v4.11-ppc64le
  + quay.io/devspaces/iib:next-v4.11-ppc64le
  + quay.io/devspaces/iib:3.5-v4.10-405933-402439-ppc64le
  + quay.io/devspaces/iib:3.5-v4.10-ppc64le
  + quay.io/devspaces/iib:next-v4.10-ppc64le
+ s390x-rhel8 IIB(s) copied:
  + quay.io/devspaces/iib:3.5-v4.12-405744-402447-s390x
  + quay.io/devspaces/iib:3.5-v4.11-405936-402443-s390x
  + quay.io/devspaces/iib:3.5-v4.11-s390x
  + quay.io/devspaces/iib:next-v4.11-s390x
  + quay.io/devspaces/iib:3.5-v4.10-405933-402439-s390x
  + quay.io/devspaces/iib:3.5-v4.10-s390x
  + quay.io/devspaces/iib:next-v4.10-s390x

nickboldt pushed a commit that referenced this pull request Jan 30, 2023
* feat: configure workspace security context for container builds

Fix eclipse-che/che#21770

Signed-off-by: Andrew Obuchowicz <[email protected]>

* Set SCC allowPrivilegeEscalation to true when container build enabled (#1596)

* Set SCC allowPrivilegeEscalation to true when container build enabled

Running Podman inside a container in OpenShift requires the pod to have
allowPrivilegeEscalation: true in its security context.

* Fix tests

Signed-off-by: Angel Misevski <[email protected]>

* fix: set scc priority to null

Signed-off-by: Anatolii Bazko <[email protected]>

---------

Signed-off-by: Andrew Obuchowicz <[email protected]>
Signed-off-by: Angel Misevski <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Co-authored-by: Angel Misevski <[email protected]>
Co-authored-by: Anatolii Bazko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants