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

Danielh/eng 833 coalesce multiple opsani dev remedies to #411

Merged

Conversation

DanielHHowell
Copy link
Contributor

No description provided.

@linear
Copy link

linear bot commented Mar 16, 2022

ENG-833 Coalesce multiple Opsani Dev remedies to reduce user app churn

The current approach used by the opsani_dev connector causes the user applicatoin to restart 3(!) times, even though once is enough (if all 3 remedies were applied at the same time).

I consider this a bug as it causes problems for customers (and our support team), especially when the application has a high number of replicas (which means an interesting application) and/or starts slowly. If the opsani_dev-based onboarding will continue to be used, this will need to be resolved.

I acknowledge that fixing this may require design changes/modifying the approach to checks-and-remedies. See some discussion at https://linear.app/opsani/issue/SVX-274/see-about-coalescing-multiple-hints as a starting point.

@DanielHHowell DanielHHowell requested a review from linkous8 March 16, 2022 16:46
@DanielHHowell
Copy link
Contributor Author

Basically there isn't a need for the full readiness check after every remedy, as I see it it's primarily important just to have a required halting prior to the tuning pod check to ensure everything was succesfully ingested before its creation

Copy link
Contributor

@linkous8 linkous8 left a comment

Choose a reason for hiding this comment

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

from a cursory glance a couple of items:

  • I dont see any automated unit/integration tests added that cover both types of the configuration
  • You added a CLI flag for the checks which is good but I thought there was some kind of check configuration section for the servo.yaml. Can we get it added there as well as it may be required for environment variable configuration

@DanielHHowell
Copy link
Contributor Author

from a cursory glance a couple of items:

  • I dont see any automated unit/integration tests added that cover both types of the configuration
  • You added a CLI flag for the checks which is good but I thought there was some kind of check configuration section for the servo.yaml. Can we get it added there as well as it may be required for environment variable configuration

@linkous8 I haven't found a check configuration outside of the CLI flag. Also I don't see any tests that cover the flags of the relevant CLI section, will see about adding that. Do you think an integration test running the two variants of checking on say, fiber-http would be good?

@linkous8
Copy link
Contributor

@linkous8 I haven't found a check configuration outside of the CLI flag.

Lets go ahead and add a new ChecksConfiguration model for that, say, around here:

Long term, I'm looking to streamline the CLI entrypoint to make the flow more understandable and will be preferring file/env var based configuration over CLI flags as the latter is more difficult to keep track of and alter programmatically

Also I don't see any tests that cover the flags of the relevant CLI section, will see about adding that. Do you think an integration test running the two variants of checking on say, fiber-http would be good?

See if you can adapt this test to test both variants of the configuration:

async def test_install_wait(

@DanielHHowell DanielHHowell requested a review from linkous8 April 25, 2022 20:10
Copy link
Contributor

@linkous8 linkous8 left a comment

Choose a reason for hiding this comment

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

On the right track 🚂

Copy link
Contributor

@linkous8 linkous8 left a comment

Choose a reason for hiding this comment

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

A few minor nitpicks aside, LGTM! 🚀

@DanielHHowell DanielHHowell merged commit d00a032 into main May 4, 2022
@DanielHHowell DanielHHowell deleted the danielh/eng-833-coalesce-multiple-opsani-dev-remedies-to branch May 4, 2022 19:05
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