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.

Sign up for GitHub

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

Jump to bottom

Add validation E2E tests #258

Merged
merged 1 commit into from
Nov 15, 2020
Merged

Add validation E2E tests #258

merged 1 commit into from
Nov 15, 2020

Conversation

avishayt
Copy link
Contributor

@avishayt avishayt commented Nov 9, 2020

No description provided.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2020
@avishayt
Copy link
Contributor Author

avishayt commented Nov 9, 2020

/hold WIP

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 9, 2020
@avishayt avishayt force-pushed the avishay/validation_test branch 3 times, most recently from b690f28 to 2aeae01 Compare November 10, 2020 08:24
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2020
@avishayt avishayt force-pushed the avishay/validation_test branch 4 times, most recently from e0d5cae to 123a482 Compare November 10, 2020 09:08
@avishayt
Copy link
Contributor Author

/unhold

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2020
nshidlin
nshidlin reviewed Nov 10, 2020
View reviewed changes
discovery-infra/test_infra/utils.py Outdated
@@ -346,6 +346,91 @@ def is_cluster_in_status(client, cluster_id, statuses):
log.exception("Failed to get cluster %s info", cluster_id)


def wait_till_cluster_validation_is_in_status(
Copy link
Contributor

Choose a reason for hiding this comment

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

these functions can go in Cluster class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

nshidlin
nshidlin reviewed Nov 10, 2020
View reviewed changes
discovery-infra/tests/test_validations.py Outdated

# Check initial validations
self.wait_until_cluster_is_pending_for_input(cluster_id, api_client)
orig_cluster = api_client.cluster_get(cluster_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

use cluster.get_details

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

lalon4
lalon4 reviewed Nov 10, 2020
View reviewed changes
discovery-infra/tests/test_validations.py Show resolved Hide resolved
discovery-infra/tests/base_test.py Outdated Show resolved Hide resolved
discovery-infra/test_infra/utils.py Outdated Show resolved Hide resolved
nshidlin
nshidlin reviewed Nov 10, 2020
View reviewed changes
discovery-infra/tests/test_validations.py Outdated
self.assert_cluster_validation(orig_cluster, "network", "ntp-server-configured", "success")

# Check base DNS domain
api_client.update_cluster(cluster_id, {"base_dns_domain": ""})
Copy link
Contributor

Choose a reason for hiding this comment

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

same here use Cluster

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

osherdp
osherdp suggested changes Nov 10, 2020
View reviewed changes
discovery-infra/tests/test_validations.py Outdated
ram_node.start()

self.wait_until_hosts_are_discovered(cluster_id=cluster_id, api_client=api_client, additional_statuses=[consts.NodesStatus.INSUFFICIENT])
self.wait_for_host_validation(api_client, cluster_id, vcpu_host_id, "hardware", "has-min-cpu-cores", ["success"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the strings part will benefit from keyword arguments, to explain what "hardware", "has-min-cpu-cores" and ["success"] are exactly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's clear, given the method deals with validations?

discovery-infra/test_infra/controllers/node_controllers/node.py Show resolved Hide resolved
discovery-infra/tests/test_validations.py Show resolved Hide resolved
lalon4
lalon4 reviewed Nov 10, 2020
View reviewed changes
discovery-infra/tests/test_validations.py Outdated Show resolved Hide resolved
discovery-infra/tests/test_validations.py Outdated Show resolved Hide resolved
lalon4
lalon4 reviewed Nov 10, 2020
View reviewed changes
discovery-infra/tests/test_validations.py Outdated
vcpu_node.start()

ram_node.shutdown()
ram_node.set_ram_kib(int(env_variables['master_memory']) * 1024)
Copy link
Contributor

Choose a reason for hiding this comment

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

Im thinking now that if test will fail somewhere, then there wont be a clean up for the CPU \ RAM changes (to bring it back to correct values).
can you please add a fixture for applying a proper cleanup for this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We reuse VMs between tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but;

  • stop all nodes
  • format disk
  • set correct boot order
    before\after each test

Copy link
Contributor

Choose a reason for hiding this comment

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

see https://github.com/openshift/assisted-test-infra/blob/master/discovery-infra/tests/base_test.py#L25

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2020
@avishayt avishayt force-pushed the avishay/validation_test branch from 0e2f743 to 933fee8 Compare November 11, 2020 10:19
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2020
@avishayt
Copy link
Contributor Author

@lalon4 @nshidlin Please take another look

@nshidlin
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2020
@lalon4
Copy link
Contributor

lalon4 commented Nov 11, 2020

/lgtm

lalon4
lalon4 reviewed Nov 11, 2020
View reviewed changes
discovery-infra/tests/base_test.py Outdated
@@ -26,6 +26,8 @@ def nodes(self, setup_node_controller):
controller = setup_node_controller
nodes = Nodes(controller, env_variables["private_ssh_key_path"])
nodes.set_correct_boot_order(start_nodes=False)
nodes.run_for_all_nodes("reset_cpu_cores")
Copy link
Contributor

Choose a reason for hiding this comment

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

missed that - this should be running after the yield (both this and the next line)
no?
save orig, run tests, do changes, then back to orig

tsorya
tsorya reviewed Nov 11, 2020
View reviewed changes
discovery-infra/consts.py Outdated Show resolved Hide resolved
tsorya
tsorya reviewed Nov 11, 2020
View reviewed changes
discovery-infra/tests/base_test.py Outdated
@@ -26,6 +26,8 @@ def nodes(self, setup_node_controller):
controller = setup_node_controller
nodes = Nodes(controller, env_variables["private_ssh_key_path"])
nodes.set_correct_boot_order(start_nodes=False)
nodes.run_for_all_nodes("reset_cpu_cores")
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not something specific to test?
I think this must be part of specific test fixture there is not need for this for all tests

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we will leave it here anyway? its one place for cleanup, more intuitive. what WDYT?

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2020
@avishayt avishayt force-pushed the avishay/validation_test branch from 933fee8 to 5199a03 Compare November 15, 2020 12:45
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2020
@avishayt avishayt force-pushed the avishay/validation_test branch from 5199a03 to 20206dc Compare November 15, 2020 13:10
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2020
@tsorya
Copy link
Contributor

tsorya commented Nov 15, 2020

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: avishayt, tsorya

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2020
@openshift-merge-robot openshift-merge-robot merged commit 8ad1d65 into openshift:master Nov 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lalon4 lalon4 lalon4 left review comments

@nshidlin nshidlin nshidlin left review comments

@tsorya tsorya tsorya left review comments

@osherdp osherdp osherdp requested changes

@RazRegev RazRegev Awaiting requested review from RazRegev

Assignees

@osherdp osherdp

@tsorya tsorya

@nshidlin nshidlin

@lalon4 lalon4

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@avishayt @nshidlin @lalon4 @tsorya @openshift-ci-robot @osherdp @openshift-merge-robot