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

Support backward compatibility for old profiles #7062

Closed
eashi opened this issue Mar 16, 2020 · 12 comments
Closed

Support backward compatibility for old profiles #7062

eashi opened this issue Mar 16, 2020 · 12 comments
Labels
area/profiles issues related to profile handling kind/bug Categorizes issue or PR as related to a bug. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.

Comments

@eashi
Copy link

eashi commented Mar 16, 2020

I have installed the latest minikube version v1.8.1 (I can't remember the previous version), but then all my clusters wouldn't work because their profiles became invalid.

I checked the code that reads the configuration from the profile and obviously the structure of the profile has changed.

I suggest to keep the profiles backward compatible as far back as possible, and display warnings about attributes that will be changed in the profile rather than prompting the users to delete the profile.

If you feel that's the right way I can start with putting back some of the renamed attributes?

The exact command to reproduce the issue:
minikube profile list

The full output of the command that failed:

⚠️ Found 4 invalid profile(s) !
keda-external-scaler
minikube
twitch-keda
twitter-scaler
💡 You can delete them using the following command(s):
$ minikube delete -p keda-external-scaler
$ minikube delete -p minikube
$ minikube delete -p twitch-keda
$ minikube delete -p twitter-scaler

The operating system version:
MacOS Catalina

@afbjorklund
Copy link
Collaborator

Possibly related to #6969 (not sure if it is enough)

@eashi
Copy link
Author

eashi commented Mar 17, 2020

Thanks a lot @afbjorklund for the prompt reply.

I think the PR you mentioned took it to a good position, but I still get invalid profile due to the attribute "Driver" instead for "VMDriver". This is where it fails:

if p.Config.Driver == "" {

@afbjorklund
Copy link
Collaborator

So related to #6620 - @sharifelgamal ?

@sharifelgamal
Copy link
Collaborator

Yep, that accidentally broke the profiles list command. It's fixable.

@sharifelgamal sharifelgamal added kind/bug Categorizes issue or PR as related to a bug. area/profiles issues related to profile handling priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Mar 17, 2020
@eashi
Copy link
Author

eashi commented Mar 29, 2020

I have been thinking about this, I think we can solve this elegantly by creating a mapping between a released version (in which the config format is changed) and a corresponding function to load the profile and translate it to the latest version.

Once the user wants to load a profile, the loader would try each mapping starting from the latest version until the earliest version we want to support, and we provide a message output as well (maybe a string property in the Profile we call stateMessage). The message would be like: "It seems that your profile is old, please update your profile manually, we are translating it for you this time :)"

I wrote about my experience about older profile here in case it helps with this issue: https://www.emadashi.com/2020/03/how-to-fix-minikube-invalid-profile/

This will require some work, but would much better experience. If there is enough pain out there it might be worth it.

@eashi
Copy link
Author

eashi commented Mar 30, 2020

I can take the first stab implementing this if you folks like the idea.

@sharifelgamal
Copy link
Collaborator

I like the idea of automatically translating from the old profile format to the new one, I would hope that we wouldn't change the format so often that need to formally version each release, but I would be happy to review a PR to fix this.

@priyawadhwa
Copy link

Hey @eashi -- are you still working on this?

@priyawadhwa priyawadhwa added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels May 28, 2020
@eashi
Copy link
Author

eashi commented May 29, 2020

Hey @priyawadhwa , I have created a draf PR to consult with the maintainers if this is a direction they are comfortable with. I didn't receive any comments on the PR and I didn't want spend time on it without the guidance.

I would love to come back to it and work on it more, but I need the feedback above :).

@priyawadhwa
Copy link

Hey @eashi thank you! I didn't notice the draft PR -- I just opened up the PR to give it some visibility. I'll try to take a look at it in the next couple days.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 27, 2020
@tstromberg
Copy link
Contributor

Fixed in v1.12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/profiles issues related to profile handling kind/bug Categorizes issue or PR as related to a bug. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants