Page MenuHomePhabricator

Figure out a way to enable volunteers to use the puppet compiler
Open, NormalPublic

Description

As a volunteer who contributes puppet patches, I want to use the puppet-compiler to verify my patches do the right thing and don't break anything inadvertently.

To run the puppet compiler, one needs the 'Job/Build' permission in jenkins. From https://wikitech.wikimedia.org/wiki/LDAP/Groups it seems this is granted to all wmf employees (by giving them access to general jenkins administration). It doesn't seem to be granted to anyone else (but that isn't really documented, just viewable from the permissions settings in jenkins itself, which I cannot see).

This means I currently have to ask on the patch or IRC to find someone who runs it for me. This wastes both my time (by having to find someone to do it, often having to ask multiple times until anybody responds) and the time of whoever runs it.

I assume jenkins offers the possibility to use a more fine-grained way to assign a user the build permission only for that job (as opposed to for every job or the do whatever you like in jenkins permission granted to ldap/wmf).

We should:

  • find out how to assign the build permission only for that job
  • figure out what the security impact is of enabling someone to use the puppet compiler form
  • based on that, decide which group to add that permission to (all logged in users, ldap/nda, some new group with a lower entry barrier than nda but a higher one than creating a account, ...)

Event Timeline

EddieGP created this task.Apr 19 2018, 9:51 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 19 2018, 9:51 AM
Dzahn awarded a token.Apr 27 2018, 2:14 AM
RobH added subscribers: Joe, RobH.May 1 2018, 10:02 PM

This maybe something for @Joe to review (since he is quite familiar with the compiler)?

Joe added a comment.EditedMay 2 2018, 4:31 AM

The compliler has little to do with @EddieGP's request, which seems sensible; what we need to tweak, maybe, are the jenkins permissions. I am not even a jenkins administrator anymore - this ticket must be handled by the Release Engineering team.

My only advice would be to make the build available to all of our active volunteers, but not open to the world (the resources are quite limited).

RobH added a subscriber: greg.May 2 2018, 2:55 PM

The compliler has little to do with @EddieGP's request, which seems sensible; what we need to tweak, maybe, are the jenkins permissions. I am not even a jenkins administrator anymore - this ticket must be handled by the Release Engineering team.

My only advice would be to make the build available to all of our active volunteers, but not open to the world (the resources are quite limited).

I knew we'd have to bring in release engineering, but I just wanted to check with you that it was a sensible and acceptable request (ie: the puppet compiler). Seems it is, so now this is indeed a release engineering tweak.

@greg: How would you suggest we go about handling this?

To run the puppet compiler, one needs the 'Job/Build' permission in jenkins. From https://wikitech.wikimedia.org/wiki/LDAP/Groups it seems this is granted to all wmf employees (by giving them access to general jenkins administration). It doesn't seem to be granted to anyone else (but that isn't really documented, just viewable from the permissions settings in jenkins itself, which I cannot see).

Currently the nda ldap group has all the same permissions as the wmf and wmde ldap groups.

Those permissions are:

Overall

  • read

Job

  • build
  • cancel
  • discover
  • read
  • View status
  • workspace

Run

  • delete
  • replay
  • update

View

  • configure
  • create
  • delete
  • read
RobH added a comment.EditedMay 2 2018, 4:34 PM

Correct, but it seems we'll need to add another ldap group, since we don't want to require NDA for this particular permission.

So then they only need the job/build, and nothing else?

If so, is the best solution to simply create a new ldap group for this? I imagine we'll add more users over time, as this will be more restrictive than simply making an account, but less than an NDA.

