Remove cache file support #50
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
klihub
reviewed
May 24, 2023
klihub
previously requested changes
May 24, 2023
jukkar
force-pushed
the
cache-file-removal
branch
from
1cbfc26
to
9bd32bd
Compare
klihub
reviewed
May 24, 2023
There was a problem hiding this comment.
Looks go to me now (although looks like some go reformatting is needed). Maybe we'll need to sit on this for a while, at least until the first CRI-O release is done with the merged related fixes.
And maybe we'll need to add a version check to pkg/resmgr/nri.go:Configure() to give an error (and maybe refuse to start up) if the runtime+version indicates a cri-o without the fixes.
jukkar
force-pushed
the
cache-file-removal
branch
from
9bd32bd
to
2f3a806
Compare
|
Fixed the format error. We can address the cri-o version check later after the cri-o release. |
jukkar
force-pushed
the
cache-file-removal
branch
from
2f3a806
to
76511a6
Compare
|
Removed the cache rm task from todo file. |
No need to save / restore cache state as all the state should come from runtime via Synchronize message so no need to cache the data even if we restart the nri plugin. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
The e2e tests want to validate things after the test and for that, they need access to the cache state. So now that the cache file is removed, the cache state needs to be fetched directly from NRI plugin via a HTTP request. Default address used by e2e tests is http://localhost:8891/cache-state The cache fetching is disabled by default and only enabled for e2e tests. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
As there is no longer a cache file, the test is no longer valid and can be removed. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
No one uses the mutex in cache object so it can be removed. Can be reintroduced if fine grained locking is needed there. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
jukkar
force-pushed
the
cache-file-removal
branch
from
76511a6
to
cd7864a
Compare
|
Rebased to main. Changed option name used by e2e test to |
|
We'll revisit this later. Closing for now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As we can get the full state of the pod and container from runtime, no need to cache the data.
Containerd already has support for this. For cri-o, one needs a version that has commit f59c1f72a837c2235010813b8f361bb57c8c551b ("sandbox: Handle PodLinuxOverhead and PodLinuxResources CRI fields") in it.