Bug 2037075: Verify Builds with CSI Volume Sources by jkhelil · Pull Request #26646 · openshift/origin

jkhelil · 2021-11-29T14:10:43Z

https://issues.redhat.com/browse/BUILD-275

yselkowitz · 2021-12-05T08:30:33Z

test/extended/testdata/builds/volumes/csi-docker-buildconfig.yaml

Please note that this image is being relocated for multi-arch support in #26598

ack @yselkowitz

gabemontero · 2021-12-06T15:40:47Z

you have to update the bindata @jkhelil ... that appears to be what the verify failure is complaining about

you also minimally have to assign @adambkaplan to this PR to get teh approved label per #26646 (comment)

gabemontero

I'm guessing the ginkgo code of the e2e's is just following the pattern @coreydaley established with his original build volume work

assign this PR to him as well so he can review these

otherwise I added some shared resource specific comments, along with a question to @adambkaplan that might mean a tweak to what you are doing in your OCM PR

gabemontero · 2021-12-06T15:43:31Z

test/extended/testdata/builds/volumes/csi-docker-buildconfig.yaml

+          csi:
+            driver: csi.sharedresource.openshift.io
+            volumeAttributes:
+              sharedSecret: my-secret


Per my comment to you in https://docs.google.com/document/d/1F3VFkiLUJbsUkFJcjnA2oyROYTI5RRlTJYl4ZWQoNls/edit?disco=AAAASUDEeYE add the `‘refreshResource’ attribute, and set it to a string value of false

Another option as well, which I might like better, is that in your OCM controller PR, if you see CSI driver volume mounts with our shared resource driver here, you adjust their volume attributes to force refreshResources to 'false'

WDYT @adambkaplan

@gabemontero I added the refreshResource attribute

OK putting a comment to reflect our team discussion yesterday on this after I posted my last set of comments, namely
we are NOT going to have the OCM force shared resource CSI volumes to have refreshResource set to false.

That said, it is a supported option we do want to let user opt into.

So I'll need to adjust my request to you here @jkhelil ..... for each of your shared resource test items here, have one where you do not set refreshResource, and then one where you add the refreshsResource: 'false' setting

@gabemontero what is the purpose for adding tests without refreshsResource: 'false', because we know that it shouldnt happen for builds and it will fail(correct me if i am wring about it)

no @jkhelil both scenarios should work

if the user does not add the refreshResource volume attribute, it should work

if the user does add the refreshResourece: 'false' volume attribute, it should work

Hence, I want you to make sure both paths work but having 2 flavors of your various tests here.

@gabemontero can you have a look please to the changes regarding this last comment

ok I'm good with the mix you have for this @jkhelil

git the approve label from @adambkaplan and see if @coreydaley is OK with the way you added to the volumes tests he did, and we'll go from there

also, if the agnostic-cmd test is the only failure in the tests, and if you examine the tests and provide a comment to use explaining that all the sig-builds test have passed, and none of any remaining flakes are build related, we can use the skip prow command to bypass that one

test/extended/testdata/builds/volumes/csi-s2i-buildconfig.yaml

test/extended/testdata/builds/volumes/csi-sharedresourcerolebinding.yaml

jkhelil · 2021-12-08T12:40:33Z

/test e2e-aws-serial

jkhelil · 2021-12-08T12:41:15Z

/test e2e-aws-single-node

gabemontero · 2021-12-08T13:46:59Z

/assign @gabemontero

/assign @coreydaley
(to minimally review your volume test cases)

/assign @adambkaplan
(to minimally get the approve label)

see if you can remember @jkhelil to start doing these assigns yourself.

jkhelil · 2021-12-08T13:56:32Z

ack @gabemontero , I do remember, Just not clear for me if I should wait until all tests are green to assign the PR for approval or not

jkhelil · 2021-12-08T13:58:13Z

/test e2e-gcp-upgrade

gabemontero · 2021-12-08T13:58:33Z

ack @gabemontero , I do remember, Just not clear for me if I should wait until all tests are green to assign the PR for approval or not

I would say if there is more than likely chance that the failed test are non-related flakes, go ahead and assign / start the review process.

jkhelil · 2021-12-10T07:28:33Z

/test e2e-gcp-builds

jkhelil · 2021-12-13T07:00:59Z

/test e2e-agnostic-cmd

gabemontero · 2022-01-05T14:18:14Z

curious the new tests pass even though we have not merged all the OCM/builder/etc. stuff yet

is that expected @jkhelil ?

@adambkaplan FYI

gabemontero · 2022-01-05T15:41:34Z

curious the new tests pass even though we have not merged all the OCM/builder/etc. stuff yet

is that expected @jkhelil ?

@adambkaplan FYI

duh @jkhelil @adambkaplan :-) ... none of the e2e's run so far were against a tech preview cluster so the tests were skipped

sorry for the noise :-)

adambkaplan

@jkhelil we should also have a test on a standard cluster which tries to mount a CSI volume. In this test case, we would expect the build to be created, and then later fail.

adambkaplan · 2022-01-05T20:04:59Z

/retitle Bug 2037075: Verify Builds with CSI Volume Sources

jkhelil · 2022-01-14T07:11:01Z

/test e2e-agnostic-cmd

jkhelil · 2022-01-14T09:12:58Z

/test e2e-aws-cgroupsv2

jkhelil · 2022-01-14T12:13:56Z

/test e2e-aws-csi
/test e2e-aws-fips

jkhelil · 2022-01-14T12:17:17Z

/test e2e-gcp-upgrade

jkhelil · 2022-01-14T14:09:45Z

