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

Reduce shell count by one when configmgr used by using pipe directly #3812

Merged
merged 4 commits into from
Apr 30, 2024

Conversation

1000TurquoisePogs
Copy link
Member

When inspecting for optimizations and reduction of shell usage for TMPDIR troubleshooting, I noticed that there's some unneeded use of shell wrappers for running each server.

In the code today, each time a start.sh of a server is run we spawn a shell to run that in, and then pipe its contents to ANOTHER shell so that we can put wait at the end.

We could have just spawned one shell with that pipe, instead of a shell-within-a-shell.
This looks to be largely from the old non-configmgr code, and configmgr can just further optimize.

The process tree goes from 7 processes in the case of zss:
before

To 6:
after

An effort here can further reduce it to 5 zowe/launcher#109
With both efforts combined, the tree of a given Zowe server would just be

  • Launcher
    • configmgr
      • server's own start script
        • server itself

But today it is

  • Launcher
    • zwe
      • configmgr
        • shell wrapper being removed in this PR
          • server's own start script
            • server itself

How to test this PR?
If the servers come up the same as they did before, success.
If a server is crashed (intentionally or unintentionally), launcher should have the same restart behavior as before.

Copy link

github-actions bot commented Apr 25, 2024

build 5128 SUCCEEDED.
Link to workflow run: https://github.com/zowe/zowe-install-packaging/actions/runs/8831911062

Copy link

github-actions bot commented Apr 25, 2024

Test workflow 4381 is started.
Running install test: Convenience Pax
The zowe artifact being used by this test workflow: libs-snapshot-local/org/zowe/2.16.0-PR-3812/zowe-2.16.0-pr-3812-5128-20240425114904.pax
Running on machine: zzow07
Result: FAILURE
Link to workflow run: https://github.com/zowe/zowe-install-packaging/actions/runs/8831984271

@Martin-Zeithaml Martin-Zeithaml self-requested a review April 25, 2024 11:57
Copy link

github-actions bot commented Apr 26, 2024

build 5131 SUCCEEDED.
Link to workflow run: https://github.com/zowe/zowe-install-packaging/actions/runs/8845240297

Copy link

github-actions bot commented Apr 26, 2024

Test workflow 4384 is started.
Running install test: Convenience Pax
The zowe artifact being used by this test workflow: libs-snapshot-local/org/zowe/2.16.0-PR-3812/zowe-2.16.0-pr-3812-5131-20240426082644.pax
Running on machine: zzow06
Result: SUCCESS
Link to workflow run: https://github.com/zowe/zowe-install-packaging/actions/runs/8845369465

@1000TurquoisePogs
Copy link
Member Author

I tested and confirmed if i kill a server process the launcher understands it needs to restart it. so, seems like this code causes no regression.

const pipeArray = os.pipe();
if (!pipeArray) {
//TODO error message
common.printFormattedError("ZWELS", "zwe-internal-start-component", `Error ZWEL???E: Could not create pipe.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

New error message or use existing. ZWEL0064E is generic, but referring to command.

@Martin-Zeithaml
Copy link
Contributor

Working fine on our system. I have noticed this node message:

...
ZWESVUSR INFO ZWEL0069I Configuration is valid
ZWESVUSR INFO ZWEL0058I WORKSPACE_DIR is '/zowe/workspace'
FSUM7422 node is not found 
Node found in NODE_HOME
ZWESVUSR INFO (zwe-internal-start-prepare) Zowe version: v2.16.0       
ZWESVUSR INFO (zwe-internal-start-prepare) build and hash: PR-3812#5131 (008872531f2a6ba28a8264e6f05266422d246d7b)
...

@1000TurquoisePogs
Copy link
Member Author

I think the "node not found" came from the PR where node was removed as a prereq. Some code somewhere must be checking for it and then complaining, yet continuing anyway.

Signed-off-by: 1000TurquoisePogs <[email protected]>
Copy link

github-actions bot commented Apr 29, 2024

build 5140 SUCCEEDED.
Link to workflow run: https://github.com/zowe/zowe-install-packaging/actions/runs/8878212985

@1000TurquoisePogs
Copy link
Member Author

Error message assigned.
Node warning found and corrected here zowe/zlux-app-server#301

Copy link

github-actions bot commented Apr 29, 2024

Test workflow 4399 is started.
Running install test: Convenience Pax
The zowe artifact being used by this test workflow: libs-snapshot-local/org/zowe/2.16.0-PR-3812/zowe-2.16.0-pr-3812-5140-20240429121125.pax
Running on machine: zzow08
Result: SUCCESS
Link to workflow run: https://github.com/zowe/zowe-install-packaging/actions/runs/8878303647

Signed-off-by: Martin Zeithaml <[email protected]>
const startScriptContents = `cd ${COMPONENT_DIR} ; . "${ZOWE_CONFIG.zowe.runtimeDirectory}/bin/libs/configmgr-index.sh" ; ${xplatform.loadFileUTF8(fullPath, xplatform.AUTO_DETECT)} ; wait;`;
const pipeArray = os.pipe();
if (!pipeArray) {
common.printFormattedError("ZWELS", "zwe-internal-start-component", `Error ZWEL064E: failed to run command os.pipe - Connot start component ${componentId}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo Connot

Copy link

github-actions bot commented Apr 29, 2024

build 5141 SUCCEEDED.
Link to workflow run: https://github.com/zowe/zowe-install-packaging/actions/runs/8881219601

Copy link

github-actions bot commented Apr 29, 2024

Test workflow 4400 is started.
Running install test: Convenience Pax
The zowe artifact being used by this test workflow: libs-snapshot-local/org/zowe/2.16.0-PR-3812/zowe-2.16.0-pr-3812-5141-20240429153423.pax
Running on machine: zzow07
Result: SUCCESS
Link to workflow run: https://github.com/zowe/zowe-install-packaging/actions/runs/8881347947

@1000TurquoisePogs 1000TurquoisePogs merged commit 5c0d368 into v2.x/staging Apr 30, 2024
11 checks passed
@1000TurquoisePogs 1000TurquoisePogs deleted the feature/v2/reduce-shell-count branch April 30, 2024 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants