Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Use argparse + make chmod not fail in all cases #1493

Merged
merged 5 commits into from
Oct 17, 2016

Conversation

dmarchand
Copy link
Contributor

This PR has two components to it:

  • Refactor executor script a bit to use argparse. This shouldn't impact any runtime behavior. This just makes it so adding and removing parameters no longer requires shifting that giant array of arguments around. Shifting of parameters here caused some tricky to debug issues with our usage of Heron, and this should make figuring things out a bit easier.
  • Change the way permission handling works a bit. The script originally just blindly attempted to chmod the log dirs and the various binaries. In a docker-based environment this ended up causing some issues for us. The executor had permission to execute these binaries already, so it didn't need to chmod them. However, it did not have permission to chmod them. As a result, this script would fail and abort even though it didn't actually need to change permissions. This solution seems a bit hacky, so I'm definitely open to something cleaner if anyone has any ideas.

@billonahill billonahill added this to the 0.14.5 milestone Oct 14, 2016
@billonahill
Copy link
Contributor

This makes me so happy, I was just thinking about this broken window! :)

I'll review soon. Would you please accept the CLA:
http://twitter.github.io/heron/docs/contributors/community/

…with unit test mocking. Fixed bug with jvm opts too
for binary in chmod_x_binaries:
stat_result = os.stat(binary)[stat.ST_MODE]
if not stat_result & stat.S_IXOTH:
binaries_to_chmod.append(binary)
Copy link
Contributor

Choose a reason for hiding this comment

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

stat seems like a good approach, provided it works on all distros.

There's no need to do all chmods in one concatenated command, so better to break it up to make the code more readable. No need to build up arrays and concated strings, we can just do this:

- chmod log dir

- for each binary
  - if chmod needed
    - do chmod 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me, I'll make that change. Stat appears to work on everything major, but I'll do some additional digging and testing on some VMs to be sure.

self.tmaster_binary = args[7]
self.stmgr_binary = args[8]
self.metricsmgr_classpath = args[9]
self.shard = int(parsed_args.shard)
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make this an int type in argparse and remove the cast

self.component_rammap =\
map(lambda x: {x.split(':')[0]: int(x.split(':')[1])}, args[16].split(','))
map(lambda x: {x.split(':')[0]:
int(x.split(':')[1])}, parsed_args.component_rammap.split(','))
Copy link
Contributor

Choose a reason for hiding this comment

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

can args.component_rammap do this and just return a map?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this comment. Can you explain?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if this logic could be encapsulated into the argparse logic via custom actions. I'm not certain it could, and maybe it's not worth the effort, so feel free to disregard.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. If @dmarchand thinks it's worth the effort, feel free to refer to https://docs.python.org/dev/library/argparse.html#action

@kramasamy
Copy link
Contributor

thanks @dmarchand - this is a great PR, we have been wanting to do this. Great to see you picked this up.

parser.add_argument("scheduler_port")
parser.add_argument("python_instance_binary")

return parser.parse_args(args[1:])
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you removed this part, can you use parse_known_args as in heron/tools/explorer/src/python/main.py#L172, and check if there is any unknown args?


def run_command_or_exit(self, command):
if self._run_blocking_process(command, True, self.shell_env) != 0:
Log.info("Failed to run command: %s. Exiting" % command)
Copy link
Contributor

Choose a reason for hiding this comment

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

Log.error


if unknown_args:
Log.error('Unknown argument: %s' % unknown_args[0])
parser.print_help()
Copy link
Contributor

Choose a reason for hiding this comment

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

since we didn't write any help message for any argument, it doesn't really matter if we'll print help message here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case argparse is supposed to dump the list of required arguments: https://docs.python.org/3/library/argparse.html#usage

It's probably not the best possible help message and we should likely take some time at some point to document the individual arguments, but this will at least clue the developer in as to which argument they added doesn't belong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for pointing out. Agreed.

@billonahill
Copy link
Contributor

👍

1 similar comment
@objmagic
Copy link
Contributor

👍

@objmagic objmagic merged commit 0af11ca into apache:master Oct 17, 2016
@objmagic objmagic mentioned this pull request Jan 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants