CGRU / cgru

CGRU - AFANASY
http://cgru.info/
GNU Lesser General Public License v3.0
278 stars 111 forks source link

Simple hosts masks. Question. #604

Open timurhai opened 6 months ago

timurhai commented 6 months ago

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?

sebastianelsner commented 6 months ago

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 commented 6 months ago

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

sebastianelsner commented 6 months ago

Sounds good!

timurhai commented 1 month ago

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 commented 1 month ago

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

timurhai commented 1 month ago

Yes, default separators are |;,: https://github.com/CGRU/cgru/blob/master/afanasy/src/libafanasy/name_af.h#L151`

timurhai commented 1 week ago

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 commented 1 week ago

Yes, I agree.

sebastianelsner commented 1 week ago

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

timurhai commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

timurhai commented 1 week ago

May be, job depend masks are regex only.