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

Fixes for testing with molecule #807

Merged
merged 11 commits into from
Jul 19, 2024

Conversation

berndfinger
Copy link
Member

These changes should ensure that the molecule idempotency tests no longer fail, see issue #752.

The task "Reload kernel parameters from file /etc/sysctl.d/sap_hana.conf"
now recognizes a parameter change.

Relates to sap-linuxlab#752.

Signed-off-by: Bernd Finger <[email protected]>
The task "Reload kernel parameters from file
/etc/sysctl.d/91-NetApp-HANA.conf" now recognizes a parameter change.

Relates to sap-linuxlab#752.

Signed-off-by: Bernd Finger <[email protected]>
The task "Reload kernel parameters from file /etc/sysctl.d/sap.conf"
now recognizes a parameter change.

Relates to sap-linuxlab#752.

Signed-off-by: Bernd Finger <[email protected]>
The task "Ensure that the required package groups are installed, RHEL
8.1" now has a tag for skipping the molecule idempotency test

Relates to sap-linuxlab#752.

Signed-off-by: Bernd Finger <[email protected]>
The task "Reload kernel parameters from file
'/etc/sysctl.d/ibm_largesend.conf'" now recognizes a parameter change.

Relates to sap-linuxlab#752.

Signed-off-by: Bernd Finger <[email protected]>
@ja9fuchs
Copy link
Contributor

Why the overhead of executing "x entries + 1" commands per sysctl file and not just sysctl -p <file> without those checks first? The reload does not make changes as such, it only activates what has been defined.

Since the checks do not validate the content of the config file, there is no benefit to checking if all of the settings are already active or not. If one deviates, it will also reload the whole file, not just the deviating entry.

It's not 100% idempotent unless you add even more overhead to check and load each entry individually.
But again, there is no benefit of doing this, unless there is an actual sanity verification of the settings to prevent a mis-configuration from being loaded.

@berndfinger
Copy link
Member Author

Why the overhead of executing "x entries + 1" commands per sysctl file and not just sysctl -p <file> without those checks first? The reload does not make changes as such, it only activates what has been defined.

We have three different sysctl files to modify according to applicable SAP notes, and because we want to follow the SAP notes as closely as possible, we do the modification for each file separately. For each file, we perform the modification of the file and then reload the parameters from the file (using sysctl -p) so that it becomes effective immediately.

Since the checks do not validate the content of the config file, there is no benefit to checking if all of the settings are already active or not. If one deviates, it will also reload the whole file, not just the deviating entry.

I am not checking if all of the settings of a config file are active or not. I want to know if any, and which one(s) of the entries or which of the current settings have been modified after reloading all the settings.

So we need to compare the desired state with the current state. The solution I offered appears to be robust and easy to understand. Happy to implement a better solution if there is one!

It's not 100% idempotent unless you add even more overhead to check and load each entry individually. But again, there is no benefit of doing this, unless there is an actual sanity verification of the settings to prevent a mis-configuration from being loaded.

We want to detect if there was a change or not. A misconfiguration should result in a task error in the tasks Reload kernel parameters from file XXX, so I don't think we need to do a sanity verification.

But maybe I misunderstood some of your points. In this case, can you please provide examples?

@glavoix
Copy link

glavoix commented Jul 18, 2024

Dear @berndfinger
After reviewing your commit, I believe the approach looks nice and we should now be able to enable "idem-potency" into our molecule testing.

Thanks a lot for working on it.

Guillaume

These additional tasks are necessary to correctly report the 'changed' state of 'sysctl -p'.

Relates to sap-linuxlab#752.

Signed-off-by: Bernd Finger <[email protected]>
@berndfinger berndfinger marked this pull request as draft July 19, 2024 08:47
This additional task is necessary to correctly report the 'changed' state of 'sysctl -p'.

Relates to sap-linuxlab#752.

Signed-off-by: Bernd Finger <[email protected]>
@berndfinger berndfinger marked this pull request as ready for review July 19, 2024 08:53
Copy link
Contributor

@ja9fuchs ja9fuchs left a comment

Choose a reason for hiding this comment

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

Thanks @berndfinger for the comments and explanation. Now I get why these tasks are there. :)

👍

@sean-freeman sean-freeman removed their request for review July 19, 2024 09:17
@berndfinger berndfinger merged commit 6e766fa into sap-linuxlab:dev Jul 19, 2024
4 of 5 checks passed
@berndfinger berndfinger deleted the issue-752-fixes-for-molecule branch July 19, 2024 09:34
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