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

[ppc64le] Remove P8 and P9 hack so tests pass for Power10 #3887

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

mtarsel
Copy link
Contributor

@mtarsel mtarsel commented Sep 19, 2024

008e04c and a6d7bbb added and used the libguestfs-ppc64le-wrapper.sh in order to address some failing tests on ppc64le. Using a Power10 machine with KVM enabled, tests are passing with this script removed. There was a successful test run with: cosa kola run --tag '!reprovision'

If we are going to use p10 for CI going forward, I think we can remove this script entirely. Otherwise, we will have to distinguish between p10 and not p10 or possibly implement a better way than relying on using uname -r output searching for el7.

Related to issue #2473

Copy link

openshift-ci bot commented Sep 19, 2024

Hi @mtarsel. Thanks for your PR.

I'm waiting for a coreos member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@marmijo
Copy link
Member

marmijo commented Sep 19, 2024

/ok-to-test

jlebon
jlebon previously approved these changes Sep 19, 2024
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

This looks sane to me. Though I'm still confused why you were hitting issues with this. Those paths shouldn't have triggered for you at all. Probably worth digging into that a bit more to understand better what went wrong there.

But anyway, I do also think that these patches are no longer relevant now. They predate the pipeline rework from a time when we were using a separate ppc64le OCP cluster, which is what I think was what was using RHEL7. Our current builders all use FCOS, so this will never trigger.

cc @ravanelli in case I'm missing something

@ravanelli
Copy link
Member

This looks sane to me. Though I'm still confused why you were hitting issues with this. Those paths shouldn't have triggered for you at all. Probably worth digging into that a bit more to understand better what went wrong there.

But anyway, I do also think that these patches are no longer relevant now. They predate the pipeline rework from a time when we were using a separate ppc64le OCP cluster, which is what I think was what was using RHEL7. Our current builders all use FCOS, so this will never trigger.

cc @ravanelli in case I'm missing something

I think we are ok in removing it, the main reason in the past was indeed the need for vsmt=8 + some other small changes for RHEL7 in our old ocp cluster. However, I agreed this code should not be running in P10 with another RHEL version.

ravanelli
ravanelli previously approved these changes Sep 27, 2024
@jlebon
Copy link
Member

jlebon commented Sep 27, 2024

LGTM. Can you fix the lint issues in the file you touched?

@mtarsel
Copy link
Contributor Author

mtarsel commented Sep 27, 2024

LGTM. Can you fix the lint issues in the file you touched?

sure thing. if im reading it correctly, it looks like I need to just remove the unused "unix" import

@mtarsel mtarsel dismissed stale reviews from jlebon and ravanelli via 1bf9fad October 4, 2024 18:20
Copy link
Member

@ravanelli ravanelli left a comment

Choose a reason for hiding this comment

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

Please remove this merge branch. And squash the commits for the same file in only one commit, you can even revert the commit that added these changes.

@@ -51,8 +51,6 @@ import (
"github.com/coreos/coreos-assembler/mantle/system"
"github.com/coreos/coreos-assembler/mantle/system/exec"
"github.com/pkg/errors"

"golang.org/x/sys/unix"
Copy link
Member

Choose a reason for hiding this comment

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

It should be part of the first commit, squash the commits to have only one commit.

008e04c and a6d7bbb added and used the libguestfs-ppc64le-wrapper.sh in order to address some failing tests on ppc64le. Using a Power10 machine with KVM enabled, tests are passing with this script removed. There was a successful test run with: cosa kola run --tag '!reprovision'

Signed-off-by: Mick Tarsel <mtarsel@gmail.com>
@mtarsel
Copy link
Contributor Author

mtarsel commented Oct 8, 2024

Im not sure why this issue was closed. I did a Sync Fork from the github website and that added the merge commit to this PR however when I did a pull on my branch locally, the merge commit was not present. I did a reset and I guess that automatically closed this.

The branch should be clean now. Reopening and hoping github picks up the same branch

@mtarsel mtarsel reopened this Oct 8, 2024
Copy link
Member

@ravanelli ravanelli left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

LGTM and good riddance! 🎉

@ravanelli ravanelli merged commit 9692c44 into coreos:main Oct 22, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants