feat: Bug OCPBUGS-13301: add support for cpu partitioning on site config #1441

eggfoobar · 2023-03-21T16:34:18Z

add new flag for cpu partitioning mode
add deprecation warning for situations where cpuset is defined for nodes

openshift-ci · 2023-03-21T16:34:30Z

Hi @eggfoobar. Thanks for your PR.

I'm waiting for a openshift-kni member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

eggfoobar · 2023-03-21T16:36:29Z

@imiller0 Would love feedback on the deprecation warning, not sure if you feel it's too much. Was of two minds if it made sense to only apply it if both cpuset and cpuPartitioningMode are defined or weather we do a blanket warning to give plenty of heads up time for older installs.

ztp/siteconfig-generator/siteConfig/siteConfigHelper.go

imiller0 · 2023-03-27T22:38:46Z

/ok-to-test

imiller0 · 2023-03-27T22:39:41Z

@serngawy @sabbir-47 can you please review

ztp/siteconfig-generator/siteConfig/siteConfigHelper.go

ztp/siteconfig-generator/siteConfig/testdata/siteConfigTestOutput.yaml

serngawy · 2023-03-31T14:57:22Z

ztp/siteconfig-generator/siteConfig/testdata/SplitAndMergeMachineConfigExpected/mergeMC.yaml

@@ -16,6 +16,7 @@ metadata:
        agent-install.openshift.io/install-config-overrides: '{"networking":{"networkType":"OVNKubernetes"}}'
        argocd.argoproj.io/sync-wave: "1"
        ran.openshift.io/ztp-gitops-generated: '{}'
+        ran.openshift.io/ztp-warning/field-deprecation/cpuset: siteConfig.clusters[].node[].cpuset will be deprecated after OCP 4.15, please use siteConfig.clusters[].cpuPartitioningMode for OCP versions >= 4.13


cpuPartitioningMode does not exist in the install-config-overrides but the warning annotation get generated ?
I think warning annotation get generated if the cpuSet is used otherwise no need for it

Yeah, since we won't know the version the customer chooses for openshift, figured if they include a cpuset we notify them that for 4.13 onwards they need to switch over to the new method.

serngawy · 2023-03-31T15:04:47Z

ztp/siteconfig-generator/siteConfig/siteConfigHelper.go

@@ -273,3 +336,49 @@ func mergeJsonCommonKey(mergeWith, mergeTo, key string) (string, error) {
 	}
 	return string(newJson), nil
 }
