Decouple standard flavors from flavor naming #319
Conversation
mbuechse
added
the
work in progress
fkr
added
standards
mbuechse
removed
the
work in progress
mbuechse
force-pushed
the
issue/267
branch
3 times, most recently
from
d795f58 to
b52bcce
Compare
| - `scs:name-vN=NAME` (where `N` is `1` or `2`, and `NAME` is some string) means that the | ||
| flavor is one of the | ||
| standard SCS flavors, and the requirements of Section "Standard SCS flavors" below apply. | ||
| - `scs:cpu-type=shared-core` means that _at least 20% of a core in >99% of the time_, |
There was a problem hiding this comment.
Don't we need to provide the full list of allowable and standardized options here?
I.e. crowded-core (corresponding to an L encoded in the name), shared-core (aka V), dedicated-thread (T), or dedicated-core (C)?
There was a problem hiding this comment.
This standard is about standard flavors. Therefore, I only listed the values that we need for the standard flavors.
There was a problem hiding this comment.
While I would also want to skip all the extra things like CPU generation, hypervisor, GPU, IB, I would like us to at least mention the things we touch anyway: cpu-type and diskN-type. Chances are high that variations on those will be offered in the wild and I'd rather not like to ask providers to change things after the fact just because we have not completed all the extra_specs yet when we could have done the obvious ones already.
Anyhow, I am not going to veto this if everyone else wants to push this decision out for later...
There was a problem hiding this comment.
OK, we discussed this in the Team IAM call and I pushed 074b12a as a consequence.
Rationale: Without having the full extra_specs discoverability/transparency thing defined, we have introduced the extra_specs scs:name-v1, scs:name-v2, scs:cpu-type and scs:diskN-type here on the fly. If we do so, let's cover them completely and leave all other extra_specs in a future standard. This avoids confusion for the reader and avoids providers doing wild things that are hard to change once they are out in the wild.
There was a problem hiding this comment.
I don't disagree, but I strongly oppose the breaking of due process here. It takes you a full week to write your comment, and then it can't happen quickly enough, so you push the change yourself. This is very disturbing.
There was a problem hiding this comment.
I did not mean to step on your toes.
But there was a feeling in the Team IaaS that this is ready to be merged with this little change, so I want ahead and volunteered to push it.
I agree that process-wise, this was not ideal.
I am personally not at all working in a way that I feel bad if others contribute to work in branches that I have created and mostly filled with content by pushing to them, but I fully appreciate that this may feel different and we have not spelled out what we consider best practices.
If you feel that the result is bad, please speak up loudly, so we don't merge this.
There was a problem hiding this comment.
I wouldn't have minded a direct contribution if the back and forth hadn't been as sluggish. I had a feeling of being left out, no real cooperation happening, and your commit to the branch was only adding to that feeling.
| standard SCS flavors, and the requirements of Section "Standard SCS flavors" below apply. | ||
| - `scs:cpu-type=shared-core` means that _at least 20% of a core in >99% of the time_, | ||
| measured over the course of one month (1% is 7,2 h/month). | ||
| - `scs:diskN-type=ssd` (where `N` is a nonnegative integer, usually `0`) means that the |
There was a problem hiding this comment.
Likewise, we should spell out the allowable diskN-type settings, no?
network (n), hdd (h), ssd (s), nvme (p) and also mention that is is allowable to advertize NVMes as SSDs (as understatement is allowable).
We could of course reference another standard here, if we prefer so.
(And as we have not yet a complete list of extra specs, maybe we do two sets of them to have the first one ready quickly?)
There was a problem hiding this comment.
See above. Yes, we can go ahead and standardize the extra specs fully, but I would rather do one step at a time. What we need is a proof of concept that discoverability in this way does indeed work.
There was a problem hiding this comment.
I never meant to do all of the extra_specs definitions that we would want.
But the ones that we touch anyway (scs:name-v1, scs-name-v2, scs:cpu-type and scs:diskN-type) feel half-defined, so let's have those complete. And do all the others in a future step.
There was a problem hiding this comment.
Yeah, that comment of mine is obsolete by now. I understand your reasoning, and I don't oppose it.
| flavor_groups: | ||
| - status: mandatory | ||
| list: | ||
| - name: SCS-1V-4 |
There was a problem hiding this comment.
One of the design principles in the old way of listing Mandatory (and Recommended) flavors was to NOT keep long lists of settings in a manually managed YAML file.
Instead we only had the names and had code that derived all settings from the name to generate the verbose form. This had the big advantage to keep the human-managed data short and avoid any divergence between the settings encoded in the file. I would consider a list here with just names. The amount of cpus, ram as well as disk, disk-type and cpu-type and name-v1 and name-v2 should be extracted from the name and generated by code.
The other option would be to have tooling that is included in our CI that validates the input for consistency. I would consider it less preferable over the condensed format, but would not veto it, as the ones who do the main work obviously have to feel comfortable with the design and tooling.
There was a problem hiding this comment.
I strongly oppose deriving the metadata from the name. One of the main principles behind my approach is separation of concerns, that is, avoiding coupling where possible:
- From the point of view of this standard, the names are opaque/arbitrary.
- It is up to the naming standard to check whether the properties and the name are consistent. (Which the script for that standard actually does, except for the extra_specs.)
There was a problem hiding this comment.
fwiw I tend to agree with @mbuechse here, because I find it cleaner to separate code - which does the checks - and data - the flavors to be checked -.
As a middle ground to keep the line count lower I'd suggest the usage of yaml anchors and aliases like this:
- foo: &SCS-2V
- name: SCS-2V-8
cpus: 2
cpu-type: shared-core
ram: 8
- foo: *SCS-2V
- name: SCS-2V-4-20s
ram: 4
disk: 20
disk0-type: ssd
name-v1: SCS-2V:4:20s
name-v2: SCS-2V-4-20s
- foo: *SCS-2V
- name: SCS-2V-16
ram: 16
name-v1: SCS-2V:16
name-v2: SCS-2V-16:edit: to make the difference more clear here are the original definitions:
- name: SCS-2V-4-20s
cpus: 2
cpu-type: shared-core
ram: 4
disk: 20
disk0-type: ssd
name-v1: SCS-2V:4:20s
name-v2: SCS-2V-4-20s
- name: SCS-2V-16
cpus: 2
cpu-type: shared-core
ram: 16
name-v1: SCS-2V:16
name-v2: SCS-2V-16There was a problem hiding this comment.
I strongly oppose deriving the metadata from the name. One of the main principles behind my approach is separation of concerns, that is, avoiding coupling where possible:
- From the point of view of this standard, the names are opaque/arbitrary.
- It is up to the naming standard to check whether the properties and the name are consistent. (Which the script for that standard actually does, except for the extra_specs.)
Fair enough.
Some consistency checks should be done as CI job then I believe, because invariably we will screw up sooner or later otherwise.
There was a problem hiding this comment.
And I'm fine with pushing this consistency check out into a new issue, so we can get this PR merged now.
There was a problem hiding this comment.
OK, I have opened #339, so we can move forward here.
| url: https://raw.githubusercontent.com/SovereignCloudStack/standards/main/Standards/scs-0100-v3-flavor-naming.md | ||
| check_tools: | ||
| - executable: ./iaas/flavor-naming/flavor-names-openstack.py | ||
| args: "--mand=./iaas/scs-0100-v3-flavors.yaml" |
There was a problem hiding this comment.
So we call the script twice and it only tests one of the aspects each time?
0103: Is the list of mandatory SCS flavors complete?
0100: Are the names starting with SCS- all according to the syntax rules and do the discoverable properties match what the names claim?
The scripts has previously done both -- separating concerns here is good.
(Obviously in CI testing, cycle time is an important goal, so running the script only once doing both tests is an option if cycle time would be heavily impacted. This test is rather fast, so the cleaner separation of concerns wins over performance optimization ...)
There was a problem hiding this comment.
Right, see #319 (comment)
(except we are talking two separate scripts, not one script being called twice)
|
ping @frosty-geek @jnull - please review (since you belong to the 'crowd of the CSPs') |
| flavor_groups: | ||
| - status: mandatory | ||
| list: | ||
| - name: SCS-1V-4 |
There was a problem hiding this comment.
fwiw I tend to agree with @mbuechse here, because I find it cleaner to separate code - which does the checks - and data - the flavors to be checked -.
As a middle ground to keep the line count lower I'd suggest the usage of yaml anchors and aliases like this:
- foo: &SCS-2V
- name: SCS-2V-8
cpus: 2
cpu-type: shared-core
ram: 8
- foo: *SCS-2V
- name: SCS-2V-4-20s
ram: 4
disk: 20
disk0-type: ssd
name-v1: SCS-2V:4:20s
name-v2: SCS-2V-4-20s
- foo: *SCS-2V
- name: SCS-2V-16
ram: 16
name-v1: SCS-2V:16
name-v2: SCS-2V-16:edit: to make the difference more clear here are the original definitions:
- name: SCS-2V-4-20s
cpus: 2
cpu-type: shared-core
ram: 4
disk: 20
disk0-type: ssd
name-v1: SCS-2V:4:20s
name-v2: SCS-2V-4-20s
- name: SCS-2V-16
cpus: 2
cpu-type: shared-core
ram: 16
name-v1: SCS-2V:16
name-v2: SCS-2V-16
Signed-off-by: Matthias Büchse <[email protected]>
Signed-off-by: Matthias Büchse <[email protected]>
Signed-off-by: Matthias Büchse <[email protected]>
Signed-off-by: Matthias Büchse <[email protected]>
…s-openstack.py (executable) Signed-off-by: Matthias Büchse <[email protected]>
…u-type instead of scs:vCPU-type Signed-off-by: Matthias Büchse <[email protected]>
Signed-off-by: Matthias Büchse <[email protected]>
Any flavor that carries any of these must also have the corresponding semantics, otherwise no discoverability Signed-off-by: Matthias Büchse <[email protected]>
Signed-off-by: Matthias Büchse <[email protected]>
…nce v2 Signed-off-by: Matthias Büchse <[email protected]>
Signed-off-by: Matthias Büchse <[email protected]>
…lementation unconformant Signed-off-by: Matthias Büchse <[email protected]>
…s yaml file Signed-off-by: Matthias Büchse <[email protected]>
Signed-off-by: Matthias Büchse <[email protected]>
Signed-off-by: Matthias Büchse <[email protected]>
Signed-off-by: Matthias Büchse <[email protected]>
Define cpu-type and diskN-types and cross-reference flavor-naming. Mention live-migration consequence. Signed-off-by: Kurt Garloff <[email protected]>
Resolves #267