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

kola/ext: bump minimum of memory to 8G on 64k page arches, ppc64le for now #1768

Merged
merged 1 commit into from
Oct 6, 2020

Conversation

jcajka
Copy link
Collaborator

@jcajka jcajka commented Oct 6, 2020

Re-provisioning tests fail on ppc64le due to lack of the memory, bumping it
beyond 6G makes them succeed. Most probably caused by slightly higher overhead
of the 64k pages in combination with slightly bigger size of COS on ppc64le.

…r now

Re-provisioning tests fail on ppc64le due to lack of the memory, bumping it
beyond 6G makes them succeed. Most probably caused by slightly higher overhead
of the 64k pages in combination with slightly bigger size of COS on ppc64le.
@miabbott
Copy link
Member

miabbott commented Oct 6, 2020

looks sane to me

/approve

@miabbott miabbott requested review from darkmuggle and ravanelli and removed request for ashcrow October 6, 2020 19:29
Comment on lines +670 to +672
if system.RpmArch() == "ppc64le" && targetMeta.MinMemory < 8192 {
targetMeta.MinMemory = 8192
}
Copy link
Member

Choose a reason for hiding this comment

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

How about we instead do * 2? That way, tests which truly don't need lots of memory aren't unnecessarily constricting how many can run in parallel.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since targetmeta.MinMemory is defined in JSON, doing * 2 could mean that we end up with up to 16Gb of RAM?

IMO, having a platform default is a bit better.

I dunno, going with a floor of 8Gb is safter than a blind *2`

Copy link
Member

Choose a reason for hiding this comment

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

I'm mostly concerned with allocating way more memory than necessary for trivial tests like misc-ro. Something like this would work

if system.RpmArch() == "ppc64le" && targetMeta.MinMemory < 4096 {
    targetMeta.MinMemory = targetMeta.MinMemory * 2
}

to cap the "auto-scaling" at 8G (so in the worse case, it's the same thing this PR is doing, and in the best case it's much less hungry).

Copy link
Collaborator Author

@jcajka jcajka Oct 7, 2020

Choose a reason for hiding this comment

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

Actually I have been thinking about it and it would probably make most sense to base it on the the (min) memory for current kola qemu tests(now it is 1G for x86_64 and 2G for rest) and just do 4x for the ext tests(as currently it is basically that) and possibly specify the minmemory either via absolute, multiplier of the base(x times 1G/2G), but no less than 4/8G or the default 4/8G (or something like that). But it kind of sounded to me as bit of over-engineered, relatively big change to directly open/create PR for. What do you think?

@darkmuggle
Copy link
Contributor

/lgtm

I think that we need to have platform defaults to deal with this.

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: darkmuggle, jcajka, miabbott

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:
  • OWNERS [darkmuggle,miabbott]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 8c2b579 into coreos:master Oct 6, 2020
jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Dec 23, 2020
With coreos#1768, on ppc64le and aarch64, we now request 8G of RAM for *all*
external tests, even trivial ones like `misc-ro`.

We already have platform defaults in `platform/qemu`. Let's rely on
those if the test doesn't declare any memory requirements (this is done
implicitly by this code by having 0 * 2 still being 0). If it *does*,
then as a heuristic, just multiply by 2 for ppc64le and aarch64 to a max
of 8G.

Obviously long term, we need to clean this up by having the test provide
a more symbolic requirement rather than a specific memory amount. (This
could be "flavours", or it could just be e.g. tags like
"reprovisions-root"). (There's also [the zram
change](coreos/fedora-coreos-config#768) which
should help a lot lowering the memory requirements for
root-reprovisioning tests.)

But for now at least, this heuristic will help with Power builds hitting
memory limits too easily.
jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Dec 23, 2020
… aarch64

With coreos#1768, on ppc64le and aarch64, we now request 8G of RAM for *all*
external tests, even trivial ones like `misc-ro`.

We already have platform defaults in `platform/qemu`. Let's rely on
those if the test doesn't declare any memory requirements (this is done
implicitly by this code by having 0 * 2 still being 0). If it *does*,
then as a heuristic, just multiply by 2 for ppc64le and aarch64 to a max
of 8G.

Obviously long term, we need to clean this up by having the test provide
a more symbolic requirement rather than a specific memory amount. (This
could be "flavours", or it could just be e.g. tags like
"reprovisions-root"). (There's also [the zram
change](coreos/fedora-coreos-config#768) which
should help a lot lowering the memory requirements for
root-reprovisioning tests.)

But for now at least, this heuristic will help with Power builds hitting
memory limits too easily.
openshift-merge-robot pushed a commit that referenced this pull request Dec 23, 2020
… aarch64

With #1768, on ppc64le and aarch64, we now request 8G of RAM for *all*
external tests, even trivial ones like `misc-ro`.

We already have platform defaults in `platform/qemu`. Let's rely on
those if the test doesn't declare any memory requirements (this is done
implicitly by this code by having 0 * 2 still being 0). If it *does*,
then as a heuristic, just multiply by 2 for ppc64le and aarch64 to a max
of 8G.

Obviously long term, we need to clean this up by having the test provide
a more symbolic requirement rather than a specific memory amount. (This
could be "flavours", or it could just be e.g. tags like
"reprovisions-root"). (There's also [the zram
change](coreos/fedora-coreos-config#768) which
should help a lot lowering the memory requirements for
root-reprovisioning tests.)

But for now at least, this heuristic will help with Power builds hitting
memory limits too easily.
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.

6 participants