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.

Sign up for GitHub

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

Jump to bottom

fix pod volume passing and alter infra inheritance #13594

Merged
merged 1 commit into from
Apr 1, 2022
Merged

fix pod volume passing and alter infra inheritance #13594

merged 1 commit into from
Apr 1, 2022

Conversation

cdoern
Copy link
Contributor

@cdoern cdoern commented Mar 22, 2022 •
edited
Loading

the infra Inherit function was not properly passing pod volume information to new containers. alter the inherit function and struct to use the new ConfigToSpec function used in clone pick and choose the proper entities from a temp spec and validate them on the spegen side rather than passing directly to a config

resolves #13548

Signed-off-by: cdoern [email protected]

@cdoern cdoern force-pushed the podVolumes branch 3 times, most recently from 7106bcb to 48642ab Compare March 22, 2022 19:03
@cdoern
Copy link
Contributor Author

cdoern commented Mar 22, 2022

@containers/podman-maintainers PTAL. This is a pretty substantial bug fix as pod volumes seem to have been broken for some time.

@mheon
Copy link
Member

mheon commented Mar 22, 2022

Cause of errors seems to be https://github.com/containers/podman/blob/main/test/apiv2/40-pods.at#L88 which is pulling from docker.io

@edsantiago @cevich PTAL - is there anything in the APIv2 tests preventing pulls from docker.io? Because that test has been there for 2 years, seemingly, and I'm amazed it only now started blowing up with rate-limiting errors

@edsantiago
Copy link
Member

@mheon I think we have some magic doohickey that masquerades docker.io, using quay.io instead. I can't remember the details. However, instead of looking them up, I do note that the error I see is:

 >Error: initializing source docker://busybox:latest: Requesting bearer token: invalid status code from registry 500 (Internal Server Error)

...which is a 500, not the usual "rate limit blah blah". And, looking at the full log, I see another 500 error, this time very clearly quay.io. This looks like a quay flake to me.

(No point in pressing Re-run, though, because of so many other errors in this CI run)

Luap99
Luap99 reviewed Mar 23, 2022
View reviewed changes
pkg/specgen/specgen.go Outdated Show resolved Hide resolved
pkg/specgen/generate/container_create.go Outdated Show resolved Hide resolved
@cevich
Copy link
Member

cevich commented Mar 23, 2022

I think we have some magic doohickey that masquerades docker.io, using quay.io instead

Yes, see test/registries.conf

@cdoern cdoern mentioned this pull request Mar 24, 2022
Merged
@cdoern cdoern force-pushed the podVolumes branch 2 times, most recently from de569a9 to aa5cac8 Compare March 24, 2022 02:49
@cdoern
Copy link
Contributor Author

cdoern commented Mar 24, 2022

@containers/podman-maintainers PTAL

rhatdan
rhatdan approved these changes Mar 24, 2022
View reviewed changes
Copy link
Member

@rhatdan rhatdan left a comment

Choose a reason for hiding this comment

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

LGTM other then a Nit/

pkg/specgenutil/volumes.go Outdated Show resolved Hide resolved
pkg/specgen/generate/container.go Outdated Show resolved Hide resolved
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 24, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, rhatdan

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2022
Luap99
Luap99 reviewed Mar 24, 2022
View reviewed changes
pkg/specgen/generate/container.go Outdated Show resolved Hide resolved
pkg/specgen/generate/container.go Outdated Show resolved Hide resolved
pkg/specgen/generate/container.go Outdated Show resolved Hide resolved
pkg/specgen/generate/container.go Outdated Show resolved Hide resolved
@cdoern
Copy link
Contributor Author

cdoern commented Mar 24, 2022

alright, I will go back and redo the mounts tonight

@cdoern
Copy link
Contributor Author

cdoern commented Mar 25, 2022 •
edited
Loading

@Luap99 @mheon tested it a bunch and i think sortuservolumes does enough to prohibit any bad options or volumes from getting through

Luap99
Luap99 reviewed Mar 25, 2022
View reviewed changes
pkg/specgenutil/volumes.go Outdated Show resolved Hide resolved
pkg/specgen/generate/container_create.go Outdated Show resolved Hide resolved
@mheon
Copy link
Member

mheon commented Mar 25, 2022

Rebase should fix the tests

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2022
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 28, 2022
fix pod volume passing and alter infra inheritance
7a53428 
the infra Inherit function was not properly passing pod volume information to new containers
alter the inherit function and struct to use the new `ConfigToSpec` function used in clone
pick and choose the proper entities from a temp spec and validate them on the spegen side rather
than passing directly to a config

resolves containers#13548

Signed-off-by: cdoern <[email protected]>
Signed-off-by: cdoern <[email protected]>
Signed-off-by: cdoern <[email protected]>
@cdoern
Copy link
Contributor Author

cdoern commented Mar 29, 2022

@containers/podman-maintainers PTAL

@rhatdan
Copy link
Member

rhatdan commented Mar 30, 2022

LGTM

@mheon
Copy link
Member

mheon commented Apr 1, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2022
@openshift-merge-robot openshift-merge-robot merged commit 23e5757 into containers:main Apr 1, 2022
@sanyer sanyer mentioned this pull request Apr 4, 2022
Closed
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@mheon mheon mheon left review comments

@rhatdan rhatdan rhatdan approved these changes

Assignees

@mheon mheon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Podman 4.x Pod volume mounts
8 participants
@cdoern @mheon @edsantiago @cevich @rhatdan @TomSweeneyRedHat @Luap99 @openshift-merge-robot