GitLab GitLab Runner Open Office Hours, 18 Mar 2020

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: GitLab Runner Open Office Hours - March 18, 2020

Description

On this call Steve from the Runner team does a live review of a Community Contribution: "Convert project unique name to RFC1123/RFC1035 standard" https://gitlab.com/gitlab-org/gitlab-runner/-/merge_requests/1911

A

All right, I'm Mike, should be working now. Yeah.

B

B

Let's just give a minute or so for anyone joined because I think if anyone's not gonna report than they might have to be installing losing client there's an easel yeah looks good, a minute or so of grace period, but I've already started recording specifically. So if anyone does join, they immediately get the notification they're not caught off guard I.

A

Notice at least one you contributor on the call too, which is great.

A

Hey Pierre, I and Fabio I think your first time joining the call. Welcome yes,.

C

B

Welcome I got a nearly full Bri bunch of you, so you call that a critical mass to start with so welcome everyone. This is the start of our third iteration of the open office hours for the lab runner project, specifically the date 21 watching according is March 18 I'm le Russian on the engineering manager for the runner team at gate lab currently on our team I'm joined by Darren Eastman, the product manager.

B

We've got three of our back-end engineers on the call Steve yogi and living there and break from our community I, always messes on a human relations team. Yep, that's perfect: okay, good I, just member what the slack tunnel is, and the goal here is to really just help anyone long with any EMRs or either that exists currently or they might want to create and get into existence for the runner project.

B

There's really no agenda. There's no, there's no heart agenda. Anyone have any questions about something they have outstanding. As an mr or like I said, are thinking of creating or and thinking of working on an issue. We can talk through them Darren's here, except there's, something like a feature there's no one's ever talked about. We could even talk about that, but as a default, what we'll do is deuce Steve will go through and do some kind of live code. Review of a couple of or I don't know what his plan is.

B

Maybe a couple of open, em ours that are authority thing contributed.

B

Anything you want to add rare anything going on from the community specific, no.

A

No I say I think that's I think you covered everything it you know just looking at the issue for the call and look like Steve, you had a few possible M ours that that you wanted that we can use as examples as well, but you know I think you covered everything. Is.

B

So yeah I'll come back to this a couple times in the call just case people join late, but at any point please either use the chat or just speak up. There's opportunity. If you have any questions about something you know certainly don't need to wait to get called on and so Steve okay to work. You I think yeah, but yeah like raise that you have a couple mm. Are you ready funny huh yeah.

D

I picked some on Mars from fabulous list that I haven't reviewed yet, and maybe we can start reviewing them. I know Pierce here on the call regarding the community contribution. We were working on last time. I still haven't had time to look at why the image printing is failing, but.

E

Since we already.

D

Did two of the servers on that it would be nice to have some other contributor working on their cookies.

F

Every day and I'll.

D

Continue looking at that, as you were, having a sink so first of all nice to meet you Fabia, it's the first time we're talking I, think and thank you so much for all these contribute I really enjoyed reviewing them so far,.

C

D

So let me share my screen.

D

This is the one that got me most excited implementing the container names to be RFC, 1123 compliant and it's a fairly small Amar's. So I picked a few merchandise from you. So the reason does this got me a bit excited because I'm I think Georgie's in the school Georgie recently did some work on making everything RFC 1123 compliance for the kubernetes executors. So it will be nice to start working over to the docker executors to be the same, because we do want to have them the same. I.

E

Think sort if we just worked on making a wise this RFC 1123 yeah, nothing else.

D

E

Nothing else just the wisest.

D

Yeah yeah yeah.

D

So quickly, looking at it, it does seem to make perfect sense because we are using our helper function. That would remove the underscore a few things that I try to keep in mind, so we're changing something fundamentally from the Ataka executor right. So are there any potential breaking changes in this case?

D

I do not think so, because if, for example, you already have a system right running, it Lebrun are with the docker executor and you upgrade to the new version when this merge, which is being included worst case scenario, is going to be that we're just going to be creating extra containers with the new with the new name, which doesn't really is completely transparent from the user right.

D

So I do not think there's any breaking changes apart from if people are having some kind of monitoring tears or something like that, that look at a specific container name but I would even I. Don't think that would be in the case of our breaking changes policy and we already have plans to change the name to have the job ID here.

