Skip to content

fix namespace reporting#14852

Merged
edsantiago merged 1 commit intocontainers:mainfrom
cdoern:podUTS
Jul 7, 2022
Merged

fix namespace reporting#14852
edsantiago merged 1 commit intocontainers:mainfrom
cdoern:podUTS

Conversation

@cdoern
Copy link
Copy Markdown
Contributor

@cdoern cdoern commented Jul 7, 2022 •
edited
Loading

somehow, #14501 got through CI even though the remote tests fail. The testa are failing due to the PodSpecGenerator not containing the UTSNs entitiy and infra's spec is not yet allowed to be accessed remotely

[NO NEW TESTS NEEDED]

resolves #14847

Signed-off-by: Charlie Doern cdoern@redhat.com

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Jul 7, 2022
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Jul 7, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cdoern
To complete the pull request process, please assign saschagrunert after the PR has been reviewed.
You can assign the PR to them by writing /assign @saschagrunert in a comment when ready.

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

Details 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 release-note-none and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jul 7, 2022
@cdoern
Copy link
Copy Markdown
Contributor Author

cdoern commented Jul 7, 2022

@Luap99 @vrothberg PTAL

vrothberg
vrothberg reviewed Jul 7, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@Luap99
Copy link
Copy Markdown
Member

Luap99 commented Jul 7, 2022

Please remove the if remote checks from the test

@cdoern
Copy link
Copy Markdown
Contributor Author

cdoern commented Jul 7, 2022

@Luap99 the remote checks do not impact this bug. that is just because os.Hostname will not match when executing remote tests.

@Luap99
Copy link
Copy Markdown
Member

Luap99 commented Jul 7, 2022

@Luap99 the remote checks do not impact this bug. that is just because os.Hostname will not match when executing remote tests.

Why, this does not make sense? Client and server are on the same system so os.Hostname must match.

@cdoern
Copy link
Copy Markdown
Contributor Author

cdoern commented Jul 7, 2022

@Luap99 ok, I will remove it but it will most likely fail

@cdoern
fix namespace reporting
0424084 
somehow, containers#14501 got through CI even though the remote tests fail. The testa are failing
due to the PodSpecGenerator not containing the UTSNs entitiy and infra's spec is not yet allowed to be accessed remotely

[NO NEW TESTS NEEDED]

resolves containers#14847

Signed-off-by: Charlie Doern <cdoern@redhat.com>
@Luap99
Copy link
Copy Markdown
Member

Luap99 commented Jul 7, 2022

@Luap99 ok, I will remove it but it will most likely fail

Which means that there is a bug and #14847 is not fixed by this.

@cdoern
Copy link
Copy Markdown
Contributor Author

cdoern commented Jul 7, 2022

@Luap99 #14847 was reporting that the NS is not passed to remote. I tested it locally and it works, If i recall correctly the reason this hostname test fails is due to some CI weirdness. maybe it will work now though who knows.

@Luap99
Copy link
Copy Markdown
Member

Luap99 commented Jul 7, 2022

How does ns:/proc/self/ns/ even work, I don't think is a valid namespace. You would need ns:/proc/self/ns/uts

@cdoern
Copy link
Copy Markdown
Contributor Author

cdoern commented Jul 7, 2022 •
edited
Loading

@Luap99 it is a dummy link used to make sure the pod gets the path, the pods are not started with these paths. all pod NS tests use this format.

@Luap99
Copy link
Copy Markdown
Member

Luap99 commented Jul 7, 2022

ack, the naming is confusing but looks like this is passing CI and we need to get CI unblocked

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2022
@edsantiago
Copy link
Copy Markdown
Member

edsantiago commented Jul 7, 2022

Does anyone understand how this ever passed CI in the original PR?

@cdoern
Copy link
Copy Markdown
Contributor Author

cdoern commented Jul 7, 2022

@edsantiago I was thinking the same thing.... yesterday I was noticing a lot of CI weirdness

@Luap99
Copy link
Copy Markdown
Member

Luap99 commented Jul 7, 2022

#14501 shows no tests at all so they were never executed I guess

@cdoern
Copy link
Copy Markdown
Contributor Author

cdoern commented Jul 7, 2022

When I cancelled the hold on that pr the tests were there and then it sat for a while, I went back and it just had a singular tide check.

@edsantiago
Copy link
Copy Markdown
Member

edsantiago commented Jul 7, 2022

OK, that makes sense: yes, Cirrus (or github? who knows) was down yesterday evening. It went into a mode where "blah blah waiting for reporting", and only two thingies were in the CI list: tide and something else. No actual tests. Meaning, yes, the PR was merged without any tests whatsoever.

@edsantiago
Copy link
Copy Markdown
Member

edsantiago commented Jul 7, 2022

Aw, phooey:

$ podman-remote --url unix:/tmp/podman_tmp_mnLg wait gfKskJHy37
Error: getting exit code of container ebff09951585fcda59dfeceabfaab084751a8303a7cd981420c32c1d0c38b656 from DB: no such exit code

This is f36 remote rootless.

I remember some recent work on exit codes and wait, but couldn't find the PR. Restarting.

@edsantiago
Copy link
Copy Markdown
Member

edsantiago commented Jul 7, 2022

/hold cancel

Anyone with in-flight PRs will need to rebase & repush

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2022
@edsantiago edsantiago merged commit d52ac44 into containers:main Jul 7, 2022
@edsantiago
Copy link
Copy Markdown
Member

edsantiago commented Jul 7, 2022

Got tired of waiting for bot. Merged manually.

@edsantiago edsantiago mentioned this pull request Jul 7, 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

1 more reviewer

@vrothberg vrothberg vrothberg left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Luap99 Luap99

Labels

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. release-note-none

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

pod create --uts doesn't work on podman-remote

4 participants

@cdoern @Luap99 @edsantiago @vrothberg