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

feat: Added tests for multi-platform dynamic IBM P&Z #1078

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

tisutisu
Copy link
Contributor

Description

This PR adds two tests related to dynamic allocation in IBM P&Z platforms

Issue ticket number and link

https://issues.redhat.com/browse/STONEBLD-1859
https://issues.redhat.com/browse/STONEBLD-1858

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

locally with ginkgo --label-filter="multi-platform" --v ./cmd

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added meaningful description with JIRA/GitHub issue key(if applicable), for example HASSuiteDescribe("STONE-123456789 devfile source")
  • I have updated labels (if needed)

@tisutisu
Copy link
Contributor Author

/test e2e-on-pull-request

Copy link

openshift-ci bot commented Mar 18, 2024

@tisutisu: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test images
  • /test load-test-ci-100u-1t
  • /test load-test-ci-10u-10t
  • /test load-test-ci-10u-10t-go
  • /test load-test-ci-10u-10t-java
  • /test load-test-ci-10u-10t-nodejs
  • /test load-test-ci-10u-10t-python
  • /test load-test-ci-10u-10t-quarkus
  • /test load-test-ci-1u-100t
  • /test load-test-ci-20u-5t
  • /test load-test-ci-2u-50t
  • /test load-test-ci-50u-2t
  • /test load-test-ci-5u-20t
  • /test load-test-ci-max-concurrency-basic-tekton-tuned
  • /test load-test-ci-poc
  • /test load-test-ci-tekton-tuning-base
  • /test load-test-ci-tekton-tuning-tuned
  • /test max-concurrency-advanced
  • /test max-concurrency-basic
  • /test redhat-appstudio-e2e

Use /test all to run the following jobs that were automatically triggered:

  • pull-ci-redhat-appstudio-e2e-tests-main-images
  • pull-ci-redhat-appstudio-e2e-tests-main-redhat-appstudio-e2e

In response to this:

/test e2e-on-pull-request

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.

Copy link
Contributor

@mmorhun mmorhun left a comment

Choose a reason for hiding this comment

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

LGTM

tests/build/multi-platform.go Outdated Show resolved Hide resolved
Expect(f.AsKubeAdmin.CommonController.DeleteSecret(ControllerNamespace, SshSecretName)).To(Succeed())
Expect(f.AsKubeAdmin.CommonController.DeleteConfigMap(HostConfig, ControllerNamespace, true)).To(Succeed())

//Forcefully remove instance incase it is not removed by multi-platform-controller
Copy link
Contributor

Choose a reason for hiding this comment

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

a typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which one?

@tisutisu tisutisu force-pushed the multi-platform-ibm-tests branch from 58d2d0a to 0df8cc1 Compare March 20, 2024 06:20

})
})
Describe("ibmp dynamic allocation", Label("ibmp-dynamic"), func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: use full spell "Power PC" instead of "p" so that it reflects the meaning straightforward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in the describe section

}

//sleep for 10 sec so that multi-platform-controller pod comes up
time.Sleep(10 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not use Eventually to wait for the pod comes up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes 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.

Added now.

@tkdchen
Copy link
Contributor

tkdchen commented Mar 20, 2024

Are these tests only for dynamic provision or the whole process of a multi-arch build?

lastBundle.Value = *tektonapi.NewStructuredValues("quay.io/redhat-appstudio-tekton-catalog/task-buildah-remote:0.1")
lastName.Value = *tektonapi.NewStructuredValues("buildah-remote")
t.Params = append(t.Params, tektonapi.Param{Name: "PLATFORM", Value: *tektonapi.NewStructuredValues("$(params.PLATFORM)")})
dockerPipelineObject.Spec.Params = append(dockerPipelineObject.PipelineSpec().Params, tektonapi.ParamSpec{Name: "PLATFORM", Default: tektonapi.NewStructuredValues(platformType)})
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know how much it matters, but you would probably be better off just having a single pipeline adding a new pipeline level parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in that case the logic to create the bundle will be changed, but still we have to create 3 bundle and push them to quay separately for all the 3 architectures arm64, s30x and ppc64le.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't, you just add a 'PLATFORM' param to the main pipeline, change this param to reference it, then pass it in in the tests. It's not a big deal either way.


BeforeAll(func() {

f, err = framework.NewFramework(utils.GetGeneratedNamespace("multi-platform-ibmz"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Other than the setup are all these tests pretty much the same? It seems like a lot of duplication here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me try to reduce the duplicates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried to reduce duplicates as much as possible. please have a look again.

@tisutisu
Copy link
Contributor Author

Are these tests only for dynamic provision or the whole process of a multi-arch build?

@tkdchen the whole suite contains tests for host-pool for aws as well, but the stories that current PR is addressing is dynamic allocation for ibmp and ibmz architectures

@tisutisu tisutisu force-pushed the multi-platform-ibm-tests branch from f1a7142 to 3c88673 Compare March 21, 2024 07:44
Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
1.4% Duplication on New Code

See analysis details on SonarCloud

@mmorhun
Copy link
Contributor

mmorhun commented Mar 21, 2024

/lgtm
/approve

@tisutisu
Copy link
Contributor Author

/approve

Copy link

openshift-ci bot commented Mar 21, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mmorhun, stuartwdouglas, tisutisu, tkdchen

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

@tisutisu tisutisu merged commit 13ba743 into konflux-ci:main Mar 21, 2024
14 checks passed
@tisutisu tisutisu deleted the multi-platform-ibm-tests branch March 21, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants