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

Pools of the same image and flavor cannot exist on the same provider #245

Closed
Zappycobra opened this issue May 9, 2024 · 3 comments · Fixed by #246
Closed

Pools of the same image and flavor cannot exist on the same provider #245

Zappycobra opened this issue May 9, 2024 · 3 comments · Fixed by #246

Comments

@Zappycobra
Copy link

To provide some context:

We have several runner groups that we make public to all but restrict the runner group to specific reusable workflows so that only those workflows can run in that runner group's runners. We could provide just one pool and consolidate usage (min, max, idle), but we want to control the usage of runners available to each of those runner groups separately due to prioritization reasons. We currently are forced to tag the same image several times in all of our providers, but we would like this not to be the case.

We have a need to have multiple pools with the same image and flavor on the same provider. Due to below we cannot have the same image and flavor combination, is there a need for the else in this check to happen:

if _, err := s.getEntityPoolByUniqueFields(tx, entity, newPool.ProviderName, newPool.Image, newPool.Flavor); err != nil {
    if !errors.Is(err, runnerErrors.ErrNotFound) {
        return errors.Wrap(err, "checking pool existence")
    }
} else {
    return runnerErrors.NewConflictError("pool with the same image and flavor already exists on this provider")
}

Please advise on workaround. We have a fork of this repo, would it be safe to remove the check?

@gabriel-samfira
Copy link
Member

gabriel-samfira commented May 9, 2024

Hi @Zappycobra,

The reasoning was that it would be easier to scale the pool up rather than create an identical one, but this was before the adition of runner groups.

It should be safe to remove that check, or expand it to include the runner group as well.

I can create a PR soon to address this.

gabriel-samfira added a commit to gabriel-samfira/garm that referenced this issue May 10, 2024
This change removes a check that denies the creation of a pool
if the new pool has the same image and flavor set on the same
provider. The reasoning for that check was that if you need to
create another pool with identical settings to an existing one,
you might as well scale up the min-idle-runners on the old one.

This was done when runner groups were not yet added. This in
turn has forced users to alias images with new names in their
provider, leading to terrible UX. In the end, being too
opinionated in this case has caused more harm than good.

Fixes cloudbase#245

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
@gabriel-samfira
Copy link
Member

@Zappycobra

I opened a PR here:

Could you give it a shot and let me know if it fixes the issue for you. Please note, the latest main branch also includes a big change to garm in terms of github credentials management . If you don't want to deal with that now (it should migrate credentials to the DB automatically), you will have to cherry-pick this change into the release/v0.1 branch and build that.

@gabriel-samfira
Copy link
Member

Feel free to reopen this if it's still not fixed by the PR I mentioned

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants