Use kube-rs resource watcher instead of Api::watch
#378
+354
−246
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kate-goldenring
requested review from
bfjelds,
Britel,
jiria and
romoh
as code owners
bfjelds
reviewed
Sep 10, 2021
| @@ -228,7 +227,7 @@ async fn handle_config_delete( | |||
| kube_interface: &impl KubeInterface, | |||
| config: &Configuration, | |||
| config_map: ConfigMap, | |||
| ) -> Result<(), Box<dyn std::error::Error + Send + Sync + 'static>> { | |||
| ) -> anyhow::Result<()> { | |||
bfjelds
reviewed
Sep 10, 2021
| } | ||
| if *first_event { |
There was a problem hiding this comment.
nit: not sure what is more expensive, branching logic or setting logic, but it might be simpler to just always set *first_event=false here?
There was a problem hiding this comment.
Good question. From a quick search i cant find an exploration someone has done on that -- might be fun to test. In the meantime, I think you're right that it makes more sense to just set it.
bfjelds
approved these changes
Sep 10, 2021
bfjelds
reviewed
Sep 10, 2021
agent/src/util/config_action.rs
Outdated
| info!("handle_config - watcher started"); | ||
| } else { | ||
| return Err(anyhow::anyhow!( | ||
| "Configuration watcher restarted - throwing error" |
There was a problem hiding this comment.
maybe say "Configuration watcher restarted - throwing error to restart agent"
kate-goldenring
force-pushed
the
use-watcher
branch
from
e8c59a9 to
2837828
Compare
vincepnguyen
pushed a commit
that referenced
this pull request
Nov 23, 2021
Signed-off-by: vincepnguyen <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
closes #374
Special notes for your reviewer:
We updated to using the
Api::watchobject inkube-rsv0.59.0in #361, expecting it to work similarly as the informer we were previously using to watch for actions on Pods, Nodes, Instances, Configurations. However, we have experienced different behavior, resulting in ref #372. and usingApi::watchinstead ofkube::runtime::watcher. I was able to reproduce unexpected behavior locally when using the current implementation. If there are pre-existing configurations,handle_config_addis called twice, once from where we handle pre-existing configs and the other from theApi::watch.Api::watchappears to pull previous events before the Agent starts, which is fairly unpredictable. Usingwatchershould resolve this.Note: currently not handling restart events other than logging them. This is due to:1. The fact that we handledWatchEvent::Errorpreviously just by logging2. A restart event occurs on start up (aka is a start event too). We'd need to know it is the first restart event and handle that differently than others. We could pass afirst_eventboolin and error so long as it is not the first event.In 19a44f9, made it so the watchers throw an error if they receive a restarted event that is not the initial start event. This will cause the controller or agent containers to error and restart and properly reconcile.
If applicable:
cargo fmt)cargo build)cargo clippy)cargo test)cargo doc)./version.sh)