Merge duplicate volume components from parent/plugins #593
Conversation
amisevsk
changed the title
amisevsk
force-pushed
the
merge-volume-components
branch
from
44e86f4 to
920fd91
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amisevsk, JPinkney, sleshchenko 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 |
amisevsk
force-pushed
the
merge-volume-components
branch
from
920fd91 to
212ab5f
Compare
|
New changes are detected. LGTM label has been removed. |
|
/test v8-devworkspace-operator-e2e, v8-che-happy-path |
|
from e2e tests
¯_(ツ)_/¯ |
|
Ran tests against RHPDS OpenShift 4.8 and they works. |
|
/test v8-devworkspace-operator-e2e |
|
e2e path seems to pass on #596. So, trying one more time |
|
/test v8-devworkspace-operator-e2e |
Merge volume components to avoid issues where e.g. a devworkspaces defines a volume that is also defined in a plugin. This makes devworkspaces more portable, as adding a plugin is less likely to break an existing workspace. Volumes are merged so that the only instance is found in the devworkspace, its parent, or its plugins in that order (i.e. volumes in plugins will be deleted in favor of a volume defined in the devworkspace itself). To try to avoid issues: * If one duplicated volume is not ephemeral, the merged volume is not ephemeral. * If two volumes define different sizes, the merged volume uses the larger size. Signed-off-by: Angel Misevski <[email protected]>
Signed-off-by: Angel Misevski <[email protected]>
Move logic for merging into a separate file and add a more abstract interface to allow adding additional merging later if necessary. Signed-off-by: Angel Misevski <[email protected]>
Signed-off-by: Angel Misevski <[email protected]>
amisevsk
force-pushed
the
merge-volume-components
branch
from
212ab5f to
c6055d9
Compare
|
I have no idea why e2e is consistently failing here. |
|
/test v8-devworkspace-operator-e2e, v8-che-happy-path |
On some clusters, pods may encounter the 'MountVolume.SetUp failed' event due to transient cluster issues, causing unnecessary workspace failures. Signed-off-by: Angel Misevski <[email protected]>
|
Lets see if the mountVolumes issue is transient I guess (still don't understand why only this PR is affected, and I also can't reproduce locally.) /test v8-devworkspace-operator-e2e |
|
The last failure had a different message:
|
|
Tests passed! 🧙♂️ 🎉 |
What does this PR do?
Adds merging functionality to combine volumes that are duplicated in plugins and parents. This is done by removing duplicate volume components from parents/plugins if they are defined earlier in the flattened devworkspace, in the order devworkspace <- parents <- plugins (i.e. if a volume is defined in the devworkspace, duplicates in the parent and plugins are removed). If a volume is defined in multiple plugins, the first processed instance is kept and the rest are dropped.
Duplicated volumes are merged so that the larger size is used, and so that a persistent volume overrides an ephemeral one. This means that if a plugin defines e.g. a
.m2volume with size1Gi, it will overwrite a main-devfile volume with size512Gi. This also means defining volume sizes in plugins can modify main-devfile volumes, which may be undesirable. Generally, a good practice is probably to define plugin volumes as ephemeral unless otherwise necessary, and let the main devworkspace make them persistent if needed.What issues does this PR fix or reference?
Closes #586
Is it tested? How?
Test cases are included in PR.
PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with Che