djangae icon indicating copy to clipboard operation
djangae copied to clipboard

Switch to gcloud

Open Kazade opened this issue 5 years ago • 37 comments

Fixes #972

Summary of changes proposed in this Pull Request:

  • Stop assuming the App Engine SDK is in the tree
  • Update sandbox to deal with latest gcloud SDK
  • Replace the remote sandbox with a remote command
  • Remove support for Django < 1.11

PR checklist:

  • [x] Updated relevant documentation
  • [x] Updated CHANGELOG.md
  • [ ] Added tests for my change

Kazade avatar Mar 07 '19 13:03 Kazade

Wow! Awesome! You rock!

jacobg avatar Mar 07 '19 13:03 jacobg

Will the remote command work with datastore, tasks and memcache?

jacobg avatar Mar 07 '19 14:03 jacobg

Yeah I'm hoping so!

Here's what's been done so far:

  • Removed the remote sandbox
  • Added a replacement 'remote' command (I intend to monkeypatch the pickling issue)
  • Assumed that we're using gcloud everywhere
  • Updated Travis to work with gcloud + Django 1.11 only

Kazade avatar Mar 08 '19 20:03 Kazade

Love it!

jacobg avatar Mar 08 '19 20:03 jacobg

Well, this is fun!

class SandboxAccessPreventionImportHook(object):
  """An import hook that prevents user code from accessing the sandbox."""

  def find_module(self, fullname, unused_path=None):
    return self if fullname == __name__ else None

  def load_module(self, fullname):
    raise ImportError(
        'Importing the devappserver sandbox module ({}) from user '
        'application code is not permitted. Please remove this '
        'import.'.format(fullname))

Google have made it impossible to import the sandbox module.

Kazade avatar Mar 08 '19 20:03 Kazade

Hey it's on the dev machine. Why not just make a script to change it?

This problem goes away in Python 3, right?

jacobg avatar Mar 08 '19 20:03 jacobg

I'd appreciate some testing of this before it's merged. Particularly the new remote command.

Kazade avatar Mar 11 '19 12:03 Kazade

@Kazade Thanks for all the updates! I'd like to test this. I'm first updating to 1.9.12. One question is about the divergence between the testapp and djangae-scaffold. I've used djangae-scaffold as my template, but it looks like testapp is more recently updated, particularly install_deps.py. The install_deps.py is quite different, including different directory for the pip-installed stuff. What is the official template moving forward?

jacobg avatar Mar 11 '19 16:03 jacobg

I suppose that testapp is just for running the unit tests, but install_deps did change a bit. What changes should we make to the scaffold (i.e., to our own app that is based on it) to use gcloud sdk? Thanks!

jacobg avatar Mar 11 '19 16:03 jacobg

@Kazade I'm trying to test your gcloud branch, updating some of my bootstrap code which is based on djangae-scaffold. When running python manage.py test tests, I get this error:

Traceback (most recent call last):
  File "manage.py", line 44, in <module>
    test_execute_from_command_line(sys.argv)
  File "/Users/jacob/workspace/f/gae/src/sitepackages/prod/djangae/core/management/__init__.py", line 90, in test_execute_from_command_line
    return _execute_from_command_line(sandbox.TEST, argv or sys.argv, **sandbox_overrides)
  File "/Users/jacob/workspace/f/gae/src/sitepackages/prod/djangae/core/management/__init__.py", line 64, in _execute_from_command_line
    **sandbox_overrides
  File "/usr/local/Cellar/python@2/2.7.16/Frameworks/Python.framework/Versions/2.7/lib/python2.7/contextlib.py", line 17, in __enter__
    return self.gen.next()
  File "/Users/jacob/workspace/f/gae/src/sitepackages/prod/djangae/sandbox.py", line 392, in activate
    sys.path[0:0] = [_find_sdk_from_path()]
  File "/Users/jacob/workspace/f/gae/src/sitepackages/prod/djangae/sandbox.py", line 99, in _find_sdk_from_path
    path = subprocess.check_output([which, _SCRIPT_NAME]).strip()
  File "/usr/local/Cellar/python@2/2.7.16/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 223, in check_output
    raise CalledProcessError(retcode, cmd, output=output)
subprocess.CalledProcessError: Command '['which', 'dev_appserver.py']' returned non-zero exit status 1

It looks like the sandbox expects dev_appserver to be on the path, and the only place I see where the bootstrap code tries to add it to the path is fix_path adding google_appengine from the /lib directory. But isn't that the old way? I would think google_appengine doesn't exist there anymore?

jacobg avatar Mar 11 '19 19:03 jacobg

Gcloud installed dev_appserver on the path for me automatically, is that not the case for you? What platform are you on?

Kazade avatar Mar 12 '19 11:03 Kazade

Mac. I just manually symlinked dev_appserver.py to /usr/local/bin and got past that problem. I'm wondering though why fix_path has this code:

        appengine_path = os.path.join(lib_path, "google_appengine")
        sys.path.insert(0, appengine_path)

It's not in lib anymore, right?

Next problem: I have a custom fix_path to deal with package name conflict with google.auth:

    # google-auth is installed in sitepackages, but its package name `google`
    # conflicts with the app engine sdk `google` package, so we need to add
    # it to the sdk `google` package path.
    import google
    google_auth_path = join(PROD_SITEPACKAGES_DIR, 'google')
    if google_auth_path not in google.__path__:
        google.__path__ = [google_auth_path] + google.__path__

But google itself is not (yet) on the package path, so I have to try to figure that one out.

jacobg avatar Mar 12 '19 12:03 jacobg

I figured it out.

  1. The GCP docs have instructions how to add to path here: https://cloud.google.com/appengine/docs/standard/python/tools/using-local-server#Python_Running_the_development_web_server -- click on link "dev_appserver.py not working?"

  2. Also, since I'm messing with python package path outside of dev_appserver.py (in fix_path), I also needed to add google_appengine to PYTHONPATH. So I added the following to my .bash_profile:

GOOGLE_APP_ENGINE_PATH=/usr/local/Caskroom/google-cloud-sdk/latest/google-cloud-sdk/platform/google_appengine
export PYTHONPATH=$PYTHONPATH:$GOOGLE_APP_ENGINE_PATH

That path is based on installing gcloud using homebrew on MacOS.

jacobg avatar Mar 12 '19 13:03 jacobg

Update:

  1. All my app's unit tests run great.
  2. Able to do local shell.
  3. Able to do runserver, but had to set sandbox overrides in my manage.py file to set enable_host_checking false in order to allow runserver 0.0.0.0:8002. I also had to explicitly set port option as 8002 in sandbox overrides, because it ignores the command line arg and defaults to 8000.
  4. Able to deploy using gcloud app deploy, and basic service seems run fine remotely. I did have to remove some deprecated app.yaml fields, because gcloud complained about them: application, module, and version.
  5. Having problems with remote shell. Firstly, it complains that it wants application field app.yaml, even with also specifying --project=PROJECT_ID on command line, but I had to remove it earlier to deploy! So I temporarily put it back in order to test the remote shell. Then when running the command, it opens browser to get creds. After entering creds, browser closes, and the following error is printed in console before exiting:
INFO     2019-03-12 14:07:58,603 client.py:570] Attempting refresh to obtain initial access_token
INFO     2019-03-12 14:07:58,604 client.py:872] Refreshing access_token
INFO     2019-03-12 14:07:58,982 stub_util.py:357] Applying all pending transactions and saving the datastore
Traceback (most recent call last):
  File "./manage.py", line 52, in <module>
    execute_from_command_line(sys.argv, **options)
  File "/Users/jacob/workspace/f-service-dev/gae/src/sitepackages/prod/djangae/core/management/__init__.py", line 43, in execute_from_command_line
    return _execute_from_command_line(djangae_namespace.sandbox, argv, parser=djangae_parser, **overrides)
  File "/Users/jacob/workspace/f-service-dev/gae/src/sitepackages/prod/djangae/core/management/__init__.py", line 75, in _execute_from_command_line
    raise
  File "/usr/local/Cellar/python@2/2.7.16/Frameworks/Python.framework/Versions/2.7/lib/python2.7/contextlib.py", line 35, in __exit__
    self.gen.throw(type, value, traceback)
  File "/Users/jacob/workspace/f-service-dev/gae/src/sitepackages/prod/djangae/sandbox.py", line 499, in activate
    yield
  File "/usr/local/Cellar/python@2/2.7.16/Frameworks/Python.framework/Versions/2.7/lib/python2.7/contextlib.py", line 35, in __exit__
    self.gen.throw(type, value, traceback)
  File "/Users/jacob/workspace/f-service-dev/gae/src/sitepackages/prod/djangae/sandbox.py", line 283, in _local
    stub_util.cleanup_stubs()
  File "/usr/local/Caskroom/google-cloud-sdk/latest/google-cloud-sdk/platform/google_appengine/google/appengine/tools/devappserver2/stub_util.py", line 359, in cleanup_stubs
    datastore_stub.Write()
AttributeError: 'RemoteDatastoreStub' object has no attribute 'Write'

I tried to run command again, and the same exact thing happened: browser auth, then error.

jacobg avatar Mar 12 '19 14:03 jacobg

Hi @Kazade. Regarding the remote error I described a couple days ago, are you seeing that error? Is it a real issue, or do you suspect it's something in my own configuration?

jacobg avatar Mar 14 '19 17:03 jacobg

