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

fix(csv): CSV update process optimization regression #407

Merged
merged 9 commits into from
Aug 7, 2018

Conversation

ecordell
Copy link
Member

@ecordell ecordell commented Aug 5, 2018

I recommend reviewing by commit.

This was an adventure:

  • The first commit fixes the actual issue; somewhere in the past we lost the GC optimization that can mark multiple CSVs at once
  • When fixing that, I switched to using the typed clients instead of operatorclient's CustomResourceClient
  • Because of that, I needed to write a test using the fakes instead of the mocks.
  • The test was so much nicer and easier to write than the mocks, that I switched the rest of the tests for the OLM operator to it.
  • Because we no longer needed the mocks I was able to delete large chunks of test-only code.
  • As part of my refactoring, I broke the re-queuing of CSVs to check for changes. Instead of fixing it short-term, I went ahead and implemented a watch on deployments, which will trigger a sync on any CSVs that own them. This addresses ALM-342
  • When verifying my work I fixed some issues in the e2e tests. I'm not 100% sure I removed all flakes, but there may be some overlap with Fix e2e Tests #406

Before merging I want to play with this on a real cluster, but everything looks good locally.

Also:

  • e2e tests now run in 2 minutes 32 seconds, down from 7 minutes 20 seconds
  • coverage is up ~2% on olm operator

@ecordell ecordell requested review from njhale and alecmerdler August 5, 2018 22:34
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// configure cluster state
clientFake := fake.NewSimpleClientset(tt.initial.csvs...)
Copy link
Member

Choose a reason for hiding this comment

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

Really cool.

@ecordell ecordell merged commit 3e7cf1f into operator-framework:master Aug 7, 2018
@ecordell ecordell deleted the csv-upgrade-gc branch August 7, 2018 14:01
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.

2 participants