-
Notifications
You must be signed in to change notification settings - Fork 311
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
[Task] Improve Harvester upgrade responder #1849
Comments
cc @bk201 |
We also need to add harvester/pkg/controller/master/upgrade/version_syncer.go Lines 39 to 40 in 60b2fe4
|
We are planning to have Harvester use the generic version of upgrade-responder at https://github.com/longhorn/upgrade-responder. In general, a version is fully uniquely identified by {name, release date, and tags}. That is why we hard-coded the response in that format. Is there another way to get the ISOURL and ISOChecksum instead of putting it into the upgrade responder's response? |
@PhanLe1010 Thanks for the reply. I understand as an upstream project, the responder wants to be generic. But it will be nice to have a way to extend the fields in the future. @guangbochen @FrankYang0529 I guess we have two choices now:
|
I think "Fork the project and run our own code" may be more friendly to users. |
I like the second approach because of its benefit in the long run:
|
TODO: add MinUpgradableVersion field to the upstream upgrade responder and migrate to it later. |
Pre Ready-For-Testing Checklist
|
Automation e2e test issue: harvester/tests#385 |
@FrankYang0529 I went through the steps here and I didn't see a prompt for an upgrade. I was running this off of the latest master bulid in a virtualized environment. I was wondering, is there is another way I should be testing this? The steps that were outlined seemed to test if the harvester system would be able to access influxdb, but not if influxdb was installed in the system. |
Hi @noahgildersleeve, thanks for helping to test this issue. Since we changed harvester/upgrade-responder to longhorn/upgrade-responder and we also change the request body in harvester, I would like to test whether longhorn/upgrade-responder can work with the latest harvester. We cannot add a dev version for testing on the production server, so we need to start longhorn/upgrade-responder in the local environment. InfluxDB is a dependency of longhorn/upgrade-responder. In macOS, I used the following commands to install and start influxDB.
After you start influxDB, you can use the following commands to start longhorn/upgrade-responder and check whether it can give you a correct response. Make sure you have a version with
After longhorn/upgrade-responder can work, you can update the |
I tested this in master-96b90714-head and I'm unable to trigger the upgrade. I setup the influxdb instance and the commandworked fine, but it does not seem to be triggering the upgrade check when I delete the pods from
influxdb is running with version v1.8.10. I attached the support bundle supportbundle_f8c598c8-4629-42f4-ac3c-d48a8caa2e88_2022-06-22T22-44-01Z.zip |
@noahgildersleeve, sorry, I know the reason. Actually, your harvester gets a new version from the upgrade-responder, but it also needs to send a request to GitHub to get more information. Currently, we don't have
I tried again in my local environment. With correct response.json, I can get an update. |
Checked the first trial on master-9f6f58ac-head ResultCan select the prompted upgrade button by using the updated version of Harvester upgrade responder https://github.com/longhorn/upgrade-responder (v0.1.4) But seems the upgrade did not actually download the image, this may related to the network environment configuration Thus I would perform the second trail to confirm again. Test Information
Verify StepsFollow the steps in harvester/tests#385 (comment)
|
Verify fixed after the second trial on ResultCan select the prompted upgrade button by using the updated version of Harvester upgrade responder https://github.com/longhorn/upgrade-responder (v0.1.4) by using the Test Information
Verify StepsFollow the steps in #1849 (comment)
|
There is still one item for this ticket:
I will handle this one |
The text was updated successfully, but these errors were encountered: