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

Add must_gather tests #943

Open
wants to merge 1 commit into
base: release-4.10
Choose a base branch
from

Conversation

marioferh
Copy link
Contributor

Add must_gather tests, manual cherry-pick from NTO openshift/cluster-node-tuning-operator#442

Signed-off-by: Mario Fernandez [email protected]

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 30, 2022

@marioferh: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

Add must_gather tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@coveralls
Copy link

coveralls commented Aug 30, 2022

Pull Request Test Coverage Report for Build 2640

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 60.532%

Totals Coverage Status
Change from base Build 2631: 0.0%
Covered Lines: 1592
Relevant Lines: 2630

💛 - Coveralls

@marioferh
Copy link
Contributor Author

/test e2e-gcp-operator-upgrade

@yanirq
Copy link
Member

yanirq commented Oct 27, 2022

if this is still needed for master we will need to create a must gather test lane instead of e2e-gcp

@marioferh
Copy link
Contributor Author

It is needed, but also good for future PR's.

@yanirq
Copy link
Member

yanirq commented Nov 2, 2022

@marioferh we actually need this cherry pick for 4.11 and master branch here dont we ?
At least until we move must gather to a different repo

}

mgImage := "quay.io/openshift-kni/performance-addon-operator-must-gather"
mgTag := "4.11-snapshot"
Copy link
Member

Choose a reason for hiding this comment

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

notice that you are using here the 4.10 branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right. Fixed.

Signed-off-by: Mario Fernandez <[email protected]>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 3, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: marioferh
Once this PR has been reviewed and has the lgtm label, please assign fromanirh for approval by writing /assign @fromanirh in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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
Copy link
Contributor

openshift-ci bot commented Nov 3, 2022

@marioferh: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gcp 0397579 link true /test e2e-gcp
ci/prow/e2e-gcp-operator-upgrade 0397579 link true /test e2e-gcp-operator-upgrade

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link
Member

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

the general direction seems good, some questions inside


It("Verify PAO cluster resources are captured", func() {
profile, _ := profiles.GetByNodeLabels(testutils.NodeSelectorLabels)
if profile == nil {
Copy link
Member

Choose a reason for hiding this comment

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

do we need this for anything else?

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 PR is a cherry-pick. We can continue in the original one: #943

}
//replace peformance.yaml for profile.Name when data is generated in the node
ClusterSpecificFiles := []string{
"cluster-scoped-resources/performance.openshift.io/performanceprofiles/performance.yaml",
Copy link
Member

Choose a reason for hiding this comment

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

...perhaps to construct the names here (and below)?

})

It("Verify hardware related information are captured", func() {
nodeSpecificFiles := []string{
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd move this down below closer to the function using this slice. Now it is unnecessarily far from the calling site.

if err != nil {
return err
}
if !info.IsDir() && info.Name() == "sysinfo.tgz" {
Copy link
Member

Choose a reason for hiding this comment

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

nit: this should probably be a constant (in a must-gather package?)

return nil
}

func Untar(root string, snapshotName string) error {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can/should reuse ghw code here

}

mgImage := "quay.io/openshift-kni/performance-addon-operator-must-gather"
mgTag := "4.10-snapshot"
Copy link
Member

Choose a reason for hiding this comment

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

should this be constructed from PAO version, or, in general, NOT be hardcoded?

@ffromani
Copy link
Member

scratch my comments, I didn't notice this is a cherry pick.

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