D

So I, don't think that's a problem, so the biggest part would be I would literally just start by doing just so or just clone it first and run it on my laptop locally to see if there are any problems and then I will actually look at the code. Looking like getting it working helps me understand like what the problem is. So let me start the JDK and.

D

D

It's my god, I try to do something.

D

Are there any questions, software or as clear.

D

Let's see if DJ Takei started.

D

For anyone not familiar and want to either view into squall or something like that, JDK is our development kit, where you can use it around local on your laptop.

D

So you can do some development on it or use it for a different look like, for example, during routine I personally use DJ tukutz, so I run jobs locally, so every move fairly quickly than just using get up to command. I can and not butter about secrets or anything like that. It's a fairly handy. Sometimes it does require some setup because you have to have everything installed locally, which is not ideal, but it gets the job done so.

D

Does take a while to boot up.

D

And if we look at our conflict, I have a simple, simple, docker executor already registered. That's just have a few configuration files, but it's just running a scaler runner register.

D

So we do have the coat-check locally, so what I'd like to do? You can run make build.

D

D

And built the docker image and use the binary, but what I like to do is simply doing program may not go just because it's fairly bed quicker and I don't really care about the hyper image. The only time you want to run I make Belt current as when you want to specify the helper image and you're doing some stuff on that very much. But in this case we're not doing anything related to that. So running directly. To go for the profile is good that your idea is taking so long.

D

Maybe we'll use good luck.

D

I'm just checking the runner, it's not paused or anything and.

G

D

Now, that's just on a simple had a word job, but the new binary name and receive everything still works.

D

So we're on top appears, we should start seeing the containers being created.

D

And that actually ran, but it was so quick that I watched them pick it up, but yeah it's trying perfectly fine so seems to be fine for now. Let's look at the actual tests so to remind myself I'm gonna check what this is doing and where we're using it.

D

Putting the name to lower checking of it looking make the font bigger.

G

D

What this function is doing is changing the name to lower string, doing a reg X match the name and replacing anything that does not match it to empty.

D

Then we're checking the max left of it, which is 63 and if it's higher than 6 today, we are truncating the name to lower chance of slipping.

D

But that seems fine.

D

Let's take a look at Gus's.

F

So can we just override one of the containers from my other project? If we do some I mean we create the same project, but with some known letters in it and then it will create containers with the exact same project, name yeah,.

D

That's that would be one of the next steps for the minimality way for sure. That's a very good point, but let's look at the tests here, because if I could risk aiming true, it seems like it's doing that so here we have a table driven desk which is normal and go.

D

It's passing in the build and the expected output, so I would already suggest to have expected output to be expected named because we're here we're talking about project, you, nickname output seems very vague, so one suggestion I would do is mmm expected.

D

So when I'm, just when someone is just coming through, we know what we mean by expected output, which is in this case it's just the expecting.

D

Project, an RFC, unique name, so this one, as with the token that has a mantis grown and the job response, there's a number and then the project earner ID is zero and the expected name would be the same token, but all lowercase and it does not have the underscore, which is fine. That's.

D

Perfectly fine and then project normal, unique name where we have dick token with the which should be valid, and we have it like this now, let's see if we can think of any other cases where we can not be dns 11:23 complaint right. So, for example, here we were just using the ids okay, so those are combined so the only time it's not compliant as the short description and we're already testing for that.

D

So one thing I also like to do is check the coverage since this edit, a new test would be nice to see if we're have full coverage part for missing some cases.

B

Love how invisible your view is for us all right now, I could probably take my glasses off and still Bjelica see everything's going on.

D

And full-coverage, so we are sure that we do have we're covering of all cases here and what we're not checking here, but this test specifically select. So this is really helpful right because we like ideally the unique name, will never be longer than six to eight characters right. But what if- and this made me think of that case- so what if we add a new case and build its test, so we make sure we a guard for that linked.

D

So, for example, well, when I'm trying to add a new test case, something like that for the community contributors I try to do it myself, just to see if it's actually viable or not and.

D

Then I'll add it as a suggestion: I'm not sure if this will work or not because we are already drunk eating. But let's see how many characters we have here.

D

D

So we have 46, we just need to add 20, more characters.

D

I'm sure this will work because I probably get some kind of work for.

D

