Fix priority class flag use #1031
Conversation
|
1. Testing on ocp esx When set "use_system_node_priority_class: True", then priority class system-node-critical is set for controller, host, openvswitch pods. When did not set any flag related to priority class, then priority class acicni-priority is set for controller, host, openvswitch pods 2. Testing on k8s hyperflex When set "use_system_node_priority_class: True", then as expected, priority class system-node-critical is set for controller, host, openvswitch pods When did not set any flag related to priority class, then priority class system-node-critical is set for controller pod and priority class system-cluster-criticial is set for host, openvswitch pods |
|
Since the PR would be merged after 6.0.3.2 release, marking it as draft PR |
pkharat
force-pushed
the
fix-priority-class-flag-use
branch
3 times, most recently
from
76ca0f1
to
f82db32
Compare
| @@ -2442,7 +2442,7 @@ spec: | |||
| {% if config.kube_config.use_system_node_priority_class %} | |||
| priorityClassName: system-node-critical | |||
| {% else %} | |||
| {% if not config.kube_config.no_priority_class %} | |||
| {% if config.kube_config.no_priority_class %} | |||
There was a problem hiding this comment.
Can you explain why this change is needed - here it is saying that the no_priority_class is true we use system-cluster-critical but ideally when it is false then we use the priorityclass. Is this a valid check ?
There was a problem hiding this comment.
Purpose of this flag is to set priority class for k8s platform.
- Either we can continue to use the same name (to go with minimal changes) or
- Use different flag name and replace 'no_priority_class' flag with the new name at all the places in
k8s etc flavors / wherever required
There was a problem hiding this comment.
@pkharat - the question is not about name - no_priority_class , its about the if check logic. The earlier logic was mentioning that if no_priority_class field is not mentioned in kube_config we use the default priorityClassName system-cluster-critical . However now we are making no_priority_class field a mandatory field in kube_config. So in case we don't mention anything e.g. use_system_node_priority_class/no_priority_class/use_acicni_priority_class we will end up with no priorityClassName, which breaks the original functionality, correct?
There was a problem hiding this comment.
@abhis2112 Only use_system_node_priority_class is expected from user, other two no_priority_class and use_acicni_priority_class are coming from flavors.yaml and its always going to be present.
When a new flavor is added it's expected and mandatory to add no_priority_class(for k8s) / use_acicni_priority_class (for ocp). If it's not added then it's an error and must be resolved during the flavor addition itself. Other than this I don't see any issue.
If I am correct, you are suggesting we add a default priority class if no option provided at all (we have to decide which one is common across all platforms), but do we really need that with above mentioned approach ?
There was a problem hiding this comment.
so if I deduce it right , in case of k8s flavor if the mandatory field is added (no_priority_class) then according to old logic the if check will always be false since no_priority_class will always be present as per flavor then we will end up with no priority class name configuration.
With the new change, the logic makes it default - with no_priority_class (always set) use system_cluster_critical priorityclassname . So the question to you is - is there a default priorityclass earlier for k8s or is it expected with this change
There was a problem hiding this comment.
There is no default priority class earlier also.
Logic used to set priority class for k8s and ocp flavours is little confusing.
-
for k8s - not adding flag
no_priority_classis the wayflavors.yaml ->
aci-containers.yaml (https://github.com/noironetworks/acc-provision/blob/master/provision/acc_provision/templates/aci-containers.yaml#L2474)
{% if not config.kube_config.no_priority_class %}
priorityClassName: system-cluster-critical
{% endif %}
-
for ocp - setting both
no_priority_classanduse_acicni_priority_classis the wayflavors.yaml ->
aci-containers.yaml (https://github.com/noironetworks/acc-provision/blob/master/provision/acc_provision/templates/aci-containers.yaml#L2477)
{% if config.kube_config.use_acicni_priority_class %}
priorityClassName: acicni-priority
{% endif %}
Setting both the flags to determine ocp flavor and then not setting any flag to determine k8s flavors is confusing. So straight forward logic should be you set the flag itself for corresponding flavor.
1) Use priority class "system-node-critical" when flag 'use_system_node_priority_class: true' is configured in acc provision input file 2) Use priority class "acicni-priority" when flag "use_acicni_priority_class: true" is set and this is set by default in flavors.yaml for OCP. Remove redundant flag "no_priority_class:True" from all OCP flavors where already "use_acicni_priority_class: True" is set. (Exception - openshift-3.9, openshift-3.10, openshift-3.11 has only "no_priority_class:True" so not touching those flavors) 3) If priority class flag is not set in acc provision or flavors.yaml then internally its "no_priority_class: true" and accordingly priority class is set for pod aci-containers-host, aci-containers-openvswith -> "system-cluster-critical" pod aci-containers-controller -> "system-node-critical"
pkharat
force-pushed
the
fix-priority-class-flag-use
branch
from
0866834
to
2f397e2
Compare
No description provided.