Add IBM Power VS: installconfig assets by clnperez · Pull Request #5612 · openshift/installer

clnperez · 2022-02-04T23:23:48Z

For more background on IPI on Power VS, refer to the enhancement
proposal here: openshift/enhancements#736

Older discussions on some of the code here can be found in #5224

Signed-off-by: Christy Norman christy@linux.vnet.ibm.com

clnperez · 2022-03-01T17:55:04Z

some failing tests now due to some updates in other PRs, e.g.

# github.com/openshift/installer/pkg/asset/installconfig/powervs
pkg/asset/installconfig/powervs/platform.go:24:3: p.APIKey undefined (type powervs.Platform has no field or method APIKey)
pkg/asset/installconfig/powervs/session.go:89:28: too many arguments in call to ibmpisession.New

I'll fix those today and maybe this can be merged next, to get to #5610 #5611, etc.

clnperez · 2022-03-03T18:24:43Z

i think this should be in good shape now @rna-afk. Sorry for the delay

rna-afk · 2022-03-03T18:28:03Z

No worries. Can you move the vendor changes to one commit and the code changes to another?

clnperez · 2022-03-03T18:30:40Z

sure @rna-afk . In that vendor commit, should I put all the files under vendor/ as well as go.mod and go.sum?

rna-afk · 2022-03-03T18:31:38Z

Yeah that would be great

clnperez · 2022-03-03T18:34:03Z

done @rna-afk

clnperez · 2022-03-04T14:55:01Z

@rna-afk all required tests are passing, and i think #5610 and #5611 should be in good shape to merge after this one

rna-afk

Looks good. Just a few nits.

rna-afk · 2022-03-04T15:51:42Z

/cc @patrickdillon for approval

openshift-ci · 2022-03-04T15:54:45Z

@rna-afk: GitHub didn't allow me to request PR reviews from the following users: for, approval.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @patrickdillon for approval

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.

clnperez · 2022-03-04T21:07:01Z

Addressed comments. So far failing tests look like environment issues but I'll check back with they're all completed.

clnperez · 2022-03-05T01:08:31Z

/retest-required

rna-afk · 2022-03-07T14:41:50Z

/lgtm

clnperez · 2022-03-07T23:43:07Z

@patrickdillon okay to merge?

Prashanth684 · 2022-03-08T21:36:05Z

the way to override would be to create the manifests and just change the image reference to point to the pre-created bootimage right? this wouldn't be needed

that's one thing we could do that hadn't been discussed before, yah. but i would still like to have some kind of var that points to an ova and still have the installer create the boot image so that the user doesn't have to both put it in a cos bucket, and import it into the appropriate service instance. and that, iirc, is what you said we shouldn't also use this OPENSHIFT_INSTALL_OS_IMAGE_OVERRIDE for. also, we're planning to create the power-iaas service instance on the fly, so have a pre-created boot image to reference wouldn't be an option.

right..an extra step of importing it into the service instance compared to other clouds but i prefer that than the approach of specifying the env override. i'm not entirely opposed to this..but just questioning the need and making sure we are consistent with other clouds

I am fine leaving this in ATM but it will need to be removed before feature freeze. Users should never use OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE. If you want users to be able to tweak it, it should go in the install config. OpenStack has a pattern with an optional ClusterOSImage if you want to check that out.

For more background on IPI on Power VS, refer to the enhancement proposal here: openshift/enhancements#736 Older discussions on some of the code here can be found in openshift#5224 Signed-off-by: Christy Norman <christy@linux.vnet.ibm.com>

a seperate commit for the vendor updates Signed-off-by: Christy Norman <christy@linux.vnet.ibm.com>

patrickdillon · 2022-03-09T20:48:17Z

/retest

patrickdillon

It looks like you fixed the vendor issues spotted by @staebler's eagle eye.

/approve

patrickdillon · 2022-03-10T01:17:54Z

I am fine leaving this in ATM but it will need to be removed before feature freeze. Users should never use OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE. If you want users to be able to tweak it, it should go in the install config. OpenStack has a pattern with an optional ClusterOSImage if you want to check that out.

patrickdillon · 2022-03-10T01:24:21Z

+// ValidateForProvisioning only validates credentials
+// @TODO: Expand this to use the install config creds
+func ValidateForProvisioning() error {
+	_, err := GetSession()


nit: I believe this is redundant with the PlatformCredsCheck

i agree. i was just under the impression that we'd need to have something for both. we can take this out later.

patrickdillon · 2022-03-10T01:25:37Z

/retest

openshift-ci · 2022-03-10T01:26:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [patrickdillon]

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

clnperez · 2022-03-10T15:45:40Z

@patrickdillon is there a chance to get some kind of override and just merge this without re-testing for those aws tests? the failures are unrelated and seem to be just some aws flakiness.

ausil · 2022-03-10T17:00:52Z

/test e2e-aws
/test e2e-aws-upgrade

patrickdillon · 2022-03-11T00:33:19Z

/skip

/override ci/prow/e2e-aws-upgrade

openshift-ci · 2022-03-11T00:33:59Z

@patrickdillon: Overrode contexts on behalf of patrickdillon: ci/prow/e2e-aws-upgrade

Details

In response to this:

/skip

/override ci/prow/e2e-aws-upgrade

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.

clnperez · 2022-03-11T15:06:58Z

thanks @patrickdillon. I believe we still may need to override e2e-aws as they also keep failing.

Prashanth684 · 2022-03-11T16:30:30Z

/lgtm

openshift-bot · 2022-03-11T18:39:42Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-03-11T20:51:38Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-03-11T22:03:37Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-03-11T23:52:13Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-03-12T01:11:06Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-03-12T03:21:06Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-03-12T03:34:06Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-03-12T05:52:08Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2022-03-12T11:40:59Z

@clnperez: 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-aws-workers-rhel7	3fa202e809bcdf6c4b41ef13411fb671607b0952	link	false	`/test e2e-aws-workers-rhel7`
ci/prow/e2e-ibmcloud	`51da6a8`	link	false	`/test e2e-ibmcloud`
ci/prow/e2e-azure-upi	`51da6a8`	link	false	`/test e2e-azure-upi`
ci/prow/e2e-openstack-kuryr	`51da6a8`	link	false	`/test e2e-openstack-kuryr`
ci/prow/e2e-libvirt	`51da6a8`	link	false	`/test e2e-libvirt`
ci/prow/e2e-metal-ipi-ovn-ipv6	`51da6a8`	link	false	`/test e2e-metal-ipi-ovn-ipv6`
ci/prow/e2e-openstack	`51da6a8`	link	false	`/test e2e-openstack`

Full PR test history. Your PR dashboard.

Details

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.

openshift-bot · 2022-03-12T12:13:10Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci Bot requested review from jstuever and patrickdillon February 4, 2022 23:24

rna-afk reviewed Feb 7, 2022

View reviewed changes

Comment thread pkg/asset/installconfig/basedomain.go Outdated

Comment thread pkg/asset/installconfig/platformcredscheck.go Outdated

clnperez force-pushed the new-installconfig-assets branch from 3fa202e to 3e8c73e Compare February 9, 2022 22:54

clnperez force-pushed the new-installconfig-assets branch 2 times, most recently from dd4d284 to 846cb29 Compare March 1, 2022 17:21

clnperez mentioned this pull request Mar 1, 2022

Add IBM Power VS: manifest assets #5610

Merged

clnperez force-pushed the new-installconfig-assets branch 3 times, most recently from bd998f7 to 36b0216 Compare March 3, 2022 18:21

clnperez force-pushed the new-installconfig-assets branch from 36b0216 to 87e2b4b Compare March 3, 2022 18:33

rna-afk reviewed Mar 4, 2022

View reviewed changes

clnperez force-pushed the new-installconfig-assets branch from 87e2b4b to b7e2a2b Compare March 4, 2022 18:32

openshift-ci Bot assigned rna-afk Mar 7, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2022

Prashanth684 reviewed Mar 8, 2022

View reviewed changes

clnperez mentioned this pull request Mar 8, 2022

Add IBM Power VS: machine assets #5611

Merged

clnperez force-pushed the new-installconfig-assets branch from b7e2a2b to 364dcdd Compare March 9, 2022 19:58

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2022

clnperez added 2 commits March 9, 2022 14:01

Add IBM Power VS: installconfig assets

5f767fa

For more background on IPI on Power VS, refer to the enhancement proposal here: openshift/enhancements#736 Older discussions on some of the code here can be found in openshift#5224 Signed-off-by: Christy Norman <christy@linux.vnet.ibm.com>

power vs installconfig assets: vendoring

51da6a8

a seperate commit for the vendor updates Signed-off-by: Christy Norman <christy@linux.vnet.ibm.com>

clnperez force-pushed the new-installconfig-assets branch from 364dcdd to 51da6a8 Compare March 9, 2022 20:01

patrickdillon reviewed Mar 10, 2022

View reviewed changes

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 10, 2022

openshift-ci Bot assigned Prashanth684 Mar 11, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2022

openshift-merge-robot merged commit d8f68f3 into openshift:master Mar 12, 2022

clnperez mentioned this pull request Mar 14, 2022

Add IBM Power VS: cluster assets #5651

Merged

Conversation

clnperez commented Feb 4, 2022

Uh oh!

Uh oh!

Uh oh!

clnperez commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clnperez commented Mar 3, 2022

Uh oh!

rna-afk commented Mar 3, 2022

Uh oh!

clnperez commented Mar 3, 2022

Uh oh!

rna-afk commented Mar 3, 2022

Uh oh!

clnperez commented Mar 3, 2022

Uh oh!

clnperez commented Mar 4, 2022

Uh oh!

rna-afk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rna-afk commented Mar 4, 2022

Uh oh!

openshift-ci Bot commented Mar 4, 2022

Uh oh!

clnperez commented Mar 4, 2022

Uh oh!

clnperez commented Mar 5, 2022

Uh oh!

rna-afk commented Mar 7, 2022

Uh oh!

clnperez commented Mar 7, 2022

Uh oh!

Prashanth684 Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

clnperez Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Prashanth684 Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickdillon Mar 10, 2022

Choose a reason for hiding this comment

Uh oh!

patrickdillon commented Mar 9, 2022

Uh oh!

patrickdillon left a comment

Choose a reason for hiding this comment

Uh oh!

patrickdillon Mar 10, 2022

Choose a reason for hiding this comment

Uh oh!

patrickdillon Mar 10, 2022

Choose a reason for hiding this comment

Uh oh!

clnperez Mar 10, 2022

Choose a reason for hiding this comment

Uh oh!

patrickdillon commented Mar 10, 2022

Uh oh!

openshift-ci Bot commented Mar 10, 2022

Uh oh!

clnperez commented Mar 10, 2022

Uh oh!

ausil commented Mar 10, 2022

Uh oh!

patrickdillon commented Mar 11, 2022

Uh oh!

clnperez commented Mar 1, 2022 •

edited

Loading

clnperez Mar 8, 2022 •

edited

Loading

Prashanth684 Mar 9, 2022 •

edited

Loading

openshift-ci Bot commented Mar 12, 2022 •

edited

Loading