Luke (Kazade) is currently away on parental leave, so is unlikely to get to this for a while. Hopefully some other people can test this as well, but if you're able to dig down into that bug and find any clues about why it's happening then that would be useful.

adamalton avatar Mar 15 '19 18:03 adamalton

Ok, I took some look at the code. It would be easier and more faster I think to have a conversation first about it, than try to infer everything from the code.

jacobg avatar Mar 17 '19 15:03 jacobg

I get the same error: RemoteDatastoreStub has no attribute Write.

I don't know if the remote datastore actually needs to call Write() on the stub (it might automatically commit when necessary), but even removing those calls produces another error down the line:

Traceback (most recent call last):                                                                                                           
  File "./manage.py", line 25, in <module>                                                                                         
    execute_from_command_line(sys.argv)                                                                                                 
  File "/home/me/.virtualenvs/tmp-4b0e560bc3cb921/nvudex/lib/djangae/core/management/__init__.py", line 43, in execute_from_command_line                                                                                                                                       
    return _execute_from_command_line(djangae_namespace.sandbox, argv, parser=djangae_parser, **overrides)                               
  File "/home/me/.virtualenvs/tmp-4b0e560bc3cb921/nvudex/lib/djangae/core/management/__init__.py", line 68, in _execute_from_command_line
    return django_management.execute_from_command_line(argv)                                                                                                                                                                                                                  
  File "/home/me/.virtualenvs/tmp-4b0e560bc3cb921/lib/python2.7/site-packages/django/core/management/__init__.py", line 364, in execute_from_command_line
    utility.execute()                                                                                                                          
  File "/home/me/.virtualenvs/tmp-4b0e560bc3cb921/lib/python2.7/site-packages/django/core/management/__init__.py", line 356, in execute                                                                                                                                        
    self.fetch_command(subcommand).run_from_argv(self.argv)                                                                       
  File "/home/me/.virtualenvs/tmp-4b0e560bc3cb921/lib/python2.7/site-packages/django/core/management/base.py", line 283, in run_from_argv
    self.execute(*args, **cmd_options)                                                                                                                                                                                                                                         
  File "/home/me/.virtualenvs/tmp-4b0e560bc3cb921/lib/python2.7/site-packages/django/core/management/base.py", line 330, in execute                      
    output = self.handle(*args, **options)                                                                                                                                                                                                                                    
  File "/home/me/.virtualenvs/tmp-4b0e560bc3cb921/nvudex/lib/djangae/management/commands/remote.py", line 26, in handle                
    call_command(options.pop('subcommand'), *args, **options)                                                                                                                                                                                                                 
  File "/home/me/.virtualenvs/tmp-4b0e560bc3cb921/lib/python2.7/site-packages/django/core/management/__init__.py", line 106, in call_command                                          
    app_name = get_commands()[command_name]                                                                                              
TypeError: unhashable type: 'list'

Looks like we're sending the subcommand as a list to django's call_command, and we need to split it into the 'command' and it's arguments.

Also looking at the remote command code, we unconditionally call the gcloud login command. Maybe we can check if the credentials file exists and/or works instead of every time.

If all of that is done, I managed to get the shell to work, ~but not migrations.~ edit: got migrations to work thanks to gcloud not deploying indexes automatically.

0xdc avatar Mar 18 '19 16:03 0xdc

My recent changes fix the problems with trying to run remote shell.

The actual error was the one highlighted by @0xdc but it was being masked by the sandbox context manager throwing its own error on cleanup. Fixing that original error solved being able to connect to the remote shell.

After the above fix the cleanup was still a problem when the shell is closed so I've implemented a workaround for it where the cleanup is ignored if it finds that it's using the remote datastore stub (which is what is ultimately causing the problem). There is probably a better fix for this (maybe a separate sandbox again for remote commands) but this workaround should suffice for now.

kevincarrogan avatar Mar 27 '19 16:03 kevincarrogan

Is this branch ready to merge into master?

jacobg avatar Apr 18 '19 12:04 jacobg

I tested the branch and found it fixed the errors. The only annoyance is still the login screen every time you use it.

The maximum supported gcloud version number will need to be updated again (or possibly removed since that restriction is now gone).

0xdc avatar Apr 19 '19 07:04 0xdc

Just getting back to this...

I agree with @0xdc that everything works except remote prompts for login screen every time. I tried a bunch of things, based on what gcloud's remote_api_shell.py does. In command.remote.py, I added save_cookies=True to the ConfigureRemoteApiForOAuth call, and also added a call after to remote_api_stub.MaybeInvokeAuthentication(). But the clincher was removing the code:

        subprocess.check_call(
            ["gcloud", "auth", "application-default", "login"]
        )

That seems to be what is opening the browser, not ConfigureRemoteApiForOAuth. And shouldn't it only be called once to create new creds, not repeatedly? https://cloud.google.com/sdk/gcloud/reference/auth/application-default/login

