fiaas-deploy-daemon icon indicating copy to clipboard operation
fiaas-deploy-daemon copied to clipboard

[WIP] Run fiaas-deploy-daemon on Python 3

Open oyvindio opened this issue 3 years ago • 5 comments

This is more or less a result of using 2to3 and then fixing various things that broke when running the test suite. Some of the changes 2to3 did might be a bit strange, and there are a few things remaining here before this is ready.

One notable change is in the way names for ApplicationStatus is generated. This used to use the hash() builtin, which has changed between python2 and python3 in two ways:

  • python3 uses a different hash algorithm than python2
  • the hash algorithm is seeded with a random value, meaning hashes aren't stable between python processes (i.e. the name we created for a deployment_id won't be the same after restarting fiaas-deploy-daemon.

This becomes a problem in the mechanism to avoid redeploying successfully redeployed applications, which checks for an ApplicationStatus with name appname-$auto_generated_name which has result: SUCCESS. I've changed the name generating function to use a stable hash function (see 4499457), but since the generated names will change completely between python2 and python3, it could lead to redeploying everything when a python3 fiaas-deploy-daemon is rolled out. To avoid this, it might be better to have fiaas-deploy-daemon check for an ApplicationStatus with a label selector like app=$appname,fiaas/deployment_id=$deployment_id and result: SUCCESS.

oyvindio avatar Nov 04 '20 13:11 oyvindio

I gave this a spin locally, and it seems ok. Is there anything missing still you know of?

gregjones avatar Apr 15 '21 14:04 gregjones

I gave this a spin locally, and it seems ok. Is there anything missing still you know of?

Thanks for taking a look and testing this! This branch needs a rebase and re-run of 2to3 for any new code, but otherwise I think it is getting pretty close to ready. I was thinking that it would be a good idea to pick the ApplicationStatus naming/lookup changes (663903a and ed6c384) out of this branch and merge that separately, before merging this branch. This could make rolling back from a python 3 based fiaas-deploy-daemon to a python 2 based fiaas-deploy-daemon safer (if necessary), without unnecessarily triggering redeploys of applications due to the change in how the ApplicationStatuses are looked up.

oyvindio avatar Apr 16 '21 08:04 oyvindio

I've thought some more about the ApplicationStatus naming changes, and I think maybe a better approach could be to set .metadata.generateName when creating ApplicationStatuses to get Kubernetes to just generate a name suffix. The details of how the name is generated has to change in any case, and with the changes in ed6c384, fiaas-deploy-daemon looks up ApplicationStatus resources via the fiaas/deployment_id label to ensure it can find resources generated by a python2 based fiaas-deploy-daemon.

oyvindio avatar Apr 16 '21 11:04 oyvindio

Anything holding this back?

Would be nice to get FDD onto Python 3 so that we could finally sunset Python 2.7 support in fiaas/k8s which would make it possible for me to start working on a feature I kinda want there. :wink:

mortenlj avatar Oct 28 '21 18:10 mortenlj

Anything holding this back?

Would be nice to get FDD onto Python 3 so that we could finally sunset Python 2.7 support in fiaas/k8s which would make it possible for me to start working on a feature I kinda want there. 😉

I think what is remaining here is mainly to rebase the branch and then update it to use py27hash for generating ApplicationStatus names since using a label selector (#153) didn't work too well (see #155 for more details). Then it requires setting aside some time to test this properly. It would indeed be very good to move away from Python 2 both here and in k8s.

oyvindio avatar Nov 03 '21 11:11 oyvindio

Closing this in favor of #197

oyvindio avatar Dec 16 '22 12:12 oyvindio