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

Minimal shell: honor full chain of hooks, on either minimal entry point #3377

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

glima
Copy link
Contributor

@glima glima commented Aug 12, 2024

We have 2 points in which LISA might decide to go with a minimal shell profile:

  1. When first contacting a node, issuing a 'cmd' command, checking if
    'Windows' is present in the output or not (rule out Windows shell
    type). A negative case will incur in either the POSIX shell type,
    if 'Unknown syntax' is not thrown as output, or an early
    decision for the 'minimal' shell type is taken and the shell object is
    set to follow that config from there on.

  2. When running the actual first lisa suite SSH command on the same
    node. It turns out we've seen cases where 'Unknown syntax' is not
    output at early node initialization (1), for some reason. Well, the
    exception path of actual command execution will check internal node
    state, that tracks if a minimal profile has been tested before or
    not. If not used yet and under a command execution exception, set the
    node to minimal type and try again was the logic.

It turns out the 'minification' of the node config was only done
partially, in (2). We missed the full logic of overriding its final
SSH command tokenizer.

Another missed detail is that the final tokenizer logic at the minimal
shell case was too greedy in trying to remove stray quotes. Let's keep
injecting quotes on tokens with spaces on them. That will not hurt any
legit commands from any minimal context.

@squirrelsc
Copy link
Member

@lubaihua33 @LiliDeng LGTM, please verify it.

@LiliDeng
Copy link
Collaborator

@glima please fix CI check error.

nox > flake8 
./lisa/util/shell.py:215:1: BLK100 Black would make changes.
./lisa/util/shell.py:215:1: E302 expected 2 blank lines, found 1
./lisa/util/shell.py:320:5: E303 too many blank lines (2)

@glima
Copy link
Contributor Author

glima commented Aug 22, 2024

@squirrelsc do I get it in?

@LiliDeng
Copy link
Collaborator

@glima please fix the CI check error, thanks.

./lisa/util/shell.py:49:19: BLK100 Black would make changes.

We have 2 points in which LISA might decide to go with a minimal shell profile:

1. When first contacting a node, issuing a 'cmd' command, checking if
   'Windows' is present in the output or not (rule out Windows shell
   type). A negative case will incur in either the POSIX shell type,
   if 'Unknown syntax' is not thrown as output, or an early decision
   for the 'minimal' shell type is taken and the shell object is set
   to follow that config from there on.

2. When running the actual first lisa suite SSH command on the same
   node. It turns out we've seen cases where 'Unknown syntax' is not
   output at early node initialization (1), for some reason. Well, the
   exception path of actual command execution will check internal node
   state, that tracks if a minimal profile has been tested before or
   not. If not used yet and under a command execution exception, set
   the node to minimal type and try again was the logic.

It turns out the 'minification' of the node config was only done
partially, in (2). We missed the full logic of overriding its final
SSH command tokenizer.

Another missed detail is that the final tokenizer logic at the minimal
shell case was too greedy in trying to remove stray quotes. Let's keep
injecting quotes on tokens with spaces on them. That will not hurt any
legit commands from any minimal context.
@LiliDeng LiliDeng merged commit 4a84849 into microsoft:main Aug 23, 2024
23 checks passed
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.

4 participants