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

Provision environment variables for initContainers of chePlugin/cheEditor #15508

Merged
merged 1 commit into from
Feb 5, 2020

Conversation

sleshchenko
Copy link
Member

@sleshchenko sleshchenko commented Dec 17, 2019

What does this PR do?

Provisions environment variables for initContainers of chePlugin/cheEditor.

The only init container we have now for chePlugin/cheEditor - remote-plugin-laucher and I'm not sure if it makes sense to tune it. But I think if we provision additional, unused env vars to init containers - it won't hurt anyone in most cases but potentially can be used if there is need to tune initContainer of chePlugin/cheEditor and we will have consistent behavior for all component types.

What issues does this PR fix or reference?

If follows up #15435

How to test this PR

Che Server image build by Happy Path job: maxura/che-server:15508

Devfile
metadata:
  name: env-override-new
components:
  - type: kubernetes
    reference: >-
      https://gist.githubusercontent.com/sleshchenko/86d28a2c4426bdd0cef57bdfe3c7a829/raw/555b20c10ff7d6519b80d0841a30735a2f4f2118/deployment.yaml
    alias: env-test
    env:
      - value: Tom
        name: NAME
  - memoryLimit: 1G
    type: cheEditor
    reference: >-
      https://gist.githubusercontent.com/sleshchenko/5e1118c93a17e6b7d4efc4cd6f88b164/raw/b90a076ab92e3a14101f0275e0573b3ce2eff49d/meta.yaml
    alias: editor
    env:
      - value: Tom
        name: NAME
  - id: ms-vscode/go/latest
    memoryLimit: 256M
    type: chePlugin
    alias: plugin
    env:
      - value: Tom
        name: NAME
apiVersion: 1.0.0
commands:
  - name: test-initContainers
    actions:
      - type: exec
        command: cd /hello && ls
        component: env-test
  - name: test-editor
    actions:
      - type: exec
        command: 'echo Hello ${NAME}'
        component: editor
  - name: test-plugin
    actions:
      - type: exec
        command: 'echo Hello ${NAME}'
        component: plugin

Release Notes

N/A

Docs PR

A note will be added to che docs if reviewers think it makes sense to provision env vars to initContainers as well.

@sleshchenko sleshchenko self-assigned this Dec 17, 2019
@che-bot che-bot added kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Dec 17, 2019
@che-bot
Copy link
Contributor

che-bot commented Dec 17, 2019

✅ E2E Happy path tests succeed 🎉

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

@che-bot
Copy link
Contributor

che-bot commented Dec 17, 2019

E2E tests of Eclipse Che Multiuser on OCP has been successful:

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

Changes LGTM, though using the posted devfile I encounter an error starting the workspace:

Unable to start workspace. Error when trying to start the workspace: Failure executing: POST at: https://172.30.0.1/apis/apps/v1/namespaces/amisevsk-che-2/deployments. Message: Deployment.apps "workspace0pe2qmn7e8843zap.empty-env" is invalid: spec.template.spec.volumes[2].name: Duplicate value: "hello". Received status: Status(apiVersion=v1, code=422, details=StatusDetails(causes=[StatusCause(field=spec.template.spec.volumes[2].name, message=Duplicate value: "hello", reason=FieldValueDuplicate, additionalProperties={})], group=apps, kind=Deployment, name=workspace0pe2qmn7e8843zap.empty-env, retryAfterSeconds=null, uid=null, additionalProperties={}), kind=Status, message=Deployment.apps "workspace0pe2qmn7e8843zap.empty-env" is invalid: spec.template.spec.volumes[2].name: Duplicate value: "hello", metadata=ListMeta(_continue=null, resourceVersion=null, selfLink=null, additionalProperties={}), reason=Invalid, status=Failure, additionalProperties={}).

I don't see what would cause this though.

@sleshchenko
Copy link
Member Author

Unable to start workspace. ... Deployment.apps "workspace0pe2qmn7e8843zap.empty-env" is invalid: spec.template.spec.volumes[2].name: Duplicate value: "hello". ...

@amisevsk Thanks for catching it. On my Che installation, I don't face it, I think Che has some issue in volumes provisioning and depending on PVC strategy it can be reproduced. In my case a common PVC strategy is used, which PVC strategy do you use?

@amisevsk
Copy link
Contributor

@sleshchenko My deployment uses the unique strategy

@sleshchenko
Copy link
Member Author

Seems the same issue happens on master branch as well #15576

@amisevsk
Copy link
Contributor

amisevsk commented Jan 6, 2020

#15576 is a slightly different issue AFAIK, where an earlier version of codeserver had mountSources: true and explicitly included a mount for /projects

@che-bot
Copy link
Contributor

che-bot commented Jan 31, 2020

Can one of the admins verify this patch?

…itor

Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Feb 4, 2020

✅ E2E Happy path tests succeed 🎉

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

@che-bot
Copy link
Contributor

che-bot commented Feb 4, 2020

E2E tests of Eclipse Che Multiuser on OCP has been successful:

@sleshchenko
Copy link
Member Author

sleshchenko commented Feb 4, 2020

@amisevsk I've reproduced the same issue you faced on the nightly image, so, it's not something that this PR introduces.
Screenshot_20200204_100827

Update: I've investigated this topic more and created a separate issue for that #15913. It's reproducible when K8s component has emptyDir, while editor provides persisted volume configuration.

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

LGTM if the issue is in master.

@sleshchenko sleshchenko merged commit edb72c2 into eclipse-che:master Feb 5, 2020
@sleshchenko sleshchenko deleted the chePluginEnv branch February 5, 2020 09:08
@che-bot che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Feb 5, 2020
@che-bot che-bot added this to the 7.9.0 milestone Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants