Skip to content

Conversation

@aleksmaus
Copy link
Contributor

What does this PR do?

Utilize new ES fleet polling API for global checkpoint monitoring
elastic/elasticsearch#71093

Tested with the agent actions and osquerybeat, all works.

The tests will fail until the new ES API is merged.
Posting is as draft until then.

Why is it important?

This is more efficient that polling every second as we do right now.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas

@aleksmaus aleksmaus added the enhancement New feature or request label Apr 5, 2021
@elasticmachine
Copy link
Contributor

elasticmachine commented Apr 5, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #200 updated

  • Start Time: 2021-04-15T12:38:00.102+0000

  • Duration: 14 min 15 sec

  • Commit: cf7122d

Test stats 🧪

Test Results
Failed 0
Passed 69
Skipped 0
Total 69

Trends 🧪

Image of Build Times

Image of Tests

@aleksmaus
Copy link
Contributor Author

aleksmaus commented Apr 13, 2021

Updated implementation to the latest API changes, tested end-to end with agent/osquerybeat. Once the ES PR merges, can kick off the CI build again, to get it to pass the tests.

@aleksmaus
Copy link
Contributor Author

The fleet system index polling API is available now with the latest elasticsearch docker image, just verified the integration tests are passing now. Opening up for code review.

@aleksmaus aleksmaus marked this pull request as ready for review April 15, 2021 13:09
Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

Looks good.

Didn't find anything that stood out. I want to do some testing with service tokens, once this is in to ensure those work with this as well. But no need to block on that.

Copy link

@scunningham scunningham left a comment

Choose a reason for hiding this comment

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

Looks ok. I'm not crazy about the special configuration casing on es.NewClient. I think we need to rethink how that works.

case m.outCh <- hits:
maxVal := hits[sz-1].SeqNo
m.storeCheckpoint(maxVal)
m.storeCheckpoint([]int64{maxVal})

Choose a reason for hiding this comment

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

type seqNo?

@aleksmaus
Copy link
Contributor Author

Looks ok. I'm not crazy about the special configuration casing on es.NewClient. I think we need to rethink how that works.

I'll add to my todo list past FF: to refactor the new client initialization

@aleksmaus aleksmaus merged commit 13a5550 into elastic:master Apr 15, 2021
mergify bot pushed a commit that referenced this pull request Apr 15, 2021
* Utilize fleet polling API for global checkpoint monitoring

* Adjust for API changes, configurable poll timeout

* Update retryDelay to 3 secs

* Update tests for monitor API change

* Adjust to the latest API changes

* Remove fleet indexes bootstrapping for tests, it is done by fleet system index plugin now

* Fix unit tests

(cherry picked from commit 13a5550)
mergify bot added a commit that referenced this pull request Apr 15, 2021
…) (#243)

* Utilize fleet polling API for global checkpoint monitoring

* Adjust for API changes, configurable poll timeout

* Update retryDelay to 3 secs

* Update tests for monitor API change

* Adjust to the latest API changes

* Remove fleet indexes bootstrapping for tests, it is done by fleet system index plugin now

* Fix unit tests

(cherry picked from commit 13a5550)

Co-authored-by: Aleksandr Maus <aleksandr.maus@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request v7.13.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants