Skip to content
This repository was archived by the owner on Nov 28, 2022. It is now read-only.

Add mba_percentage for /workloads #48

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Shihta
Copy link
Contributor

@Shihta Shihta commented Oct 15, 2018

Accept mba_percentage configuration for guarantee and besteffort pools
Deny mba_percentage configuration for shared pools
RMD rejects mba_mbps configuration because it didn't implemented

Request example:
$ curl -H "Content-Type: application/json" --request POST --data
'{"task_ids":["1148"], "max_cache": 1, "min_cache": 1, "mba_percentage": 10}'
http://127.0.0.1:8081/v1/workloads

Response example:
{
"id": "1",
"task_ids": [
"1148"
],
"status": "Successful",
"cos_name": "1148-guarantee",
"max_cache": 1,
"min_cache": 1,
"mba_percentage": 10
}

@Shihta
Copy link
Contributor Author

Shihta commented Oct 15, 2018

The check script of RMD seems to be no longer applies to new golang environment.
I have no idea how to deal with that.

@dakshinai
Copy link
Contributor

@Shihta Could you rebase your code to accept the latest travis.yml from master? That should fix build.
Also please refer the R&R table. We would like to disable MBA by % for best-effort group (and not shared). Shared/Guaranteed groups will leverage MBA.

if mbaInfo.MbaOn == false {
return fmt.Errorf("This platform does not support MBA")
}
if w.MaxCache != nil && w.MinCache != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When both mba and cache are specified, if min_cache<max_cache and mba is specified, report best-effort not supported. So we just need to remove line 56 and line 58 should say best-effort

@@ -44,7 +75,7 @@ func Validate(w *tw.RDTWorkLoad) error {
}
}

if w.Policy == "" {
if w.MbaPercentage == nil && w.MbaMbps == nil && w.Policy == "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrite if statement here, if policy is empty, user should give either (min_cache & max_cache) or mba_percentage or both. Remove MbaMbps from any further checks since it is not implemented.

}
}

if w.MbaPercentage != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider a situation when user gives only mba fields (no min_cache and max_cache). Hence determine pool type as follows, if mbaPercentage=0, shared and mba_percentage>0, guaranteed. In our design we are skipping best-effort pool and not shared. So for mba only setting there is no need to check for best effort (since there is no min_mba and max_mba)
User can supply mba_percentage=0, so exclude that from check

@@ -417,6 +531,9 @@ func populateEnforceRequest(req *tw.EnforceRequest, w *tw.RDTWorkLoad) *rmderror
req.MaxWays = *w.MaxCache
populatePolicy = false
}
if w.MbaPercentage != nil {
populatePolicy = false
}
// else get max/min from policy
if populatePolicy {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Shihta,
populatePolicy takes default values from a file and user can say policy file name instead of saying min_cache, max_cache and mba_percentage. We need to repeat lines 552-557 to get populate request with default value for mba_percentage from file. If this is too much for you. I can add the change too. Please let me know what works.

Copy link
Contributor

@dakshinai dakshinai Nov 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Shihta,
As I mentioned above, this is not in R&R. If lines 552-557 is too much for you, skip it, i shall cover it.

if ok := mbaTarget[k]; ok {
newCandidate[k] = mixedCandidate{v, &mbaStr}
} else {
newCandidate[k] = mixedCandidate{v, &maxMba}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line 256 is optional

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skip to assign maxMba is okay, but the variable "v" is necessary, it is CAT settings.
I suggest that it is more readable to keep this maxMba assignment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright lets keep it. Please merge close on the other comments as possible.

Convey("Validate mba with shared pools", func() {
wl.MaxCache = &sharedCache
err := Validate(wl, mbaInfo)
So(err, ShouldNotBeNil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shared pool is allowed. This should pass after the code changes.

@dakshinai
Copy link
Contributor

dakshinai commented Nov 1, 2018

Hi Shihta, The code tests fine, please incorporate the changes suggested. Also remove any fmt statements after testing or move them to log.Debug if needed.

I have fixed the travis build error again. Please rebase and submit the code changes.

Accept mba_percentage configuration for guarantee pool
Deny mba_percentage configuration for besteffort and shared pools
RMD rejects mba_mbps configuration because it didn't implemented

Request example:
$ curl -H "Content-Type: application/json" --request POST --data \
    '{"task_ids":["1148"], "max_cache": 1, "min_cache": 1, "mba_percentage": 10}' \
    http://127.0.0.1:8081/v1/workloads

Response example:
{
  "id": "1",
  "task_ids": [
   "1148"
  ],
  "status": "Successful",
  "cos_name": "1148-guarantee",
  "max_cache": 1,
  "min_cache": 1,
  "mba_percentage": 10
}
1. Set populatePolicy=false if mba is set
2. Validate if mba value is in range 0-100
3. Update candidate schemata with MBA to use newResAss fn
1. Remove MbaMbps from any further checks since it is not implemented
2. Remove MBA step check
1. Add new parameter MbaPercentage on OSGroup/InfraGroup blocks of rmd.toml
2. Add new parameter MbaPercentageShared on CachePool block of rmd.toml
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants