Kubernetes SIG Testing, 9 Aug 2022

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: SIG-Testing Weekly Meeting for 20220809

Description

SIG-Testing Weekly Meeting for 20220809

A

Okay, folks I'll give people a couple minutes.

A

Okay, sorry about that, the uh shutdown button on my computer is very finicky. Okay, um hey folks, so I think as usual. um If anybody wants to introduce themselves because they're new here and want to just introduce the rest of the group, please feel free to come off me and do some now.

B

Hi this is, I think, my first time joining a sick testing meeting so nice to be here. Thank you for having me um my name is madhav. I am here primarily wearing my sick, contrabax hat uh and wanted to talk about a few things with you all today, thanks for having me again.

A

A

Oh all, right, I think it's seeing nobody else. um I think you have the first uh item on the agenda as well. So if you want to uh go ahead with that.

B

uh Yeah, so I wanted to sort of stop by and try and discuss a few ways that we can try, and you know like revive the granular approval um feature for the approved plugin. So for those who aren't aware of this um in the 1.23 release time frame, there was a feature worked on for the approval plugin, which supports granular approval.

B

So right now, if you do so right now, the way prs are approved is you do slash approve and that is sort of like a blanket approval and that approves all files um in a sub directory. Basically, so what granular approval lets you do? Is um you can approve individual files and then the approval plugin then sort of checks if all the required requisite files are approved by the required number required owners or not, um and this sort of is an extension to the approval plugin and now it's sort.

B

This functionality sits behind a feature gate as part of the pr. um There were a few demos which were done at sick testing meetings a while back, but considering that this is a significant change, like significantly large change.

B

I think the pr is about 5000 lines of code edition uh like 5000 likes, I added so um it's not too easy to review as well, uh because of that um things. Sort of dropped off from there, but having this now in the community is, um would be really really helpful for like a lot for like multiple reasons as well. um So I wanted to try and understand what could be done as next steps to sort of try and revive this. um So I know that reviewer bandwidth is tight, um so I basically.

C

B

Hear your thoughts on like what could be potential next steps. I know a few folks here have already uh tried, try to review the vr already uh when it was created. So thank you for that. Maybe your inputs also are like super valuable here.

A

uh Anybody who want to join our comments.

A

I'll give you a little bit of time to look at some of the links things as well from the notes. If you need.

D

D

Do we have specific use cases that we know that we want this for I'm kind of curious how important this is to achieve? uh I'm pretty sure we could probably achieve this in specific cases today by putting files into subdirectories if they need, if we expect them to need granular approval, it's not really on the fly, but I'm just kind of wondering how important this is to to implement.

B

Yeah, that's a fair question um so, from the use case perspective, one important use case now would be that um at least from talking from, like the kubernetes kubernetes perspective uh right now, especially, what tends to happen is that a lot of pr's tend to get like blanket approvals from like either root owners or like a parent subdirectory owner.

B

uh We could move the owner's files into like more granular sub directory so that those can then go ahead and get approved, uh but there is also still the case of uh blanket approvals coming in which this plug-in does not override like that. Functionality still remains, um but I don't have a concrete answer for like. Is this something that we really want to do?

B

um I I will get back to you on that for sure I will add a thread and like include the relevant folks who started the effort back then so that we can firstly gauge if this is still needed or not, and if it is then like figure out a way forward. So that sounds okay.

A

uh Yeah call if you want to direct and then then.

D

Yeah yeah, I mean that that sounds helpful. um I do think it could be a little bit challenging to you know, determine how important any of these kind of things are, but yeah some some some insight to that would be helpful. I feel like.

B

Sounds good I'll take that up. Thank you.

E

uh So so I would say that, like all the aspects of the existing functionality and the and the open pull request are things that we would really really like, and that I've observed use cases for constantly um being able to specify what you're approving is done, informally as a ad hoc social contract. Right now, um where you say like approve and then for whatever, um like on a line below you say like a four like hack directory um and but like it would be much better. If automation could enforce this.

E

It means that people with a lot of permissions are hesitant to approve when they're on, like multi-passage prs, for example, the root approvers in kubernetes will sometimes like be asked to bring their expertise to a particular issue, but they would have maybe delegated on some other aspect of the pr. I see that a lot and right now they just there's like once they give approve it. It proves whole pr and there's no way for them to like down scope, their approval um and the granular approved pattern matching.

E

Would let us delegate files that can't be moved um like go mod and go some. We have a dependency review team in kubernetes and we can't move those files because go won't. Let you sim link them um and they need to be in the root of the module, which is the repo root. But if we grant you a directory permission over the repo root, that's the whole project.

E

um I think there's a few other use cases like that or we could stop needing to rely on sim links in other places right now. The way that we have people that can improve the build system and kubernetes is every thing about the build that needs to be in the top level is same linked from some lower directory and, uh for example, sim links, don't work well when you try to clone get on windows.

E

um Things like that.

E

They'd both be really helpful for kubernetes kubernetes, at least.

D

Yeah for the go mod thing, um I can definitely see the use case there it'd be, I think, there's more useful or more interesting ways that we can address that like if we had regular expression based owners, files and stuff like that. That would also be helpful there, but yeah. I could totally see what all the points you made are really good like that. um So the I think that yeah, it seems pretty clear. That's really important for us to.

E

Implement them, we actually at some point attempt in this, and it's like it's already configured in the owner's file as like a as like a regular expression or glob. I can't remember. I believe that that's what the pr uh implements now is like you should be able to regex and say, like these people can own these files.

D

We're talking about using regular expressions now I thought that was uh a separate effort compared to the granular approvals.

F

G

Both according to the description.

E

Gotcha yeah, it was originally rewriting a proven to like a just like a second take plug-in it was approved to, and then we had some discussions here in this meeting and we decided that we we asked them to just like put it behind like a config option, the new code paths um instead of a whole new plug-in, uh I think we're thinking like the diff- will be smaller or something like that.

E

But yeah it does both.

A

um Putting myself on stack, but uh is this the like, so I understand the pr like feature is guarded, but also the pr is large so review. It might be difficult. um Is this something where like if there is reasonable review that this works correctly? It might be easy enough to just test that this works on like some smaller repos and then start gradually rolling it out.

B

um So one of the recordings that I've linked um so aaron talks about like what the order of rolling out would preferably be so it would start off with. uh So I think ben recommended kind as the first repo to roll this out too, and then uh then second rapper would be tess central because of high traffic and then third would be k community and then um either kk and then like org wide.

B

So if we do have enough proof that this sort of works correctly, we can try rolling it out to kind and see how things go from there, um but like uh considering that this has not been reviewed for a while. I think it would instill a little more confidence if we could have like one more pass of. You know like reviews before getting like a plus one or go ahead to sort of see where we can go from there.

E

The package is also pretty well uh unit tested and mature, and it's it's mostly just like logic, changes to support uh instead of directories, the paths or like the additional comment, format and those are like feature gated- that we can roll up a repo.

B

Yeah there are, there are a few uh things in the pr that need to be changed to like get it in a mergable state like getting rid of bazel files, and all of that um I think post that um we can probably see if we can roll that out to uh one of the repos and see how it works out. So that's, okay,.

E

Yeah, I think, besides review the main, I don't think the approved plugin has been touched much so it should just need the we've deleted bazel since then, so it should be a pretty light. uh Rebase, hopefully.

B

This sounds good, okay, so uh just to make sure that, like I don't miss out on anything um I'll make sure that the pr gets in a mergable state so like there are a few renting errors and removal of basic fires, and all of that um after getting that done, uh let me let me sort of bring you all on stick testing slack again and then sort of take it from that. If that, if that sounds, okay.

E

How feasible do you do you think, maybe to make it easier for review? We can like break it into like break apart. The two features or something.

B

uh By two features, do you mean.

E

Like the new approve command format and the support for uh regex.

B

Okay, okay, yeah, that that's a reasonable suggestion. Let me let me make that down. Thank you.

E

I think the code changes are interlinked, but still if we could like down scope it into until, like two commits or two pr's or something like that, might help.

B

Sounds good. Thank you.

A

uh Putting self-esteem again as well, um this is something where, like.

A

For instance, I don't have deep, proud knowledge, so I can't speak to like making sure that this fits well with like the existing software plugin or whatnot. But I can review for like uh go language, for instance. um Would that potentially be helpful or probably just leave it over oxford?.

A

D

Sorry go ahead, I think we're just talking over each other, but I was going to say this shouldn't require proud knowledge. Really. um This should be pretty well scoped. Okay,.

E

Yeah, since it's extending the existing plug-in all the skeleton for like receiving stuff from github and that sort of thing is already like implemented, it just needs to support an additional command text format and the logic for the regex files that uh it's like entirely. Config and logic changes.

E

B

um Are there any operational concerns that.

D

B

I'm sorry please go ahead.

D

I just had a quick question, or do we keep the existing unit? Tests are those all carried over.

D

Sorry, could you hear me no, I was asking if we kept the existing unit test. Did those did we carry those over.

E

Yeah, because most of the code is in a new it's in the new sub package of a proof um and yeah, it looks like it should be, carrying over the tests and just adding more cool.

D

I mainly asked that, because we have, we have pretty good uh test coverage there, so we should have good confidence in things if uh all of that is still working.

E

D

F

E

Of this diff is the test.

D

The tests almost have yeah they're, almost too specific in most cases, so but they're they're good coverage. So I think we have good confidence if those are all working.

B

Yeah so um the main approve uh package: there are a few tests added there, but I don't see any tests being modified or deleted to sort of adapt for this change.

A

C

If maintenance is back.

A

All right um any other questions on this or.

E

Thank you. Thank you.

B

A

See all right, um I am so I think uh rajas and someone else also has the oh, I'm guessing.

H

Okay, because someone else is me: hello, hi,.

A

Whoever wants to present for the next one go for it: yeah.

H

Yeah. Okay, so I guess I can start because I I I'm the one who suggested doing this. um I started out experimenting with enhancing our pre-merge linting process, because the current linting configuration for golang sea island for entry code is fairly limited. Our code is pretty bad in some areas. We certainly could do better and ask more from our developers when they add new code, even before we fix the existing code.

H

I have a pr pending that does that it have that strict at linting. But my observation is that if we, if we were to enable that it would be a bit a pretty bad experience, in particular for novice users, the ones that are most likely to benefit from good, linda annotations for their new code, because they would have to figure out what this brow job is about.

H

That is failing where to find the log output correlate the text description of the failure with the changes that they are making and there's a lot there's a much better interface for that, and that's the github annotations for your pr, where github immediately next to the code that it's complaining about shows what's wrong with it and that's what I would like to see enabled uh before we turn on this stricter linting that I've been working on so at cubecon.

H

First of all, I think on slack I asked about that and then at kubecon europe I asked around. I attended the presentation about prow at kipcon yuwu and together we came up with some kind of approach and rogers then showed up at keep on you and said that he will, but he will be able to work on this and that's where we are.

H

I think he hasn't done much, but I can let him talk about that uh or just do you want to describe the approach that we settled on so that people can get an idea. What we are talking about here.

C

Yeah sure uh so, yeah so hi everyone and, uh as patrick pointed out, um we were just brainstorming around uh some ideas wherein the way prow is currently displaying the status of proud jobs on github. We could probably uh have some convention wherein, if you want to uh pass on some logs from the project failures, then you put them with some naming convention in the artifacts directory and then uh at that time we were. I remember on tony on patrick and I were brainstorming on something like cryo may.

C

You know pick them up and uh populate github comments with uh you know the failures which which are difficult, so this was something that we will be installing on. um I don't have anything uh working as of now. uh I I couldn't find some bandwidth uh a few weeks ago, but uh I can see that I'll be able to give some more time to it in the coming weeks. uh So this is like the approach that we decided to take on, so uh I think we need to identify the next next step.

C

So should we come up with like a design dock or something which we can present.

G

D

G

General having a design would be great.

G

The basic approach of we dump the file in the artifact and require reports that I think.

F

G

G

One thing I'm noticing, though patrick in one of these comments, mentioned that they want to use output, producing github actions, format um and github actions. Format means that it has. The report be reported as a github check, one which you can only do if you have a github app, which power that cater or currently is not um yeah. Just something like this.

H

Yeah we we haven't settled on the format for that file, but we are storing it as an artifact. That's one of the things that we need to investigate. I just noticed that golong ci lind has something that the github action for goloxy island is using and it I don't know what format that is in the text.

H

Output actually is something that has a formal description or anything, but that's really an implementation detail that where we probably need to prototype a bit and see what's most suitable as the api between a brow job and this new feature in cryo, it could also be some other format.

H

E

The biggest concern I have here is if the tool that is producing the format runs in pre-submit and runs it with code from the repo um and then we like will hook up, comment permission uh abuse, but it seems like if you, if we, if we do the commenting completely outside of it, by integrating it into prow.

E

uh With this feature, then I guess you can only spam your own pr is that the intent.

H

Okay, that's the intent yeah.

E

And because previously sorry just super quick, some people previously tried to do this by just, like, oh we'll, add like an account to the proud job and that's kind of a big nope.

H

Yeah we we. We noticed that we discussed that, I think already on slack, but we can also do some rate limiting or maximum number of commons. So if there is a runaway drop brow job that, even although the damage is limited to the pr itself, we can limit the number of comments easily to something that is sane.

D

Yeah, I think the biggest concerns are the the rate limit and security, with the rate limit being the much more realistic one. um So if there's a throttling mechanism there, um we should probably also have some mechanism for this to be up for jobs up into this in their spec.

D

I'm not sure if that's included in it so either supplying a proud job label. That indicates that we should look for this, or is the idea to actually try to scrape this information from every job that runs.

H

I guess we could turn it on for everyone, because the name would be special and there's if we pick something, that's fairly, unique and unlikely to be created by accident by an existing brow. Job then creating that file is basically the opt-in mechanism. For this new feature.

D

Yeah, I just wonder if there's potential for uh abuse by that, um because then a pre-submit could be made to somebody could modify the pre-submit to create the file when it was not originally intended. For that job to be capable of commenting.

E

Yeah one interesting abuse thing is, I think we should probably just like super binder throw this out. I think we should probably scope this to a different robot account than we use like the triage account or something so that, if you manage to like trigger github's abuse mechanism or something it is that blast radius isn't on the like main ci account.

E

I could I could imagine someone just like posting a bunch of comments that are intended to get flagged or something, um but I think we're like fairly okay there as long as it's like like as long as we move this a separate account.

D

Yeah, we have a few accounts that we might be able to reuse for that, but we might want a new one. We definitely shouldn't use khci robot.

E

Yeah, like that's the worst thing, I could imagine uh off the top of my head happening and uh it would be really unfortunate on kci robot as long as it's a dedicated like as long as a separate one than that. I I think that's not super likely and if it does happen, we we'd like get through it.

E

Rajas you want to go.

C

Yeah, uh so should we go for a new account or should we use the triage.

E

I'm not sure yet, but I think we can probably discuss that further uh later. I think it's more like, because we know we can do something like that. We know that we shouldn't need to get blocked on that abuse concern, but because we could just isolate the account.

E

I also I we didn't really. I think we'd skim past this, but you mentioned the github actions format. That actually seems like a good idea to me because we might it might make it so that people don't have to write more tools to like emit prow format. If there's just like a flag, we can turn on and tools if that's becoming a widespread thing, um picking up that support for free seems really nice.

G

Would it, then, would it, from an organizational point of view, be easy to set up a new github app in the commander's empire 6 organization? Just for this.

E

G

