-
Notifications
You must be signed in to change notification settings - Fork 55
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 support for overriding any pod spec field in DevWorkspace deployment #868
Conversation
40b14a2
to
163d8cd
Compare
I've pushed fixes to take I've just tested again and it appears that everything works as intended. A container image from the current changes is built and pushed to To test it on a cluster, run the following: export NAMESPACE=dw #or your choice
export DWO_IMG=quay.io/amisevsk/devworkspace-controller:pod-spec-overrides
make install |
Update devfile/api commit used for go dependency and CRDs to devfile/api@2609b35 Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Add support for specifying overrides to the workspace deployment's pod spec via stategic merge patch. Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Since updating the DevWorkspace and DevWorkspaceTemplate CRDs, it is no longer possible to use `kubectl apply` to create the object in a cluster, due to the 'last-applied-configuration' annotation becoming too long for an annotation. To avoid this, we pass --server-side=true in kubectl apply commands. Since the CA bundle injection in CRDs results in a different manager for the .spec.conversion.webhook.clientConfig.caBundle field, we have to pass --force-conflicts in `make install` to be able to re-apply the CRDs if they are already applied. Signed-off-by: Angel Misevski <amisevsk@redhat.com>
163d8cd
to
774edda
Compare
PRs in devfile/api are merged and this PR is ready for review. Note one issue that is a side-effect of this PR is that the DevWorkspace and DevWorkspaceTemplate CRDs are now too big to apply using
To avoid this issue, the
Before merging this PR, it will be necessary to update documentation on applying generated yaml files directly and verify that tools which apply them can accomodate the change. |
To test these changes via OLM, I've build an index image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me (I love how elegant the actual implementation is) 👍
I ended up testing this on OLM which worked well.
The deployment did fail because the kata
RuntimeClass wasn't found, but I could see the workspace deployment was modified as expected:
kind: Deployment
apiVersion: apps/v1
metadata:
annotations:
deployment.kubernetes.io/revision: '1'
resourceVersion: '52614'
name: workspace2d70776a4c454fa4
uid: 0ab3d1e8-e3cf-463f-b954-b637cd8b61e3
creationTimestamp: '2022-09-22T17:17:00Z'
generation: 2
(...)
namespace: default
ownerReferences:
- apiVersion: workspace.devfile.io/v1alpha2
kind: DevWorkspace
name: theia-next
uid: 2d70776a-4c45-4fa4-b5bb-c7c683ba5110
controller: true
blockOwnerDeletion: true
labels:
controller.devfile.io/creator: ''
controller.devfile.io/devworkspace_id: workspace2d70776a4c454fa4
controller.devfile.io/devworkspace_name: theia-next
spec:
replicas: 0
selector:
matchLabels:
controller.devfile.io/devworkspace_id: workspace2d70776a4c454fa4
template:
metadata:
name: workspace2d70776a4c454fa4
namespace: default
creationTimestamp: null
labels:
controller.devfile.io/creator: ''
controller.devfile.io/devworkspace_id: workspace2d70776a4c454fa4
controller.devfile.io/devworkspace_name: theia-next
annotations:
+ io.kubernetes.cri-o.userns-mode: 'auto:size=65536;map-to-root=true'
+ io.openshift.userns: 'true'
+ openshift.io/scc: container-build
spec:
(...)
serviceAccountName: workspace2d70776a4c454fa4-sa
imagePullSecrets:
- name: workspace2d70776a4c454fa4-sa-dockercfg-4wtnn
+ schedulerName: stork
terminationGracePeriodSeconds: 10
+ runtimeClassName: kata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and works well 👍 (tested with OLM)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amisevsk, AObuchow, dkwon17 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:
Approvers can indicate their approval by writing |
I did notice though, if try to test by running DWO via:
and then when I run:
the I had to add the |
Superceded by #944 |
What does this PR do?
Add support for specifying Pod spec overrides in DevWorkspaces. This PR is open as a draft for review/discussion on approach.
The first commit in this PR updates the devfile/api dependency to point at my fork, to pull in changes from PR devfile/api#872. Before merging this PR, the devfile/api PR should be merged and the devfile/api dependency should be updated normally.
What issues does this PR fix or reference?
Related: devfile/api#860
Closes #852
Is it tested? How?
To test the changes, there's a new sample:
This workspace will likely fail to start as it sets the scheduler to
stork
and the runtimeClassName tokata
, which are likely not available. To verify things are working as expected, check that fields are overridden in the Deployment for that workspacePR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-path
to trigger)v8-devworkspace-operator-e2e
: DevWorkspace e2e testv8-che-happy-path
: Happy path for verification integration with Che