GitLab GitLab Runner Open Office Hours, 7 Aug 2020

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: GitLab Runner office hour (August, 2020)

Description

Steve Azzopardi from the GitLab Runner team reviews two merge requests (MRs) from the wider community.

A

Hi, my name is ray pake. I manage a code contributor program at get lab and this is the runner office hour for august. um So I think I'll just turn things over to steve, who has a number of merger quests that he proposed to review. uh Let me paste the issue here where that discussion is taking place and uh steve I'll turn things over to you.

B

Thank you um so, since alexander is in this call, let me first share my screen.

B

Let us review seconds to close itself.

B

That is better, so uh correct me if I wrong, but this is your merge request right, alexander.

C

Yeah, that's right.

B

Perfect, um so, to give some background to anyone watching this or people who are not at work, um we have to keep paradise executor and kubernetes has a lot of ways to say this spot should be scheduled.

B

Node the spot should take priority over teams and things like that and a few months back. Actually, a year ago, we went to everything that we have like so kubernetes have these node selectors total variations, node affinity, port affinity and we went through which one we support and which ones we don't and alexander was kind enough to start work on the note affinity for from the conflict of tongue. Part. uh Am I understanding correctly alexander.

C

Yeah, that's right. I also looking for port affinity, but I focused right now on node affinity, perfect.

B

um So that that's already like uh plus one like we don't uh have one merchandise trying to do a lot of things so that, like focusing on one thing, is very, very nice.

B

um So uh as a disclaimer like, I do not have a lot of knowledge of uh note affinity, so I did not put a lot of research beforehand, but um since this is still early days of the merge request, we can just review it from a higher point of view.

B

So since this is a config change, first thing I would like to look is at the conflict 25 here we are adding a new affinity keyword and we're calling it kubernetes uh affinity, which is fine the command. So here we're specifying that's the key for summons affinity, json.

B

So one suggestion I have is, you may be.

B

We have a mixture of affinity and node affinity right like, for example, tunnel, there's affinity. Jason is not infinity.

C

If I'm, if I might interrupt you, um yes, that's that's true. Like um most of the comments there are not like finished, because I really would like to have some feedback on the overall approach, because this affinity structure from kubernetes is a quite complex one. I think it's one of the complex that is there are there in the code. So yes, like there are, is some changes between underscores camera case and stuff. So I'm sorry for that, but yeah.

B

You don't have to be sorry, I'm only pointing them out, so we like, we remember to fix them in the future, even if we don't fix them right now but uh like if we just leave a comment on them. We'll remember in the future that we have to fix it. So I'm just gonna leave one small comment. We don't have to fix it right.

D

B

It's just so uh we don't forget. First I'll, probably forget myself.

C

Yeah, that's what I meant with the mixture. This is the fin. Of course, it wraps not only the node affinity, but also the pod affinity, which you can see in line 300 and 301 like there are two um yeah like structures missing there, which is part affinity and another one. I think it's anti-affinity, which could also be extended to the affinity.

B

uh Okay, so what you want, what you were gonna do is basically have affinity as the main one and then you'll.

C

B

Okay, now I understand okay, okay, that makes that makes more sense, then than this comment. Okay. So let's remove that for now. Okay,.

B

Now I understand it: okay, so uh this one.

E

B

I'm guessing these are just.

B

B

So just one thing that, like yeah, I sometimes struggle the most with uh community contributions is getting the knowledge from uh some feature that I'm not aware of so okay, um so we have affinity. Okay. That makes that this is even better because even kubernetes splits it into affinity and no different t so that that follows kubernetes structure, even better nice work on that uh so required and preferred that is awesome.

B

And now we are using the node selector here.

C

Yeah, so basically the naming is um copied from the api include. So the api include of the kubernetes api already. Has this structure and I yeah like copied it and add the wrappers to yeah, convert it from the structure here to the kubernetes one, because this was one of my of my hardest struggles like there's a lot of copy paste from the api from kubernetes.

D

C

But in any other place in the config file itself, it was also like copied, even if there was a matching structure in the kubernetes api yeah.

B

And we do that for a specific reason right: um the kubernetes api changes from time to time um new structures are added and so on and so forth.

B

uh If we have our own structure, yes, it's a bit of a maintenance burden from our end, but then we can have control of the config tumbler file because we, as the runner team, have to be very aware of any breaking changes or any new flags that come in that we want or don't want to support, and things like that. So.

E

B

I do understand it's a bit awkward, copy-pasting these structures, but uh I think, having that is better than just having the conflict of domain out of our country.

C

Yeah, I thought so that's why I copied it and changed all the comments to tomorrow, but so, basically, all the the names for the structures are the same ones like in the kubernetes api.

B

B

E

B

What selector returns.

B

So this terms, okay,.

E

E

B

So I'm guessing these are all from the operator here. um If, when you remember correctly, we do have some.

B

Notes tolerations like this as well. Let me see how we do.

D

D

B

B

Okay, so it's just.

B

B

Okay, so for the tolerations, for example, the operators itself, we actually use the api one. So if we look at uh gets note tolerations here, we use the api one, and that makes sense because at the end of the day, we don't want to duplicate that. We only want to duplicate the configuration, but.

E

C

You can have a look for a node selector operator because I use it to cast. I.

E

E

B

Okay- and we want this- we want to send this to the uh a kubernetes api right.

C

C

Right but this is in the kubernetes part of the.

B

Unit so like in the.

C

Community's executor, um I use this structure to to cast it from the input configuration file to a value and then use it in the getter function to cast it to the kubernetes enum.

C

It should be in the conflict too. I think it's somewhere in the in the conflict.

B

B

We are updating to be required here, depending on.

E

Guest nonsense: okay,.

E

So we're looking.

B

Through all the.

E

E

E

B

I'm just trying to uh get the grasp of the translations.

C

Yeah and there in the uh I don't see the line number, but in the operator api, node selector operator. This is the um the enum or the structure that matches which uses the key in and inside axis and stuff. So.

B

To support your version.

D

B

This will panic right so.

B

If you try to cast something, no, that is type assertion, so give me one second, if we do, for example,.

B

B

Applications- okay, it it would be completely different because, yes,.

A

B

Casting is will not panic, so many type of solutions and.

D

This is just a stream.

B

So it works just.

E

E

B

Selected there's one thing I wish we had: the conflict of thunder file is getting a bit too big enough. If configures it's in its own package, we could have this uh concealed in one file for just uh affinity spots. That's.

B

B

So to answer your original question regarding, if the configuration looks fine, I would say so: yes, um at the end of the day, what we want to do is what you are doing right, it's taking the thomas structure and transforming into transforming it into uh the um kubernetes api request right. um I'm.

D

B

uh Petrol or adrian have any concerns about that or suggestions. um I know in the past we discussed how we can prevent this like. It seems like it's a lot of work for us to keep translating kubernetes uh kubernetes api to uh tunnel file structure for our conflict of tunnel. There were some ideas, but none of them really panned out.

D

B

We like so this seems to be what we need. I to be honest, I do not see the need for these um I mean we can.

A

B

Cut to the to the cube and this one directly.

B

Let me double check my.

B

D

B

I don't think we really need this one, I mean if it's just a string um and then we could type casted. So, for example, here we are.

B

Here we are just asserting it's uh uh I mean so there's no need to die pass it here and even.

C

One one question about that: if, uh if, if we go with string, I I get your point and if we go with string, then the runner would start without an error. But when he wants to spawn, he has to cast the string to the api node selector operator, and then he would crash. Wouldn't he.

B

um I think it would happen the same way.

B

It would be the same with or without it. So, let's test out your theory so to confirm it. So, uh let's open up a jdk conflict and I have microvanities executed here right.

B

So if you have runway, you cannot use affinity.

B

C

I think you could also test it with the kubernetes unit test, because I think I have that in the unit test itself, I have unit test for the kubernetes runway. He tries to cast it. I think.

B

Okay, so are you talking about the conflict abdomen or in the executor tests.

C

The kubernetes executor unit.

B

Okay, yes, there we go nice part.

E

B

So if we do this, for example like this right.

B

Because what we're doing here in this part you're creating a new type called no selector, which is of type string right and here you're, just that we're just creating a bunch of constants which are type of node selector operator. But at the end of the day they are still strings like if we have.

B

Even if we have these constants like we can still do.

B

You can still do this so, for example, uh let's let's do it here right, so node, selector.

B

Operator uh asdf, for example, and that would still be valid.

B

Still valid go code and it will still print properly. uh So, even if we have our own type with these questions defined, it does not mean it's. It's not really an enum. So if you're familiar with any other programming languages like c sharp or something like that, like this you're, not defining enums you go, go does not have enums actually.

C

Yeah, okay, yeah.

B

uh So we're here we're just defining constants of this type with this value, so we're not preventing anyone from generating new types here.

B

So we can remove this part, and then we just rely as a string and then um even the kubernetes will do the typecasting and it will work fine, it's only the api when we actually send the request to the computer's api. The validation will happen on the api level, and so it will not happen on the type customer.

C

Okay, so I will remove.

C

C

E

B

Apart from that,.

B

So I'll I'll take a look at these properly of language yeah. It is just a bunch of translations, um it would be just.

E

B

I really appreciate it that you added tests on both spikes that you touched on the concrete on the configuration.

B

Which we're doing.

B

So again, I do not think we need these typecasts here and are actually so. We can improve this a lot. So, let's go to this.

E

B

So, since we're using the testify package, uh it actually has a bunch of hyper functions where we can do a certain here and just do parts.

B

And we just wanted to so it's a little bit easier to read. uh It's mostly a network right.

B

You just should just work this one, because by default, control is.

B

B

B

So here we are testing, hey computers, conflicted atomic file and the api request should look like.

E

E

E

E

B

Let's see they'll.

E

B

These are small, really small changes, but mostly our programmability.

B

um To be honest, it looks really really good.

B

The only thing that I'm gonna I have to look at is that all this translation, but it does seem to be fine and then I'm gonna, look at the.

B

That seems to do you have any uh questions or concerns.

C

No, I'm fine, I'm happy for the review.

B

Okay, um so the pipeline is only playing because of stuff just out of previously um okay, it's just.

C

Yeah, like characters, linting and stuff.

B

Okay, that is fine. um We can fix that later, but yeah. That seems really really good. From my point of view and.

B

I'll, probably uh like, after this meeting, I'm ending my day for some application tomorrow, so I'll probably take a look at the translations. Next week, I usually review community contributions on friday.

B

I'm not sure if I can try and do it earlier.

B

So I really really apologize, but maybe someone else from the team will look at it with the pedro or somewhere else can particularly get the translations to swim through kids.

B

Any other concerns pedro and adrian.

E

B

So thanks a lot, you want to discuss anything else or do you have any other questions, even if it's not about this merchandise,.

C

No, I think, I'm fine. I know what to do, and I know that I have to add the comments and because I use it in my in my cluster already, I think I will fix it up and wait till you review it again after you apply suggestions, perfect.

B

A

Thanks alexander, for not only for your contribution but also being on the call- and it's sometimes it's nerve-wracking, getting your work reviewed uh like live, but thanks for thanks for doing this.

C

Yeah no problem um as a suggestion I it was only luck that I found the the meeting here, um because I I got in the mentioning under my merch request that there is a mentioning of this issue in another issue and otherwise I wouldn't have been here yeah exactly like.

B

uh It was this one uh yeah that was, uh I.

C

Don't see this one.

B

That is my fault. uh I prepared last minute for this, so I just looked at the new community contributions that came in the past two three days and just said: hey we're gonna review those, but I are you're right. I should have like uh announced it properly as it's like that, I will review it and the office hours. um Probably what I should have done to me with my idea is uh thank you directly in the measurements, and I will tell you hey where we'll be doing this in the office hours, if you're enjoying.

C

Yeah no, thank you very much. Then I will leave now and we'll do the rest of the commit. Thank you very much. Thank.

B

B

E

E

B

B

A

It looks like the other two are from first time contributors to right, the ones on your list.

A

I don't know if any of these are like quick ones that you can review in like 15 minutes.

B

uh No um well the odd first one might be, but I need to do some research.

B

Executor we can define um tokens which are basically a way to authenticate within api now, what I'm not sure about if we actually.

E

E

B

B

File, I think the change makes sense, because if these are already overwritten, we should we should.

B

Understand it is actually.

B

B

Okay, what I'm only afraid of like the change is super super simple and we can probably merge it. What I would want to do is uh actually test it, because if we end up emptying the token file uh right now, how is it going to be?

B

Is it going to completely debate, or is it going to use the token file uh instead of the bare token, if that is the case, it might be a small breaking change um for missed computer drivers, because uh the user thoughts that there's an independent token, but it's actually using uh the very token file which might.

E

Change the permissions so.

B

I can't test this.

E

B

Ellipse actually also opened an issue right in this command. We're saying host here, but we're still building for each architecture.

D

E

B

E

B

So I'm just enabling.

E

E

B

And the reason we need experimental mods is because we're using the two cause products which allows us to build structure, images from multiple.

E

E

E

B

So even a simple line change takes a few minutes to actually test out properly and some assumptions without having things like that, which is a bit unfortunate for the contributor, but I'd much rather move slow than.

B

B

E

E

E

E

E

E

B

Yeah so reading this line here, the last successful value takes precedence over here.

B

That means that if the third token that is available in the cluster has more permissions uh than the third token that is configured in the conflict of time file, it will start one not even if it's, if there's a bomb there, and it can be a breaking change, because if this token file has more permissions, it might be the case that some ci job works as.

A

B

While, if we just do this, we'll be using the actual barrier, token that's complicated in the comfort zone, which might has less might have less perfection, so it might make the user what.

B

To prevent the braking change, uh the best thing to do is add a feature back, so uh we can have a feature, connect turned on by default um and if the user has some good setup, they can just turn on the project like what and then they will fix it on time and then we'll duplicate it and you leave it in 14.0 uh adrian.

B

Does that sound like a valid concern, or am I just thinking.

D

B

B

Okay, so I'll read.

B

E

E

B

Let's roll with.

E

B

Boring and tricky to be honest,.

E

B

I think that's it for me.

A

Yeah, it's like a four line. Changes could involve a lot of things to look at. I guess it's a good illustration cool. Well, yeah thanks, for I mean we managed to review two, mrs, which is good, um I think. As for the next call, we have a hackathon in about a month, uh so I think I'll try to schedule uh on the day one of the hackathon on wednesday september, 2nd I'll put that on the calendar, but also thanks for everyone for joining I'll, see you folks again next month, if not soon, bye.