OCPBUGS-5529: Fix updating numa core siblings map in GetCpuSiblings function

[mrniranjan]

move the delete operation outside of for loop.
Also minor fixes to offline tests to check if the worker node has 20 cores earlier in the test function

Signed-off-by: Niranjan M.R mrniranjan@redhat.com

[openshift-ci-robot]

@mrniranjan: This pull request references Jira Issue OCPBUGS-5529, which is invalid:

• expected the bug to be in one of the following states: NEW, ASSIGNED, POST, but it is ON_QA instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

move the delete operation outside of for loop.
Also minor fixes to offline tests to check if the worker node has 20 cores earlier in the test function

Signed-off-by: Niranjan M.R mrniranjan@redhat.com

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.

[mrniranjan]

/jira refresh

[openshift-ci-robot]

@mrniranjan: This pull request references Jira Issue OCPBUGS-5529, which is invalid:

• expected the bug to be in one of the following states: NEW, ASSIGNED, POST, but it is ON_QA instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

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.

[mrniranjan]

/jira refresh

[openshift-ci-robot]

@mrniranjan: This pull request references Jira Issue OCPBUGS-5529, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.13.0) matches configured target version for branch (4.13.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (nkononov@redhat.com), skipping review request.

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.

[mrniranjan]

/retest-required

[mrniranjan]

/test e2e-gcp-pao-updating-profile

[MarSik]

This is corekey, not numa node.

[MarSik]

Oh, nevermind, I see what you did here.

[MarSik]

But shouldn't this be inside the numaCoreSibligs loop and use the corekey? Your code will not work if the siblings are not coming from the same NUMA node anyway.

[MarSik]

Why not simply use:

for key := range numaCoreSiblings {
    for _, c := range numaCoreSiblings[key][coreKey] {
        cpuSiblings = append(cpuSiblings, strconv.Itoa(c))
    }
    delete(numaCoreSiblings[key], coreKey)
}

[mrniranjan]

If we put the delete operation inside the outer for loop, the key will get modified because the for loop continues further . So we will have to add a break and end the loop , otherwise it will endup removing a some other core or fail

[mrniranjan]

I have updated the Patch with a if condition to check if the map has core key exists , if exist, we move to the internal for loop.

[jlojosnegros]

First I have my concerns about a "Get" function that modifies its input parameters...

Regarding delete operation. If I understood correctly you are gonna end deleting all the CPUs of a Core. All of them would usually belong to the same NUMA node so deleting them at the end should work ( I think) but it is confusing.
Either you enforce explicitly in the code the fact that all the cpus in a core must belong to one NumaNode or you create a list of numanodes in the for loops and then delete them afterwards... something like

func GetCpuSiblings(numaCoreSiblings map[int]map[int][]int, coreKey int) []string {
	var cpuSiblings []string
	var targetNumaNodes []int
	for numaNode := range numaCoreSiblings {
		for _, c := range numaCoreSiblings[numaNode][coreKey] {
			cpuSiblings = append(cpuSiblings, strconv.Itoa(c))
			targetNumaNodes = append(targetNumaNodes, numaNode)
		}
	}

	for numaNode := range targetNumaNodes {
		delete(numaCoreSiblings[numaNode], coreKey)
	}

	return cpuSiblings
}

or even better extract deletion to another function.

[mrniranjan]

@jlojosnegros , let me explain the use case more elaborately :

For example, i wanted to offline cpus from higher core ids

1. For the first time, i query the worker nodes and populate the map with numa node information which may bee like this:

Map = map[0:map[0:[0 40] 2:[2 42] 4:[4 44] 6:[6 46] 8:[8 48] 10:[10 50] 12:[12 52] 14:[14 54] 16:[16 56] 18:[18 58] 20:[20 60] 22:[22 62] 24:[24 64] 26:[26 66] 28:[28 68] 30:[30 70] 32:[32 72] 34:[34 74] 36:[36 76] 38:[38 78]] 1:map[1:[1 41] 3:[3 43] 5:[5 45] 7:[7 47] 9:[9 49] 11:[11 51] 13:[13 53] 15:[15 55] 17:[17 57] 19:[19 59] 21:[21 61] 23:[23 63] 25:[25 65] 27:[27 67] 29:[29 69] 31:[31 71] 33:[33 73] 35:[35 75] 37:[37 77] 39:[39 79]]]

Now i need pickup 38th core from node0 and 39th core from node 1 , this is stright forward,
a. loop to each numa node
b. sort all the cores from a numa node
c. pick the highest core id , gets its siblings, in a offlineCpuList
d. once fetched we remove that core from the Map

Now i need cpusiblings from reserved cpus, For that i am taking core 0, 1, 2 as my reserved cores: In this case core 0 , 2 fall on numa node 0 and core 1 on numa node 1

Now in the GetCpuSiblings function, i passing the map and core id, (core ids are always unique). So i query each node, if core exist in that map, fetch the cpu siblings and remove them from the map .

The reason i need to delete the coreid once fetched is because, when i want cpus for isolated, I can use all the remaining core siblings, if i don't delete, i will get the cpu siblings which are already used in offline and reserved.

The code to me looks simple ,

func GetCpuSiblings(numaCoreSiblings map[int]map[int][]int, coreKey int) []string {                                                                                                                           
         var cpuSiblings []string                                                                                                                                                                              
         // key here is the Numa Node                                                                                                                                                                          
         for key := range numaCoreSiblings {                                                                                                                                                                   
                 // Here we check in that specific numa node , the coreid exists                                                                                                                               
                 _, ok := numaCoreSiblings[key][coreKey]                                                                                                                                                       
                 // if exists                                                                                                                                                                                  
                 if ok {                                                                                                                                                                                       
                         for _, sibling := range numaCoreSiblings[key][coreKey] {                                                                                                                              
                                 cpuSiblings = append(cpuSiblings, strconv.Itoa(sibling))                                                                                                                      
                         }                                                                                                                                                                                     
                         // once the siblings are returned remove the core                                                                                                                                     
                         // from which the siblings are returned                                                                                                                                               
                         delete(numaCoreSiblings[key], coreKey)                                                                                                                                                
                 }                                                                                                                                                                                             
          }                                                                                                                                                                                                     
         return cpuSiblings                                                                                                                                                                                    
 }     

Now as you suggested its possible to remove the delete Operation to another function, in which case i have to duplicate some of the above like this:

func DeleteCpuSiblings(numaCoreSiblings map[int]map[int][]int, coreKey int) {                                                                                                                                 
         for key := range numaCoreSiblings {                                                                                                                                                                   
                _, ok := numaCoreSiblings[key][coreKey]                                                                                                                                                       
                 if ok {                                                                                                                                                                                       
                         delete(numaCoreSiblings[key], coreKey)                                                                                                                                                
                }                                                                                                                                                                                             
         }                                                                                                                                                                                                     
}   

I can do it this , let me know your views.

[jlojosnegros]

Ok ... I understand what you wanna do.

My only point here, and maybe it is just a matter of my "taste" so to say, is that I would not expect a "get" function to modify its parameters. I know it is just a matter of naming ... so not a big deal if everybody else is ok with that.

[ffromani]

The point about naming is right and I concur.

[jlojosnegros]

apart from that lgtm ...
Let's give some time to see other reviewer's opinion

[mrniranjan]

@ffromani @jlojosnegros Thanks for inputs, i have renamed the function to GetAndRemoveCpuSiblingsFromMap ,

[mrniranjan]

/retest-required

[mrniranjan]

/test e2e-gcp-pao-updating-profile

[mrniranjan]

/test e2e-gcp-pao-updating-profile

[mrniranjan]

/test e2e-gcp-pao-updating-profile

[mrniranjan]

/test e2e-gcp-pao-updating-profile

[mrniranjan]

/test e2e-gcp-pao-updating-profile

[mrniranjan]

/test e2e-gcp-pao-updating-profile

[jlojosnegros]

minor comment : as this is numa node id maybe using numaKey ?

[jlojosnegros]

/lgtm

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD 77f6222 and 2 for PR HEAD 6cd35e3 in total

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD 484ea42 and 1 for PR HEAD 6cd35e3 in total

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD ce8bd90 and 0 for PR HEAD 6cd35e3 in total

[ffromani]

/override ci/prow/e2e-gcp-pao

[ffromani]

failures in the e2e-gcp-pao are (very) unlikely to be caused or worsened by this PR and should be investigated in another PR.

[openshift-ci]

@ffromani: Overrode contexts on behalf of ffromani: ci/prow/e2e-gcp-pao

In response to this:

/override ci/prow/e2e-gcp-pao

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.

[ffromani]

/retest

[openshift-ci-robot]

/hold

Revision 6cd35e3 was retested 3 times: holding

[mrniranjan]

/test e2e-aws-ovn

[openshift-ci]

@mrniranjan: all tests passed!

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/test-infra repository. I understand the commands that are listed here.

[mrniranjan]

/hold cancel

[openshift-ci-robot]

@mrniranjan: Jira Issue OCPBUGS-5529: All pull requests linked via external trackers have merged:

• openshift/cluster-node-tuning-operator#564

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

In response to this:

move the delete operation outside of for loop.
Also minor fixes to offline tests to check if the worker node has 20 cores earlier in the test function

Signed-off-by: Niranjan M.R mrniranjan@redhat.com

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.

[mrniranjan]

/cherry-pick release-4.13

[openshift-cherrypick-robot]

@mrniranjan: new pull request created: #669

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.