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

Upgrade tests: reexamine cross-testing matrix #13697

Merged
merged 1 commit into from
Mar 30, 2022
Merged

Upgrade tests: reexamine cross-testing matrix #13697

merged 1 commit into from
Mar 30, 2022

Conversation

edsantiago
Copy link
Member

  • removed: v1.9.0, v2.0.6
  • added: v3.4.0

(Cannot add v4 because there's no such image on quay. As soon
as one appears, we should add it.)

Signed-off-by: Ed Santiago santiago@redhat.com

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 29, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago

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 29, 2022
edsantiago
edsantiago commented Mar 29, 2022
View reviewed changes
.cirrus.yml Outdated
- env:
PODMAN_UPGRADE_FROM: v2.1.1
- env:
PODMAN_UPGRADE_FROM: v3.1.2
- env:
PODMAN_UPGRADE_FROM: v3.4.0
Copy link
Member Author

Choose a reason for hiding this comment

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

quay also has v3.4.1, 2, and 4. Should we use one of those instead?

Copy link
Member

Choose a reason for hiding this comment

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

I would go with .4 to have more bug fixes in it.

@edsantiago edsantiago mentioned this pull request Mar 29, 2022
Merged
Luap99
Luap99 reviewed Mar 29, 2022
View reviewed changes
.cirrus.yml
@@ -721,14 +721,12 @@ upgrade_test_task:
depends_on:
- local_system_test
matrix:
- env:
PODMAN_UPGRADE_FROM: v1.9.0
Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep the oldest one, assuming we still want to support upgrades from such an old version.
Given that it still works I so no reason to remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We discussed this a week or two ago, and my recollection was that nobody would ever upgrade from v1 to v4. But I may be wrong. Does anyone remember that discussion?

Copy link
Member

Choose a reason for hiding this comment

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

I cannot remember such discussion (maybe I was not part of it).

I think someone could try updating from rhel7 to rhel8 in-place. I don't think we have to worry about this but as long as it works I see no strong reason to remove it.

Copy link
Member

Choose a reason for hiding this comment

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

I think we want to support stable version -> stable version - so 1.6.4 to 3.0 would be my expectation of the oldest upgrade we'd reasonably expect.

.cirrus.yml Outdated
- env:
PODMAN_UPGRADE_FROM: v2.1.1
- env:
PODMAN_UPGRADE_FROM: v3.1.2
- env:
PODMAN_UPGRADE_FROM: v3.4.0
Copy link
Member

Choose a reason for hiding this comment

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

I would go with .4 to have more bug fixes in it.

@edsantiago
Copy link
Member Author

`podman pod create no longer works in v3.4.x (containerized). Simple reproducer:

$ sudo bin/podman run --rm --privileged --net=host quay.io/podman/stable:v3.4.0 podman pod create --name foo
Error: invalid config provided: cannot set hostname when running in the host UTS namespace: invalid configuration

(works with v3.1.2).

Any suggestions on how to fix that?

@edsantiago
Upgrade tests: reexamine cross-testing matrix
2a882b7 
 - removed: v1.9.0, v2.0.6
 + added:   v3.4.0

(Cannot add v4 because there's no such image on quay. As soon
as one appears, we should add it.)

Add a workaround for a UTS namespace conflict new in v3.4

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago edsantiago force-pushed the upgrade_test_matrix branch from a3bb9c6 to 2a882b7 Compare March 29, 2022 21:25
@edsantiago
Copy link
Member Author

Issue resolved, thanks to Dan #11969 (comment)

@Luap99
Copy link
Member

Luap99 commented Mar 30, 2022

Issue resolved, thanks to Dan #11969 (comment)

This sounds like an ugly workaround. If our image does not support podman pod create out of the box it is broken!

@edsantiago
Copy link
Member Author

I was thinking the same thing, but ass-u-med there must be a good reason for the default.

@Luap99
Copy link
Member

Luap99 commented Mar 30, 2022

I cannot think of any reason why the uts namespace is set to host by default. I see reasons using host for cgroup and netns but not the other namespaces.

@edsantiago
Copy link
Member Author

As best I can tell, it dates to PR 5467 which added the containers.conf file. I can't tell if the presence of utsns is intentional or an oversight; and this is not something we can resolve in this PR. Would you like to file an issue (or PR)? Ordinarily I would do so but I don't understand the problem well enough to describe it properly.

@Luap99 Luap99 mentioned this pull request Mar 30, 2022
Closed
@Luap99
Copy link
Member

Luap99 commented Mar 30, 2022

Created #13714, lets move the discussion there.

@edsantiago
Copy link
Member Author

@containers/podman-maintainers this is as ready as I can make it.

@rhatdan
Copy link
Member

rhatdan commented Mar 30, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2022
@openshift-merge-robot openshift-merge-robot merged commit c08e8c3 into containers:main Mar 30, 2022
@edsantiago edsantiago deleted the upgrade_test_matrix branch March 30, 2022 21:06
@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

@mheon mheon mheon left review comments

@Luap99 Luap99 Luap99 left review comments

Assignees

@rhatdan rhatdan

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.
5 participants
@edsantiago @Luap99 @rhatdan @mheon @openshift-merge-robot