jacobg avatar May 31 '19 16:05 jacobg

Now I'm trying to migrate to the datastore emulator as described here: https://cloud.google.com/appengine/docs/standard/python/tools/migrate-cloud-datastore-emulator

I set the command line option 'support_datastore_emulator': True in manage.py to enable it, but got some exception about rfind. The cause is deep down in the code which bridges devappserver and the emulator. It accept an option to be set for the path of the emulator script (datastore_emulator_cmd). Normally, the devappserver.py high-level bootstrap code does auto-discover the emulator in the sdk and sets the option, but it looks like in sandbox.py we are bypassing that high-level bootstrap and constructing the api_server directly. And it doesn't appear that the runserver djangae command is even called! Am I understanding that correctly? Are we not using runserver.py anymore?

Basically, it seems that djangae needs some more work to support the datastore emulator.

After hardcoding datastore_emulator_cmd in manage.py, it looks like the emulator is starting, but then django fails to connect to it on startup and aborts. I'm currently looking into that issue.

jacobg avatar May 31 '19 19:05 jacobg

Update on my last comment: Django boot-up of settings.py setting SECRET_KEY = get app_config().secret_key calls the datastore apparently before the datastore is started. If I hardcode the secret_key, then the app starts fine and after warm-up, the datastore works fine.

So it looks like there's two issues with using the datastore emulator:

  1. datastore_emulator_cmd not set due to django calling create_api_server directly instead of using the higher-level dev_appserver api (what's runserver.py used for?).
  2. can't access datastore from settings.py (and maybe this problem extends further into boot up)

I also just noticed that a 3rd party library couldn't import pkg_resources, because it tried to read system platform info from the /System folder and fails with IOError. I didn't notice that error before switching to datastore emulator, but it's possible it's another issue with gcloud switch.

jacobg avatar May 31 '19 20:05 jacobg

Seems that the fix for the two issues I mentioned in my last comments is to start the api server during sandbox activation, and then stub the api server's start function afterward to a no-op, similar to the way we already patch the function to create an api server. Code-wise, this means in the sandbox.py's _local function, right after we create the api server, add this code:

_API_SERVER.start()
def start_api_server():
    pass
api_server.start = start_api_server

jacobg avatar Jun 02 '19 02:06 jacobg

There's another issue using remote shell with datastore emulator. ConfigureRemoteApiForOAuth needs to be called in sandbox activation rather than in django command processing. That way, the datastore stub is setup when django initializes the app, so that we can look up the SECRET_KEY. It's a similar issue to needing to start the emulator in the sandbox activation, which I mentioned in my previous comment. We used to not have this issue, because:

  1. in master branch, we set up remote stub in sandbox
  2. when running local dev server, it doesn't need a datastore service, probably because it just setups the sqlite stuff internally as needed

So basically, I think we're going to need to put back the remote sandbox.

At this point, since the changes are getting heavier, I'm going to just create a fork, and submit a PR on top of the gcloud branch.

jacobg avatar Jun 02 '19 14:06 jacobg

Here's the PR: https://github.com/potatolondon/djangae/pull/1172 Please review and merge.

jacobg avatar Jun 02 '19 18:06 jacobg

Thanks for testing this and looking into the code, Jacob.

I also just noticed that a 3rd party library couldn't import pkg_resources, because it tried to read system platform info from the /System folder and fails with IOError. I didn't notice that error before switching to datastore emulator, but it's possible it's another issue with gcloud switch.

Did you figure out what this was? I.e. do we need to worry about it?

Are we not using runserver.py anymore?

As far as I know we should still be using Djangae's custom runserver command. Are you sure that's it's not being called? If so, have you checked that 'djangae' is the first item in INSTALLED_APPS?

So basically, I think we're going to need to put back the remote sandbox.

I've just made a comment on your new PR about (re-)updating the docs accordingly. But I'm also thinking… does this affect or undo this change? https://github.com/potatolondon/djangae/pull/1163/commits/0de0b71674aa48d49965ef217fd22cce750ff01c

adamalton avatar Jun 04 '19 16:06 adamalton

Thanks @adamalton.

I did not figure out why pkg_resources fails. It came up in the user of the pycountry library, and I ended up monkey patching pycountry as a workaround.

Please ignore my comments on runserver. It is called. Those comments were made earlier when I had less understanding about how the bootstrapping works.

What should I update in the docs?

Regarding https://github.com/potatolondon/djangae/commit/0de0b71674aa48d49965ef217fd22cce750ff01c, I deleted the remote command entirely, as the remote sandbox replaces it.

jacobg avatar Jun 04 '19 16:06 jacobg

Docs have been updated.

jacobg avatar Jun 04 '19 16:06 jacobg