Skip to content

Conversation

l0rd
Copy link
Member

@l0rd l0rd commented Oct 6, 2025

This is to avoid CI/CD flakes and is based on this analysis of the WSL flakes logs:
microsoft/WSL#13301 (comment)

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 6, 2025
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM, you need to rebase once #27234 is merged to unblock CI.

Copy link
Contributor

openshift-ci bot commented Oct 6, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: l0rd, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

@Luap99
Copy link
Member

Luap99 commented Oct 6, 2025

Ok it still flaked, I added the info to microsoft/WSL#13301 (comment)

See the comment there with how the tests overwrite USERPROFILE so I wonder if this setting was actually read or must written as part of the test suite setup to take effect.

@l0rd l0rd force-pushed the wslconfig branch 2 times, most recently from d83e149 to 3734c33 Compare October 7, 2025 08:09
@l0rd
Copy link
Member Author

l0rd commented Oct 7, 2025

See the comment there with how the tests overwrite USERPROFILE so I wonder if this setting was actually read or must written as part of the test suite setup to take effect.

I suspect that the problem was that we created the wslconfig file if $env:CI -eq "true". But that variable doesn't exist, the variable to test is $env:CIRRUS_CI.

@Luap99
Copy link
Member

Luap99 commented Oct 7, 2025

I am not sure how that works with the USERPROFILE override in the tests as well though

However I just realized that our tests do some "mocking" as the test overwrites USERPROFILE and some other env vars with a temp dir to avoid cluttering the actual user home if it is run locally. So I wonder if this actually worked or we need to add the config to the new USERPROFILE for each test case.

if runtime.GOOS == "windows" {
if err := os.Setenv("USERPROFILE", homeDir); err != nil {
Fail("unable to set home dir on windows")
}
}

microsoft/WSL#13301 (comment)

Could be that we actually have to create the file in the BeforeEach() where we setup the temp home as well as I don't know how wsl reads the file.

Anyhow seems like test passed this time but this is no solid indicator though its general flakiness, we should keep pressing rerun a couple of times https://cirrus-ci.com/task/5074728311848960 to see if we can it to fail still

@Luap99
Copy link
Member

Luap99 commented Oct 7, 2025

Ok it still failed after the third rerun https://cirrus-ci.com/task/6196427237359616

@Luap99
Copy link
Member

Luap99 commented Oct 7, 2025

Mind adding the config write into the go test code where we update USERPROFILE to see if that changes things?

This is to avoid CI/CD flakes and is based on this analysis of
the WSL flakes logs:
microsoft/WSL#13301 (comment)

Signed-off-by: Mario Loriedo <[email protected]>
@l0rd
Copy link
Member Author

l0rd commented Oct 7, 2025

I have updated the BeforeEach(). Let's see if this work better. For the record yesterday I was able to run the e2e suite with a .wslconfig in my $USERPROFILE to specify a custom kernel. That was for testing nftables support and it actually worked (test were passing so the new kernel was used). But I am not sure if the properties wsl2.kernelBootTimeout (undocumented) and wsl2.kernel are evaluated at the same time.

@TomSweeneyRedHat
Copy link
Member

Tests are still barking, but the code LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. machine release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants