MGMT-14453: Fix bugs in installercache #7205

paul-maidment · 2025-01-20T10:50:04Z

This PR is for the purpose of resolving multiple bugs within the installer cache, due to the poor condition of the current cache, it makes sense to fix this in a single PR.

https://issues.redhat.com/browse/MGMT-14452
Installer cache removes in-used cached image when out of space
https://issues.redhat.com/browse/MGMT-14453
INSTALLER_CACHE_CAPACITY small value cause to assisted-service crash
https://issues.redhat.com/browse/MGMT-14457
Installer cache - fails to install when running parallel with same version
Additionally, the cache did not respect limits, so this has been addressed here.

Fixes:

I have implemented fixes for each of the following issues.

Mutex was ineffective as not instantiated corrctly, leading to MGMT-14452, MGMT-14453.
Naming convention for hardlinks changed to be UUID based to resolve MGMT-14457.
Any time we either extract or use a release, the modified time must be updated, not only for cached releases. This was causing premature pruning of hardlinks.
LRU cache order updated to be based on microseconds instead of seconds.
Eviction checks updated to consider max release size and also cache threshold.
We now check there is enough space before writing.
During eviction - releases without hard links will be evicted before releases with hard links.

openshift-ci-robot · 2025-01-20T10:50:09Z

@paul-maidment: This pull request references MGMT-14453 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.19.0" version, but no target version was set.

In response to this:

Presently we instantiate a new installercache.Installers instance per execution of the code generator. This renders any mutex handling pointless as we will never have any mutex conflict over a single request.

This PR moves the instantiation into the main and effectively makes it a singleton so that mutexes will function as intended.

This is the root cause behind a couple of reported issues

https://issues.redhat.com/browse/MGMT-14452 - Installer cache removes in-used cached image when out of space https://issues.redhat.com/browse/MGMT-14453 - INSTALLER_CACHE_CAPACITY small value cause to assisted-service crash

Both of these issues are reproducible on master and do not reproduce when running on code based on this PR.

If the cache size is set to a low value such as 1 then the current release will be deleted once the caller has finished with the release.

List all the issues related to this PR

New Feature

Enhancement

Bug fix

Tests

Documentation

CI/CD

What environments does this code impact?

Automation (CI, tools, etc)

Cloud

Operator Managed Deployments

None

How was this code tested?

assisted-test-infra environment

dev-scripts environment

Reviewer's test appreciated

Waiting for CI to do a full test run

Manual (Elaborate on how it was tested)

No tests needed

Checklist

Title and description added to both, commit and PR.

Relevant issues have been associated (see CONTRIBUTING guide)

This change does not require a documentation update (docstring, docs, README, etc)

Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?

Is there a bug required (and linked) for this change?

Should this PR be backported?

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2025-01-20T10:50:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: paul-maidment

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [paul-maidment]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2025-01-20T11:13:40Z

@paul-maidment: This pull request references MGMT-14453 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.19.0" version, but no target version was set.

In response to this:

Presently we instantiate a new installercache.Installers instance per execution of the code generator. This renders any mutex handling pointless as we will never have any mutex conflict over a single request.

This PR moves the instantiation into the main and makes sure there is only a single instance so that mutexes will function as intended.

This is the root cause behind a couple of reported issues

https://issues.redhat.com/browse/MGMT-14452 - Installer cache removes in-used cached image when out of space https://issues.redhat.com/browse/MGMT-14453 - INSTALLER_CACHE_CAPACITY small value cause to assisted-service crash

Both of these issues are reproducible on master and do not reproduce when running on code based on this PR.

If the cache size is set to a low value such as 1 then the current release will be deleted once the caller has finished with the release.

List all the issues related to this PR

New Feature

Enhancement

Bug fix

Tests

Documentation

CI/CD

What environments does this code impact?

Automation (CI, tools, etc)

Cloud

Operator Managed Deployments

None

How was this code tested?

assisted-test-infra environment

dev-scripts environment

Reviewer's test appreciated

Waiting for CI to do a full test run

Manual (Elaborate on how it was tested)

No tests needed

Checklist

Title and description added to both, commit and PR.

Relevant issues have been associated (see CONTRIBUTING guide)

This change does not require a documentation update (docstring, docs, README, etc)

Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?

Is there a bug required (and linked) for this change?

Should this PR be backported?

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 openshift-eng/jira-lifecycle-plugin repository.

internal/installercache/installercache.go

cmd/main.go

rccrdpccl · 2025-02-04T13:00:49Z