Another thing that I would comment on as this is not a realistic view of what the token is right. I had two token is longer than these characters, if, if I'm not mistaking so one thing that I would do is I'll just.

D

Comment: let's use a real-life token, so, for example, this one right, that's just.

D

Not even sure how to word it. Yes,.

D

So, for example, we'll use that's right.

D

This one turns to be like that. That means we expect.

F

D

I think it should be like that, so, let's go and this for now and see.

D

And that's working so here we made sure that okay, we're using array of token that in production we use because and before it wasn't a real token we used in production right. How do we make sure that, even if the underscore is in the middle, it's being dropped off and everything is put in lowercase? So let's go back to this test case and see. I would would work for us.

D

D

So sorry, okay, so we did actually go over.

D

63, so we are we're also making sure that we actually truncate the unique name to a specific variable. Now what we're truncating I! Imagine the concurrent ID.

D

Which might not be ideal because we're truncating the concurrent ID? That means there might be a chance of a collision of concurrent IDs.

C

And it should be, should do it only on the on the top on the short name, where his name is, it's the runner and the token it's the description, short description.

D

Okay, but then, if we, we still add the concurrent ID afterwards, it still might be.

D

That's a bit tricky right because.

D

If we do that, if we just do it for the runner and token short description, that means we tap first to check how long the concurrent ID is and the project ID r and then make sure when we truncate. We have enough space for those. So.

D

To be fair, this is a very, very edge case, because it's very I would be impressed if the runner can run this. Many concurrent bins at the same time on a single host, so I'm not sure. If we actually should butter solving this or not, it might be words to still add the test case. So we make it clear that hey. We are aware that the concurrent ID will be truncated. I would still add it like this I would I would actually put this discussion async and check with Thomas.

D

Who has any concerns about this because to me to be asked having click on current ID truncated when it's such a long value I, don't see it being ever a problem, because um I haven't seen one a single Duggar house being able to handle this many amount of concurrent jobs, but I can always be around. Of course, the.

F

Short description, it is limited as well, because the users are as controlling the project description right so could put whatever very long hay in it.

D

The short description is just a token, so my.

F

Short sojourn, so the project name then.

D

We don't use project, name, use, project, ID.

D

And the descendant so let's see what.

D

We'll see if it's actually gonna be an issue or not.

F

Usually, the question we ask is what to the user, has control or an attacker has control to.

D

F

Does make very the.

D

User doesn't have control over anything, does not have control over. The token does not have control over the ID nor the concurrent.

D

Now what we need to be careful is what happens if, for example, the project ID is maximum right? Do we truncate the concurrent ID with that constant problem? Do we even have a key clip instance that has the maximum integer of an ID, for example, I investigate this a bit later, I think I, don't think that's actually a problem and I'm just being really patented about it.

G

I kind of agree there, it's magical logic unit can happen. It's gonna take ages to debug. If something like that happens, yeah so it could be, could be okay to just validate. If the input is equal to the output in length and if it's dedicated, then at least you show learning a warning low or something yeah.

D

Yeah, that's that's an idea as well.

D

So the maximum and run copy pasted correctly there's this one and we don't seem to be truncating it, which is fine.

D

Okay, so I don't think this is an issue because we would truncate it normally within the JSON encoding.

D

That's me: you could clear that we actually track it.

D

D

Go so I leave a detailed comment on it and America silver make sure unclear on what needs to be done here. So let's add this to the review I.

D

Leave it here and revisited later tomorrow, so the test does seem fine and it's checking what we want to and it's doing what it says so to be as I. Don't have any objections in the merchandise unless any other engineer has any questions or thoughts about.

B

It's like a speak now or forever hold your peace thing.

D

Speak no or don't say, yeah, let's be about it. Freaks.

B

D

D

Write the detailed comments later today, but Gatos is ready for like being merged today so this week. Sorry, not today, so add the milestone and assign it to me and make sure we have the correct label. So I would say it is a feature and that would be for the executor.

D

And let's double check and what issues disclosing yeah.

C

I think I think it's related to kubernetes. Also so.

D

Good point yes, but we can only have one executed, one executor since exam; ok, scope, clay that yeah. That's a very good point.

B

Really we'll just like about containers I'm sure we do. Does anyone know of a label, they were just gonna pile it. It was like a container topic. No, don't worry about.

D

B

Label we don't want perfect labeling like perfect the word I'm looking for partitioning of our labels, otherwise, well that changes every time we had something well.

D

To be honest, is not just for to talk or executor, it's not even kubernetes, it's also shadow executors and things like that, because we use that for the pot. So I was wrong. Yeah.

D

So let me take a look quickly at what these issues are explaining to make sure that when we merge this, we actually closed around corrupt issues.

D

Okay, so well I'm familiar with this issue, pers, because even for our autoscaler, we have the same problem: characters for host name- and this is cute.

G

D

D

D

I'm not too familiar with RFC, we know trades, be honest.

G

You don't have it memorized by home.

D

I'll take a look at that's the clicker. This would be really boring. Watching me, you're on about RFC's and I hosting, cannot contain and understood jobs. It.

B

Would be a boring, YouTube video? You know people want to call collectively reading RFC quietly.

D

Okay, so these issues do you seem legit like with this fix, so thank you so much for spending time on this.

D

Submit my review when I put in the comment for just one, but let's move on to the next my request and.

D

This one as a breaking change and that we have to release and 13.0.

D

And I see Fabio already dictated so this should be much quicker to review. So first thing would be to check out code locally and before.

D

That's what this is doing.

D

And seated coverage rent by 10 percent, which might be a problem, but we'll look at it why that can be the case? We already have some conflicts.

D

I'll do that at the end of the review.

D

So here we're moving something from cold climate I'm assuming because we're deleting the sprite. Yes.

D

We did back purchase from going south. You remember.

D

And probably that's why the it has coverage went down to 10% because we deleted this dust.

D

So we're no longer depending on this package because we're not using glitters, we expand we're using OS X time. It seems fine.

D

D

Here we are updating the test.

D

Which I forget so since we're removing something we added recently I would go back to the major quest when she wants the changes, because I did. It should be a one to one revert kind of thing.

D

So yes, I think that.

D

Yeah the test has updated because we want to support to go for money, so one thing I will take a look as because I'm, the familiar with this test is take a look at what the test is actually doing.

D

We're expecting khu to be expanded like this.

D

D

Okay, so this basically escaping the dollar sign properly, we'll go one tote, 11 one dot, I'm, sorry.

D

Okay, so this dis comment explains it pretty well, so in 1.11, what changed is that it will actually expand it if the dollar sign is in our way that there's an variable so before it used to expand this, but now it does not expand it to anymore and that's why our test case actually caught it, which is very lucky for us I guess so this does seem. Fine um I, don't have any objections to this.

D

Let's make sure it has the correct label, so it has.

D

Huge contribution, Turtledove, zero.

D

Prove it as well and I'll, ask you to.

D

Solve the conflicts and that pretty much everyone.

D

So I'd get so much about support.

D

So I see churchy already left some comments here, so Fabio maybe to speed this up a bit. Can you tell me what tests Marsh was doing and why it was needed because I don't see any attached issues or anything to it. So.

C

Yes, of course, actually today when, when we used the get some model behavior of the of the brother, you cannot exclude or include as few a few sub modules that you want to to think. So this this merge request adds this capability to the to the runner. So you can just pass the this variable here. The get model path using the same, the same pattern of the Geats model and it schools or includes some sub models. That's basically it okay.

D

So it's a way for us to prevent cloning or thinking of sub modules that we don't want to write.

C

Yes, I have a have a case on my team that we have a bunch of models on a specific variable that we don't want to sink every sub module. You know just a few months.

D

D

One idea that comes to mind, as would the environment, where it will be more here than having the user, which, and in this cases you updating to submergence that you want inside of the script. Part of your github, see. I am at five.

A

C

Sheep good glad I was.

D

Gonna say like: is it more clear to the user that you just put it in the script than trying to figure out how? What how does fireable works? I know it follows kids standards. I can see that, but, like me, I would have to like learn how to do that as a user right sure.

D

Would it make sense like like? Would it be possible for you, for example, to just use the script tag.

C

We are doing this today using the script, but yeah I would like to use the capability of the runner to do the job for us. You know.

G

C

Have to be writing the script and just put some verbal or I, don't know some key on the on the on the globe. Runner, the mo.

F

C

To do that, for me.

D

Yeah I think that makes sense.

D

