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

FIM IT: add fim sync overlap testing #3380

Conversation

Deblintrake09
Copy link
Contributor

@Deblintrake09 Deblintrake09 commented Sep 29, 2022

Related issue
#3053

Description

This PR aims to add new tests related to the behavior of the response_timeout synchronization option. When a new sync is fired after the sync interval has elapsed, if response_timeout time has not elapsed since the last sync message, the sync interval value will be doubled. After a successful synchronization, when a new sync is fired, the interval value will be reset to the configured value.

Added

  • test_fim/test_synchronization/test_sync_overlap.py

Testing performed

The test test_max_eps.py module has erratic behavior and sometimes fails. It was fixed on Issue #3036, that was merged into the target branch, so it has not been fixed here again.

Tester Component Test path Jenkins Local OS Commit Notes
@Deblintrake09 Manager test_fim 🟡 🟡 🟡 🟡🟡🟡 Centos 220a2e2 Failing tests are fixed in target branch
@Deblintrake09 Agent test_fim 🟢 🟢 🟢 Centos 220a2e2
@Deblintrake09 Agent test_fim 🟢 🟢 🟢 Windows 220a2e2
@Deblintrake09 Agent test_fim 🟢 🟢 🟢 Solaris 220a2e2
@Deblintrake09 Agent test_fim 🟢 🟢 🟢 macOS 220a2e2
@damarisg (Reviewer) Manager test_fim 🟢 🚫 Centos 220a2e2
@damarisg (Reviewer) Agent test_fim 🟢 🚫 Centos 220a2e2
@damarisg (Reviewer) Agent test_fim 🟢 🚫 Windows 220a2e2
@damarisg (Reviewer) Agent test_fim 🟢 🚫 Solaris 220a2e2
@damarisg (Reviewer) Agent test_fim 🟢 🚫 macOS 220a2e2

Comment on lines +48 to +59
@pytest.fixture(scope='function')
def create_files_in_folder(files_number):
"""Create files in monitored folder and files"""

for file in range(0, files_number):
create_file(REGULAR, MONITORED_DIR_1, f"test_file_{time.time()}_{file}")

yield

delete_path_recursively(MONITORED_DIR_1)


Copy link
Member

Choose a reason for hiding this comment

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

  • In tools/file.py file we store different methods related to files. Why don't you add it there?
  • Why don't you add the arguments in the documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not added there because this is a fixture, and needs to be located in the conftest file.

Comment on lines +81 to +94
@pytest.fixture(scope='session')
def configure_local_internal_options_fim():
"""Fixture to configure the local internal options file."""

# Backup the old local internal options
backup_local_internal_options = get_wazuh_local_internal_options()

# Set the new local internal options configuration
set_wazuh_local_internal_options(create_local_internal_options(FIM_DEFAULT_LOCAL_INTERNAL_OPTIONS))

yield

# Backup the old local internal options cofiguration
set_wazuh_local_internal_options(backup_local_internal_options)
Copy link
Member

Choose a reason for hiding this comment

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

Why do you add it there? In the integration/conftest.py file are these methods.

I read the description, but could you clarify your decision applied here? I do not see sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not the same as the one in integration/conftest.py, which needs the get_local_internal_options fixture to be present in the test module, and that uses the get_configuration fixture.

Comment on lines +128 to +135
@pytest.fixture(scope="function")
def restart_syscheck_function():
"""
Restart syscheckd daemon.
"""
control_service("stop", daemon="wazuh-syscheckd")
truncate_file(LOG_FILE_PATH)
control_service("start", daemon="wazuh-syscheckd")
Copy link
Member

Choose a reason for hiding this comment

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

Why do you add it there? In the integration/conftest.py file has defined it.

When you create a new method, you should make sure it doesn't exist elsewhere so as not to duplicate code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This version This has been added here, following the structure applied to the VDT test suite refactor. Having a specific function that restarts only syscheck allows for tests to be a bit cleaner by not needing to pass the daemon to stop as a parameter.

Copy link
Member

@damarisg damarisg left a comment

Choose a reason for hiding this comment

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

I suggested some cases, but I didn't see them added. Was it for any reason?

Cases:

  • I don't put response_timeout and max_interval, and I have synchronization enabled.
    • I don't put response_timeout, and I have synchronization enabled.
    • I don't put max_interval, and I have synchronization enabled.
    • I put response_timeout and max_interval, the interval is greater than max_interval , and I have synchronization enabled.
    • the synchronization is done before the max_interval and interval is reached.

@Deblintrake09
Copy link
Contributor Author

Update 2022/10/21

  • Applied requested changes
  • Added new test module test_sync_time
  • On Hold until Core does rebase on branch to run Jenkins instances.

@wazuh wazuh deleted a comment from Deblintrake09 Oct 28, 2022
@wazuh wazuh deleted a comment from Deblintrake09 Oct 28, 2022
@wazuh wazuh deleted a comment from Deblintrake09 Oct 28, 2022
@Deblintrake09
Copy link
Contributor Author

Deblintrake09 commented Nov 23, 2022

Update 2022/11/22

Update 2022/11/23

  • Debug and manual test errors.
  • Ask Core about messages not appearing

@damarisg damarisg merged commit b4581d1 into 1915-fim-rsync-new-synchronization-mechanism Nov 25, 2022
@damarisg damarisg deleted the 3053-sync-overlap-mechanism branch November 25, 2022 14:24
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.

Tests development - Test for new FIM DB sync options
2 participants