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

Simple hosts masks. Question. #604

Closed
timurhai opened this issue May 11, 2024 · 15 comments
Closed

Simple hosts masks. Question. #604

timurhai opened this issue May 11, 2024 · 15 comments
Assignees
Milestone

Comments

@timurhai
Copy link
Member

I think it will more handy if consts mask will use "contain" not "match".
If "contain" you can just write my-blade and not my-blade.* to render on all my-blade-###.
It is more user-friendly, I think.

In most cases regular expressions are not needed.
We can just "find" some string, and not run heavy std::regex::match function.
It will work tens times faster.

We can add an option, use "regex" or just "find". It can be a user or (and) job parameter.

Also we can use "find" with "split" by "|" (or just space or comma):
my-blade|his-blade will run on my-blade-### and his-blade-### without regular expressions.

What do you think?

@timurhai timurhai added this to the 3.4.0 milestone May 11, 2024
@timurhai timurhai self-assigned this May 11, 2024
@sebastianelsner
Copy link
Contributor

Yes, this is mostly how we use hostmasks, BUT we have a few cases where advanced users actually use regexes to pick out specific hosts they want. So, if you make this a global config flag, we will need to leave this to the more complex regex setting until we find a a good way to make all the users happy.

I think this is valuable but for us it comes with a long migration path.

@timurhai
Copy link
Member Author

Job can have a parameter (flag) use complex regex or simple "find".

@sebastianelsner
Copy link
Contributor

Sounds good!

timurhai added a commit that referenced this issue Oct 14, 2024
Now find function is used to match hosts mask and hosts exclude mask.
This is the default behaviour for user, job and branch.
You can configure user, job and branch to use Regular Expressions.
If a new job does not have hosts_mask_type parameter, user host mask
type will be used.

References #604.
@timurhai
Copy link
Member Author

Now find function is used to match hosts mask and hosts exclude mask.
This is the default behaviour for user, job and branch.
You can configure user, job and branch to use Regular Expressions.
If a new job does not have hosts_mask_type parameter, user host mask
type will be used.

ps
Should be (and will be) tested on a real work.

@sebastianelsner
Copy link
Contributor

Nice, does it already include the splitting at "|"?

@timurhai
Copy link
Member Author

@timurhai
Copy link
Member Author

Hello!
We are already testing 3.4.0 with a new hosts mask for about 2 weeks.
No bugs found.
Now find tries to find mask anywhere in a string: find(...) != std::str::npos.
But i think that it should be better to find only in the beginning of a string: find(...) == 0.
For example we some machines named ll-###, and use ll mask to match it.
Then some users got some small machines named some-name-small.
And a mask ll starts to work incorrectly.
So I think that hosts mask find behavior should find mask only at the beginning of a name.

@sebastianelsner
Copy link
Contributor

Yes, I agree.

@sebastianelsner
Copy link
Contributor

We have not tested yet, but will definately. Are you seeing performance improvements in prod?

@timurhai
Copy link
Member Author

No. As we are using hosts masks not so often just in some special cases.
We are always using hosts masks on some branches, but it is just a few special branches.
But I std::string::find() is definitetely very much faster than std::regex::match().

ps
Also, some times not so technical experienced users need to use hosts masks.
They very often miss .* at the end.
Miss that it is a regular expression and thinks that hosts mask is just the beggining of a name (exactly like find() == 0).

@sebastianelsner
Copy link
Contributor

I was testing this yesterday. It worked great. Thank you.

I noticed, that our CPU usage did only go down slightly. Then I understood, this is because of the dependency masks. We have lots of Block dependency and job dependency masks like: block1|block2|block3 and job1|job2. These are regexes as well. We do not use something like block-render.*|block3. So, would it be possible to implement this "find" with splitting at "|" for dependencies as well?

@timurhai
Copy link
Member Author

Are you sure that block masks slows afserver?
Block masks are calculated once, on job registration and mask update.
After calculation Block class stores ids and on solve uses ids only, w/o regex::match.

@sebastianelsner
Copy link
Contributor

Hm ok, this makes it unlikely then. Could it be Job depend masks or global job depend mask?

@timurhai
Copy link
Member Author

May be, job depend masks are regex only.

@timurhai
Copy link
Member Author

It works fine for us. Issue can be closed.

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

No branches or pull requests

2 participants