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 checks processing for multiple connectors #351

Merged
merged 10 commits into from
Oct 26, 2021

Conversation

linkous8
Copy link
Contributor

@linkous8 linkous8 commented Oct 21, 2021

  • Set Initial ready value to false so we don't report ready when no
    checks are run
  • Reduce all progressive check event results into single list of checks
    instead of only processing the first result
  • refactor non-progressive check readiness accumulation to report
    unready if no checks are run or any connector responds with empty list
    of checks
  • fix successful prometheus check reporting caused errors when logged
    due to curly braces in the check names
  • fix kubernetes checks shadowed by opsani_dev checks by renaming
    check methods to produce unique check IDs

- Set Initial ready value to false so we don't report ready when no
checks are run
- Reduce all progressive check event results into single list of checks
instead of only processing the first result
- refactor non-progressive check readiness accumulation to report
unready if no checks are run or any connector responds with empty list
of checks
@linear
Copy link

linear bot commented Oct 21, 2021

ENG-503 Servox CLI check command does not process all checks

While testing the HPAPlus connector (ENG-466) I ran across an issue where the opsani_dev checks were not showing up in the output as well as all checks being reported as passing which prevented envoy-proxy from being injected into the target application

Example output of this behavior is attached:

After digging into the check code, I think I've found the source of the issue here:

if result := next(iter(results), None):

What happens is the logic is only processing check results for the first event result in the list of results without processing any subsequent results in that list which means its only processing the checks for a single connector before considering all checks to have passed. I have a proposed fix which I will push for testing shortly after this ticket is created

servo_log1.log

servo_log2.log

@linkous8 linkous8 marked this pull request as draft October 22, 2021 16:40
@linkous8 linkous8 marked this pull request as ready for review October 25, 2021 17:18
- add "kubernetes" to kubernetes test check IDs
- add "opsani_dev" to opsani dev test check IDs
- undo timeout changes to xfaling test test_rollout_check_annotations
Copy link
Contributor

@ekalosak ekalosak left a comment

Choose a reason for hiding this comment

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

LGTM, if I got it right, the changes are:

  1. Multiple checks work now
  2. Curly brackets no longer kill logger, solved using the escaped_name
  3. Naming convention changes in the connectors

Question:

  1. Does the if results have the side effects of iter or next on the results generator? Or is the truthyness check blissfully devoid of side effects?
  2. Why use functools when you could do sum([b.value for b in results])?

@linkous8
Copy link
Contributor Author

Corrections:

  1. Multiple checks work now

Checks from multiple connectors work now

  1. Naming convention changes in the connectors

The check functions have been renamed to avoid duplicate IDs but there is no enforcement of a naming convention WRT to avoiding duplicate IDs yet (Task has been added to the backlog for this)

Answers:

  1. Does the if results have the side effects of iter or next on the results generator? Or is the truthyness check blissfully devoid of side effects?

You may need to elaborate more on what you mean by the side effects but I think the answer is that the truthyness check of the results list is devoid of side effects. The next and iter calls being replaced were the logical equivalent of other languages' firstOrDefault methods which was the source of this bug

  1. Why use functools when you could do sum([b.value for b in results])?

Mainly for styling reasons. @blakewatters established a precedent for avoiding comprehensions in favor of map/filter/reduce calls which I try to follow (I assume he finds it more readable that way)

@ekalosak
Copy link
Contributor

re Blake using map(reduce(lambda)), I think he's the only one... It goes against most Python style guides iirc. I think, unless he shows up himself to deny, this is a Life of Brian moment.

e.g. Google Python style guide

@linkous8 linkous8 merged commit 1ff6ab1 into main Oct 26, 2021
@linkous8 linkous8 deleted the fred/eng-503-servox-cli-check-command-does-not branch October 26, 2021 16:51
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.

3 participants