Then you could actually create check once and do this.

E

That seems like a really good thing for us to bring to the uh github admin team. um I imagine they would be supportive of that, but I can't say for certain.

E

uh There's a channel in slack folks, uh github admin, I believe, there's a it's a subgroup of uh sig contrabex that manages the like github accounts and everything.

E

If we're making new robot accounts, we should probably be making them under the uh the github, like the community github team.

E

Our nose with us in the comments in plus one.

A

Awesome all right, uh it sounds like there's some good action around here and potentially some action items to take. um Is there anything else that folks want to discuss on this topic?.

A

All right sounds good. um I think we're good there there's nothing else listed on the agenda right now, but does anybody have any last minute things that they want to discuss here while we're here.

F

Hi, um I have a question about onboarding brooklister, not power of gcp. Do we have a process for that.

F

My understanding is, you need to have.

F

We only consider gcp gk cluster, because we have gen cred, but if some entity want on board breakers on that part of gcp do on in pro, do we have a process for that.

D

um I'm not sure if we have a process for it, but it should be possible. um The gen credits thing is for uh rotating our cube, config credentials that we use to connect. um We need that for uh some of the policies that we have on our gke clusters, but that wouldn't necessarily apply to uh another cloud, um and we would just have a separate cube config file for that, but we can combine those cube, config files in our deployment configuration so um yeah. There's no there's no stop there.

E

And they're cube config format so that, like the output of the gen credit tool, so like that standards compliant it's just it's just a utility for rotating the cube configs. We use.

D

Yeah, basically, if you can supply a cube config to the to the pro service cluster, it can treat it like a build cluster.

E

Depending on what you're using it for, if it's like, if would try to do this for kubernetes, there's like some like additional considerations um around like assumptions that the jobs can make for things we've done in the cluster like we have some daemon sets deployed to like bump up cis cuddles and things like that um or like dev loop devices. If preoccupated, we have a couple of little like hacks that we deployed to the cluster to make ci runs.

F

The context of my question is related to ib some job running on some on some to set up a request inside ibm and just that's. Why I'm at this question, because that is owned by them.

F

I give them the flexibility to do those custom configuration and it's only is only going to be for pro job target for stuff, like cluster api provider or specific, specific drone, specific pro jobs target for a specific architecture, because there was a proposal to integrate bosco's and running I'm running that ins on the gk cluster, but I feel like we should just give them the possibility to to provide the cube config for a for a beer cluster inside ibm.

E

I don't think we have any technical blockers. I think you know, rotation is something to cons to. You know have on the radar, and just those like other things I mentioned are more like. If someone wants to know, we can uh point them to that, just because we don't have a process right now, but we kind of have one for the gke clusters, uh but it could pretty much just be a cube config and whatever they want to do with it.

D

I've there are a couple things that I can think of. That could be a little interesting, I'm assuming. Presumably they would still be using gcs to store the job results um if they're.

D

um Yeah, we need to figure out um kind of how the credentials would work for that. I guess um some of the constraints that the policy constraints we have for our uh projects might apply.

E

We can support more buckets right, so if they, if they got some kind of s3 compatible bucket running, we could probably get pro and test grid point at that. It's like a bucket just for those jobs.

D

Yeah, I'm thinking we if this gets set up, though as a it might be better to set it up on the the gates infra prime instance, rather than kate's pro, if possible, there's probably fewer policy constraints. There.

F

Okay, I will try to follow a slack about about this, because I didn't, I didn't think about um credential, for the service account responsible to upload the buildblocks.

E

And just from our past experience and how we wound up with gen cred, you probably want to plan up front for like how are you going to handle rotating the cubeconfig credentials eventually.

F

A

All right cool um anything more on this topic.

A

All right um anybody else have any uh other items that they want to discuss now.

A

All right, um I think, I'll, let everyone go then uh thanks. Everybody, a lot of good discussion, um a lot of follow-ups too, but awesome thanks.

D

A

E

You thanks all have a happy tuesday.

E