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.

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

TestEnvVarValue fails on several architectures #14819

Closed
siretart opened this issue Jul 4, 2022 · 14 comments · Fixed by #14823
Closed

TestEnvVarValue fails on several architectures #14819

siretart opened this issue Jul 4, 2022 · 14 comments · Fixed by #14823
Labels
kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@siretart
Copy link
Contributor

siretart commented Jul 4, 2022

/kind bug
Description

I've updated the Debian package to version 4.1, and while the package builds and seeimgly works fine on amd64 and arm64, any other architectures fail while running the testsuite. I've seen two kind of failures:

On:

  • i386 (32bit intel)
  • armel
  • armhf
  • mipsel

The test TestEnvVarValue fails with this output:

=== RUN   TestEnvVarValue/ResourceFieldRefNoLimitMemory
-246611968 8343322624
    play_test.go:787: 
        	Error Trace:	play_test.go:787
        	Error:      	Not equal: 
        	            	expected: (*string)(0xa3f73ec)
        	            	actual  : (*string)(0xa3491c8)
        	            	
        	            	Diff:
        	Test:       	TestEnvVarValue/ResourceFieldRefNoLimitMemory
=== RUN   TestEnvVarValue/ResourceFieldRefNoRequestMemory
-246611968 8343322624
    play_test.go:787: 
        	Error Trace:	play_test.go:787
        	Error:      	Not equal: 
        	            	expected: (*string)(0xa3f752c)
        	            	actual  : (*string)(0xa349260)
        	            	
        	            	Diff:
        	Test:       	TestEnvVarValue/ResourceFieldRefNoRequestMemory
=== RUN   TestEnvVarValue/ResourceFieldRefNoLimitCPU
4 4
=== RUN   TestEnvVarValue/ResourceFieldRefNoRequestCPU
4 4
--- FAIL: TestEnvVarValue (0.01s)

I note that these are all 32bit architectures, and the test fails while comparing a pointer.

There is a different failure signature on:

  • s390x
  • mips64el
  • ppc64el
=== RUN   TestMonitorTwoDirGood/bad-primary-new-addition
time="2022-07-03T23:35:24Z" level=error msg="Failed loading hooks for /tmp/hooks-test-primary-1983903746/0a.json: parsing hook \"/tmp/hooks-test-primary-1983903746/0a.json\": unrecognized hook version: \"-1\""
time="2022-07-03T23:35:24Z" level=error msg="Failed loading hooks for /tmp/hooks-test-primary-1983903746/0a.json: parsing hook \"/tmp/hooks-test-primary-1983903746/0a.json\": unrecognized hook version: \"-1\""
expected:  <nil>
actual:  &{[{/bin/sh [] [] 0xc00001a458}] [] [] [] [] []}

(at least that's what I think fails).

I note that all these architectures are 64bit.

@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Jul 4, 2022
@siretart
Copy link
Contributor Author

siretart commented Jul 4, 2022

All build logs can be seen from https://buildd.debian.org/status/package.php?p=libpod&suite=experimental which currently looks like this (click on "Build-Attempted" in the status column):

image

@siretart
Copy link
Contributor Author

siretart commented Jul 4, 2022

Also, this has been reported as https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1014309

@Luap99
Copy link
Member

Luap99 commented Jul 4, 2022

I don't think the TestEnvVarValue test is correct. It should not compare pointers there, I don't understand how this even works on x86_64.

@siretart
Copy link
Contributor Author

siretart commented Jul 4, 2022

I tend to concur:

for _, test := range tests {
test := test
t.Run(test.name, func(t *testing.T) {
result, err := envVarValue(test.envVar, &test.options)
assert.Equal(t, err == nil, test.succeed)
if test.expected == nilString {
assert.Nil(t, result)
} else {
fmt.Println(*result, test.expected)
assert.Equal(t, &(test.expected), result)
}
})
}

indicates that result in line 787 is actually a pointer that needs to be dereferenced instead of comparing to the pointer of the reference value. How can this possibly match?

@siretart siretart changed the title TestEnvVarValue fails on several architecutres TestEnvVarValue fails on several architectures Jul 4, 2022
@Luap99
Copy link
Member

Luap99 commented Jul 4, 2022

From https://pkg.go.dev/github.com/stretchr/testify/assert#Equal

Pointer variable equality is determined based on the equality of the referenced values (as opposed to the memory addresses). Function equality cannot be determined and will always fail.

So the test already compares by value even if pass pointers, therefore the test works.
From your test output we can see both values printed -246611968 8343322624

The test case converts directly the int64 memtotal value from ReadMemInfo() info to an string. The actual code seems to cast it to and int: https://github.com/containers/podman/blob/main/pkg/specgen/generate/kube/kube.go#L813
That would explain why it fails on 32 bit systems only.

@siretart Can you test it again with this diff applied?

$ git diff
diff --git a/pkg/specgen/generate/kube/kube.go b/pkg/specgen/generate/kube/kube.go
index 689c740f0..39e15f950 100644
--- a/pkg/specgen/generate/kube/kube.go
+++ b/pkg/specgen/generate/kube/kube.go
@@ -810,8 +810,8 @@ func envVarValueResourceFieldRef(env v1.EnvVar, opts *CtrSpecGenOptions) (*strin
        }
 
        // k8s rounds up the result to the nearest integer
-       intValue := int(math.Ceil(value.AsApproximateFloat64() / divisor.AsApproximateFloat64()))
-       stringValue := strconv.Itoa(intValue)
+       intValue := int64(math.Ceil(value.AsApproximateFloat64() / divisor.AsApproximateFloat64()))
+       stringValue := strconv.FormatInt(intValue, 10)
 
        return &stringValue, nil
 }

@siretart
Copy link
Contributor Author

siretart commented Jul 4, 2022

I can (probably) upload a new package with that patch applied later today.

I guess the failure in TestMonitorTwoDirGood is unrelated? -- would you prefer a separate issue for that?

@Luap99
Copy link
Member

Luap99 commented Jul 4, 2022

Yes a new issue for the other failure would be good, I haven't looked at that one.

@siretart
Copy link
Contributor Author

siretart commented Jul 4, 2022

@Luap99 yes, I can confirm that your patch makes the test pass on i386, but now it is failing on the TestMonitorTwoDirGood issue:

=== RUN   TestMonitorTwoDirGood                                                                                                                                                               
=== RUN   TestMonitorTwoDirGood/good-fallback-addition                                                                                                                                        
=== RUN   TestMonitorTwoDirGood/good-primary-override                                                                                                                                         
=== RUN   TestMonitorTwoDirGood/good-fallback-removal                                                                                                                                         
=== RUN   TestMonitorTwoDirGood/good-fallback-restore                                                                                                                                         
=== RUN   TestMonitorTwoDirGood/bad-primary-new-addition                                                                                                                                      
time="2022-07-04T15:04:04Z" level=error msg="Failed loading hooks for /tmp/hooks-test-primary-2603618593/0a.json: parsing hook \"/tmp/hooks-test-primary-2603618593/0a.json\": unrecognized ho
ok version: \"-1\""                                                                                                                                                                           
time="2022-07-04T15:04:04Z" level=error msg="Failed loading hooks for /tmp/hooks-test-primary-2603618593/0a.json: parsing hook \"/tmp/hooks-test-primary-2603618593/0a.json\": unrecognized ho
ok version: \"-1\""                                                                                                                                                                           
expected:  <nil>                                                                                                                                                                              
actual:  &{[{/bin/sh [] [] 0x963a2b8}] [] [] [] [] []}                                                                                                                                        

@Luap99
Copy link
Member

Luap99 commented Jul 4, 2022

@Luap99 yes, I can confirm that your patch makes the test pass on i386, but now it is failing on the TestMonitorTwoDirGood issue:

=== RUN   TestMonitorTwoDirGood                                                                                                                                                               
=== RUN   TestMonitorTwoDirGood/good-fallback-addition                                                                                                                                        
=== RUN   TestMonitorTwoDirGood/good-primary-override                                                                                                                                         
=== RUN   TestMonitorTwoDirGood/good-fallback-removal                                                                                                                                         
=== RUN   TestMonitorTwoDirGood/good-fallback-restore                                                                                                                                         
=== RUN   TestMonitorTwoDirGood/bad-primary-new-addition                                                                                                                                      
time="2022-07-04T15:04:04Z" level=error msg="Failed loading hooks for /tmp/hooks-test-primary-2603618593/0a.json: parsing hook \"/tmp/hooks-test-primary-2603618593/0a.json\": unrecognized ho
ok version: \"-1\""                                                                                                                                                                           
time="2022-07-04T15:04:04Z" level=error msg="Failed loading hooks for /tmp/hooks-test-primary-2603618593/0a.json: parsing hook \"/tmp/hooks-test-primary-2603618593/0a.json\": unrecognized ho
ok version: \"-1\""                                                                                                                                                                           
expected:  <nil>                                                                                                                                                                              
actual:  &{[{/bin/sh [] [] 0x963a2b8}] [] [] [] [] []}                                                                                                                                        

Isn't this the same failure you mentioned above already?

@Luap99
Copy link
Member

Luap99 commented Jul 4, 2022

I took a look at the full log and I do not see any errors with the TestMonitorTwoDirGood tests. All are passing. The output are just logs that are expected.

If you scroll further you find this:

FAIL	github.com/containers/podman/pkg/machine [build failed]
FAIL	github.com/containers/podman/pkg/machine/qemu [build failed]

@siretart
Copy link
Contributor Author

siretart commented Jul 4, 2022

Indeed, it is.

Thanks, the log is quite long, and I was mistakenly looking for FAIL:, whereas I really should have looked for build failed. It is, however, quite surprising that the test is producing output about mismatching values, without the test is actually failing.

Further down in the log, I see this:

# github.com/containers/podman/pkg/machine [github.com/containers/podman/pkg/machine.test]
src/github.com/containers/podman/pkg/machine/config_test.go:25:8: undefined: RemoteConnectionType
# github.com/containers/podman/pkg/machine/qemu [github.com/containers/podman/pkg/machine/qemu.test]
src/github.com/containers/podman/pkg/machine/qemu/config_test.go:41:18: undefined: machine.VMFile
src/github.com/containers/podman/pkg/machine/qemu/config_test.go:83:15: undefined: machine.VMFile
src/github.com/containers/podman/pkg/machine/qemu/config_test.go:94:20: undefined: machine.VMFile
src/github.com/containers/podman/pkg/machine/qemu/config_test.go:106:22: undefined: machine.VMFile
src/github.com/containers/podman/pkg/machine/qemu/config_test.go:124:22: undefined: machine.VMFile
src/github.com/containers/podman/pkg/machine/qemu/config_test.go:131:24: undefined: machine.NewMachineFile
src/github.com/containers/podman/pkg/machine/qemu/machine_test.go:10:12: undefined: MachineVM

It seems that this test was written to only compile on amd64 and arm64, maybe qemu support wasn't written for other platforms? -- given that pkg/machine code is not used in the Debian package, I'm inclined to just skip that test.

Thanks for helping me navigate reading the log!

@Luap99
Copy link
Member

Luap99 commented Jul 4, 2022

You can just skip the pkg/machine test for now. AFAIK the code should be used but only for amd64 and arm64 builds.
You could just check if the podman machine command is available.

I will open a PR to fix the missing build tags on the tests.

Luap99 added a commit to Luap99/libpod that referenced this issue Jul 4, 2022
int can be 32 or 64 bit depending on the architecture.
The total memory is int64 so we have to use int64 for the value as
well otherwise we get an overflow on 32 bit systems.

Fixes containers#14819

Signed-off-by: Paul Holzinger <[email protected]>
@Luap99
Copy link
Member

Luap99 commented Jul 4, 2022

@siretart I think #14823 should fix all your issues, you can use this patch: https://patch-diff.githubusercontent.com/raw/containers/podman/pull/14823.patch

@siretart
Copy link
Contributor Author

siretart commented Jul 4, 2022

Awesome, I've uploaded your patch as version 4.1.1+ds1-2, let's see how it goes at https://buildd.debian.org/status/package.php?p=libpod&suite=experimental this time. It seems that at least i386, armhf and ppc64el all builds have already succeeded!

Thank you!

@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 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants