Cirrus: add podman_machine_aarch64

[lsm5]

Depends on #14801

Does this PR introduce a user-facing change?

None

[edsantiago]

Task map is depressing:

I'm not seeing any way that the separate arch64 tasks can be combined into the existing ones, so I suppose we have to live with it.

I'm seeing this as an attempt to get CI working, and am assuming that everyone's goal is to merge #14972 first, then #14719, then rebase and review this one. If that is not a correct assumption, please respond here.

[lsm5]

I'm seeing this as an attempt to get CI working, and am assuming that everyone's goal is to merge #14972 first, then #14719, then rebase and review this one. If that is not a correct assumption, please respond here.

@edsantiago Exactly as you said. I only included those cause @cevich said it contains some fixes.

[lsm5]

@cevich remind me please, do i need some label to get the machine tests to run ?

[lsm5]

can't quite figure out the exact failure in x86_64 machine test

[lsm5]

@cevich @edsantiago would test_podman_machine also trigger aarch64 tests or does it need a new label? I don't see anything in the repo nor in the label info

[edsantiago]

As best I can tell, the new stanza is identical to the old one except for the AARCH64 parts (and whitespace differences). The tag should apply, I don't see anything in cirrus docs indicating that "it only works for one task" or anything like that. So, no idea why the new one didn't trigger, and no idea why the first one failed. Sorry.

[edsantiago]

Go expert needed. Looks like go lint is smart enough to realize that Skip() means "code below doesn't get run", but too stupid to realize that Skip() is temporary and it should just shut up. So:

pkg/machine/e2e/config_basic_test.go:15:24: func `(*basicMachine).withPodmanCommand` is unused (unused)
func (s *basicMachine) withPodmanCommand(args []string) *basicMachine {
                       ^
pkg/machine/e2e/config_basic_test.go:7:23: func `basicMachine.buildCmd` is unused (unused)
func (s basicMachine) buildCmd(m *machineTestBuilder) []string {
                      ^
pkg/machine/e2e/config_basic_test.go:3:6: type `basicMachine` is unused (unused)
type basicMachine struct {
     ^

Go expert, please give the solution.

[edsantiago]

Oh, never mind. @lsm5 try this:

diff --git a/pkg/machine/e2e/basic_test.go b/pkg/machine/e2e/basic_test.go
index 2228259f9..923e7cde4 100644
--- a/pkg/machine/e2e/basic_test.go
+++ b/pkg/machine/e2e/basic_test.go
@@ -1,6 +1,8 @@
 package e2e_test
 
 import (
+       "os"
+
        . "github.com/onsi/ginkgo"
        . "github.com/onsi/gomega"
        . "github.com/onsi/gomega/gexec"
@@ -20,7 +22,9 @@ var _ = Describe("run basic podman commands", func() {
        })
 
        It("Basic ops", func() {
-               Skip("FIXME: #15347 - Basic ops fails on PR runs and on x86_64")
+               if os.Getenv("CIRRUS_CI") != "" {
+                       Skip("FIXME: #15347 - Basic ops fails on PR runs and on x86_64")
+               }
 
                name := randomString()
                i := new(initMachine)

[mtrmac]

Very minimally tested: Just above the Skip, do something like

		bmOnlyToMarkUsed := basicMachine{}
		_ = bmOnlyToMarkUsed.withPodmanCommand
		Skip("…")

and possibly similarly for other unused methods.

[mtrmac]

FWIW the os.Getenv approach is a better solution, because it continues to make the rest of the test fully visible to the validator, and bugs introduced in that code would be more likely to be caught.

I’d only recommend a minor improvement: something like if os.Getenv("this-variable-is-never-expected-to-be-set") != "and-especially-not-to-this-value", to make the test behave consistently in CI and local runs.

[mtrmac]

… and to add a comment to the Getenv call explaining that the purpose is to make the Skip invisible to dead-code linters.

[lsm5]

PTAL at changes.

[cdoern]

Sorry that I missed this today. If needed I can assist @baude with the known_hosts patch. Everything @mtrmac is right, we opted not to write to a file if the key did not exist.

Adding a way to enable/disable this (with the default option to be disable) seems like the best fix. I can fire away a c/common PR if no one has tried that route yet.

I'm back from PTO this Sunday, so I will look for other issues with the machine tests that might have slipped through before then to ensure nothing else breaks.

[cevich]

I’d only recommend a minor improvement: something like if os.Getenv("this-variable-is-never-expected-to-be-set") != "and-especially-not-to-this-value", to make the test behave consistently in CI and local runs.

Careful, for anybody using hack/get_ci_vm.sh the $CIRRUS_CI variable won't be set, but IIRC $GET_CI_VM is set non-zero.

[edsantiago]

That's a feature (albeit not one I had intended): anyone using get_ci_vm is going to be doing so expressly for the purpose of fixing this bug, so they don't want the test skipped.

The important thing right now is to get this PR merged, and the way to do that is by skipping the test in Cirrus.

[cevich]

is going to be doing so expressly for the purpose of fixing this bug

Well, we can hope at least. Otherwise this will be a constant nag for anybody trying to play with anything podman-machine related in get_ci_vm. Many times, temporary==permanent, which is why I'm nervous.

Either way, you're right, we do need to get this PR merged ASAP.

[lsm5]

@baude @edsantiago @cevich good to go. PTAL.

[edsantiago]

@lsm5 I'm sitting here watching the spinning CI and waiting for it to finish...

[lsm5]

@lsm5 I'm sitting here watching the spinning CI and waiting for it to finish...

yup 😆 I meant apart from the artifacts test

[edsantiago]

/lgtm

\o/

[cevich]

Good job everybody, esp. @lsm5 leading this. Pretty soon I'll be completely out of a job 🤣

[lsm5]

Good job everybody, esp. @lsm5 leading this. Pretty soon I'll be completely out of a job rofl

with my awesome yaml skills, you have at least a 100 years worth of job security 😆