/assign @adambkaplan for approval

openshift-ci · 2022-01-14T14:09:46Z

@jkhelil: GitHub didn't allow me to assign the following users: for, approval.

Note that only openshift members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

/assign @adambkaplan 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.

gabemontero · 2022-01-14T14:18:04Z

/assign @adambkaplan

jkhelil · 2022-01-18T14:25:31Z

/test e2e-agnostic-cmd

gabemontero · 2022-01-18T14:43:03Z

bump @adambkaplan

are you at least willing to apply the approve label to @jkhelil PR and have others complete the review, or do you think you'll be able to get lgtm level cycles to look at this soon.

other than non related flakes in the agnostic cmd suite, this appears green

adambkaplan

/lgtm

openshift-ci · 2022-01-20T13:56:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, gabemontero, jkhelil

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:

~~test/extended/OWNERS~~ [adambkaplan]

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

openshift-bot · 2022-01-20T16:04:50Z

/retest-required

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

openshift-bot · 2022-01-20T16:40:52Z

/retest-required

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

openshift-ci · 2022-01-20T19:01:07Z

@jkhelil: The following test 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-agnostic-cmd	`4870fbd`	link	false	`/test e2e-agnostic-cmd`

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-01-20T21:27:37Z

/retest-required

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

openshift-bot · 2022-01-20T22:06:39Z

/retest-required

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

openshift-bot · 2022-01-20T22:32:39Z

/retest-required

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

openshift-bot · 2022-01-20T22:45:40Z

/retest-required

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

openshift-bot · 2022-01-20T23:50:39Z

/retest-required

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

openshift-bot · 2022-01-21T00:38:39Z

/retest-required

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

openshift-bot · 2022-01-21T01:14:38Z

/retest-required

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

openshift-ci · 2022-01-21T01:43:59Z

@jkhelil: All pull requests linked via external trackers have merged:

Bugzilla bug 2037075 has been moved to the MODIFIED state.

Details

In response to this:

Bug 2037075: Verify Builds with CSI Volume Sources

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.

openshift-ci bot requested review from coreydaley and deads2k November 29, 2021 14:12

jkhelil changed the title ~~add support for build csi volume source~~ BUILD-275: add support for build csi volume source Nov 29, 2021

yselkowitz reviewed Dec 5, 2021

View reviewed changes

jkhelil force-pushed the build-275 branch 2 times, most recently from 098e68a to 3f945c1 Compare December 6, 2021 15:13

gabemontero mentioned this pull request Dec 6, 2021

Bug 2037638: Add support for build csi volume source openshift/builder#273

Merged

gabemontero suggested changes Dec 6, 2021

View reviewed changes

gabemontero mentioned this pull request Dec 6, 2021

BUILD-275: add support for build csi volume source openshift/openshift-controller-manager#210

Closed

jkhelil force-pushed the build-275 branch from 3f945c1 to d57a866 Compare December 7, 2021 04:30

openshift-ci bot assigned adambkaplan, coreydaley and gabemontero Dec 8, 2021

jkhelil force-pushed the build-275 branch 2 times, most recently from d09498d to d3e8990 Compare December 9, 2021 17:05

adambkaplan requested changes Jan 5, 2022

View reviewed changes

openshift-ci bot changed the title ~~BUILD-275: add support for build csi volume source~~ Bug 2037075: Verify Builds with CSI Volume Sources Jan 5, 2022

openshift-ci bot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Jan 5, 2022

jkhelil force-pushed the build-275 branch from 1282eb6 to 6bcd00e Compare January 13, 2022 13:21

add support for build csi volume source

4870fbd

jkhelil force-pushed the build-275 branch from 6bcd00e to 4870fbd Compare January 13, 2022 13:21

adambkaplan approved these changes Jan 20, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2022

openshift-merge-robot merged commit 2e20177 into openshift:master Jan 21, 2022

yselkowitz mentioned this pull request Jan 25, 2022

Bug 2037075: builds: fix filename in csi build volumes within Tech Preview test #26783

Closed

deads2k mentioned this pull request Jan 26, 2022

Revert "add support for build csi volume source" #26790

Merged

Conversation

jkhelil commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabemontero commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabemontero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkhelil commented Dec 8, 2021

Uh oh!

jkhelil commented Dec 8, 2021

Uh oh!

gabemontero commented Dec 8, 2021

Uh oh!

jkhelil commented Dec 8, 2021

Uh oh!

jkhelil commented Dec 8, 2021

Uh oh!

gabemontero commented Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkhelil commented Dec 10, 2021

Uh oh!

jkhelil commented Dec 13, 2021

Uh oh!

gabemontero commented Jan 5, 2022

Uh oh!

gabemontero commented Jan 5, 2022

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

adambkaplan commented Jan 5, 2022

Uh oh!

jkhelil commented Jan 14, 2022

Uh oh!

jkhelil commented Jan 14, 2022

Uh oh!

jkhelil commented Jan 14, 2022

Uh oh!

jkhelil commented Jan 14, 2022

Uh oh!

jkhelil commented Jan 14, 2022

Uh oh!

openshift-ci bot commented Jan 14, 2022

Uh oh!

gabemontero commented Jan 14, 2022

Uh oh!

jkhelil commented Jan 18, 2022

Uh oh!

gabemontero commented Jan 18, 2022

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

jkhelil commented Nov 29, 2021 •

edited

Loading

gabemontero commented Dec 6, 2021 •

edited

Loading

gabemontero commented Dec 8, 2021 •

edited

Loading

openshift-ci bot commented Jan 20, 2022 •

edited

Loading