internal/installercache/installercache_test.go

+	// runParallelTest launches a batch of fetches from the installer cache in order to simulate multiple requests to the same node
+	// releases are automatically cleaned up as they are gathered
+	// returns the first error encountered or nil if no error encountered.
+	runParallelTest := func(params []launchParams) error {


I agree this is not a unit test, but we already have many "unit-tests" which are not properly unit.
As long as it's not slowing down the total "unit-test" time, I think it could be a good test to have in case this code needs to be changed in the future (deadlock here would be horrible).

We could argue that we could split properly unit and more "integration" tests by tag, and execute them separately, but I think it'd be outside the scope of this PR.

Wouldn't ginkgo support parallel execution of table tests? That would make it way more readable than currently. If ginkgo does not support it, I think we should improve readability of these functions

internal/installercache/installercache_test.go

internal/metrics/os_disk_stats_helper_test.go

paul-maidment · 2025-02-04T14:30:27Z

@paul-maidment Any chance you can provide some description about the installerCache now with your changes (what configuration it should get (and why), what is its main purpose, how it operators, etc. ?

I know it should function as a cache for openshift/installer binaries, but not much more than that

Added something to the docs directory to describe the functionality

gamli75 · 2025-02-04T14:51:42Z

@paul-maidment let's hold this PR or change it to WIP until we finish the code review process - there is no need to run tests every few minutes.

carbonin

Generally looking much better after the last round of changes.

internal/installercache/installercache.go

internal/installercache/installercache_test.go

carbonin · 2025-02-04T19:36:41Z

internal/installercache/installercache_test.go

+	testGet := func(releaseID, version string, clusterID strfmt.UUID, expectCached bool) (string, string) {
+		workdir := filepath.Join(manager.config.CacheDir, "quay.io", "release-dev")
+		fname := filepath.Join(workdir, releaseID)
+		mockReleaseCalls(releaseID, version)


Should this depend on expectCached as well?

I am not sure what you mean exactly?

But the intent here is to be able to pass expectations about the cached status within the test so that assertions may be made.

It seems to be doing the job?

I guess I'm asking if we want to ensure we're not calling the release code if we're expecting the release to be cached.

Ah, ok, makes sense

I mean, TBH, calling release.Cleanup is not a huge issue, we need to remember that this is just cleaning up the hardlinks to simulate a normal flow of the system.

Even if something is cached, a hardlink is issued every time someone requests that release via Get

Clearing the hardlink may assist the eviction of that release in some cases (as there will be no hardlinks) but I don't think any of the cases in testGet are doing anything like this.

In fact, I just tested and for these tests, we should never fail to call release.Cleanup as some of the tests are checking scenarios where space is at a premium. If we don't clean up, we get deadlocked!

So TLDR. I don't think we need to change this.

carbonin · 2025-02-04T19:40:26Z

internal/installercache/installercache_test.go

 		_, err = os.Stat(r2)
 		Expect(os.IsNotExist(err)).To(BeFalse())
 		_, err = os.Stat(r3)
 		Expect(os.IsNotExist(err)).To(BeFalse())

+		By("verify that the links were purged")


Isn't this just testing that you called Cleanup in testGet?

It tests that cleanup actually cleans the links.
Code that was here before in any case, I tried not to interfere too much with existing tests

internal/installercache/installercache_test.go

pkg/generator/generator.go

…in the installer cache This PR is for the purpose of resolving multiple bugs within the installer cache, due to the poor condition of the current cache, it makes sense to fix this in a single PR. * https://issues.redhat.com/browse/MGMT-14452 Installer cache removes in-used cached image when out of space * https://issues.redhat.com/browse/MGMT-14453 INSTALLER_CACHE_CAPACITY small value cause to assisted-service crash * https://issues.redhat.com/browse/MGMT-14457 Installer cache - fails to install when running parallel with same version * Additionally, the cache did not respect limits, so this has been addressed here. Fixes: I have implemented fixes for each of the following issues. * Mutex was ineffective as not instantiated corrctly, leading to [MGMT-14452](https://issues.redhat.com//browse/MGMT-14452), [MGMT-14453](https://issues.redhat.com//browse/MGMT-14453). * Naming convention for hardlinks changed to be UUID based to resolve [MGMT-14457](https://issues.redhat.com//browse/MGMT-14457). * Any time we either extract or use a release, the modified time must be updated, not only for cached releases. This was causing premature pruning of hardlinks. * LRU cache order updated to be based on microseconds instead of seconds. * Eviction checks updated to consider max release size and also cache threshold. * We now check there is enough space before writing. * During eviction - releases without hard links will be evicted before releases with hard links.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 20, 2025

openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 20, 2025

openshift-ci bot requested review from carbonin and mlorenzofr January 20, 2025 10:50

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2025

paul-maidment marked this pull request as draft January 20, 2025 11:12

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 20, 2025

paul-maidment force-pushed the MGMT-14453 branch 4 times, most recently from 0369404 to 715734c Compare January 26, 2025 14:54

openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 26, 2025

paul-maidment force-pushed the MGMT-14453 branch 12 times, most recently from fa9cd8d to 27013f9 Compare January 26, 2025 23:43

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2025

paul-maidment force-pushed the MGMT-14453 branch from 27013f9 to 285a8bd Compare January 27, 2025 00:17

rccrdpccl requested changes Feb 4, 2025

View reviewed changes

openshift-ci bot assigned rccrdpccl Feb 4, 2025

paul-maidment force-pushed the MGMT-14453 branch from cd5ac30 to 55b7676 Compare February 4, 2025 14:15

paul-maidment force-pushed the MGMT-14453 branch 4 times, most recently from ab62cb3 to 65f68d5 Compare February 4, 2025 14:45

paul-maidment force-pushed the MGMT-14453 branch from 65f68d5 to b52b84e Compare February 4, 2025 18:06

paul-maidment marked this pull request as draft February 4, 2025 18:06

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 4, 2025

paul-maidment force-pushed the MGMT-14453 branch 2 times, most recently from 4239446 to b67780b Compare February 4, 2025 18:39

carbonin reviewed Feb 4, 2025

View reviewed changes

paul-maidment force-pushed the MGMT-14453 branch from b67780b to b8cf08d Compare February 4, 2025 19:59

openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 4, 2025

paul-maidment force-pushed the MGMT-14453 branch 3 times, most recently from 48e34dc to 78b6d48 Compare February 4, 2025 21:03

paul-maidment requested review from carbonin and rccrdpccl February 4, 2025 22:16

paul-maidment force-pushed the MGMT-14453 branch 5 times, most recently from 77da59c to 6938c2f Compare February 4, 2025 22:57

paul-maidment force-pushed the MGMT-14453 branch from 6938c2f to f4842bf Compare February 4, 2025 23:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGMT-14453: Fix bugs in installercache #7205

MGMT-14453: Fix bugs in installercache #7205

paul-maidment commented Jan 20, 2025 •

edited

Loading

openshift-ci-robot commented Jan 20, 2025 •

edited by openshift-ci bot

Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Jan 20, 2025

openshift-ci-robot commented Jan 20, 2025 •

edited by openshift-ci bot

Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

rccrdpccl Feb 4, 2025

paul-maidment commented Feb 4, 2025

gamli75 commented Feb 4, 2025

carbonin left a comment

carbonin Feb 4, 2025

paul-maidment Feb 4, 2025

carbonin Feb 4, 2025

paul-maidment Feb 4, 2025

paul-maidment Feb 4, 2025 •

edited

Loading

carbonin Feb 4, 2025

paul-maidment Feb 4, 2025 •

edited

Loading

MGMT-14453: Fix bugs in installercache #7205

Are you sure you want to change the base?

MGMT-14453: Fix bugs in installercache #7205

Conversation

paul-maidment commented Jan 20, 2025 • edited Loading

openshift-ci-robot commented Jan 20, 2025 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Jan 20, 2025

openshift-ci-robot commented Jan 20, 2025 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

rccrdpccl Feb 4, 2025

Choose a reason for hiding this comment

paul-maidment commented Feb 4, 2025

gamli75 commented Feb 4, 2025

carbonin left a comment

Choose a reason for hiding this comment

carbonin Feb 4, 2025

Choose a reason for hiding this comment

paul-maidment Feb 4, 2025

Choose a reason for hiding this comment

carbonin Feb 4, 2025

Choose a reason for hiding this comment

paul-maidment Feb 4, 2025

Choose a reason for hiding this comment

paul-maidment Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

carbonin Feb 4, 2025

Choose a reason for hiding this comment

paul-maidment Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

paul-maidment commented Jan 20, 2025 •

edited

Loading

openshift-ci-robot commented Jan 20, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 20, 2025 •

edited by openshift-ci bot

Loading

paul-maidment Feb 4, 2025 •

edited

Loading

paul-maidment Feb 4, 2025 •

edited

Loading