+
+func applyWorkloadPinningInstallConfigOverrides(clusterSpec *Clusters) (result string, err error) {


As far as I understand this func to replace the use of getWorkloadManifest is that correct ? if yes we need to remove the machineConfig OR the machineConfig for workload will till remain ?

The siteConfig tool (binary) can be used to deploy clusters on prior releases as well. This new feature shows up in 4.13, so if the user is deploying a 4.12 cluster they will need the MachineConfig, but if they are deploying 4.13 they need this annotation (and do not need the MachineConfig)

I believe that if the user leaves "cpuset" out of their config we will automatically leave out the workload partitioning manifests. We will need to update the documentation to note that in 4.13+ the user specifies cpuPartitioningMode and not cpuset.

serngawy · 2023-03-31T15:06:21Z

ztp/siteconfig-generator/siteConfig/siteConfigHelper.go

+	const (
+		deprecationCR      = "AgentClusterInstall"
+		deprecationField   = "cpuset"
+		deprecationMessage = "siteConfig.clusters[].node[].cpuset will be deprecated after OCP 4.15, please use siteConfig.clusters[].cpuPartitioningMode for OCP versions >= 4.13"


better to have min message like "cpuset is deprecated instead use cpuPartitioningMode"

I minimized it but left the version information since we won't know which version the user chooses, would that be okay?

serngawy · 2023-03-31T15:12:19Z

The ci job required the commit message to start with ztp

imiller0

Along with the changes to siteConfig generator, we need to update the reference siteconfigs to make use of the cpuPartitioningMode instead of cpuset. The PerformanceProfile in the reference configurations already set the cpu appropriately so only the SiteConfig CRs need to be updated.
https://github.com/openshift-kni/cnf-features-deploy/blob/master/ztp/gitops-subscriptions/argocd/example/siteconfig/example-sno.yaml
And the 3NC and standard clusters in the same directory.

add new flag for cpu partitioning mode add deprecation warning for situations where cpuset is defined for nodes Signed-off-by: ehila <ehila@redhat.com>

Signed-off-by: ehila <ehila@redhat.com>

imiller0 · 2023-04-24T22:06:36Z

/retest

Signed-off-by: ehila <ehila@redhat.com>

eggfoobar · 2023-04-25T21:15:50Z

@imiller0 and @serngawy Tests are passing now, would mind giving this another look?

imiller0

/lgtm

openshift-ci · 2023-05-02T21:53:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eggfoobar, imiller0, serngawy

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:

~~ztp/OWNERS~~ [imiller0,serngawy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

imiller0 · 2023-05-02T23:13:03Z

@eggfoobar can we remove the hold?

eggfoobar · 2023-05-03T14:07:27Z

yup! We should be good to remove it.

/unhold

imiller0 · 2023-05-04T13:08:31Z

/cherry-pick release-4.13

openshift-cherrypick-robot · 2023-05-04T13:09:11Z

@imiller0: #1441 failed to apply on top of branch "release-4.13":

Applying: ztp: add support for cpu partitioning on site config
Using index info to reconstruct a base tree...
M	ztp/siteconfig-generator/siteConfig/siteConfig.go
Falling back to patching base and 3-way merge...
Auto-merging ztp/siteconfig-generator/siteConfig/siteConfig.go
CONFLICT (content): Merge conflict in ztp/siteconfig-generator/siteConfig/siteConfig.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 ztp: add support for cpu partitioning on site config
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.13

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/test-infra repository.

openshift-ci-robot · 2023-05-11T14:04:39Z

@eggfoobar: Jira Issue OCPBUGS-13301: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

openshift-kni/cnf-features-deploy#1495 is open

These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-13301 has not been moved to the MODIFIED state.

In response to this:

add new flag for cpu partitioning mode
add deprecation warning for situations where cpuset is defined for nodes

/hold
/cc @imiller0

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/test-infra repository.

imiller0 · 2023-05-11T14:05:18Z

/jira refresh

openshift-ci-robot · 2023-05-11T14:05:22Z

@imiller0: Jira Issue OCPBUGS-13301: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-13301 has been moved to the MODIFIED state.

In response to this:

/jira refresh

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/test-infra repository.

openshift-ci bot requested a review from imiller0 March 21, 2023 16:34

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 21, 2023

eggfoobar commented Mar 23, 2023

View reviewed changes

ztp/siteconfig-generator/siteConfig/siteConfigHelper.go Outdated Show resolved Hide resolved

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 27, 2023

eggfoobar force-pushed the workload-pinning-feature branch from faaea65 to d459d1a Compare March 30, 2023 14:16

serngawy suggested changes Mar 31, 2023

View reviewed changes

openshift-ci bot assigned serngawy Mar 31, 2023

imiller0 reviewed Apr 17, 2023

View reviewed changes

eggfoobar force-pushed the workload-pinning-feature branch from d459d1a to 1c3ae55 Compare April 19, 2023 08:05

eggfoobar added 2 commits April 19, 2023 10:42

ztp: add support for cpu partitioning on site config

50315c7

add new flag for cpu partitioning mode add deprecation warning for situations where cpuset is defined for nodes Signed-off-by: ehila <ehila@redhat.com>

ztp: added unit tests and updated deprecation warning message

68d1df9

Signed-off-by: ehila <ehila@redhat.com>

eggfoobar force-pushed the workload-pinning-feature branch from 1c3ae55 to 68d1df9 Compare April 19, 2023 14:42

ztp: fixed unit tests

b914c52

Signed-off-by: ehila <ehila@redhat.com>

eggfoobar force-pushed the workload-pinning-feature branch from 3667534 to b914c52 Compare April 25, 2023 19:02

imiller0 reviewed May 1, 2023

View reviewed changes

openshift-ci bot assigned imiller0 May 1, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 1, 2023

serngawy approved these changes May 2, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 2, 2023

imiller0 approved these changes May 2, 2023

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 3, 2023

openshift-merge-robot merged commit df92582 into openshift-kni:master May 3, 2023
4 checks passed

eggfoobar mentioned this pull request May 8, 2023

[release-4.13] Bug OCPBUGS-13313: Backport cpu partitioning siteconfig #1495

Merged

imiller0 changed the title ~~feat: add support for cpu partitioning on site config~~ feat: Bug OCPBUGS-13301: add support for cpu partitioning on site config May 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Bug OCPBUGS-13301: add support for cpu partitioning on site config #1441

feat: Bug OCPBUGS-13301: add support for cpu partitioning on site config #1441

eggfoobar commented Mar 21, 2023

openshift-ci bot commented Mar 21, 2023

eggfoobar commented Mar 21, 2023

imiller0 commented Mar 27, 2023

imiller0 commented Mar 27, 2023

serngawy Mar 31, 2023 •

edited

eggfoobar Apr 19, 2023

serngawy Mar 31, 2023

imiller0 Apr 10, 2023

imiller0 Apr 19, 2023

serngawy Mar 31, 2023

eggfoobar Apr 19, 2023

serngawy commented Mar 31, 2023

imiller0 left a comment

imiller0 commented Apr 24, 2023

eggfoobar commented Apr 25, 2023

imiller0 left a comment

openshift-ci bot commented May 2, 2023

imiller0 commented May 2, 2023

eggfoobar commented May 3, 2023

imiller0 commented May 4, 2023

openshift-cherrypick-robot commented May 4, 2023

openshift-ci-robot commented May 11, 2023

imiller0 commented May 11, 2023

openshift-ci-robot commented May 11, 2023

feat: Bug OCPBUGS-13301: add support for cpu partitioning on site config #1441

feat: Bug OCPBUGS-13301: add support for cpu partitioning on site config #1441

Conversation

eggfoobar commented Mar 21, 2023

openshift-ci bot commented Mar 21, 2023

eggfoobar commented Mar 21, 2023

imiller0 commented Mar 27, 2023

imiller0 commented Mar 27, 2023

serngawy Mar 31, 2023 • edited

Choose a reason for hiding this comment

eggfoobar Apr 19, 2023

Choose a reason for hiding this comment

serngawy Mar 31, 2023

Choose a reason for hiding this comment

imiller0 Apr 10, 2023

Choose a reason for hiding this comment

imiller0 Apr 19, 2023

Choose a reason for hiding this comment

serngawy Mar 31, 2023

Choose a reason for hiding this comment

eggfoobar Apr 19, 2023

Choose a reason for hiding this comment

serngawy commented Mar 31, 2023

imiller0 left a comment

Choose a reason for hiding this comment

imiller0 commented Apr 24, 2023

eggfoobar commented Apr 25, 2023

imiller0 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 2, 2023

imiller0 commented May 2, 2023

eggfoobar commented May 3, 2023

imiller0 commented May 4, 2023

openshift-cherrypick-robot commented May 4, 2023

openshift-ci-robot commented May 11, 2023

imiller0 commented May 11, 2023

openshift-ci-robot commented May 11, 2023

serngawy Mar 31, 2023 •

edited