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

Add resource version for status reporting and update configuration types #1004

Merged
merged 8 commits into from
Mar 3, 2023

Conversation

pkosiec
Copy link
Member

@pkosiec pkosiec commented Mar 3, 2023

Description

Changes proposed in this pull request:

  • Update approach for getting Botkube config
  • Add resource version for status reporting
  • Add retries for status reporting
  • Modify Botkube config to use it externally
    • Fix marshalling plugins under source / executors
    • To allow configuring logger, I needed to make this config public.
    • I needed to reorganize the code to avoid import cycles.
  • Add skeleton for deployment config updater
    • currently it updates resource version only for status reporting

Testing

Run with remote config:

export CONFIG_PROVIDER_IDENTIFIER="c6100422-89a7-42ee-a0bf-ebcb24cbb785"
export CONFIG_PROVIDER_ENDPOINT="http://localhost:8080/graphql"
go run ./cmd/botkube/main.go

@pkosiec pkosiec added the enhancement New feature or request label Mar 3, 2023
@pkosiec pkosiec marked this pull request as ready for review March 3, 2023 09:47
@pkosiec pkosiec requested review from a team and PrasadG193 as code owners March 3, 2023 09:47
@pkosiec pkosiec changed the title Add resource version for status reporting Add resource version for status reporting and update configuration types Mar 3, 2023
commGroupFieldKey = "commGroup"
healthEndpointName = "/healthz"
printAPIKeyCharCount = 3
configUpdaterInterval = 15 * time.Second
Copy link
Member Author

Choose a reason for hiding this comment

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

I will make this configurable in one of later PRs.

@@ -326,23 +326,22 @@ sources:
my-annotation: "true"
labels:
my-label: "true"
plugins:
Copy link
Member Author

Choose a reason for hiding this comment

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

This was invalid nesting when marshalling config, which was fixed in this PR 🙂

@josefkarasek
Copy link

I see 2 issues:

  • Botkube issues request with invalid resource version - we just fail. But we should try to get valid resource version and try again.
  • Future extensibility - new component will modify remote Deployment and will get new resource version. How can this resource version be shared with other components, such as status reporter?

Both these issues stem from the push semantics of distributing resource version.
Eg k8s approach is list & watch and get when update fails on invalid resource version.

@pkosiec
Copy link
Member Author

pkosiec commented Mar 3, 2023

Future extensibility - new component will modify remote Deployment and will get new resource version. How can this resource version be shared with other components, such as status reporter?

The idea is that Config Updater will update the resource version. See this snippet (setResourceVersionForAll): https://github.com/kubeshop/botkube/pull/1004/files#diff-d3a4641227ada1de58b084392d0235221af3fccff95a05fae276381fd76f13c9R92-R97

@josefkarasek
Copy link

Future extensibility - new component will modify remote Deployment and will get new resource version. How can this resource version be shared with other components, such as status reporter?

The idea is that Config Updater will update the resource version. See this snippet (setResourceVersionForAll): https://github.com/kubeshop/botkube/pull/1004/files#diff-d3a4641227ada1de58b084392d0235221af3fccff95a05fae276381fd76f13c9R92-R97

That's every interval:15s - all update operations will have to assume their resource version is invalid and include retry on failure. That's OK, but we should remember to do this retry check.

@pkosiec
Copy link
Member Author

pkosiec commented Mar 3, 2023

all update operations will have to assume their resource version is invalid and include retry on failure

Not really, we should just report error and let user try again do that operation (e.g. edit SourceBindings or disable notifications). We should retry only if this is something only Deployment should do (e.g. update status, like in this case).

@pkosiec
Copy link
Member Author

pkosiec commented Mar 3, 2023

@josefkarasek retry added - you can test it by starting the backend server, running Botkube to fetch the initial configuration and then quickly close it 😄 Or wait for startup reporting and then close Botkube to report shutdown.

Copy link

@josefkarasek josefkarasek left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@pkosiec pkosiec merged commit efd7cd5 into kubeshop:main Mar 3, 2023
@pkosiec pkosiec deleted the deployment-config branch March 3, 2023 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants