Skip to content

Implement Intel RDT matching line check for pre-existing ClosId schemata.#3833

Open
ipuustin wants to merge 1 commit intoopencontainers:mainfrom
ipuustin:intel-rdt-line-check-pr
Open

Implement Intel RDT matching line check for pre-existing ClosId schemata.#3833
ipuustin wants to merge 1 commit intoopencontainers:mainfrom
ipuustin:intel-rdt-line-check-pr

Conversation

@ipuustin
Copy link
Copy Markdown
Contributor

@ipuustin ipuustin commented Apr 19, 2023

OCI Linux config spec specifies that if ClosId is set, the assigned values must match to the current values (if the closid resctrl sub-directory was pre-existing). Previously there was a TODO indicating that this functionality was missing.

@ipuustin ipuustin force-pushed the intel-rdt-line-check-pr branch 3 times, most recently from 965a493 to 5d46fd0 Compare April 19, 2023 14:22
ipuustin
ipuustin commented Apr 19, 2023
View reviewed changes
libcontainer/intelrdt/intelrdt.go Outdated
// They also remove whitespace around the parsed keys and values. Note that not
// all theoretically correct lines are accepted (for example lines beginning with
// ';' are not allowed), but all the "reasonable" ones should be.
mbLineMatch = regexp.MustCompile(`^MB:(?:\s*\w+\s*=\s*\w+\s*;+)*(?:\s*\w+\s*=\s*\w+\s*);*\s*$`)
Copy link
Copy Markdown
Contributor Author

@ipuustin ipuustin Apr 19, 2023

Choose a reason for hiding this comment

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

Note that the easiest way to review the regexp is to use an online regexps creator tool, which allows to graphically match the regexp against different inputs.

@ipuustin
Copy link
Copy Markdown
Contributor Author

ipuustin commented Apr 19, 2023

@marquiz

@ipuustin ipuustin force-pushed the intel-rdt-line-check-pr branch 5 times, most recently from bfccafc to ec0e49e Compare April 24, 2023 19:43
@marquiz
Copy link
Copy Markdown
Contributor

marquiz commented Apr 25, 2023

ping @kolyshkin @xiaochenshen

xiaochenshen
xiaochenshen reviewed Apr 25, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@xiaochenshen xiaochenshen left a comment

Choose a reason for hiding this comment

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

@ipuustin @marquiz Do we need to move the lines parsing and schemata checking helper functions and related code out from intelrdt.go to a separate file? And then just call checkExistingSchemataI() in Set() in intelrdt.go. It may make file intelrdt.go simpler.

@ipuustin ipuustin force-pushed the intel-rdt-line-check-pr branch from ec0e49e to 9381660 Compare April 26, 2023 10:34
@ipuustin
Copy link
Copy Markdown
Contributor Author

ipuustin commented Apr 26, 2023

@ipuustin @marquiz Do we need to move the lines parsing and schemata checking helper functions and related code out from intelrdt.go to a separate file? And then just call checkExistingSchemataI() in Set() in intelrdt.go. It may make file intelrdt.go simpler.

Thanks for the review! I moved the schemata line parsing and comparsion to a separate file now.

xiaochenshen
xiaochenshen approved these changes Apr 28, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@xiaochenshen xiaochenshen left a comment

Choose a reason for hiding this comment

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

LGTM

@ipuustin ipuustin force-pushed the intel-rdt-line-check-pr branch from 9381660 to 1621ea2 Compare May 2, 2023 06:27
@ipuustin
Copy link
Copy Markdown
Contributor Author

ipuustin commented May 2, 2023

Rebased.

@ipuustin ipuustin force-pushed the intel-rdt-line-check-pr branch from 1621ea2 to eac9f98 Compare May 9, 2023 11:14
@ipuustin
Copy link
Copy Markdown
Contributor Author

ipuustin commented May 9, 2023

Rebased.

kolyshkin
kolyshkin requested changes May 17, 2023
View reviewed changes
@kolyshkin kolyshkin mentioned this pull request May 18, 2023
Merged
@ipuustin ipuustin force-pushed the intel-rdt-line-check-pr branch 4 times, most recently from 007d457 to 5dee697 Compare May 31, 2023 13:55
@ipuustin
Copy link
Copy Markdown
Contributor Author

ipuustin commented Jun 1, 2023

Thanks @kolyshkin for the review! All the review comments should be addressed now. I think it makes sense to work on this PR first, because the other PR (with the proposed spec changes) will need rework when we modify the spec according to your proposal there.

@ipuustin ipuustin force-pushed the intel-rdt-line-check-pr branch from 5dee697 to f99bb27 Compare June 13, 2023 07:11
@ipuustin
Copy link
Copy Markdown
Contributor Author

ipuustin commented Jun 13, 2023

Rebased. @kolyshkin WDYT?

@ipuustin
Implement Intel RDT line check.
ff864db 
OCI Linux config spec specifies that if ClosId is set, the assigned
values must match to the current values.

Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>
@ipuustin ipuustin force-pushed the intel-rdt-line-check-pr branch from f99bb27 to ff864db Compare July 27, 2023 06:17
@ipuustin
Copy link
Copy Markdown
Contributor Author

ipuustin commented Jul 27, 2023

Rebased. Ping @kolyshkin !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kolyshkin kolyshkin kolyshkin requested changes

+1 more reviewer

@xiaochenshen xiaochenshen xiaochenshen approved these changes

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ipuustin @marquiz @kolyshkin @xiaochenshen