Also this needs the sign off of Release-Engineering-Team since it affects their services/responsibilities. Adding more users to use the puppet compliler could lead to resource contention. (Ideally it shouldn't, but since it could, they need to be aware and accepting of the change.)

Dzahn added a subscriber: Dzahn.May 2 2018, 4:44 PM

fwiw, we already have a listed of "trusted devs" somewhere in integration/config to allow users to run tests in the test pipeline. can that be re-used? or maybe should it be merged into a new LDAP group we make for this purpose? trying to avoid 2 separate places that keep a list of trusted users for similar but slightly different things.

^^ that will work zuul side, but this requires users to have permissions to do it through jenkins ui (so needs to be configured jenkins side)

@thcipriani: Could you document this on https://wikitech.wikimedia.org/wiki/LDAP/Groups please? By now I only see a vague point "Jenkins administration", and that only for the wmf group.

So then they only need the job/build, and nothing else?

As I understand it, that global permission allows to build *any* jobs that can be triggered by jenkins. If we can't limit it to puppet compiler only, we need to review all the jobs for whether we can entrust people in this new group to run them.

fwiw, we already have a listed of "trusted devs" somewhere in integration/config to allow users to run tests in the test pipeline. can that be re-used? or maybe should it be merged into a new LDAP group we make for this purpose? trying to avoid 2 separate places that keep a list of trusted users for similar but slightly different things.

No. We aren't even able to keep "do not run the check pipeline for these users" and "do run the test pipeline for these users" in a single list with how zuul config works. That list (or rather those two lists) are meant to go away, they're a PITA (T192217), so the work of migrating it to LDAP seems unnecessary. We might want to copy it to a ldap group, but I don't expect zuul to pull from ldap, but rather keeping it duplicated until it goes away from zuul. Note that when thinking about the limited resources it could be considered too inclusive. Basically everyone who contributed anything constructive to any of out gerrit repositories in the past will easily be added if they ask for it.

I'd propose to start with something like "everyone not in ldap/wmf, ldap/wmde or ldap/nda who has 3+ patches merged in operations/puppet" (replace 3 with any number you find more appropriate). I'd consider that a good balance in terms of resource management and inclusivity.

greg added a comment.May 2 2018, 9:30 PM

Also this needs the sign off of Release-Engineering-Team since it affects their services/responsibilities. Adding more users to use the puppet compliler could lead to resource contention. (Ideally it shouldn't, but since it could, they need to be aware and accepting of the change.)

In principle we are happy with this change. Let's just figure out how to do it technically :)

RobH triaged this task as Normal priority.May 3 2018, 4:38 PM
Krenair added a subscriber: Krenair.Aug 9 2018, 1:12 PM

These are the people who fit @EddieGP's suggested criteria: P7440

i just noticed T97580

Is that the same thing and it got solved??

As I understand it T97580 was about those volunteers who are already entrusted with root access to toolforge, and the solution seems to have been to add a user to the nda ldap group. Membership in the nda group and especially toolforge root access require far more trust than access just to the puppet compiler.

I'm wondering if it would be easier to unhook this thing from Jenkins and have it provide it's own UI for triggering jobs.

Legoktm added a subscriber: Legoktm.Nov 2 2018, 4:06 AM

Thanks to Krenair bringing it up on IRC, I took a stab at implementing this. You can now comment "check experimental" on a operations/puppet patch and it'll trigger PCC.

To pass the list of hosts (so it doesn't take hours to run), you can specify it via the commit message, for example: https://gerrit.wikimedia.org/r/c/operations/puppet/+/463519 and https://gerrit.wikimedia.org/r/c/operations/puppet/+/471195

This is currently implemented via the https://integration.wikimedia.org/ci/job/operations-puppet-catalog-compiler-test/ job, which is a fork of the standard PCC job. Are there any usecases that triggering via Gerrit/zuul doesn't handle? I'd like to replace the current 'operations-puppet-catalog-compiler' job with the -test one.

You can now comment "check experimental" on a operations/puppet patch and it'll trigger PCC.

https://media.giphy.com/media/mbhseRYedlG5W/giphy.gif

Thanks to Krenair bringing it up on IRC, I took a stab at implementing this. You can now comment "check experimental" on a operations/puppet patch and it'll trigger PCC.

To pass the list of hosts (so it doesn't take hours to run), you can specify it via the commit message, for example: https://gerrit.wikimedia.org/r/c/operations/puppet/+/463519 and https://gerrit.wikimedia.org/r/c/operations/puppet/+/471195

This is currently implemented via the https://integration.wikimedia.org/ci/job/operations-puppet-catalog-compiler-test/ job, which is a fork of the standard PCC job. Are there any usecases that triggering via Gerrit/zuul doesn't handle? I'd like to replace the current 'operations-puppet-catalog-compiler' job with the -test one.

Hah, funny! We were just talking about a solution exactly like this last week as an MVP for T166066. I think @colewhite was looking for ways to make Zuul accept arguments in those comments last we talked about it.

Thanks so much for working on that!

Also seems like we have 2-3 overlapping tasks here, should probably clean up! :)

hashar added a subscriber: hashar.Nov 5 2018, 1:58 PM

...
To pass the list of hosts (so it doesn't take hours to run), you can specify it via the commit message, for example: https://gerrit.wikimedia.org/r/c/operations/puppet/+/463519 and https://gerrit.wikimedia.org/r/c/operations/puppet/+/471195
...

Hosts: cobalt.wikimedia.org, gerrit2001.wikimedia.org

That is smart :) Well done @Legoktm .

faidon added a comment.Nov 5 2018, 2:02 PM

While this is great, I fear that it will unnecessarily spam the commit messages with information that isn't really about the commit itself.

Is it possible to move this hosts stanza in the Gerrit comment? e.g. by leaving a comment like "pcc for cobalt.wikimedia.org, gerrit2001.wikimedia.org"?

I can see the Hosts field in the commit message potentially breaking the commit message validator. Can this tool handle line breaks?

An example of this can be found here: https://gerrit.wikimedia.org/r/#/c/operations/puppet/+/471360/

Yeah you just repeat the 'Hosts: ' part on each line: https://gerrit.wikimedia.org/r/#/c/operations/puppet/+/471195/

While this is great, I fear that it will unnecessarily spam the commit messages with information that isn't really about the commit itself.

My initial thought was that knowing which hosts a patch affects might be useful for reviewers, and therefore valuable to have in the commit message. But I don't really contribute to ops/puppet regularly, so I could be totally off.

Is it possible to move this hosts stanza in the Gerrit comment? e.g. by leaving a comment like "pcc for cobalt.wikimedia.org, gerrit2001.wikimedia.org"?

Probably. The API to get the current commit message is much easier than looking for the latest comment, but we can figure something out.

More importantly, it's not clear to me whether we should be running PCC on every patchset that's submitted, or only when it's manually invoked. For similar CI jobs where we're diffing configuration, we currently have those jobs run on every patchset, which I think helps out a lot. And if we're running PCC on every patchset, then we can't rely on Gerrit comments, it has to be part of the patch itself (aka the commit message).

Also, my reading of T166066: Integrate the puppet compiler in the puppet CI pipeline suggests that the manual specification of hosts is meant to be a temporaryish thing until the puppet-compiler is smart enough to figure out the affected hosts by itself?

hashar added a comment.EditedNov 6 2018, 3:22 PM

Is it possible to move this hosts stanza in the Gerrit comment? e.g. by leaving a comment like "pcc for cobalt.wikimedia.org, gerrit2001.wikimedia.org"?

Probably. The API to get the current commit message is much easier than looking for the latest comment, but we can figure something out.

Gerrit comments are handled by Zuul to add a change to pipelines. The event is not attached to the change being used to run the jobs. What I mean is that when triggering the jobs, we can not retrieve the comment. At least not with Zuul.

The Zuul parameter functions are passed the QueueItem object which has the change and might have the comments. Would have to investigate though. Potentially:

def set_pcc_params(item, job params):
    params.update['COMMIT_MESSAGE'] = item.change._data.get('commitMessage', None)

Similarly the comments should be in a list in item.change._data.get('comments'). As best as I can tell, it seems the comments are just a time ordered list, they do not seem to be attached to a specific patchset though the message string is prefixed with Patch Set XXX: .

We might be able to parse the comments in the Zuul parameter function, but it seems better to delegate that to the job or tool being executed. Maybe retrieve the comments from Gerrit and assume that the last comment matching Patchset \d+: pcc (.*) is the right one?