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

sap_swpm: Option to enable SWPM observer mode #749

Merged
merged 4 commits into from
Jun 26, 2024

Conversation

rob0d
Copy link
Contributor

@rob0d rob0d commented Jun 5, 2024

@sean-freeman sean-freeman changed the title Option to enable SWPM observer mode sap_swpm: Option to enable SWPM observer mode Jun 14, 2024
Copy link
Member

@sean-freeman sean-freeman left a comment

Choose a reason for hiding this comment

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

@rob0d I'm good to merge this PR, it's non-destructive and can work before the overhaul of the sap_swpm code structure in the coming weeks.

However, in defaults the sap_swpm_swpm_remote_access_user should be uncommented and I would recommend it is by default set to root. If an end user wants to enable this feature and observe using a different OS User, then it can be changed of course. All SWPM executions are otherwise root so I think this default is OK.

@berndfinger
Copy link
Member

Hi @rob0d, we are working on finalizing a new version, and your PR can be part of it. Can you please respond to the suggestion from @sean-freeman?

@rob0d
Copy link
Contributor Author

rob0d commented Jun 20, 2024

@sean-freeman @berndfinger apologies, a bit snowed under.
I would prefer not to do that as it will change the role's default behaviour.
Currently when SWPM is executed SAPINST_REMOTE_ACCESS_USER= and SAPINST_REMOTE_ACCESS_USER_IS_TRUSTED=true are not set. This makes SWPM use root (unix) or Administrator(windows) automatically.
By setting the default to root, these two parameters will be set even thought it's not required as they basically emulate the default behaviour. I have never tried to use SAPINST_REMOTE_ACCESS_USER=root and I don't know how will SWPM react. It should behave the same way, but I am not sure and I won't be able to test it for at least 4 weeks.

So setting the default to root, will change the current default behaviour of the role and I am not sure what impact it will have.

@berndfinger
Copy link
Member

So setting the default to root, will change the current default behaviour of the role and I am not sure what impact it will have.

@rob0d I think in this case, we better not merge this PR into 1.4.1 but continue this discussion afterwards, for including it into 1.4.2, together with more changes for role sap_swpm.

@sean-freeman
Copy link
Member

... in defaults the sap_swpm_swpm_remote_access_user should be uncommented and I would recommend it is by default set to root. If an end user wants to enable this feature and observe using a different OS User, then it can be changed of course. All SWPM executions are otherwise root so I think this default is OK.

Still awaiting this change to be made in defaults, otherwise PR is blocked

@rob0d
Copy link
Contributor Author

rob0d commented Jun 25, 2024

Updated the default to be an empty variable and added code to handle this in a way that it doesn't change SWPM parameters and behaviour.
Unless this is enabled by setting sap_swpm_swpm_observer_mode to true, the behaviour is the same as before, so this is a non breaking change.
If sap_swpm_swpm_remote_access_user is not explicitly set and sap_swpm_swpm_observer_mode = true, it will execute SWPM with standard parameters.
If sap_swpm_swpm_remote_access_user is set, it will add appropriate SWPM command line parameters to use the requested user.

Copy link
Member

@sean-freeman sean-freeman left a comment

Choose a reason for hiding this comment

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

lgtm 👍

Copy link
Member

@berndfinger berndfinger left a comment

Choose a reason for hiding this comment

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

LGTM

@Wabri Wabri self-requested a review June 26, 2024 09:27
@berndfinger berndfinger merged commit 4eb06b4 into sap-linuxlab:dev Jun 26, 2024
3 of 4 checks passed
@rob0d rob0d deleted the swpm_observer_mode branch July 24, 2024 18:29
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