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.

Sign up for GitHub

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

fix: Remove wait after SQLInstance periodic test steps #3189

Merged
merged 1 commit into from
Nov 27, 2024
Merged

fix: Remove wait after SQLInstance periodic test steps #3189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/controller/dynamic/dynamic_controller_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,7 @@ func testDriftCorrection(ctx context.Context, t *testing.T, testContext testrunn

// Underlying APIs may not have strongly-consistent reads due to caching. Sleep before attempting a re-reconcile, to
// give the underlying system some time to propagate the deletion info.
time.Sleep(resourceContext.RecreateDelay)
time.Sleep(time.Second * 10)
Copy link
Collaborator

@yuwenma yuwenma Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is this a revert to a previous const var? I like that we make this a configurable field. It is more manageable and can help future resources which needs additional delays.

Copy link
Collaborator Author

@jasonvigil jasonvigil Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There are no known circumstances where we need to add additional delays. Since there are no more uses, I think we should remove the configurability, because it is now dead code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

"dead code should be cleaned up" --> Right, that makes the KCC code base much better.
I consider this not a dead code but a feature that has been approved working well. For dynamic test, we do not expect the contributors can be able to fix the source code when their resource need some special treatment, the only option for them is to explore the configurables in the resource-context file. Enriching the configurable pools is always appreciated, especially if the feature does not introduce performance regression to the current test. Therefore, can we keep it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aside from this code no longer being used, I also feel like this code is not really a desirable feature. It allowed me to miss an actual bug, which was that my direct controller was not waiting correctly for delete LRO's to complete. From this one example of a previous use, this configuration seems like more like a "foot-gun", rather than a desirable feature. If any contributors hit issues in the future where they feel like something like this is actually necessary, I think not having this configuration available would cause us to examine the situation more closely, rather than allow a "quick workaround" of extending the wait time and potentially miss a similar bug.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok. If it is not approved to be a desired feature, agree that we should drop it .


// get the current state
t.Logf("reconcile with %v\r", testUnstruct)
Expand Down
10 changes: 0 additions & 10 deletions pkg/test/resourcefixture/contexts/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,6 @@ type ResourceContext struct {
// services in GCP that claim to be done with creating / updating the resource before it is actually available.
PostModifyDelay time.Duration

// Time to delay before recreating the resource as part of the drift detection test.
// The default wait time is 10 seconds. However, some resources appear to need to
// wait longer before recreating, so this value is customizable.
RecreateDelay time.Duration
// If true, skip drift detection test.
SkipDriftDetection bool

Expand All @@ -92,12 +88,6 @@ func GetResourceContext(fixture resourcefixture.ResourceFixture, serviceMetadata
rc.ResourceGVK = fixture.GVK
}

if rc.RecreateDelay == 0 {
// By default, wait for 10 seconds before recreating resource as part of drift detection test.
// Some resources appear to need to wait longer than 10 seconds, so this value is customizable.
rc.RecreateDelay = time.Second * 10
}

// If CRD has DCL controller label, fetch DCL schema.
crd, err := crdloader.GetCRDForGVK(rc.ResourceGVK)
if err != nil {
Expand Down
Loading
Loading