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

Jump to bottom

Added Preflight Update Checks #3365

Closed
wants to merge 2 commits into from
Closed

Added Preflight Update Checks #3365

wants to merge 2 commits into from

Conversation

carvalhe
Copy link
Contributor

Which issue this PR addresses:

Fixes https://issues.redhat.com/browse/ARO-3854

What this PR does / why we need it:

Currently we have preflight set up for create operations but also need it for update. These changes allow for both put/patch operations on preflight so that it can leave the testing stage and go live. This PR also cleans up the way we validate for preflight and adds new checks for it as well. There was a previous PR for this here: #3181 but it was getting messy, and issues were arising so creating this new PR instead.

Test plan for issue:

Created additional unit tests that are similar to put/patch tests. Kusto logs also show preflight success and failures with more details than before.

mociarain
mociarain previously approved these changes Jan 24, 2024
View reviewed changes
Copy link
Collaborator

@mociarain mociarain left a comment

Choose a reason for hiding this comment

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

LGTM and sorry for being the person who gets my random questions :D

pkg/api/preflight.go Show resolved Hide resolved
pkg/frontend/openshiftcluster_preflightvalidation.go Show resolved Hide resolved
SudoBrendan
SudoBrendan reviewed Feb 2, 2024
View reviewed changes
Copy link
Collaborator

@SudoBrendan SudoBrendan left a comment

Choose a reason for hiding this comment

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

Overall LGTM, I worry about a few internal-error-looking cases being returned to customers directly.

pkg/frontend/openshiftcluster_preflightvalidation.go Outdated Show resolved Hide resolved
pkg/frontend/openshiftcluster_preflightvalidation.go Outdated Show resolved Hide resolved
pkg/frontend/openshiftcluster_preflightvalidation_test.go Show resolved Hide resolved
jaitaiwan
jaitaiwan reviewed Feb 15, 2024
View reviewed changes
Copy link
Contributor

@jaitaiwan jaitaiwan left a comment

Choose a reason for hiding this comment

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

LGTM apart from Brendan's concerns about returning internal errors to our customers. I am also concerned that based on recently experiences, that a 404 might just mean "it's not in the db yet". It might be good to make sure that cosmosdb does indeed return 404 for "it's not there and you shouldn't wait".

@carvalhe carvalhe requested a review from tiguelu as a code owner April 29, 2024 22:27
SudoBrendan
SudoBrendan requested changes May 17, 2024
View reviewed changes
Copy link
Collaborator

@SudoBrendan SudoBrendan left a comment

Choose a reason for hiding this comment

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

Hey Brian, sorry to take so long on re-review. I really like the changes, thank you!

I mostly have nits here - feel free to ignore/action them. I think it is worth reverting/rewriting the Subscription test data so we don't conflate Subscription docs with OpenShiftCluster docs.

pkg/frontend/openshiftcluster_preflightvalidation.go Outdated Show resolved Hide resolved
pkg/frontend/openshiftcluster_preflightvalidation.go Outdated Show resolved Hide resolved
pkg/frontend/openshiftcluster_preflightvalidation_test.go Outdated Show resolved Hide resolved
pkg/frontend/openshiftcluster_preflightvalidation_test.go Outdated Show resolved Hide resolved
pkg/frontend/openshiftcluster_preflightvalidation_test.go Show resolved Hide resolved
pkg/frontend/openshiftcluster_preflightvalidation_test.go Outdated Show resolved Hide resolved
pkg/frontend/openshiftcluster_preflightvalidation_test.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added needs-rebase branch needs a rebase and removed ready-for-review labels Jun 6, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 6, 2024

Please rebase pull request.

@github-actions github-actions bot removed the needs-rebase branch needs a rebase label Jun 12, 2024
@SudoBrendan
Copy link
Collaborator

/azp run ci

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@cadenmarchese
Copy link
Collaborator

/azp run e2e

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@cadenmarchese
Copy link
Collaborator

/azp run e2e

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

cadenmarchese
cadenmarchese reviewed Jun 24, 2024
View reviewed changes
Copy link
Collaborator

@cadenmarchese cadenmarchese left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The e2e failure should be resolved before merge (the branch might need to be rebased) but the changes look good to me! I left a comment on something I noticed during review we might want to fix while we're here.

pkg/frontend/openshiftcluster_preflightvalidation.go Outdated
@@ -52,9 +52,9 @@ func (f *frontend) preflightValidation(w http.ResponseWriter, r *http.Request) {
continue
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't all subsequent validation fail if we continue here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessarily as we are unmarshalling each resource separately. If one fails to unmarshall it doesn't mean the rest will, and according to preflight, is not considered a validation failure either.

@cadenmarchese
Copy link
Collaborator

/azp run e2e

@azure-pipelines Azure Pipelines
Copy link

No pipelines are associated with this pull request.

@carvalhe carvalhe mentioned this pull request Jul 12, 2024
Merged
@carvalhe carvalhe closed this Jul 12, 2024
@carvalhe carvalhe deleted the preflight_add_update branch July 12, 2024 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaitaiwan jaitaiwan jaitaiwan left review comments

@cadenmarchese cadenmarchese cadenmarchese left review comments

@SudoBrendan SudoBrendan SudoBrendan requested changes

@mociarain mociarain mociarain left review comments

@jewzaam jewzaam Awaiting requested review from jewzaam jewzaam is a code owner

@bennerv bennerv Awaiting requested review from bennerv bennerv is a code owner

@hawkowl hawkowl Awaiting requested review from hawkowl hawkowl is a code owner

@rogbas rogbas Awaiting requested review from rogbas rogbas is a code owner

@petrkotas petrkotas Awaiting requested review from petrkotas petrkotas is a code owner

@jharrington22 jharrington22 Awaiting requested review from jharrington22 jharrington22 is a code owner

@cblecker cblecker Awaiting requested review from cblecker cblecker is a code owner

@UlrichSchlueter UlrichSchlueter Awaiting requested review from UlrichSchlueter UlrichSchlueter is a code owner

@s-amann s-amann Awaiting requested review from s-amann

@Shivkumar13 Shivkumar13 Awaiting requested review from Shivkumar13

@yjst2012 yjst2012 Awaiting requested review from yjst2012 yjst2012 is a code owner

@anshulvermapatel anshulvermapatel Awaiting requested review from anshulvermapatel anshulvermapatel is a code owner

@hlipsig hlipsig Awaiting requested review from hlipsig hlipsig is a code owner

@dofinn dofinn Awaiting requested review from dofinn

@tiguelu tiguelu Awaiting requested review from tiguelu tiguelu is a code owner

@SrinivasAtmakuri SrinivasAtmakuri Awaiting requested review from SrinivasAtmakuri SrinivasAtmakuri is a code owner

@AldoFusterTurpin AldoFusterTurpin Awaiting requested review from AldoFusterTurpin

@kimorris27 kimorris27 Awaiting requested review from kimorris27 kimorris27 is a code owner

@tsatam tsatam Awaiting requested review from tsatam tsatam is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@carvalhe @SudoBrendan @cadenmarchese @jaitaiwan @bennerv @mociarain