eclipse-archived / codewind

The official repository of the Eclipse Codewind project
https://codewind.dev
Eclipse Public License 2.0
113 stars 45 forks source link

Files generated by PFE are deleted during project update #1533

Closed GeekArthur closed 4 years ago

GeekArthur commented 4 years ago

Codewind version: 0.7.0 PFE SHA:

sha256:8b1c94bd1e47b9e6c93459003e5bd3cc2e90df1b0cacc7674775e81a80d32086

OS: MacOS

Che version: 7.3.1 IDE extension version: 0.7.0 IDE version: Latest Kubernetes cluster: OKD 3.11

Description: Currently I find this issue on odo project type, how the odo project build works is that odo project generates .odo folder + files under .odo folder on PFE side during first time odo project build, and it uses .odo folder for the following project builds as well. The current behavior is .odo folder generates on PFE side successfully on project creation, but it's deleted on project update. From my investigation, seems like it gets deleted (https://github.com/eclipse/codewind/blob/master/src/pfe/portal/routes/projects/remoteBind.route.js#L229) during project syncing step which is before triggering the actual build process on Turbine layer, so the build fails due to missing .odo folder, below are logs.

Updating odo application
Step 1 of 1:
- Building and deploying odo component: cw-nodenode
 ✗  Blank source location, does the .odo directory exist?
- Failed to build and deploy odo component: cw-nodenode
Failed to update odo application

I think culprit may from this PR: https://github.com/eclipse/codewind/pull/1310, which changes the condition here: https://github.com/eclipse/codewind/pull/1310/files#diff-b73b1946195409164e2ccc8a3b7fe79cR216

Steps to reproduce:

  1. Create odo project (any language is fine)
  2. Update odo project

Workaround:

Since currently we don't support backwards project syncing (PFE -> IDE), probably we can ignore .odo folder in cwctl here: https://github.com/eclipse/codewind-installer/blob/526607c24afce8c89e523b367f61fed9d2d160e7/pkg/project/sync.go#L270 or somewhere else? Basically I think what we need is if we create odo project we can see .odo in ignoredPaths of odo project .cw-settings, so that project syncing can ignore .odo folder and we only handle .odo folder on PFE side.

So it would be similar to mc-target for liberty project. (mc-target is generated by PFE and it's not deleted during project update).

makandre commented 4 years ago

So this is in fact the same root cause identified in #1472.

Appsody projects are affected too because the project folder (in PFE) is mounted to appsody container. When appsody runs, it will generate runtime resources (such as node_modules) into the project folder (again in PFE). These files are not on the user's local copy of the project, causing PFE to think they are deleted by user and so PFE will attempt to delete the files in PFE.

The additional problem in #1472 is node_modules contains a large number of files which PFE is doing parallel rm which hits the process limit causing the connection to PFE to drop.

GeekArthur commented 4 years ago

@makandre From your description, it's the same issue! Only difference is odo project doesn't lose connection but it's same root cause. Thanks for pointing this out!

GeekArthur commented 4 years ago

This issue is important since all the following issues (most of them are stopship issues) are affected by this issue. Think the following issues will be resolved once this issue is fixed. https://github.com/eclipse/codewind/issues/1472 https://github.com/eclipse/codewind/issues/1514 https://github.com/eclipse/codewind/issues/1527

jopit commented 4 years ago

Losing the connection doesn't always happen even for appsody projects -- for me it happens almost all the time, but for others it happens rarely. However, the file deletion and the spawning off of hundreds of processes happens whether the connection drops or not.

malincoln commented 4 years ago

@tobespc pls review and assign. This seems to be affecting several of the stop-ship issues.

jcockbain commented 4 years ago

/assign

jcockbain commented 4 years ago

We agree with the reasoning above for the cause of this. Anything that is generated in the build container, but is not in the users project, is then deleted on the next sync.

In terms of a long-term fix, the ignoredPaths list at the moment is specific to the sync from the users project to the build container. It contains the things that shouldn't be in PFE. Our preference to fix this issue is to call the extension and get back a protectedPaths list, containing the paths which have been generated on a build and therefore shouldn't be deleted when we sync.

This means the ODO extension can explicitly tell us what not to delete (i.e. .odo). This could be added into the YAML here https://github.com/eclipse/codewind-appsody-extension/blob/master/codewind.yaml maybe?

To fix this specific bug for 0.7.0, we can just add a check for any paths starting with .odo here https://github.com/eclipse/codewind/blob/master/src/pfe/portal/routes/projects/remoteBind.route.js#L262.

GeekArthur commented 4 years ago

@jcockbain Regarding long-term fix, the codewind.yaml that you point is Appsody, odo uses this one: https://github.com/eclipse/codewind-odo-extension/blob/master/codewind.yaml#L6, we can see odo already added .odo as detection so when Portal loads .odo extension, Portal already knew that and probably can use that directly for not to delete case, or we may have a new design for that.

Also FYI odo has it's own sync mechanism from PFE to build container, so we only need to make sure the sync is good from user's local file system to PFE, then all odo projects will be good. Can ignoredPaths in .cw-settings use for ignoring sync from user's local file system to PFE? If so, each time we create odo project we can directly add .odo to .cw-settings by default to resolve this.

malincoln commented 4 years ago

@GeekArthur pls verify and close. Thanks

GeekArthur commented 4 years ago

I verified the issue was fixed on 0.7.0, so I remove the stopship label and add 0.8.0 release. I can close the issue once it's fixed in master (master has different implementation for the long-term fix).

GeekArthur commented 4 years ago

PR for master is not merged yet: #1559, I can verify and close the issue once the PR is merged.

GeekArthur commented 4 years ago

Verified the fix on master, close the issue.