Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rte: add option to compute the pod set fingerprint #109

Merged
merged 6 commits into from
Jun 15, 2022
Merged

Conversation

ffromani
Copy link
Contributor

@ffromani ffromani commented May 30, 2022

Add an option to compute the fingerprint of the current pod set, and report it as annotation to NRT objects. This is meant to enable better caching in the scheduler side.

Signed-off-by: Francesco Romani [email protected]

@ffromani ffromani requested review from swatisehgal and Tal-or May 30, 2022 11:06
@ffromani
Copy link
Contributor Author

/hold
missing e2e test

@ffromani ffromani force-pushed the node-signature branch 11 times, most recently from 9439d09 to 396ec54 Compare June 1, 2022 08:18
@ffromani
Copy link
Contributor Author

ffromani commented Jun 1, 2022

/unhold
ready for review

@ffromani
Copy link
Contributor Author

ffromani commented Jun 7, 2022

e2e tests are currently broken, I'll investigate and fix them

@ffromani ffromani force-pushed the node-signature branch 2 times, most recently from 69f0a6b to fc2c735 Compare June 13, 2022 16:42
@ffromani
Copy link
Contributor Author

e2e tests are currently broken, I'll investigate and fix them

should be fixed now. PR ready.

@ffromani ffromani force-pushed the node-signature branch 2 times, most recently from a68bf16 to b3a022a Compare June 14, 2022 07:24
Copy link
Contributor

@Tal-or Tal-or left a comment

Choose a reason for hiding this comment

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

Initial review. looks very good. left a few small comments.
Will do another review in the following days.

pkg/resourcemonitor/resourcemonitor.go Outdated Show resolved Hide resolved
pkg/resourcemonitor/resourcemonitor.go Show resolved Hide resolved
pkg/resourcemonitor/resourcemonitor.go Show resolved Hide resolved
ffromani added 5 commits June 14, 2022 17:20
Add an option to compute the fingerprint of the current pod set,
and report it as annotation to NRT objects. This is meant to
enable better caching in the scheduler side.

Signed-off-by: Francesco Romani <[email protected]>
Add a trivial package to consolidate the annotations.
We are adding more of them, scattered all across packages.
Moving all the definitions in a single place, we can make it
easier to consume.

Signed-off-by: Francesco Romani <[email protected]>
This is meant to help clients, most notable e2e tests.
We expose this information as annotation (and not as status field)
mostly because this is status/metadata of the updater agent,
not of the NRT object proper.

Signed-off-by: Francesco Romani <[email protected]>
add basic e2e tests for the fingerprinting functionality:
- PFP should be stable with no pod churn
- PFP MUST change if we add or delete pods

Signed-off-by: Francesco Romani <[email protected]>
Make sure we always report meaningful data.
Most notable, 1 second granularity is sufficient for all
the expected use cases (e2e tests)

In general, if the update period is less than 1 second, the
configuration is very suspicious anyway.

Signed-off-by: Francesco Romani <[email protected]>
Copy link
Collaborator

@swatisehgal swatisehgal left a comment

Choose a reason for hiding this comment

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

Thanks for all the work on this. This is a great start!

Left a few minor comments but the main one that got me wondering was if it would be a better design choice to use pod UUID instead of name/namespace pair for fingerprint evaluation.

manifests/resource-topology-exporter.yaml Show resolved Hide resolved
pkg/resourcemonitor/resourcemonitor.go Show resolved Hide resolved
test/data/TestArgsParse.default.json Outdated Show resolved Hide resolved
test/data/TestArgsParse.default.json Show resolved Hide resolved
test/e2e/utils/nodetopology/nodetopology.go Outdated Show resolved Hide resolved
explain all the failure conditions when validating NRT objects.

Signed-off-by: Francesco Romani <[email protected]>
@swatisehgal
Copy link
Collaborator

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants