Bug 1951084: remove mounts.conf again, but patch buildah change needed to make transient mounts work for us#239
Merged
openshift-merge-robot merged 2 commits intoopenshift:masterfrom Apr 20, 2021
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
Contributor
Author
I was able to pull in the containers/buildah#3117 version of the buildah change and also verify that host entitlements still work |
gabemontero
force-pushed
the
fix-host-entitlements-on-openshift-buildah-level
branch
from
5db1a99 to
8ceff95
Compare
gabemontero
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
I've redone this PR to vendor in the specific buildah commit that pulls in the change I verified in #239 (comment) |
adambkaplan
reviewed
Apr 13, 2021
Contributor
adambkaplan
left a comment
adambkaplan
left a comment
There was a problem hiding this comment.
/hold
We are rebasing buildah in a separate PR, waiting for this to merge first.
openshift-ci-robot
added
the
do-not-merge/hold
Contributor
Author
Yep was just thinking this AM that when the 1.20.0 bump merges, this PR could lose its bump buildah commit |
openshift-ci-robot
added
the
needs-rebase
Contributor
Author
I was incorrect. containers/buildah#3117 is NOT in v1.20.0 That said, still need to rebase, then add new bump. |
gabemontero
force-pushed
the
fix-host-entitlements-on-openshift-buildah-level
branch
from
8ceff95 to
078a2c6
Compare
openshift-ci-robot
removed
the
needs-rebase
Contributor
Author
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
gabemontero
commented
Apr 13, 2021
| return err | ||
| } | ||
|
|
||
| allMounts := util.SortMounts(append(append(append(append(append(volumes, builtins...), secretMounts...), bindFileMounts...), specMounts...), sysfsMount...)) |
Contributor
Author
There was a problem hiding this comment.
this is the buildah fix we need that is not in the latest buildah tag, v1.20.0
Contributor
Author
|
/retest |
gabemontero
changed the title
openshift-ci-robot
added
bugzilla/severity-medium
Contributor
|
@gabemontero: This pull request references Bugzilla bug 1951084, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
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. |
Contributor
Author
|
/assign @adambkaplan minimally for approval please let me know @adambkaplan if you just want to approve this PR and have me solicit review from other members of @openshift/openshift-team-build-api thanks |
Contributor
Author
|
@xiuwang - verification for this will entail
|
nalind
reviewed
Apr 19, 2021
| return mounts | ||
| } | ||
| mounts = append(mounts, fmt.Sprintf("%s:/run/secrets/%s:rw,nodev,noexec,nosuid", tmpDir, pathEnd)) | ||
| return mounts |
Contributor
Author
There was a problem hiding this comment.
hmm ... curious that even compiled ... thanks @nalind
gabemontero
force-pushed
the
fix-host-entitlements-on-openshift-buildah-level
branch
from
078a2c6 to
0da2bcd
Compare
adambkaplan
reviewed
Apr 19, 2021
Contributor
adambkaplan
left a comment
adambkaplan
left a comment
There was a problem hiding this comment.
/approve
Mostly nits, otherwise in good shape.
Comment on lines
+362
to
+363
| // since the presence of dir secret is not a given, we won't log this a V(0) | ||
| log.V(0).Infof("Red Hat subscription content will not be available in this build: failed to stat directory %s: %v", path, err) |
Contributor
There was a problem hiding this comment.
Meant to log this at V(4)?
Contributor
Author
There was a problem hiding this comment.
part of next push
| log.V(0).Infof("Red Hat subscription content will not be available in this build: failed to stat directory %s: %v", path, err) | ||
| return mounts | ||
| } | ||
| if !st.IsDir() && st.Mode()&os.ModeSymlink == 0 { |
Contributor
There was a problem hiding this comment.
space for readability? (looks like we are doing a bitwise AND here)
Suggested change
| if !st.IsDir() && st.Mode()&os.ModeSymlink == 0 { | |
| if !st.IsDir() && (st.Mode() & os.ModeSymlink == 0) { |
go fmt may remove my parens in the suggestion
Contributor
Author
There was a problem hiding this comment.
go fmt leaves the parens but removes the spaces you added @adambkaplan
if !st.IsDir() && (st.Mode()&os.ModeSymlink == 0) {
that ^^ will be part of next push
| github.com/Nvveen/Gotty v0.0.0-20120604004816-cd527374f1e5 // indirect | ||
| github.com/containers/buildah v1.20.0 | ||
| github.com/containers/common v0.35.3 | ||
| github.com/containers/buildah v1.20.1-0.20210331202609-d29b04dba7fa |
Contributor
There was a problem hiding this comment.
As @nalind pointed out in slack, we now have a v1.20.1 tag upstream.
Contributor
Author
There was a problem hiding this comment.
part of next push
openshift-ci-robot
added
the
approved
gabemontero
force-pushed
the
fix-host-entitlements-on-openshift-buildah-level
branch
from
0da2bcd to
8d766bd
Compare
Contributor
Author
|
updates pushed @adambkaplan @nalind thanks note what |
Contributor
Author
|
e2e-aws hit that new kubelet panic the node team is tracking (per recent slack messages I've seen) /retest |
|
/refresh |
Contributor
|
@gabemontero: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
Launched a cluster and tested entitlement build, no warning anymore. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, gabemontero, wewang58 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/label qe-approved |
openshift-ci-robot
added
the
qe-approved
|
e2e-aws job failed with clusteroperator/ingress should not change condition/Available |
|
/retest e2e-aws |
Contributor
|
@wewang58: The
Use
DetailsIn response to this:
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. |
|
/test e2e-aws |
Contributor
|
/retest Please review the full test history for this PR and help us cut down flakes. |
Contributor
|
@gabemontero: All pull requests linked via external trackers have merged: Bugzilla bug 1951084 has been moved to the MODIFIED state. DetailsIn response to this:
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
deleted the
fix-host-entitlements-on-openshift-buildah-level
branch
Contributor
Author
|
/cherrypick release-4.7 |
|
@gabemontero: #239 failed to apply on top of branch "release-4.7": DetailsIn response to this:
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@nalind @bparees the "meets min" change to buildah that allows host entitlements to work for openshift builds, but without a mounts.conf file in the builder image, and warning message when we run on rhcos
containers/buildah#3117 is a slightly different take
I'll next see about porting that in, and verify on our UPI with entitlements cluster