perfprof: add enablement annotation #1278
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ffromani The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
ffromani
force-pushed
the
llc-enablement-file-from-experimental
branch
from
c1fcd0f to
5319763
Compare
ffromani
force-pushed
the
llc-enablement-file-from-experimental
branch
2 times, most recently
from
79d1742 to
50243cb
Compare
ffromani
force-pushed
the
llc-enablement-file-from-experimental
branch
from
50243cb to
c29a442
Compare
| Expect(ok).To(BeFalse(), "expected path %q found in ignition", expectedPath) | ||
| }) | ||
|
|
||
| It("should not be generated if missing control annotation", func() { |
There was a problem hiding this comment.
You should also check that the prefer-align-cpus-by-uncorecache is false in the final KubeletConfig. Otherwise the kubelet dies.
There was a problem hiding this comment.
we did a different test for this though, and I'm not sure this in scope with this PR.
| klog.V(4).InfoS("components manifests", "autogenerateEnablement", autogen, "LLCEnabled", llcEnabled) | ||
|
|
||
| mcOpts := opts.MachineConfig.Clone() | ||
| mcOpts.LLCFileEnabled = autogen && llcEnabled |
There was a problem hiding this comment.
What we need is also - when autogen == false force llcEnabled to false.
Then not generating the file is cleaner, but makes no functional difference to kubelet.
There was a problem hiding this comment.
When you enable the feature in Kubeletconfig without the trigger file, kubelet fails to start with the unknown config error.
There was a problem hiding this comment.
ok, this means we need a different feature then. Is no longer a simplification but rather a master switch. At this point we should evaluate if we should add to our APIs and take full ownership of the flag.
There was a problem hiding this comment.
It is both. I view it as simplification that at the same time prevents human error that is hard to recover from. We can either develop a master switch or if you like just not apply the setting when both pieces are not in place and report an error.
But allowing the unsafe combination is going to backfire. In fact, it has already.
In order to use the cpumanager policy option `prefer-align-cpus-by-uncorecache`
users need to create an enablement file through a MachineConfig.
This is because the feature is opt-in and supported only on selected
cases. By providing MachineConfigs, users explicitely opt in and this is
pretty evident on support scenarios.
Problem is: the required MachineConfig has error prone components.
We would like to streamline the flow but keep the explicit opt-in step.
A possible improvement is to automate the MachineConfig generation when
the cpumanager option is injected through the
`kubletconfig.experimental` annotation, which is currently the only
supported way to enable this feature.
To keep the opt-in component, users would also need to supply another
annotation:
```
performance.openshift.io/autogenerate-enablement: "true"
```
So the performance profile could look like
```
annotations:
"kubeletconfig.experimental": "{\"cpuManagerPolicyOptions\":{\"prefer-align-cpus-by-uncorecache\":\"true\"}}"
"performance.openshift.io/autogenerate-enablement": "true"
```
Signed-off-by: Francesco Romani <[email protected]>
ffromani
force-pushed
the
llc-enablement-file-from-experimental
branch
from
c29a442 to
998e392
Compare
|
/retest-required |
1 similar comment
|
/retest-required |
|
@ffromani: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
In order to use the cpumanager policy option
prefer-align-cpus-by-uncorecacheusers need to create an enablement file through a MachineConfig.This is because the feature is opt-in and supported only on selected cases. By providing MachineConfigs, users explicitely opt in and this is pretty evident on support scenarios.
Problem is: the required MachineConfig has error prone components.
We would like to streamline the flow but keep the explicit opt-in step.
A possible improvement is to automate the MachineConfig generation when the cpumanager option is injected through the
kubletconfig.experimentalannotation, which is currently the only supported way to enable this feature.To keep the opt-in component, users would also need to supply another annotation:
So the performance profile could look like