Fix for podman machine init not creating necessary JSON file when an ignition-path is passed #24321
Conversation
pkg/machine/shim/host_test.go
Outdated
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if err := Init(tt.args.opts, tt.args.mp); (err != nil) != tt.wantErr { |
There was a problem hiding this comment.
This is not an acceptable unit test, this is actually creating a real machine in my home dir. A unit test should be independent of any host specifics.
please add such a test to pkg/machine/e2e which runs the actual podman cli on every OS.
There was a problem hiding this comment.
Thanks for the feedback! That makes a lot of sense. It looks to me like I can just add a test to pkg/machine/e2e/init_test.go.
However, I'm having trouble running the tests on my machine. Is that expected since seccomp is not available on Mac OS and it is trying to run every OS?
Here is what I am seeing when I try to run machine init with volume in pkg/machine/e2e/init_test.go.
make remoteintegration FOCUS="machine init with volume"
Makefile:152: invalid `override' directive
CGO_ENABLED=0 GOOS=darwin GOARCH=arm64 go build -ldflags '-X github.com/containers/podman/v5/libpod/define.gitCommit=24c178f39b0bca2c558a803309a93c8bcaef5109-dirty -X github.com/containers/podman/v5/libpod/define.buildInfo=1729524947 -X github.com/containers/podman/v5/libpod/config._installPrefix=/usr/local -X github.com/containers/podman/v5/libpod/config._etcDir=/etc -X github.com/containers/podman/v5/pkg/systemd/quadlet._binDir=/usr/local/bin -X github.com/containers/common/pkg/config.additionalHelperBinariesDir= ' -tags " exclude_graphdriver_btrfs btrfs_noversion exclude_graphdriver_devicemapper seccomp " -o test/checkseccomp/checkseccomp ./test/checkseccomp
# github.com/containers/podman/v5/test/checkseccomp
test/checkseccomp/checkseccomp.go:13:17: undefined: unix.Prctl
test/checkseccomp/checkseccomp.go:13:28: undefined: unix.PR_GET_SECCOMP
test/checkseccomp/checkseccomp.go:15:18: undefined: unix.Prctl
test/checkseccomp/checkseccomp.go:15:29: undefined: unix.PR_SET_SECCOMP
test/checkseccomp/checkseccomp.go:15:50: undefined: unix.SECCOMP_MODE_FILTER
make: *** [test/checkseccomp/checkseccomp] Error 1
In the meantime, I have modified my test like so:
It("machine init with ignition path", func() {
skipIfWSL("Ignition is not compatible with WSL machines since they are not based on Fedora CoreOS")
tmpDir, err := os.MkdirTemp("", "")
Expect(err).ToNot(HaveOccurred())
tmpFile, err := os.CreateTemp(tmpDir, "test-ignition-*.ign")
Expect(err).ToNot(HaveOccurred())
// Test that all files are written when creating a machine. See https://github.com/containers/podman/issues/23544
mockIgnitionContent := `{"ignition":{"version":"3.4.0"},"passwd":{"users":[{"name":"core"}]}}`
_, err = tmpFile.WriteString(mockIgnitionContent)
Expect(err).ToNot(HaveOccurred())
err = tmpFile.Close()
Expect(err).ToNot(HaveOccurred())
defer func() { _ = utils.GuardedRemoveAll(tmpDir) }()
name := randomString()
i := new(initMachine)
session, err := mb.setName(name).setCmd(i.withImage(mb.imagePath).withIgnitionPath(tmpFile.Name()).withNow()).run()
Expect(err).ToNot(HaveOccurred())
Expect(session).To(Exit(0))
configDir := filepath.Join(testDir, ".config", "containers", "podman", "machine", testProvider.VMType().String())
fileExtensions := []string{".lock", ".json", ".ign"}
for _, ext := range fileExtensions {
filename := filepath.Join(configDir, fmt.Sprintf("%s%s", name, ext))
_, err := os.Stat(filename)
Expect(err).ToNot(HaveOccurred())
}
})Does this look more appropriate? If so, I can edit my PR to include this despite not being able to run all the tests.
There was a problem hiding this comment.
make localintegration/remoteintegration are for test/e2e which only work on linux.
you need make localmachine for pkg/machine/e2e which should work on macos and linux.
Test wise it looks about right, you should move the defer func() { _ = utils.GuardedRemoveAll(tmpDir) }() direcyl after you created the dir otherwise it is leaked if any of the matches before that fail.
Also I think there is a bit of a testing gap in that sense if podman were to ignore the ignition file argument the test would still pass. To make this solid maybe add some custom random username that would not exists otherwise or create a random filem something like that. Then have the test do a podman machine ssh id RandomUserName to ensure yes the correct ignition was indeed used
There was a problem hiding this comment.
you need
make localmachinefor pkg/machine/e2e which should work on macos and linux.
Ah, this definitely works better. However, it looks like I still have some debugging since I'm getting two errors that look like they might be related to each other:
failed to set TMPDIR: "/var/folders/7m/z_f3vkmd6t30fhydp4z513gw0000gn/T/ path length should be less than 22 characters"
Running Suite: Podman Machine tests - /Users/gaufde/GolandProjects/podman/pkg/machine/e2e
=========================================================================================
Random Seed: 1729527567
Will run 1 of 62 specs
------------------------------
[BeforeSuite]
/Users/gaufde/GolandProjects/podman/pkg/machine/e2e/machine_test.go:56
> Enter [BeforeSuite] TOP-LEVEL - /Users/gaufde/GolandProjects/podman/pkg/machine/e2e/machine_test.go:56 @ 10/21/24 09:19:30.188
[FAILED] failed to pull disk: "invalid machine file path"
In [BeforeSuite] at: /Users/gaufde/GolandProjects/podman/pkg/machine/e2e/machine_test.go:73 @ 10/21/24 09:19:30.189
I'll have to debug this later when I get a bit more time.
To make this solid maybe add some custom random username that would not exists otherwise or create a random filem something like that. Then have the test do a podman machine ssh id RandomUserName to ensure yes the correct ignition was indeed used
Won't ssh only work if the custom ignition file has the ready.service unit and the ssh key for Podman machine? I agree that this would be a better test, but I'm not exactly sure how I would go about adding those into the mockIgnitionContent. At first glance, I'm guessing I would have to use CreateReadyUnitFile from pkg/machine/ignition/ready.go and addConnection from pkg/machine/connection/connection.go.
@baude originally mentioned in #23544 (comment):
...we should be writing a JSON and we should write a regression test as such with your PR (just init and check the json is there, we dont have to start it).
Anyways, I'm willing to give it a shot, but a bit more guidance might be helpful for me.
There was a problem hiding this comment.
ah right I forget about the arbitrary low socket path length limits in the macos kernel this is trying trying to catch.
use TMPDIR=/tmp make localmachine, that is how I used them before IIRC
if booting is to complicated then sure, I am not to familiar with the ignition requirements.
There was a problem hiding this comment.
That was it! Though I had to use TMPDIR=/private/tmp make localmachine to match the directory mounted on podman machine.
There was a problem hiding this comment.
@Luap99 I have implemented the new test. Rather than trying to build a valid ignition file that would allow the VM to start, I decided to simply check that the ignition file passed to Podman is the same as the one that Podman writes for the machine. It doesn't test that the ignition works, but it should check that Podman passes the ignition through properly. Let me know what you think!
There was a problem hiding this comment.
Please sqash your commits into one, also you need to fix the lint errors
pkg/machine/e2e/init_test.go:253:3: commentFormatting: put a space between `//` and comment text (gocritic)
//test that all required machine files are created
^
pkg/machine/e2e/init_test.go:262:3: commentFormatting: put a space between `//` and comment text (gocritic)
//enforce that the raw ignition is copied over verbatim
^
pkg/machine/shim/host.go:203:10: unnecessary leading newline (whitespace)
^
pkg/machine/e2e/config_init_test.go
Outdated
| // The exception below is required for testing custom ignition files. | ||
| if strings.Contains(session.errorToString(), "failed to remove machines files: unable to find connection named") { | ||
| return | ||
| } |
There was a problem hiding this comment.
Can you file a new bug for this add a then add a FIXME: #xxx work around for custom ignition removal comment here? and replace xxx with the real github issue number
|
Note your commit message title is to long. Generally speaking commit message lines should not exceed 72 chars and after the title should need to keep an empty line although that is not strictly enforced, but CI does enforce that the title is less than 90 chars. Also we you should add Another thing the DCO check is failing because your git Author fields do not match the ones in the sign of line, while I do consider your sign of valid it is best to match them, i.e. you can update the author on the commit with |
gaufde
force-pushed
the
main
branch
3 times, most recently
from
0871fd8 to
f0b47e8
Compare
@Luap99 If I understood everything correctly, I believe I have fixed all these issues with my latest commit. Let me know if anything else is needed! Also, thank you for your patience while I learn how to do all of this! |
|
Your change breaks the machine tests, for some reason the volumes are no longer mounted. The |
gaufde
force-pushed
the
main
branch
4 times, most recently
from
f85a6b9 to
bef24d0
Compare
|
I made the change as suggested, and it is definitely better than before. However, locally I'm running into a rosetta error now when running Interestingly, if I revert my changes in |
There was a problem hiding this comment.
LGTM, for the future the "Fixes: #..." part in the commit message should be at the end of the body not the title. The title should be a short summary of the change, i.e. "fix podman machine init --ignition-path". Anyhow I am not going to block over it and make you force push again.
Restarted the last test failures as they are flakes and not related to your change.
openshift-ci
bot
added
the
approved
pkg/machine/shim/host.go
Outdated
| if err != nil { | ||
| return err | ||
| if len(opts.IgnitionPath) == 0 { | ||
| err = ignBuilder.Build() |
There was a problem hiding this comment.
your choice but I think that the if err := ignBuilder.Build(); err != nil { syntax is cleaner here? nit
There was a problem hiding this comment.
Whose choice are you referring to? I'm happy to change it, I'm just unsure if this is an internal discussion between you and @Luap99, or if I should change it.
| @@ -196,30 +196,33 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) error { | |||
| // copy it into the conf dir | |||
| if len(opts.IgnitionPath) > 0 { | |||
There was a problem hiding this comment.
should we just create a new function to deal with this given the more complexity here? wdyt @Luap99
There was a problem hiding this comment.
The function is pretty long already so splitting it up makes sense but I would not force a community contributor to do that work. It only adds one more if/else here so I don't think it is required to rework just for that.
HyperV still seems to be broken |
|
Hmm, it looks a bit different than the one I couldn't get past on my machine.
Oh wait...that line is in my new test. I'll have to test it again, but I'm pretty sure the e2e tests for the test I added worked for me. |
|
@gaufde yes I restarted it a few times so it seems to fail consistently (which makes sense given it is a nil deref) This is the hyperV test so it is only failing on windows with the hyperV hypervisor, macos and linux both pass. That is the difficult thing with machine support. I cannot really help with windows. So if you do not use windows either I can try to ask someone else to take a look. |
That would be very helpful! I don't use windows either. |
|
@l0rd Can you look at the hyperV issue here? |
|
@Luap99 Yep, I will look at it later today |
|
I confirm that the new test |
|
mc.HyperVHypervisor.NetworkVSock = *networkHVSock |
pkg/machine/shim/host.go
Outdated
| err = ignBuilder.BuildWithIgnitionFile(opts.IgnitionPath) | ||
| return err | ||
| } | ||
|
|
||
| err = ignBuilder.GenerateIgnitionConfig() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| err = ignBuilder.GenerateIgnitionConfig() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| readyIgnOpts, err := mp.PrepareIgnition(mc, &ignBuilder) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| readyIgnOpts, err := mp.PrepareIgnition(mc, &ignBuilder) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| readyUnitFile, err := ignition.CreateReadyUnitFile(mp.VMType(), readyIgnOpts) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| readyUnitFile, err := ignition.CreateReadyUnitFile(mp.VMType(), readyIgnOpts) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| readyUnit := ignition.Unit{ | ||
| Enabled: ignition.BoolToPtr(true), | ||
| Name: "ready.service", | ||
| Contents: ignition.StrToPtr(readyUnitFile), | ||
| readyUnit := ignition.Unit{ | ||
| Enabled: ignition.BoolToPtr(true), | ||
| Name: "ready.service", | ||
| Contents: ignition.StrToPtr(readyUnitFile), | ||
| } | ||
| ignBuilder.WithUnit(readyUnit) | ||
| } | ||
| ignBuilder.WithUnit(readyUnit) | ||
|
|
There was a problem hiding this comment.
The Hyper-V test is passing with this change
| err = ignBuilder.BuildWithIgnitionFile(opts.IgnitionPath) | |
| return err | |
| } | |
| err = ignBuilder.GenerateIgnitionConfig() | |
| if err != nil { | |
| return err | |
| } | |
| if err != nil { | |
| return err | |
| } | |
| } else { | |
| err = ignBuilder.GenerateIgnitionConfig() | |
| if err != nil { | |
| return err | |
| } | |
| readyIgnOpts, err := mp.PrepareIgnition(mc, &ignBuilder) | |
| if err != nil { | |
| return err | |
| } | |
| readyIgnOpts, err := mp.PrepareIgnition(mc, &ignBuilder) | |
| if err != nil { | |
| return err | |
| } | |
| readyUnitFile, err := ignition.CreateReadyUnitFile(mp.VMType(), readyIgnOpts) | |
| if err != nil { | |
| return err | |
| } | |
| readyUnitFile, err := ignition.CreateReadyUnitFile(mp.VMType(), readyIgnOpts) | |
| if err != nil { | |
| return err | |
| } | |
| readyUnit := ignition.Unit{ | |
| Enabled: ignition.BoolToPtr(true), | |
| Name: "ready.service", | |
| Contents: ignition.StrToPtr(readyUnitFile), | |
| readyUnit := ignition.Unit{ | |
| Enabled: ignition.BoolToPtr(true), | |
| Name: "ready.service", | |
| Contents: ignition.StrToPtr(readyUnitFile), | |
| } | |
| ignBuilder.WithUnit(readyUnit) | |
| } | |
| ignBuilder.WithUnit(readyUnit) | |
| err = ignBuilder.BuildWithIgnitionFile(opts.IgnitionPath) | |
| if err != nil { | |
| return err | |
| } | |
| } else { | |
| err = ignBuilder.GenerateIgnitionConfig() | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| readyIgnOpts, err := mp.PrepareIgnition(mc, &ignBuilder) | |
| if err != nil { | |
| return err | |
| } | |
| readyUnitFile, err := ignition.CreateReadyUnitFile(mp.VMType(), readyIgnOpts) | |
| if err != nil { | |
| return err | |
| } | |
| readyUnit := ignition.Unit{ | |
| Enabled: ignition.BoolToPtr(true), | |
| Name: "ready.service", | |
| Contents: ignition.StrToPtr(readyUnitFile), | |
| } | |
| ignBuilder.WithUnit(readyUnit) | |
|
/cherry-pick v5.3 |
|
@mheon: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
we are going to cut a 5.3 release next week. if you would like this PR included, please use what was provided to fix the test failure by EOD Nov 11 |
|
look legit to me unfortunately. |
|
@baude I think I somehow forgot to add my changes before committing them a few days ago. I should have that fixed now. I'm now hoping I did everything correctly now. I was able to fix my bad rebase and do it again. |
gaufde
force-pushed
the
main
branch
2 times, most recently
from
6357f37 to
76442bd
Compare
Fix the issue where podman machine init does not create all the necessary machine files when ignition-path is used. Fixes: containers#23544 Signed-off-by: Graceson Aufderheide <[email protected]>
|
congrats, it passed |
|
LGTM |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gaufde, Luap99 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 |
openshift-merge-bot
bot
merged commit 5dbb567
into
containers:main
|
@mheon: new pull request created: #24560 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This bug was first described in #23544, and @cheesesashimi did most of the detective work to find the error!
Signed-off-by: Graceson Aufderheide [email protected]
Fixes #23544
Does this PR introduce a user-facing change?