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(envoy): upgrade models in experiment #5874

Merged

Conversation

sakoush
Copy link
Member

@sakoush sakoush commented Sep 5, 2024

What this PR does / why we need it:

This change addresses 2 issues related to Experiments:

  • When upgrading to a new version of a model, an experiment route in envoy containing this model would still point to the old (and the new version). This is problematic as we unload the old versions after a new version in made available.
  • When upgrading to a new version of model and in an experiment that has a Default route pointing to this model, the old versions of the model are not unloaded. This is incorrect behaviour.

The changes are respectively:

  • When we process events to update an experiment due to a model change, we first remove an existing state (which had the old route) before creating the routes for the experiment which updated models etc.
  • We always clean old versions of models regardless of how many routes existed for it. This was introduced initially to guard against 404s which is not true anymore as we have a first remove the routes from envoy (UnloadEnvoyRequested) before unloading a model (UnloadRequested).

Which issue(s) this PR fixes:

Fixes # INFRA-1001 (internal)

Special notes for your reviewer:

@sakoush sakoush requested a review from lc525 as a code owner September 5, 2024 17:27
@sakoush sakoush force-pushed the INFRA-1001/upgrade_models_in_experiment branch from 2456147 to 3fea100 Compare September 5, 2024 17:50
lc525

This comment was marked as duplicate.

Copy link
Member

@lc525 lc525 left a comment

Choose a reason for hiding this comment

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

lgtm;
I haven't done a smoke test in kind but everything looks ok code-wise. For some reason I thought this would require a larger change.

@lc525 lc525 added the v2 label Sep 5, 2024
@sakoush
Copy link
Member Author

sakoush commented Sep 6, 2024

lgtm; I haven't done a smoke test in kind but everything looks ok code-wise. For some reason I thought this would require a larger change.

I have done a smoke test in kind and added the steps for release testing.

@sakoush sakoush merged commit 23a0fb1 into SeldonIO:v2 Sep 6, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants