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

Change default value for ephemral preference #4360

Conversation

kadel
Copy link
Member

@kadel kadel commented Jan 14, 2021

What type of PR is this?

Uncomment only one /kind line, and delete the rest.
For example, > /kind bug would simply become: /kind bug

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind failing-test
/kind feature
/kind flake
/kind code-refactoring

Documentation changes: Please include [skip ci] in your commit message as well
/kind documentation
[skip ci]

What does does this PR do / why we need it:

For now, it assumes default value for ephemeral 'false'.

In future releases, we might change it 'true'. But for now, we should
play it safe and keep the old behavior. We are not sure about all the
consequences that this can have.

Which issue(s) this PR fixes:

Fixes #?

PR acceptance criteria:

How to test changes / Special notes to the reviewer:

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jan 14, 2021
@kadel
Copy link
Member Author

kadel commented Jan 14, 2021

/test unit

For not it assumes default value for ephemeral 'false'.
It future  releases we might change it 'true'. But for now we should
play it safe and keep the old behaviour. We are not sure about all the
consequences that this can have.
@dharmit
Copy link
Member

dharmit commented Jan 15, 2021

@kadel Can we add doc for this as well? I'm asking because it would take the user to set Ephemeral to false in preferences for PVC to actually be used. Simply unsetting it won't suffice. That is, odo preference set Ephemeral false is required for odo to use a PVC instead of emptyDir.

@kadel
Copy link
Member Author

kadel commented Jan 15, 2021

@kadel Can we add doc for this as well? I'm asking because it would take the user to set Ephemeral to false in preferences for PVC to actually be used. Simply unsetting it won't suffice. That is, odo preference set Ephemeral false is required for odo to use a PVC instead of emptyDir.

unsetting (odo preference unset Ephemeral, or if user never did anything with that variable) and odo preference set Ephemeral false both result in using emptyDir.

I would be happy to add documentation, but where it should be?

@dharmit
Copy link
Member

dharmit commented Jan 15, 2021

unsetting (odo preference unset Ephemeral, or if user never did anything with that variable)

This doesn't work for me. 😞

Without setting any value for Ephemeral

$ odo version
odo v2.0.3 (b8c0bb87d)

$ odo preference view
PARAMETER             CURRENT_VALUE
UpdateNotification    
NamePrefix            
Timeout               
BuildTimeout          3000
PushTimeout           480
Experimental          
PushTarget            
Ephemeral             

$ odo create nodejs nodejs-ex
Validation
 ✓  Checking devfile existence [66493ns]
 ✓  Creating a devfile component from registry: DefaultDevfileRegistry [62813ns]
 ✓  Validating devfile component [252701ns]

Please use `odo push` command to create the component with source deployed

$ odo push

Validation
 ✓  Validating the devfile [38873ns]

Creating Kubernetes resources for component nodejs-ex
 ✓  Waiting for component to start [11s]
 ✓  Waiting for component to start [17ms]

Applying URL changes
 ✓  URL http-3000: http://http-3000-nodejs-ex-newproject.apps-crc.testing/ created

Syncing to component nodejs-ex
 ✓  Checking files for pushing [573362ns]
 ✓  Syncing files to the component [249ms]

Executing devfile commands for component nodejs-ex
 ✓  Waiting for component to start [2ms]
 ✓  Waiting for component to start [3ms]
 ✓  Waiting for component to start [3ms]
 ✓  Executing install command "npm install" [3s]
 ✓  Executing run command "npm start" [1s]

Pushing devfile component nodejs-ex
 ✓  Changes successfully pushed to component

$ kubectl get po,pvc
NAME                                      READY   STATUS    RESTARTS   AGE
pod/cockroach-operator-74bdfd6bf4-p7zjl   1/1     Running   0          3h19m
pod/nodejs-ex-74fd56cd8c-x2lvp            1/1     Running   0          38s

Explicitly setting Ephemeral to false

$ odo preference view
PARAMETER             CURRENT_VALUE
UpdateNotification    
NamePrefix            
Timeout               
BuildTimeout          3000
PushTimeout           480
Experimental          
PushTarget            
Ephemeral             false

odo create nodejs nodejs-ex
Validation
 ✓  Checking devfile existence [34051ns]
 ✓  Creating a devfile component from registry: DefaultDevfileRegistry [43657ns]
 ✓  Validating devfile component [241173ns]

Please use `odo push` command to create the component with source deployed

$ odo push 

Validation
 ✓  Validating the devfile [54278ns]

Creating Kubernetes resources for component nodejs-ex
 ✓  Waiting for component to start [10s]
 ✓  Waiting for component to start [15ms]

Applying URL changes
 ✓  URL http-3000: http://http-3000-nodejs-ex-newproject.apps-crc.testing/ created

Syncing to component nodejs-ex
 ✓  Checking files for pushing [557761ns]
 ✓  Syncing files to the component [275ms]

Executing devfile commands for component nodejs-ex
 ✓  Waiting for component to start [2ms]
 ✓  Waiting for component to start [2ms]
 ✓  Waiting for component to start [3ms]
 ✓  Executing install command "npm install" [4s]
 ✓  Executing run command "npm start" [1s]

Pushing devfile component nodejs-ex
 ✓  Changes successfully pushed to component

