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 azure mailer deployment issue #6325

Merged
merged 10 commits into from Dec 4, 2020
Merged

Conversation

h-dub
Copy link
Contributor

@h-dub h-dub commented Dec 3, 2020

Fix for #6012
pkg_resources seems to be unneeded and is causing packaging issues when deploying the azure function

@h-dub h-dub requested a review from kapilt as a code owner December 3, 2020 07:55
@kapilt
Copy link
Collaborator

kapilt commented Dec 3, 2020

can your confirm successful e2e message delivery with the change?

tracking back on why pkg_resources was included it looks like it was splunk support in #4044 .. and it looks like azure was working afterwards, but i suspect the function version change in the msi/csi support may have resulted in the issue here.

its also a little unclear why this would fix as afaics pkg_resources never makes it into the requirements file (testing on osx, py 3.8 though)

>>> from c7n_mailer.azure_mailer import deploy
>>> reqs_txt = deploy.get_mailer_requirements()
>>> "pkg_resources" in reqs_txt
False

doing a fresh install in a virtualenv its a little hard to see any explicit dep on pkg_resources in the output. the usages modes between the two providers are a little different though, CORE_DEPS is assembled in aws from local imports, where as in azure we generate a requirements file server side installation and pkg_resources itself comes from setuptools which would always be installed.

a safer solution would be to add it to azure_mailer/deploy generate_requirements ignore.. but given its not in the output of that function its a little more unclear to me why it would resolve.

@h-dub
Copy link
Contributor Author

h-dub commented Dec 4, 2020

Looks like the root cause is a bug between ubuntu and venv that shows as fixed in may
It incorrectly adds pkg_resources==0.0.0.0 on pip freeze
https://bugs.launchpad.net/ubuntu/+source/python-pip/+bug/1635463

@h-dub
Copy link
Contributor Author

h-dub commented Dec 4, 2020

my guess is the original author of the splunk change was trying to figure out which deps needed to be added and was working from a pip freeze with the bug. this matches the vague comment on the pkg_resources dep being added.

we should have never needed pkg_resources as a listed dep
i will validate its working in aws without pkg_resources:
if it works in aws, we can take that fix
if it fails in aws, I have a second working fix that just excludes it from azure function packaging

@h-dub
Copy link
Contributor Author

h-dub commented Dec 4, 2020

in aws lambda, it works because there is a pkg_resources=0.0.0 available as a package and it is installed :)
everywhere else, pkg_resources isn't available as a package and should not be in the requirements.txt
I will update this PR with the azure specific fix on the next cycle. We should be able to accept it with minimal risk.

@kapilt
Copy link
Collaborator

kapilt commented Dec 4, 2020

pkg_resources comes in via setuptools, which isn't in the aws environment, we actually upload clientside by name into the package, but in gcp and azure we do server side building.

@kapilt
Copy link
Collaborator

kapilt commented Dec 4, 2020

working with hugh i was able to identify why i never hit this on e2e testing because i was using pyenv installed python, which differs significantly from system python installs at least on ubuntu wrt to distutils/setuptools.

Copy link
Collaborator

@kapilt kapilt left a comment

Choose a reason for hiding this comment

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

thanks, lgtm

@kapilt kapilt merged commit 3734652 into cloud-custodian:master Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants