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

Implement cluster size check and loggin addon test case on test_upgrade.py #1485

Merged
merged 7 commits into from
Sep 12, 2024

Conversation

albinsun
Copy link
Contributor

@albinsun albinsun commented Aug 28, 2024

Which issue(s) this PR fixes & What it does:

  1. [BUG] Upgrade is NOT blocked even there are Degraded volume harvester#6425
    • Skip test_upgrade.py::TestInvalidUpgrade::test_degraded_volume when cluster size < 3.
    • If not skip, 1 and 2 nodes cluster will just start upgrade in this negtive case Class.
  2. [e2e] [BUG] Addon, rancher-logging only shows "processing" after it had been DeploySuccessful prior to Upgrade #901
    • Implement test_preq_setup_logging TODO
    • Post-upgrade check rancher-logging addon status in test_verify_logging_pods

Special notes for your reviewer:

n/a

Additional documentation or context

Verification
image

@lanfon72
Copy link
Member

We should check whether it is a feature or not.
If it's a bug, the failure of test case will be expected, which means we would not need to skip it. (or skip it when the bug is not fixed in previous version.)

@albinsun
Copy link
Contributor Author

albinsun commented Aug 29, 2024

We should check whether it is a feature or not. If it's a bug, the failure of test case will be expected, which means we would not need to skip it. (or skip it when the bug is not fixed in previous version.)

Yes, currently it's by design (feature).
Ref. harvester/harvester#6425 (comment)

@lanfon72
Copy link
Member

We should check whether it is a feature or not. If it's a bug, the failure of test case will be expected, which means we would not need to skip it. (or skip it when the bug is not fixed in previous version.)

Yes, currently it's by design (feature). Ref. harvester/harvester#6425 (comment)

Let's add it as reference with comment in the code.
cluster_state is used to store the information before and after upgrade, so we didn't use it in this test case, we shouldn't too.

I think I would need to add some docstring to explain the ClusterState 🤔

@@ -494,6 +499,10 @@ def test_degraded_volume(
3. Immediately upgrade Harvester.
4. Upgrade should fail.
"""
# https://github.com/harvester/harvester/issues/6425
if cluster_state.size < 3:
pytest.skip(f"Degraded only checked when nodes >= 3, skip for {cluster_state.size}.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: Degraded volumes only checked when nodes >= 3 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 702026f.
image

@albinsun albinsun changed the title Skip test_degraded_volume when cluster size less than 3 Implement cluster size check and loggin addon test case on test_upgrade.py Aug 30, 2024
@albinsun
Copy link
Contributor Author

albinsun commented Aug 30, 2024

Guys, as we on the way of test_upgrade.py enhancement, so I also implement a TODO and #901 together.
Please help to review again for the new committed about logging addon.

Sorry for inconvenience, no more new item will be add to this PR. :P

Verification (harvester-upgrade-test/281)
image

Copy link
Member

@lanfon72 lanfon72 left a comment

Choose a reason for hiding this comment

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

I need to explain how the TestUpgrade works, then you can know why the implementation need to change.

As the manual test, steps would like:

  1. Set up environment before upgrade
  2. Perform upgrade
  3. Verify the environment is expected

What we do in upgrade automation is,

  1. test case starts with test_pre_setup is going to be executed before upgrade
  2. test case test_perform_upgrade will start the upgrade
  3. test case starts with test_verify is going to verify the test case (after upgraded) hit criteria

cluster state changes during upgrade will be recorded in cluster_state. (that's why we will not use it in TestInvalidUpgrade)

Let's look into the case of setup_logging, we move logging to addon after v1.2.0, so we need to consider following cases:

  1. A upgrade to v1.2.0 (implemented)
  2. >v1.2.0 upgrade to B

When you implement case 2, you would still need to consider the case 1 in both pre_setup and verify.

harvester_e2e_tests/integrations/test_upgrade.py Outdated Show resolved Hide resolved
harvester_e2e_tests/integrations/test_upgrade.py Outdated Show resolved Hide resolved
@albinsun
Copy link
Contributor Author

albinsun commented Sep 6, 2024

Hi @lanfon72 ,
Have tried to refactor according comments in 7516c7e, please help to review, thanks.

  • Single node
    image
  • Three nodes
    image

@albinsun
Copy link
Contributor Author

albinsun commented Sep 9, 2024

Verification for 6a19ea8

  • Logging addon enabled already
    logging_enabled_already
  • Logging addon enabled yet
    logging_enabled_yet

Copy link
Member

@lanfon72 lanfon72 left a comment

Choose a reason for hiding this comment

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

LGTM

@khushboo-rancher khushboo-rancher merged commit 3534acd into harvester:main Sep 12, 2024
4 checks passed
@albinsun albinsun deleted the enhance_test_upgrade branch September 12, 2024 01:49
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