What I'm afraid of is, we would be adding more friable, but let me see what variables we have currently.

D

There we have these travertine.

D

Take it so much.

D

D

So I think this does make sense to add it for now.

C

They can add some more fuel examples in the documentation. Yes,.

D

For sure- and we would probably I don't think like if you want to do it you're more than welcome, but one thing we would do is we would also have to update this document over here. So because this is the single source of true of all the variables that you can specify to configure the chronic stuff right and that's.

C

Why I think I have yeah I? Think I have done this? Let me check.

D

Giorgia, do you have anything to add to this because I know you already reviewed us.

E

Yeah I think mostly I had to taste it I kind of looked at it yesterday.

E

There should be much left, except from a glance. I saw this sub-module command in the test, which basically does the same on command in the code itself.

E

So I was thinking. Maybe we should try to figure out the way not to implement that on two places, but I think that's, basically it so.

D

You're talking about the spot right.

E

D

Yeah we do have this problem on other tests, so if you look at the abstract transfer, actually do you have this problem. I could not think of any other way to improve it, we're basically having kind of a spellcheck rights for our commands or making sure that we're generating the correct command. We just gave them bad right. It's good to make sure that hey we're passing in the correct flags, it's bad stuff. Every time we have to change the the get command.

D

We need to make sure that our test changes, because it's a one-to-one relationship, so yeah I, don't think we can really improve it. I cannot think of any way. We can really improve it. So you.

B

D

So one thing we would I would like to add: maybe.

D

That link to you the documentation of the sub-module parts because I don't see it in the issue and someone needed to run a corpus. My not saying so. Oh you.

E

Mean the kid dogs yeah it's it's above the gates of model parts function.

G

D

There we go lovely.

C

Yeah but they are not to clear how to use it. Yeah.

D

That's not too much of a surprise from time to time. Unfortunately, so here we're getting the variable.

D

D

Okay, we're getting a if it's empty, we're just getting the empty string here.

D

And for some real Ike, for example, is there a point why we should be passing this I start over just calling it directly here.

D

So instead of creating a type error, variable, don't see it use anywhere else. So maybe you can move this to here.

D

Let's, let's open up in the editor and see if it works good.

D

And the reason I thought of this, as does something we discussed recently in an issue right like how many parameters a function should have, and here we saw that we added another parameter and I know just because I'm familiar with the runner code means that this one is being called twice in two different cases, one with true and one with false.

D

So if we're passing again the same thing and we're not using the same at the module part and where else in the right, sub module, update, CMT's I think we can get rid of heads here and reduce the parameters. So, let's open it up and double check that.

D

F

D

Have to go to check.

D

Then see where I cannot see, the changes might be just mad, that our being.

D

I'm, a better computer. Why I see change here, but I do not see it. My editor, oh.

D

It is because, on your own branch.

D

1838 not 1892.

B

You just want to play on the time we leave about three or four minutes left. Okay,.

D

So one thing that would be as just removing.

C

D

And moving it here, so we don't duplicate the parameters and things like that, but that's magnetic and I'll leave it up to Georgie to finish of the review and then probably assign it to me, who are the final maintainer, merge and review so yeah. That's it for me. Unless anyone has anything and more questions or anything.

A

Thank you for you for not only your contributions but letting us review your your mrs live. Sometimes this is thankfully, it didn't feel like we're putting you on the spot, but yeah.

B

Yeah thanks Steve and Fabio and ring, and everyone for being part of this, and anyone who watches it can benefit from this I think we're still going to a good rhythm doing these Bunkley. So we'll probably do another one around the third week of April based on kids.

B

Thanks again, people.

A

Have any I'm, just one final thought, I'm sure you'll open an issue Elliott on on the next next office. Our people have any questions or again the topics that you want us. You want the team to cover it just feel free to Adam 2d to the issue. Will.

B

A

B

Do that right now, I probably put a terrible data in there yeah.

A

D

And the people can provide feedback that would be appreciated, like even Fabio freaking time. You know like it was super boring for me to like get in your code reviewed or like if I was not clear or something like something. A specific topic like I would.

G

Appreciate any feedback that.

D

I can got you can either be sign it over email or on the office hour and sure sure.

C

But it's it's! Okay! For me, it's very clear and it was cool to see my code being revealed alive.

G

A

Everybody Cheers right.