$ kubectl get po,pvc
NAME                                      READY   STATUS    RESTARTS   AGE
pod/cockroach-operator-74bdfd6bf4-p7zjl   1/1     Running   0          3h19m
pod/nodejs-ex-6bf76f657d-2gxb6            1/1     Running   0          17s

NAME                                                STATUS   VOLUME   CAPACITY   ACCESS MODES   STORAGECLASS   AGE
persistentvolumeclaim/odo-projects-nodejs-ex-tzos   Bound    pv0016   100Gi      RWO,ROX,RWX                   17s

@kadel
Copy link
Member Author

kadel commented Jan 18, 2021

That is weird. There is a test to test the default value https://github.com/openshift/odo/pull/4360/files#diff-a22366462d78266515a5177dc92bc1ef5901a1b05d9de4f7825a5a74d6f63a52R274-R292
And I tried the same thing that you did manually and it looks ok.

▶ odo preference unset ephemeral; odo preference view; odo push; oc get po,pvc
 ✗  preference already unset, cannot unset a preference which is not set
PARAMETER             CURRENT_VALUE
UpdateNotification
NamePrefix
Timeout
BuildTimeout
PushTimeout
Experimental
PushTarget
Ephemeral

Validation
 ✓  Validating the devfile [6ms]

Creating Kubernetes resources for component nodejs
 ✓  Waiting for component to start [5s]
 ✓  Waiting for component to start [146ms]

Applying URL changes
 ✓  URL http-3000: http://http-3000-nodejs-tkral-test2.apps.testocp4x.psiodo.net/ created

Syncing to component nodejs
 ✓  Checking files for pushing [3ms]
 ✓  Syncing files to the component [3s]

Executing devfile commands for component nodejs
 ✓  Waiting for component to start [144ms]
 ✓  Waiting for component to start [427ms]
 ✓  Waiting for component to start [146ms]
 ✓  Executing install command "npm install" [7s]
 ✓  Executing run command "npm start" [2s]

Pushing devfile component nodejs
 ✓  Changes successfully pushed to component


NAME                          READY   STATUS    RESTARTS   AGE
pod/nodejs-7dcfc86b4d-h2xm5   1/1     Running   0          19s



▶ odo preference set ephemeral true; odo preference view; odo push; oc get po,pvc
Global preference was successfully updated
PARAMETER             CURRENT_VALUE
UpdateNotification
NamePrefix
Timeout
BuildTimeout
PushTimeout
Experimental
PushTarget
Ephemeral             true

Validation
 ✓  Validating the devfile [53321ns]

Creating Kubernetes resources for component nodejs
 ✓  Waiting for component to start [5s]
 ✓  Waiting for component to start [146ms]

Applying URL changes
 ✓  URL http-3000: http://http-3000-nodejs-tkral-test2.apps.testocp4x.psiodo.net/ created

Syncing to component nodejs
 ✓  Checking files for pushing [3ms]
 ✓  Syncing files to the component [3s]

Executing devfile commands for component nodejs
 ✓  Waiting for component to start [144ms]
 ✓  Waiting for component to start [283ms]
 ✓  Waiting for component to start [276ms]
 ✓  Executing install command "npm install" [7s]
 ✓  Executing run command "npm start" [2s]

Pushing devfile component nodejs
 ✓  Changes successfully pushed to component


NAME                          READY   STATUS    RESTARTS   AGE
pod/nodejs-7dcfc86b4d-nf6cj   1/1     Running   0          19s
▶ odo preference set ephemeral false; odo preference view; odo push; oc get po,pvc
? ephemeral is already set. Do you want to override it in the config Yes
Global preference was successfully updated
PARAMETER             CURRENT_VALUE
UpdateNotification
NamePrefix
Timeout
BuildTimeout
PushTimeout
Experimental
PushTarget
Ephemeral             false

Validation
 ✓  Validating the devfile [205978ns]

Creating Kubernetes resources for component nodejs
 ✓  Waiting for component to start [18s]
 ✓  Waiting for component to start [165ms]

Applying URL changes
 ✓  URL http-3000: http://http-3000-nodejs-tkral-test2.apps.testocp4x.psiodo.net/ created

Syncing to component nodejs
 ✓  Checking files for pushing [3ms]
 ✓  Syncing files to the component [3s]

Executing devfile commands for component nodejs
 ✓  Waiting for component to start [143ms]
 ✓  Waiting for component to start [144ms]
 ✓  Waiting for component to start [439ms]
 ✓  Executing install command "npm install" [7s]
 ✓  Executing run command "npm start" [2s]

Pushing devfile component nodejs
 ✓  Changes successfully pushed to component


NAME                          READY   STATUS    RESTARTS   AGE
pod/nodejs-6c8cb96555-2p5r5   1/1     Running   0          34s

NAME                                             STATUS   VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS   AGE
persistentvolumeclaim/odo-projects-nodejs-fzuu   Bound    pvc-bcaabdbe-adb2-4375-9286-ab7466d7102d   2Gi        RWO            standard       35s

@dharmit
Copy link
Member

dharmit commented Jan 19, 2021

And I tried the same thing that you did manually and it looks ok.

Not sure why, but it works for me now. 🤷‍♂️

Copy link
Member

@dharmit dharmit left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jan 19, 2021
@openshift-merge-robot openshift-merge-robot merged commit 38c2f9d into redhat-developer:master Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants